linux-kernel - Re: [PATCH 2/7] fs/locks: Replace lg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160906082328.GF10153@twins.programming.kicks-ass.net>
Date:   Tue, 6 Sep 2016 10:23:28 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Andreas Mohr <andi@...as.de>
Cc:     der.herr@...r.at, Al Viro <viro@...IV.linux.org.uk>,
        mingo@...hat.com, torvalds@...ux-foundation.org, dave@...olabs.net,
        Oleg Nesterov <oleg@...hat.com>, riel@...hat.com,
        tj@...nel.org, paulmck@...ux.vnet.ibm.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/7] fs/locks: Replace lg_global with a percpu-rwsem

On Tue, Sep 06, 2016 at 03:58:00AM +0200, Andreas Mohr wrote:
> Hi,
> 
> [no properly binding reference via In-Reply-To: available thus manually re-creating, sorry]
> 
> https://lkml.org/lkml/2016/9/5/832

Subscribe to lkml already.. also lkml.org is near useless these days,
please use any other archive.

> Two thoughts:
> 
> ***multiple locks
> 
> Don't have much insight into this
> (didn't spend much thinking on this),
> but of course it's unfortunate
> that two lock types need to be serviced
> rather than having the atomic handling exchange area/section
> be sufficiently guarded by one lock only
> (the question here could possibly be:
> what kind of currently existing
> structural disadvantage/layer distribution
> prevents us from being able to
> have things simply serviced
> within one granular lock area only?)

percpu rwsem is a sleeping lock, flc_flock is not. flc_flock also nests
under other non-sleeping locks, and therefore cannot (trivially) be made
a sleeping lock.

This is in the Changelog.

Furthermore, in the fast path of percpu_{down,up}_read() are exactly 0
atomic/serializing instructions.

> ***lock handling
> 
> > +	percpu_down_read(&file_rwsem);
> >  	spin_lock(&ctx->flc_lock);
> 
> 
> >  	spin_unlock(&ctx->flc_lock);
> > +	percpu_up_read(&file_rwsem);
> 
> 
> These are repeated multiple times in this commit, thus error-prone.
> 
> A possibly good way to commit-micro-manage this would be:
> 1. commit shoves things into a newly created encapsulation/wrapper helper
>    stuff_lock(&flc_lock);  /* <---- naming surely can be improved here */

No, because not all instances of flc_flock need the percpu-rwsem held,
creating such a wrapper could mistakenly create the impression it
should.

> That way it is pretty much guaranteed that:
> a) neither one nor the other lock type
>    will get forgotten later, at a specific use site

That's what we have lockdep_assert_held() for. Note how this patch also
introduces percpu_rwsem_assert_held() usage, this insures we shall never
'forget' the appropriate locking.

> b) lock order will be correctly maintained at all times
>    (AB-BA deadlock......)

lockdep is rather good at finding those, also might_sleep() debugging
would trivially catch those since percpu-rwsem is a sleeping lock while
fcl_flock is not.