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

[연]

> -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 

> 
> Coly Li
> 
> 
>  Forwarded Message 
> Return-path: 
> Envelope-to: i...@coly.li
> Delivery-date: Tue, 26 Sep 2017 07:45:21 +
> Received: from vger.kernel.org ([209.132.180.67]:56115) by
> server.coly.li with esmtp (Exim 4.87) (envelope-from
> ) id 1dwkYT-0002cE-Mh for i...@coly.li;
> Tue, 26 Sep 2017 07:45:21 +
> Received: (majord...@vger.kernel.org) by vger.kernel.org via listexpand
>   id S936950AbdIZHhM (ORCPT );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 ESMTPid
> S936949AbdIZHhL (ORCPT );
> 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 + (UTC)
> From: Coly Li 
> To: linux-bca...@vger.kernel.org, linux-block@vger.kernel.org
> Cc: Coly Li 
> 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-col...@suse.de>
> X-Mailer: git-send-email 2.13.5
> Sender: linux-bcache-ow...@vger.kernel.org
> Precedence: bulk
> List-ID: 
> X-Mailing-List: linux-bca...@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 
> Reported-by: Michael Lyle 
> ---
>  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(_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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 04/12] bcache: Don't reinvent the wheel but use existing llist API

[연]

> -Original Message-
> From: Coly Li [mailto:i...@coly.li]
> Sent: Tuesday, September 26, 2017 4:09 PM
> To: Michael Lyle; Coly Li
> Cc: linux-bca...@vger.kernel.org; linux-block@vger.kernel.org;
> ax...@kernel.dk; Eric Wheeler; Byungchul Park; Kent Overstreet
> Subject: Re: [PATCH 04/12] bcache: Don't reinvent the wheel but use existing
> llist API
> 
> On 2017/9/26 下午12:38, Michael Lyle wrote:
> > I believe this introduces a critical bug.
> >
> > cl->list is used to link together the llists for both things waiting,
> > and for things that are being woken.
> >
> > If a closure that is woken decides to wait again, it will corrupt the
> > llist that __closure_wake_up is using.
> >
> > The previous iteration structure gets the next element of the list
> > before waking and is therefore safe.
> >
> 
> Hi Mike,
> 
> Good catch! I see llist_del_all() but forget cl->list can be modified in
> closure_wait(). Yes there is potential chance to mislead
> llist_for_each_entry() to iterate wrong list.
> llist_for_each_entry_safe() should be used here. I will send a fix to
> Jens, hope to catch up 4.14 still.

I see. You have a plan to do it. Please fix it.

Thank you.

> Thanks!
> --
> Coly Li

RE: [PATCH 04/12] bcache: Don't reinvent the wheel but use existing llist API

[연]

> -Original Message-
> From: Coly Li [mailto:i...@coly.li]
> Sent: Tuesday, September 26, 2017 4:09 PM
> To: 박병철/선임연구원/SW Platform(연)AOT팀(byungchul.p...@lge.com);
> Michael Lyle; Coly Li
> Cc: linux-bca...@vger.kernel.org; linux-block@vger.kernel.org;
> ax...@kernel.dk; Eric Wheeler; Kent Overstreet; kernel-t...@lge.com
> Subject: Re: [PATCH 04/12] bcache: Don't reinvent the wheel but use existing
> llist API
> 
> On 2017/9/26 下午2:39, 박병철/선임연구원/SW
> Platform(연)AOT팀(byungchul.p...@lge.com) wrote:
> >> -Original Message-
> >> From: Michael Lyle [mailto:ml...@lyle.org]
> >> Sent: Tuesday, September 26, 2017 1:38 PM
> >> To: Coly Li
> >> Cc: linux-bca...@vger.kernel.org; linux-block@vger.kernel.org;
> >> ax...@kernel.dk; Eric Wheeler; Byungchul Park; Kent Overstreet
> >> Subject: Re: [PATCH 04/12] bcache: Don't reinvent the wheel but use 
> >> existing
> >> llist API
> >>
> >> I believe this introduces a critical bug.
> >>
> >> cl->list is used to link together the llists for both things waiting,
> >> and for things that are being woken.
> >>
> >> If a closure that is woken decides to wait again, it will corrupt the
> >> llist that __closure_wake_up is using.
> >>
> >> The previous iteration structure gets the next element of the list
> >> before waking and is therefore safe.
> >
> > Do you mean we have to use llist_for_each_entry_safe() instead of non-safe
> version?
> > Is it ok if we use it instead?
> 
> Yes, we should use llist_for_each_entry_safe(), there is a quite
> implicit race here.

Hi coly,

As you know, my first patch used the safe version, but you suggested to replace
It with non-safe one. :(

I will change it so it does the same as the original patch did. :)

Thank you very much,
Byungchul

> --
> Coly Li

RE: [PATCH 04/12] bcache: Don't reinvent the wheel but use existing llist API

[연]

> -Original Message-
> From: Michael Lyle [mailto:ml...@lyle.org]
> Sent: Tuesday, September 26, 2017 1:38 PM
> To: Coly Li
> Cc: linux-bca...@vger.kernel.org; linux-block@vger.kernel.org;
> ax...@kernel.dk; Eric Wheeler; Byungchul Park; Kent Overstreet
> Subject: Re: [PATCH 04/12] bcache: Don't reinvent the wheel but use existing
> llist API
> 
> I believe this introduces a critical bug.
> 
> cl->list is used to link together the llists for both things waiting,
> and for things that are being woken.
> 
> If a closure that is woken decides to wait again, it will corrupt the
> llist that __closure_wake_up is using.
> 
> The previous iteration structure gets the next element of the list
> before waking and is therefore safe.

Do you mean we have to use llist_for_each_entry_safe() instead of non-safe 
version?
Is it ok if we use it instead?

> Mike