lkml.org 
[lkml]   [2010]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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!


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