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
| ||
|
Date: Wed, 23 Aug 2017 11:43:23 +0900 From: Byungchul Park <byungchul.park@....com> To: Peter Zijlstra <peterz@...radead.org> Cc: mingo@...nel.org, linux-kernel@...r.kernel.org, kernel-team@....com, Arnaldo Carvalho de Melo <acme@...nel.org>, Dave Chinner <david@...morbit.com>, Tejun Heo <tj@...nel.org>, johannes@...solutions.net Subject: Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING On Tue, Aug 22, 2017 at 03:49:22PM +0200, Peter Zijlstra wrote: > On Tue, Aug 22, 2017 at 12:08:40PM +0200, Peter Zijlstra wrote: > > > > > > I meant: > > > > > > > > > > mutex_lock(&A) > > > > > <work> > > > > > lockdep_map_acquire_read(&work) > > > > > mutex_lock(&A) > > > > > > > > > > lockdep_map_acquire(&work) > > > > > flush_work(&work) > > > > > > > > > > I mean it can still be detected with a read acquisition in work. > > > > > Am I wrong? > > > > > > > > Think so, although there's something weird with read locks that I keep > > > > forgetting. But I'm not sure it'll actually solve the problem. But I can > > > > > > I mean, read acquisitions are nothing but ones allowing read ones to be > > > re-acquired legally, IOW, we want to check entrance of flush_work() and > > > works, not between works. That's why I suggested to use read ones in work > > > in that case. > > > > Does seem to work. > > So I think we'll end up hitting a lockdep deficiency and not trigger the > splat on flush_work(), see also: > > https://lwn.net/Articles/332801/ > > lock_map_acquire_read() is a read-recursive and will not in fact create > any dependencies because of this issue. > > In specific, check_prev_add() has: > > if (next->read == 2 || prev->read == 2) > return 1; > > This means that for: > > lock_map_acquire_read(W)(2) > down_write(A) (0) > > down_write(A) (0) > wait_for_completion(C) (0) > > lock_map_acquire_read(W)(2) > complete(C) (0) > > All the (2) effectively go away and 'solve' our current issue, but: > > lock_map_acquire_read(W)(2) > mutex_lock(A) (0) > > mutex_lock(A) (0) > lock_map_acquire(W) (0) > > as per flush_work() will not in fact trigger anymore either. It should be triggered. Lockdep code should be fixed so that it does. > See also the below locking-selftest changes. > > > Now, this means I also have to consider the existing > lock_map_acquire_read() users and if they really wanted to be recursive > or not. When I change lock_map_acquire_read() to use > lock_acquire_shared() this annotation no longer suffices and the splat > comes back. > > > Also, the acquire_read() annotation will (obviously) no longer work to > cure this problem when we switch to normal read (1), because then the > generated chain: > > W(1) -> A(0) -> C(0) -> W(1) Please explain what W/A/C stand for. > > spells deadlock, since W isn't allowed to recurse. > > > /me goes dig through commit: > > e159489baa71 ("workqueue: relax lockdep annotation on flush_work()") > > to figure out wth the existing users really want. > > > [ 0.000000] ---------------------------------------------------------------------------- > [ 0.000000] | spin |wlock |rlock |mutex | wsem | rsem | > [ 0.000000] -------------------------------------------------------------------------- > > [ 0.000000] -------------------------------------------------------------------------- > [ 0.000000] recursive read-lock: | ok | | ok | > [ 0.000000] recursive read-lock #2: | ok | | ok | > [ 0.000000] mixed read-write-lock: | ok | | ok | > [ 0.000000] mixed write-read-lock: | ok | | ok | > [ 0.000000] mixed read-lock/lock-write ABBA: |FAILED| | ok | > [ 0.000000] mixed read-lock/lock-read ABBA: | ok | | ok | > [ 0.000000] mixed write-lock/lock-write ABBA: | ok | | ok | > [ 0.000000] -------------------------------------------------------------------------- > > --- > lib/locking-selftest.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 116 insertions(+), 1 deletion(-) > > diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c > index 6f2b135dc5e8..b99d365cf399 100644 > --- a/lib/locking-selftest.c > +++ b/lib/locking-selftest.c > @@ -363,6 +363,103 @@ static void rsem_AA3(void) > } > > /* > + * read_lock(A) > + * spin_lock(B) > + * spin_lock(B) > + * write_lock(A) > + */ > +static void rlock_ABBA1(void) > +{ > + RL(X1); > + L(Y1); > + U(Y1); > + RU(X1); > + > + L(Y1); > + WL(X1); > + WU(X1); > + U(Y1); // should fail > +} > + > +static void rwsem_ABBA1(void) > +{ > + RSL(X1); > + ML(Y1); > + MU(Y1); > + RSU(X1); > + > + ML(Y1); > + WSL(X1); > + WSU(X1); > + MU(Y1); // should fail > +} > + > +/* > + * read_lock(A) > + * spin_lock(B) > + * spin_lock(B) > + * read_lock(A) > + */ > +static void rlock_ABBA2(void) > +{ > + RL(X1); > + L(Y1); > + U(Y1); > + RU(X1); > + > + L(Y1); > + RL(X1); > + RU(X1); > + U(Y1); // should NOT fail > +} > + > +static void rwsem_ABBA2(void) > +{ > + RSL(X1); > + ML(Y1); > + MU(Y1); > + RSU(X1); > + > + ML(Y1); > + RSL(X1); > + RSU(X1); > + MU(Y1); // should fail > +} > + > + > +/* > + * write_lock(A) > + * spin_lock(B) > + * spin_lock(B) > + * write_lock(A) > + */ > +static void rlock_ABBA3(void) > +{ > + WL(X1); > + L(Y1); > + U(Y1); > + WU(X1); > + > + L(Y1); > + WL(X1); > + WU(X1); > + U(Y1); // should fail > +} > + > +static void rwsem_ABBA3(void) > +{ > + WSL(X1); > + ML(Y1); > + MU(Y1); > + WSU(X1); > + > + ML(Y1); > + WSL(X1); > + WSU(X1); > + MU(Y1); // should fail > +} > + > +/* > * ABBA deadlock: > */ > > @@ -1057,7 +1154,7 @@ static void dotest(void (*testcase_fn)(void), int expected, int lockclass_mask) > unexpected_testcase_failures++; > pr_cont("FAILED|"); > > - dump_stack(); > +// dump_stack(); > } else { > testcase_successes++; > pr_cont(" ok |"); > @@ -1933,6 +2030,24 @@ void locking_selftest(void) > dotest(rsem_AA3, FAILURE, LOCKTYPE_RWSEM); > pr_cont("\n"); > > + print_testname("mixed read-lock/lock-write ABBA"); > + pr_cont(" |"); > + dotest(rlock_ABBA1, FAILURE, LOCKTYPE_RWLOCK); > + pr_cont(" |"); > + dotest(rwsem_ABBA1, FAILURE, LOCKTYPE_RWSEM); > + > + print_testname("mixed read-lock/lock-read ABBA"); > + pr_cont(" |"); > + dotest(rlock_ABBA2, SUCCESS, LOCKTYPE_RWLOCK); > + pr_cont(" |"); > + dotest(rwsem_ABBA2, FAILURE, LOCKTYPE_RWSEM); > + > + print_testname("mixed write-lock/lock-write ABBA"); > + pr_cont(" |"); > + dotest(rlock_ABBA3, FAILURE, LOCKTYPE_RWLOCK); > + pr_cont(" |"); > + dotest(rwsem_ABBA3, FAILURE, LOCKTYPE_RWSEM); > + > printk(" --------------------------------------------------------------------------\n"); > > /*
Powered by blists - more mailing lists