lkml.org 
[lkml]   [2010]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/2] lockdep: check the depth of subclass
From
Date
On Wed, 2010-10-13 at 11:26 +0900, Hitoshi Mitake wrote:
> >> @@ -639,6 +639,21 @@ look_up_lock_class(struct lockdep_map *lock, unsigned int subclass)
> >> }
> >> #endif
> >>
> >> + if (unlikely(subclass>= MAX_LOCKDEP_SUBCLASSES)) {
> >> + /*
> >> + * This check should be done not only in __lock_acquire()
> >> + * but also here. Because register_lock_class() is also called
> >> + * by lock_set_class(). Callers of lock_set_class() can
> >> + * pass invalid value as subclass.
> >> + */
> >> +
> >> + debug_locks_off();
> >> + printk(KERN_ERR "BUG: looking up invalid subclass: %u\n", subclass);
> >> + printk(KERN_ERR "turning off the locking correctness validator.\n");
> >> + dump_stack();
> >> + return NULL;
> >> + }
> >
> > Would we catch all cases if we moved this check from __lock_acquire()
> > into register_lock_class()? It would result in only a single instance of
> > this logic.
> >
>
> I think that __lock_acquire() should also check the value of subclass.
> Because it access to the lock->class_cache as array
> before calling look_up_lock_class() after applying this patch.
>
> So if the check isn't done in __lock_acquire(),
> the invalid addresses might be interpreted as the addresses of
> struct lock_class.


But __lock_acquire() does:

if (subclass < NR_LOCKDEP_CACHING_CLASSES)
class = lock->class_cache[subclass];

if (!class)
class = register_lock_class();

So by moving the: subclass >= MAX_LOCKDEP_SUBCLASSES, check into
register_lock_class() it would still trigger for __lock_acquire().
Because NR_LOCKDEP_CACHING_CLASSES <= MAX_LOCKDEP_SUBCLASSES, and thus
for subclass >= MAX_LOCKDEP_SUBCLASSES we'll always call into
register_lock_class() and trigger the failure there, no?




\
 
 \ /
  Last update: 2010-10-13 09:37    [W:0.111 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site