| 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: Tue, 22 Aug 2017 15:49:22 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Byungchul Park <byungchul.park@....com>
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 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.
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)
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