Messages in this thread | | | Date | Wed, 13 Oct 2010 17:13:55 +0900 | From | Hitoshi Mitake <> | Subject | Re: [PATCH 1/2] lockdep: check the depth of subclass |
| |
On 10/13/10 16:33, Peter Zijlstra wrote: > 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? > > >
Ahh, sorry, you are right. So current checking subclass >= MAX_LOCKDEP_SUBCLASSES is redundant, I'll remove this checking and resend second version later.
Thanks for your advice!
| |