| 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
| ||
|
Message-ID: <20190527150251.GE2623@hirez.programming.kicks-ass.net>
Date: Mon, 27 May 2019 17:02:51 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Imre Deak <imre.deak@...el.com>
Cc: LKML <linux-kernel@...r.kernel.org>,
Ville Syrjälä
<ville.syrjala@...ux.intel.com>, Ingo Molnar <mingo@...hat.com>,
Will Deacon <will.deacon@....com>
Subject: Re: [PATCH v2 1/2] lockdep: Fix OOO unlock when hlocks need merging
On Fri, May 24, 2019 at 11:15:08PM +0300, Imre Deak wrote:
>
> ww_mutex_lock(&ww_lock_a, &ww_ctx);
>
> mutex_lock(&lock_c);
>
> ww_mutex_lock(&ww_lock_b, &ww_ctx);
>
> mutex_unlock(&lock_c); (*)
> triggers the following WARN in __lock_release() when doing the unlock at *:
>
> DEBUG_LOCKS_WARN_ON(curr->lockdep_depth != depth - 1);
>
> The problem is that the WARN check doesn't take into account the merging
> of ww_lock_a and ww_lock_b which results in decreasing curr->lockdep_depth
> by 2 not only 1.
Cute...
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index c40fba54e324..967352d32af1 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -3714,7 +3714,7 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
> hlock->references = 2;
> }
>
> - return 1;
> + return 2;
> }
> }
>
> @@ -3920,22 +3920,33 @@ static struct held_lock *find_held_lock(struct task_struct *curr,
> }
>
> static int reacquire_held_locks(struct task_struct *curr, unsigned int depth,
> - int idx)
> + int idx, bool *first_merged)
> {
> struct held_lock *hlock;
> + int first_idx = idx;
>
> if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
> return 0;
>
> for (hlock = curr->held_locks + idx; idx < depth; idx++, hlock++) {
> - if (!__lock_acquire(hlock->instance,
> + switch (__lock_acquire(hlock->instance,
> hlock_class(hlock)->subclass,
> hlock->trylock,
> hlock->read, hlock->check,
> hlock->hardirqs_off,
> hlock->nest_lock, hlock->acquire_ip,
> - hlock->references, hlock->pin_count))
> + hlock->references, hlock->pin_count)) {
> + case 0:
> return 1;
> + case 1:
> + break;
> + case 2:
> + *first_merged = idx == first_idx;
> + break;
> + default:
> + WARN_ON(1);
> + return 0;
> + }
> }
> return 0;
> }
Does it work for you if I change it like so?
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -3712,7 +3712,7 @@ static int __lock_acquire(struct lockdep
hlock->references = 2;
}
- return 1;
+ return 2;
}
}
@@ -3918,22 +3918,33 @@ static struct held_lock *find_held_lock(
}
static int reacquire_held_locks(struct task_struct *curr, unsigned int depth,
- int idx)
+ int idx, unsigned int *merged)
{
struct held_lock *hlock;
+ int first_idx = idx;
if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
return 0;
for (hlock = curr->held_locks + idx; idx < depth; idx++, hlock++) {
- if (!__lock_acquire(hlock->instance,
+ switch (__lock_acquire(hlock->instance,
hlock_class(hlock)->subclass,
hlock->trylock,
hlock->read, hlock->check,
hlock->hardirqs_off,
hlock->nest_lock, hlock->acquire_ip,
- hlock->references, hlock->pin_count))
+ hlock->references, hlock->pin_count)) {
+ case 0:
return 1;
+ case 1:
+ break;
+ case 2:
+ *merged += (idx == first_idx);
+ break;
+ default:
+ WARN_ON(1);
+ return 0;
+ }
}
return 0;
}
@@ -3944,9 +3955,9 @@ __lock_set_class(struct lockdep_map *loc
unsigned long ip)
{
struct task_struct *curr = current;
+ unsigned int depth, merged = 0
struct held_lock *hlock;
struct lock_class *class;
- unsigned int depth;
int i;
if (unlikely(!debug_locks))
@@ -3971,14 +3982,14 @@ __lock_set_class(struct lockdep_map *loc
curr->lockdep_depth = i;
curr->curr_chain_key = hlock->prev_chain_key;
- if (reacquire_held_locks(curr, depth, i))
+ if (reacquire_held_locks(curr, depth, i, &merged))
return 0;
/*
* I took it apart and put it back together again, except now I have
* these 'spare' parts.. where shall I put them.
*/
- if (DEBUG_LOCKS_WARN_ON(curr->lockdep_depth != depth))
+ if (DEBUG_LOCKS_WARN_ON(curr->lockdep_depth != depth - merged))
return 0;
return 1;
}
@@ -3986,8 +3997,8 @@ __lock_set_class(struct lockdep_map *loc
static int __lock_downgrade(struct lockdep_map *lock, unsigned long ip)
{
struct task_struct *curr = current;
+ unsigned int depth, merged = 0;
struct held_lock *hlock;
- unsigned int depth;
int i;
if (unlikely(!debug_locks))
@@ -4012,7 +4023,7 @@ static int __lock_downgrade(struct lockd
hlock->read = 1;
hlock->acquire_ip = ip;
- if (reacquire_held_locks(curr, depth, i))
+ if (reacquire_held_locks(curr, depth, i, &merged))
return 0;
/*
@@ -4021,6 +4032,11 @@ static int __lock_downgrade(struct lockd
*/
if (DEBUG_LOCKS_WARN_ON(curr->lockdep_depth != depth))
return 0;
+
+ /* Merging can't happen with unchanged classes.. */
+ if (DEBUG_LOCKS_WARN_ON(merged))
+ return 0;
+
return 1;
}
@@ -4035,8 +4051,8 @@ static int
__lock_release(struct lockdep_map *lock, int nested, unsigned long ip)
{
struct task_struct *curr = current;
+ unsigned int depth, merged = 1;
struct held_lock *hlock;
- unsigned int depth;
int i;
if (unlikely(!debug_locks))
@@ -4091,14 +4107,15 @@ __lock_release(struct lockdep_map *lock,
if (i == depth-1)
return 1;
- if (reacquire_held_locks(curr, depth, i + 1))
+ if (reacquire_held_locks(curr, depth, i + 1, &merged))
return 0;
/*
* We had N bottles of beer on the wall, we drank one, but now
* there's not N-1 bottles of beer left on the wall...
+ * Pouring two of the bottles together is acceptable.
*/
- DEBUG_LOCKS_WARN_ON(curr->lockdep_depth != depth-1);
+ DEBUG_LOCKS_WARN_ON(curr->lockdep_depth != depth - merged);
/*
* Since reacquire_held_locks() would have called check_chain_key()
Powered by blists - more mailing lists