* [PATCH 0/1] bcache fix for 4.14-rc4
@ 2017-09-26 9:54 Coly Li
2017-09-26 9:54 ` [PATCH 1/1] bcache: use llist_for_each_entry_safe() in __closure_wake_up() Coly Li
0 siblings, 1 reply; 8+ messages in thread
From: Coly Li @ 2017-09-26 9:54 UTC (permalink / raw)
To: axboe, linux-bcache, linux-block; +Cc: Coly Li
Hi Jens,
Michael Lyle catches a race bug in bcache 4.14 patch set. Here is the fix
for this issue. Could you please to pick it for 4.14-rc4 ? Then we don't need
to stable@vger.kernel.org in next cycle.
Thanks in advance.
Coly Li
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/1] bcache: use llist_for_each_entry_safe() in __closure_wake_up()
2017-09-26 9:54 [PATCH 0/1] bcache fix for 4.14-rc4 Coly Li
@ 2017-09-26 9:54 ` Coly Li
2017-09-27 19:16 ` Coly Li
2017-09-27 20:29 ` Jens Axboe
0 siblings, 2 replies; 8+ messages in thread
From: Coly Li @ 2017-09-26 9:54 UTC (permalink / raw)
To: axboe, linux-bcache, linux-block; +Cc: Coly Li
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>
Reviewed-by: Byungchul Park <byungchul.park@lge.com>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] bcache: use llist_for_each_entry_safe() in __closure_wake_up()
2017-09-26 9:54 ` [PATCH 1/1] bcache: use llist_for_each_entry_safe() in __closure_wake_up() Coly Li
@ 2017-09-27 19:16 ` Coly Li
2017-09-27 20:27 ` Jens Axboe
2017-09-27 20:29 ` Jens Axboe
1 sibling, 1 reply; 8+ messages in thread
From: Coly Li @ 2017-09-27 19:16 UTC (permalink / raw)
To: axboe; +Cc: linux-bcache, linux-block
Hi Jens,
Could you please take a look on this patch? It will be helpful if we can
have it in 4.14, then we can fix a bug introduced in 4.14-rc1.
This patch is reported by Michael Lyle, reviewed by Byungchul Park, and
finally verified by Michael Lyle after I posted the patch.
Many thanks in advance.
Coly Li
On 2017/9/26 下午5:54, Coly Li wrote:
> 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>
> Reviewed-by: Byungchul Park <byungchul.park@lge.com>
> ---
> 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);
> }
>
--
Coly Li
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] bcache: use llist_for_each_entry_safe() in __closure_wake_up()
2017-09-27 19:16 ` Coly Li
@ 2017-09-27 20:27 ` Jens Axboe
2017-09-27 20:48 ` Michael Lyle
0 siblings, 1 reply; 8+ messages in thread
From: Jens Axboe @ 2017-09-27 20:27 UTC (permalink / raw)
To: Coly Li; +Cc: linux-bcache, linux-block
On 09/27/2017 09:16 PM, Coly Li wrote:
> Hi Jens,
>
> Could you please take a look on this patch? It will be helpful if we can
> have it in 4.14, then we can fix a bug introduced in 4.14-rc1.
>
> This patch is reported by Michael Lyle, reviewed by Byungchul Park, and
> finally verified by Michael Lyle after I posted the patch.
It looks fine to me, I'll get it queued up. BTW, it's technically
a use-after-free bug, not a race condition.
--
Jens Axboe
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] bcache: use llist_for_each_entry_safe() in __closure_wake_up()
2017-09-26 9:54 ` [PATCH 1/1] bcache: use llist_for_each_entry_safe() in __closure_wake_up() Coly Li
2017-09-27 19:16 ` Coly Li
@ 2017-09-27 20:29 ` Jens Axboe
1 sibling, 0 replies; 8+ messages in thread
From: Jens Axboe @ 2017-09-27 20:29 UTC (permalink / raw)
To: Coly Li, linux-bcache, linux-block
On 09/26/2017 11:54 AM, Coly Li wrote:
> Commit 09b3efec ("bcache: Don't reinvent the wheel but use existing llist
> API") replaces the following while loop by llist_for_each_entry(),
If it's fixing a specific commit, please add a proper
Fixes: sha ("title")
line to your patch. That way automated scripts can pick it up, if they
backport the problematic patch.
Also, as mentioned in the other email, this is technically a
use-after-free problem, not a race.
Can you resend with commit message fixed up?
--
Jens Axboe
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] bcache: use llist_for_each_entry_safe() in __closure_wake_up()
2017-09-27 20:27 ` Jens Axboe
@ 2017-09-27 20:48 ` Michael Lyle
2017-09-27 20:52 ` Jens Axboe
0 siblings, 1 reply; 8+ messages in thread
From: Michael Lyle @ 2017-09-27 20:48 UTC (permalink / raw)
To: Jens Axboe; +Cc: Coly Li, linux-bcache, linux-block
Jens--
I think it's a race condition-- the individual closures remain valid.
It's just that the list element has different meanings-- it's either a
list actively being used to wake, or a linkage on one of several lists
that is being used to await wake. If a closure goes back to wait very
quickly after being woken, it can end up connecting its new wait-list
with the being-woken list.
Mike
On Wed, Sep 27, 2017 at 1:27 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 09/27/2017 09:16 PM, Coly Li wrote:
>> Hi Jens,
>>
>> Could you please take a look on this patch? It will be helpful if we can
>> have it in 4.14, then we can fix a bug introduced in 4.14-rc1.
>>
>> This patch is reported by Michael Lyle, reviewed by Byungchul Park, and
>> finally verified by Michael Lyle after I posted the patch.
>
> It looks fine to me, I'll get it queued up. BTW, it's technically
> a use-after-free bug, not a race condition.
>
> --
> Jens Axboe
>
> --
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] bcache: use llist_for_each_entry_safe() in __closure_wake_up()
2017-09-27 20:48 ` Michael Lyle
@ 2017-09-27 20:52 ` Jens Axboe
2017-09-28 0:46 ` Coly Li
0 siblings, 1 reply; 8+ messages in thread
From: Jens Axboe @ 2017-09-27 20:52 UTC (permalink / raw)
To: Michael Lyle; +Cc: Coly Li, linux-bcache, linux-block
On 09/27/2017 10:48 PM, Michael Lyle wrote:
> Jens--
>
> I think it's a race condition-- the individual closures remain valid.
> It's just that the list element has different meanings-- it's either a
> list actively being used to wake, or a linkage on one of several lists
> that is being used to await wake. If a closure goes back to wait very
> quickly after being woken, it can end up connecting its new wait-list
> with the being-woken list.
Reading it a second time, looks like you are right. OK, then let's just
queue it up as-is, I'll add the Fixes line this time.
--
Jens Axboe
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] bcache: use llist_for_each_entry_safe() in __closure_wake_up()
2017-09-27 20:52 ` Jens Axboe
@ 2017-09-28 0:46 ` Coly Li
0 siblings, 0 replies; 8+ messages in thread
From: Coly Li @ 2017-09-28 0:46 UTC (permalink / raw)
To: Jens Axboe; +Cc: Michael Lyle, linux-bcache, linux-block
On 2017/9/28 上午4:52, Jens Axboe wrote:
> On 09/27/2017 10:48 PM, Michael Lyle wrote:
>> Jens--
>>
>> I think it's a race condition-- the individual closures remain valid.
>> It's just that the list element has different meanings-- it's either a
>> list actively being used to wake, or a linkage on one of several lists
>> that is being used to await wake. If a closure goes back to wait very
>> quickly after being woken, it can end up connecting its new wait-list
>> with the being-woken list.
>
> Reading it a second time, looks like you are right. OK, then let's just
> queue it up as-is, I'll add the Fixes line this time.
>
Hi Jens,
Copied, next time for similar situation, Fixes will be there.
Thanks for doing this.
--
Coly Li
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-09-28 0:46 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-26 9:54 [PATCH 0/1] bcache fix for 4.14-rc4 Coly Li
2017-09-26 9:54 ` [PATCH 1/1] bcache: use llist_for_each_entry_safe() in __closure_wake_up() Coly Li
2017-09-27 19:16 ` Coly Li
2017-09-27 20:27 ` Jens Axboe
2017-09-27 20:48 ` Michael Lyle
2017-09-27 20:52 ` Jens Axboe
2017-09-28 0:46 ` Coly Li
2017-09-27 20:29 ` Jens Axboe
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.