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

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

On Tue, Nov 08, 2011 at 10:58:47AM +0800, Yong Zhang wrote:
> On Mon, Nov 07, 2011 at 05:10:29PM +0100, Peter Zijlstra wrote:
> > On Mon, 2011-11-07 at 16:28 +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()?
> > 
> > We could move the key and name pointer to the start of the structure and
> > memset everything after that, however wouldn't that leave kmemcheck with
> > the same problem? It wouldn't know those two pointers would be
> > initialized properly.
> > 
> > > 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:
> 
> We should take ->calss_cache more carefully, because if we memset() it
> unconditionnally we will have no chance to set it anymore. Thus the
> performace brought by ->class_cache will be gone.
> 
> 1) for lock_set_subclass(): we can't initialize ->class_cache because
>    it's still valid and we need it.
> 2) for lock_set_class(): we have to initialize ->class_cache because
>    it's invalid anymore.
> 
> Maybe we could unconditionally set it we look_up_lock_class() find the
> class?
> 
> > 
> > Something like this, although it would be best to come up with a nicer
> > way to write it..
> > 
> > ---
> >  include/linux/lockdep.h |    2 +-
> >  kernel/lockdep.c        |    3 ++-
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> > index b6a56e3..7d66268 100644
> > --- a/include/linux/lockdep.h
> > +++ b/include/linux/lockdep.h
> > @@ -148,9 +148,9 @@ void clear_lock_stats(struct lock_class *class);
> >   * This is embedded into specific lock instances:
> >   */
> >  struct lockdep_map {
> > +	const char			*name;
> >  	struct lock_class_key		*key;
> >  	struct lock_class		*class_cache[NR_LOCKDEP_CACHING_CLASSES];
> > -	const char			*name;
> >  #ifdef CONFIG_LOCK_STAT
> >  	int				cpu;
> >  	unsigned long			ip;
> > diff --git a/kernel/lockdep.c b/kernel/lockdep.c
> > index e69434b..81855cf 100644
> > --- a/kernel/lockdep.c
> > +++ b/kernel/lockdep.c
> > @@ -2948,7 +2948,8 @@ 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, 2*sizeof(void *));
> > +	memset(&lock->class_cache[0], 0, sizeof(*lock)-2*sizeof(void *));
> 
> That means ->key have chance to be 0 at some time, right? Then I think it'll
> lead to another false positive warning like what Borislav has reported:
> http://marc.info/?l=linux-kernel&m=132039877026653
> 
> The reason is some rq->lock could carry a wrong key at certain time.
> 
> 	CPU A				CPU B
>   lock_set_subclass(lockA)
>     __lock_set_class(lockA)
>       lockdep_init_map(lockA)
>         memset() /* ->key = NULL */
>       				__lock_acquire(lockA)
> 				  register_lock_class(lockA)
> 				    look_up_lock_class(lockA)
> 				      if (unlikely(!lock->key))
> 				              lock->key = (void *)lock;
> 	->key = key;
> 				/* lockA maybe carry wrong class in later running
> 				 * due to ->class_cache
> 				 */

And lockA could also carry different key:

	CPU A				CPU B
  lock_set_subclass(lockA)
    __lock_set_class(lockA)
      lockdep_init_map(lockA)
        memset() /* ->key = NULL */
      				__lock_acquire(lockA)
				  register_lock_class(lockA)
				    look_up_lock_class(lockA)
				      if (unlikely(!lock->key))
	->key = key;
				              lock->key = (void *)lock;
				/* lockA maybe carry wrong key in later running
				 * due to ->class_cache
				 */

Thanks,
Yong

> 
> 
> Then when another lock_set_subclass() comes:
>         CPU A                           CPU B
>   lock_set_subclass(lockA);
>     lock_set_class(lockA);
>       				__lock_acquire(lockA)
>                                   /* lockA->class_cache[] is not set,
> 				   * different subclass */
>                                   register_lock_class(lockA);
>                                      look_up_lock_class(lockA); /* retrun NULL */
>       lockdep_init_map(lockA);
>         memset(lockA); /* ->key = NULL */
>                                   if (!static_obj(lock->key))
>                                   /* we get warning here */
> 
> 
> So maybe the simplest way is just annotating ->lock like this:
> 	kmemcheck_mark_initialized(lock, sizeof(*lock));
> 
> Thanks,
> Yong

-- 
Only stand for myself