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

[Vegard Nossum]

On 7 November 2011 13:34, Peter Zijlstra <a.p.zijlstra@xxxxxxxxx> 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()?

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?)

Thanks,

And sorry for the inconvenience.

I've been thinking about chucking kmemcheck and reimplementing it
using LibVEX (the dynamic translation framework used by Valgrind
itself). Whoever is interested, feel free to have a look:

https://github.com/vegard/linux-2.6/tree/kmemcheck2


Vegard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/