[lkml]   [2011]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [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?


     \ /
      Last update: 2011-11-08 03:25    [W:0.033 / U:278.640 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site