Re: [PATCH 1/2] lockdep: check the depth of subclass

From: Peter Zijlstra
Date: Wed Oct 13 2010 - 03:34:14 EST


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?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/