RE: [PATCH] bcache: use llist_for_each_entry_safe() in __closure_wake_up()

Coly Li


-------- Forwarded Message --------
Return-path: [off-list ref]
Envelope-to: i@coly.li
Delivery-date: Tue, 26 Sep 2017 07:45:21 +0000
Received: from vger.kernel.org ([209.132.180.67]:56115) by
server.coly.li with esmtp (Exim 4.87) (envelope-from
[off-list ref]) id 1dwkYT-0002cE-Mh for i@coly.li;
Tue, 26 Sep 2017 07:45:21 +0000
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
      id S936950AbdIZHhM (ORCPT <rfc822;i@coly.li>);        Tue, 26 Sep
2017 03:37:12 -0400
Received: from mx2.suse.de ([195.135.220.15]:36732 "EHLO mx1.suse.de"
    rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP        id
S936949AbdIZHhL (ORCPT <rfc822;linux-bcache@vger.kernel.org>);
Tue, 26 Sep 2017 03:37:11 -0400
X-Virus-Scanned: by amavisd-new at test-mx.suse.de
Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254])
      by mx1.suse.de (Postfix) with ESMTP id BB460AE5F;        Tue, 26
Sep 2017 07:37:09 +0000 (UTC)
From: Coly Li <redacted>
To: linux-bcache@vger.kernel.org, linux-block@vger.kernel.org
Cc: Coly Li <redacted>
Subject: [PATCH] bcache: use llist_for_each_entry_safe() in
__closure_wake_up()
Date: Tue, 26 Sep 2017 15:37:02 +0800
Message-Id: [ref]
X-Mailer: git-send-email 2.13.5
Sender: linux-bcache-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-bcache.vger.kernel.org>
X-Mailing-List: linux-bcache@vger.kernel.org

Commit 09b3efec ("bcache: Don't reinvent the wheel but use existing llist
API") replaces the following while loop by llist_for_each_entry(),

-
-	while (reverse) {
-		cl = container_of(reverse, struct closure, list);
-		reverse = llist_next(reverse);
-
+	llist_for_each_entry(cl, reverse, list) {
 		closure_set_waiting(cl, 0);
 		closure_sub(cl, CLOSURE_WAITING + 1);
 	}

This modification introduces a potential race by iterating a corrupted
list. Here is how it happens.

In the above modification, closure_sub() may wake up a process which is
waiting on reverse list. If this process decides to wait again by calling
closure_wait(), its cl->list will be added to another wait list. Then
when llist_for_each_entry() continues to iterate next node, it will travel
on another new wait list which is added in closure_wait(), not the
original reverse list in __closure_wake_up(). It is more probably to
happen on UP machine because the waked up process may preempt the process
which wakes up it.

Use llist_for_each_entry_safe() will fix the issue, the safe version fetch
next node before waking up a process. Then the copy of next node will make
sure list iteration stays on original reverse list.

Signed-off-by: Coly Li <redacted>
Reported-by: Michael Lyle <redacted>
---
 drivers/md/bcache/closure.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
index 7d5286b05036..1841d0359bac 100644
--- a/drivers/md/bcache/closure.c
+++ b/drivers/md/bcache/closure.c

@@ -64,7 +64,7 @@ EXPORT_SYMBOL(closure_put);
 void __closure_wake_up(struct closure_waitlist *wait_list)
 {
 	struct llist_node *list;
-	struct closure *cl;
+	struct closure *cl, *t;
 	struct llist_node *reverse = NULL;
  	list = llist_del_all(&wait_list->list);

@@ -73,7 +73,7 @@ void __closure_wake_up(struct closure_waitlist *wait_list)
 	reverse = llist_reverse_order(list);
  	/* Then do the wakeups */
-	llist_for_each_entry(cl, reverse, list) {
+	llist_for_each_entry_safe(cl, t, reverse, list) {
 		closure_set_waiting(cl, 0);
 		closure_sub(cl, CLOSURE_WAITING + 1);
 	}
--

2.13.5

--
To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html