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

[Hitoshi Mitake]

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!
--
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/