Skip to contentSkip to conversation

locking fix

GIT PULL Merged Ingo Molnar 5 participants 7 messages May 16, 2019 – May 17, 2019

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

Linus,

Please pull the latest locking-urgent-for-linus git tree from:

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking-urgent-for-linus

# HEAD: a9e9bcb45b1525ba7aea26ed9441e8632aeeda58 locking/rwsem: Prevent decrement of reader count before increment

A single rwsem fix.

Thanks,

Ingo

------------------>
Waiman Long (1):
locking/rwsem: Prevent decrement of reader count before increment

 kernel/locking/rwsem-xadd.c | 46 ++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)
kernel/locking/rwsem-xadd.c +31 -15
index 6b3ee9948bf1..0b1f77957240 100644
@@ -130,6 +130,7 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
 {
 	struct rwsem_waiter *waiter, *tmp;
 	long oldcount, woken = 0, adjustment = 0;
+	struct list_head wlist;
 
 	/*
 	 * Take a peek at the queue head waiter such that we can determine
@@ -188,18 +189,43 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
 	 * of the queue. We know that woken will be at least 1 as we accounted
 	 * for above. Note we increment the 'active part' of the count by the
 	 * number of readers before waking any processes up.
+	 *
+	 * We have to do wakeup in 2 passes to prevent the possibility that
+	 * the reader count may be decremented before it is incremented. It
+	 * is because the to-be-woken waiter may not have slept yet. So it
+	 * may see waiter->task got cleared, finish its critical section and
+	 * do an unlock before the reader count increment.
+	 *
+	 * 1) Collect the read-waiters in a separate list, count them and
+	 *    fully increment the reader count in rwsem.
+	 * 2) For each waiters in the new list, clear waiter->task and
+	 *    put them into wake_q to be woken up later.
 	 */
-	list_for_each_entry_safe(waiter, tmp, &sem->wait_list, list) {
-		struct task_struct *tsk;
-
+	list_for_each_entry(waiter, &sem->wait_list, list) {
 		if (waiter->type == RWSEM_WAITING_FOR_WRITE)
 			break;
 
 		woken++;
-		tsk = waiter->task;
+	}
+	list_cut_before(&wlist, &sem->wait_list, &waiter->list);
+
+	adjustment = woken * RWSEM_ACTIVE_READ_BIAS - adjustment;
+	lockevent_cond_inc(rwsem_wake_reader, woken);
+	if (list_empty(&sem->wait_list)) {
+		/* hit end of list above */
+		adjustment -= RWSEM_WAITING_BIAS;
+	}
+
+	if (adjustment)
+		atomic_long_add(adjustment, &sem->count);
+
+	/* 2nd pass */
+	list_for_each_entry_safe(waiter, tmp, &wlist, list) {
+		struct task_struct *tsk;
 
+		tsk = waiter->task;
 		get_task_struct(tsk);
-		list_del(&waiter->list);
+
 		/*
 		 * Ensure calling get_task_struct() before setting the reader
 		 * waiter to nil such that rwsem_down_read_failed() cannot
@@ -213,16 +239,6 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
 		 */
 		wake_q_add_safe(wake_q, tsk);
 	}
-
-	adjustment = woken * RWSEM_ACTIVE_READ_BIAS - adjustment;
-	lockevent_cond_inc(rwsem_wake_reader, woken);
-	if (list_empty(&sem->wait_list)) {
-		/* hit end of list above */
-		adjustment -= RWSEM_WAITING_BIAS;
-	}
-
-	if (adjustment)
-		atomic_long_add(adjustment, &sem->count);
 }
 
 /*
#1 lore raw
Quoted 2 lines · Ingo Molnar

A single rwsem fix.

Side note, this one isn't marked for stable, but I'm hoping stable picks it up just from the "Fixes" tag.

Stable people, we're talking about

a9e9bcb45b15 ("locking/rwsem: Prevent decrement of reader count
before increment")

that I just pulled into my tree, and needs to go in 4.9 and later.

Linus

#2 lore raw
Quoted 7 lines · Linus Torvalds
Quoted 2 lines · Ingo Molnar

A single rwsem fix.

Side note, this one isn't marked for stable, but I'm hoping stable picks it up just from the "Fixes" tag.

Quoted 6 lines

Stable people, we're talking about

a9e9bcb45b15 ("locking/rwsem: Prevent decrement of reader count
before increment")

that I just pulled into my tree, and needs to go in 4.9 and later.

Thanks, I'll work on that later tonight...

greg k-h

#3 lore raw
Quoted 2 lines · Greg KH

Thanks, I'll work on that later tonight...

Note that it probably is almost entirely impossible to trigger the problem in practice, so it's not like this is a particularly important stable back-port.

I just happened to look at it and go "hmm, it's not _marked_ for stable".

Linus

maintainer
#4 lore raw
Quoted 9 lines · Linus Torvalds
Quoted 2 lines · Greg KH

Thanks, I'll work on that later tonight...

Note that it probably is almost entirely impossible to trigger the problem in practice, so it's not like this is a particularly important stable back-port.

I just happened to look at it and go "hmm, it's not _marked_ for stable".

I've addressed a missing a8654596f03 ("locking/rwsem: Enable lock event counting") and queued up a backport for 5.1-4.9.

--
Thanks,
Sasha

#5 lore raw
Quoted 5 lines · Sasha Levin
Quoted 4 lines · Linus Torvalds
Quoted 2 lines · Greg KH

Thanks, I'll work on that later tonight...

Quoted 8 lines
Quoted 5 lines

Note that it probably is almost entirely impossible to trigger the problem in practice, so it's not like this is a particularly important stable back-port.

I just happened to look at it and go "hmm, it's not _marked_ for stable".

I've addressed a missing a8654596f03 ("locking/rwsem: Enable lock event counting") and queued up a backport for 5.1-4.9.

Thanks for doing this.

greg k-h

merge bot Merged
#6 lore raw
In reply to Ingo Molnar
The pull request you sent on Thu, 16 May 2019 18:01:35 +0200:

The pull request you sent on Thu, 16 May 2019 18:01:35 +0200:

Quoted 1 line

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking-urgent-for-linus

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/f57d7715d7645b7c3d1e7b7cb79ac7690fe2d260

Thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker