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

[Michael Lyle]

Thanks everyone--

Yes, this looks good to me and works in testing.

Mike

On Tue, Sep 26, 2017 at 12:46 AM, Coly Li  wrote:
> 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 Michael and Byungchul,
>
> This is my fault to suggest Byungchul to change his correct patch into
> wrong one. But it's good to learn such an implicit race behind bcache
> code. I just post a patch to explain how this race may happen and
> corrupt the reverse list iteration. Could you please to review the fix ?
>
> And thanks to Michael again to catch this bug.
>
> --
> Coly Li

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

[Coly Li]

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 Michael and Byungchul,

This is my fault to suggest Byungchul to change his correct patch into
wrong one. But it's good to learn such an implicit race behind bcache
code. I just post a patch to explain how this race may happen and
corrupt the reverse list iteration. Could you please to review the fix ?

And thanks to Michael again to catch this bug.

-- 
Coly Li

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

[Coly Li]

On 2017/9/26 下午3:16, 박병철/선임연구원/SW
Platform(연)AOT팀(byungchul.p...@lge.com) wrote:
>> -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.

Oh, I just see this email, when I replied your last one.
Sure, let me fix my fault.

-- 
Coly Li

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

[Coly Li]

On 2017/9/26 下午3:15, 박병철/선임연구원/SW
Platform(연)AOT팀(byungchul.p...@lge.com) wrote:
>> -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. :(

No doubt, it's my fault. I didn't find the implicit potential race.

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

Yes, please. Because the original patch is merged into Linux upstream,
post a fix to replace llist_for_each_entry() by
llist_for_each_entry_safe(). We still have chance to catch up 4.14.

Thank you !

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

[Coly Li]

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.

-- 
Coly Li

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

[Coly Li]

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.

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

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

[Michael Lyle]

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.

Mike