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

[Peter Zijlstra <a.p.zijlstra@chello.nl>]

On Mon, 2011-11-07 at 16:28 +0100, Vegard Nossum wrote:
> On 7 November 2011 13:34, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> > Arguably kmemcheck is on crack or so since both name and key pointers
> > should be in .data so there cannot be a leak by copying the thing over.
> 
> There is no leak, it's only about uninitialised memory.
> 
> The problem is that kmemcheck cannot track "initialisedness" of stack
> data and we're doing a heap-to-stack copy in process_one_work(), as
> Tejun pointed out:
> 
>         struct lockdep_map lockdep_map = work->lockdep_map;
> 
> This is of course fine in itself. But how can we tell kmemcheck that
> it's fine? There are basically two options:
> 
> 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:

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 *));
 
 #ifdef CONFIG_LOCK_STAT
 	lock->cpu = raw_smp_processor_id();