Messages in this thread | | | Date | Tue, 8 Nov 2011 10:22:38 +0800 | From | Yong Zhang <> | Subject | Re: [PATCH 1/4] lockdep: lock_set_subclass() fix |
| |
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
| |