Re: [PATCH 1/4] lockdep: lock_set_subclass() fix

[Yong Zhang <yong.zhang0@gmail.com>]

On Mon, Nov 07, 2011 at 04:28:19PM +0100, Vegard Nossum wrote:
> 
> 1. Initialise the thing completely before doing the copy, or
> 2. Ignore the warning.
> 
> The memset() patch (f59de8992aa6dc85e81aadc26b0f69e17809721d) attempts
> to do the first, i.e. to clear the whole struct in lockdep_init_map().
> 
> I think nr. 1 is the best way to go in principle, but I don't know
> what it takes for this to work properly. The blanket-clear memset()
> presumably doesn't work because it clears out something that was
> already initialised by the caller (right?).
> 
> Yong Zhang, can you think of a way to avoid the race you described,
> perhaps by memset()ing only the right/relevant parts of struct
> lockdep_map in lockdep_init_map()?

That could work, but we should take more care on the member 'class_cache',
because under some condition (lock_set_subclass()) we don't need
to initialise it for performance issue, but under other condtion (
set a new valid key to a class) we need to initialise it since it's
invalid anymore.

Another option is always seting ->class_cache if lookup_lock_class()
find the class. Will talk about it with Peter in another thread.

> 
> Peter Zijlstra, if you prefer, we can also just tell kmemcheck that
> this particular copy is fine, but it means that kmemcheck will not be
> able to detect any real bugs in this code. It can be done with
> something like:
> 
> diff --git a/kernel/lockdep.c b/kernel/lockdep.c
> index e69434b..08a2b1b 100644
> --- a/kernel/lockdep.c
> +++ b/kernel/lockdep.c
> @@ -2948,7 +2948,7 @@ static int mark_lock(struct task_struct *curr,
> struct held_lock *this,
>  void lockdep_init_map(struct lockdep_map *lock, const char *name,
>                       struct lock_class_key *key, int subclass)
>  {
> -       memset(lock, 0, sizeof(*lock));
> +       kmemcheck_mark_initialized(lock, sizeof(*lock));
> 
>  #ifdef CONFIG_LOCK_STAT
>         lock->cpu = raw_smp_processor_id();
> 
> Christian Casteyde, do you mind testing this patch as well?
> 
> (Yong Zhang, do you think this would still be vulnerable to the race
> you described?)

No, this will work because we just retore the previous behavior except
kmemcheck annotation, right?

Thanks,
Yong