| Message ID | 20170926073702.71606-1-colyli@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Coly Li [mailto:i@coly.li] > Sent: Tuesday, September 26, 2017 5:00 PM > To: Byungchul Park > Subject: Fwd: [PATCH] bcache: use llist_for_each_entry_safe() in > __closure_wake_up() > > Hi Byungchul, > > I posted the fix on linux-bcache mailing list, could you please to > review my fix ? > > Sorry for the confusion and thanks for your review in advance. All right :) Reviewed-by: Byungchul Park <byungchul.park@lge.com> > > Coly Li > > > -------- Forwarded Message -------- > Return-path: <linux-bcache-owner@vger.kernel.org> > 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 > <linux-bcache-owner@vger.kernel.org>) 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 <colyli@suse.de> > To: linux-bcache@vger.kernel.org, linux-block@vger.kernel.org > Cc: Coly Li <colyli@suse.de> > Subject: [PATCH] bcache: use llist_for_each_entry_safe() in > __closure_wake_up() > Date: Tue, 26 Sep 2017 15:37:02 +0800 > Message-Id: <20170926073702.71606-1-colyli@suse.de> > 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 <colyli@suse.de> > Reported-by: Michael Lyle <mlyle@lyle.org> > --- > 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
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); }
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 <colyli@suse.de> Reported-by: Michael Lyle <mlyle@lyle.org> --- drivers/md/bcache/closure.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)