[PATCH-block] blk-cgroup: Use css_tryget() in blkcg_destroy_blkgs()

[PATCH-block] blk-cgroup: Use css_tryget() in blkcg_destroy_blkgs()

[Waiman Long]

Commit 951d1e94801f ("blk-cgroup: Flush stats at blkgs destruction
path") incorrectly assumes that css_get() will always succeed. That may
not be true if there is no blkg associated with the blkcg. If css_get()
fails, the subsequent css_put() call may lead to data corruption as
was illustrated in a test system that it crashed on bootup when that
commit was included. Also blkcg may be freed at any time leading to
use-after-free. Fix it by using css_tryget() instead and bail out if
the tryget fails.

Fixes: 951d1e94801f ("blk-cgroup: Flush stats at blkgs destruction path")
Reported-by: Yi Zhang <yi.zhang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 block/blk-cgroup.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 57941d2a8ba3..74fefc8cbcdf 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1088,7 +1088,12 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
 
 	might_sleep();
 
-	css_get(&blkcg->css);
+	/*
+	 * If css_tryget() fails, there is no blkg to destroy.
+	 */
+	if (!css_tryget(&blkcg->css))
+		return;
+
 	spin_lock_irq(&blkcg->lock);
 	while (!hlist_empty(&blkcg->blkg_list)) {
 		struct blkcg_gq *blkg = hlist_entry(blkcg->blkg_list.first,
-- 
2.31.1

Re: [PATCH-block] blk-cgroup: Use css_tryget() in blkcg_destroy_blkgs()

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

Hello.

On Sun, Nov 27, 2022 at 10:30:57PM -0500, Waiman Long <longman@redhat.com> wrote:
> That may not be true if there is no blkg associated with the blkcg. If
> css_get() fails, the subsequent css_put() call may lead to data
> corruption as was illustrated in a test system that it crashed on
> bootup when that commit was included.

Do you have a stacktrace of the underflowing css_put() in
blkcg_destroy_blkgs()?

It looks to me slightly as a mistake of the caller site that it passes
struct blkcg * without any references.

By a cursory look, could it be cgwb_release_workfn?

--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -390,11 +390,11 @@ static void cgwb_release_workfn(struct work_struct *work)
        wb_shutdown(wb);

        css_put(wb->memcg_css);
-       css_put(wb->blkcg_css);
        mutex_unlock(&wb->bdi->cgwb_release_mutex);

        /* triggers blkg destruction if no online users left */
        blkcg_unpin_online(wb->blkcg_css);
+       css_put(wb->blkcg_css);

        fprop_local_destroy_percpu(&wb->memcg_completions);

Does your crash involve this stack?

Thanks,
Michal

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH-block] blk-cgroup: Use css_tryget() in blkcg_destroy_blkgs()

[Waiman Long]

On 11/28/22 09:06, Michal Koutný wrote:
> Hello.
>
> On Sun, Nov 27, 2022 at 10:30:57PM -0500, Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> That may not be true if there is no blkg associated with the blkcg. If
>> css_get() fails, the subsequent css_put() call may lead to data
>> corruption as was illustrated in a test system that it crashed on
>> bootup when that commit was included.
> Do you have a stacktrace of the underflowing css_put() in
> blkcg_destroy_blkgs()?
>
> It looks to me slightly as a mistake of the caller site that it passes
> struct blkcg * without any references.
>
> By a cursory look, could it be cgwb_release_workfn?
>
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -390,11 +390,11 @@ static void cgwb_release_workfn(struct work_struct *work)
>          wb_shutdown(wb);
>
>          css_put(wb->memcg_css);
> -       css_put(wb->blkcg_css);
>          mutex_unlock(&wb->bdi->cgwb_release_mutex);
>
>          /* triggers blkg destruction if no online users left */
>          blkcg_unpin_online(wb->blkcg_css);
> +       css_put(wb->blkcg_css);
>
>          fprop_local_destroy_percpu(&wb->memcg_completions);
>
> Does your crash involve this stack?

That looks like a possible cause for the system crash that we are 
seeing. In my testing, I do see one case out of more than a dozen calls 
to blkcg_destroy_blkgs() where css_tryget() fail in the reproducing 
system during bootup. However, I didn't force a stack dump at that point 
and so I am not sure if that is the place, though it looks likely. One 
of the crashes that was reported does involve blkcg_unpin_online(). So 
maybe that is it.

Cheers,
Longman

Re: [PATCH-block] blk-cgroup: Use css_tryget() in blkcg_destroy_blkgs()

[Jens Axboe]

On 11/27/22 8:30 PM, Waiman Long wrote:
> Commit 951d1e94801f ("blk-cgroup: Flush stats at blkgs destruction
> path") incorrectly assumes that css_get() will always succeed. That may
> not be true if there is no blkg associated with the blkcg. If css_get()
> fails, the subsequent css_put() call may lead to data corruption as
> was illustrated in a test system that it crashed on bootup when that
> commit was included. Also blkcg may be freed at any time leading to
> use-after-free. Fix it by using css_tryget() instead and bail out if
> the tryget fails.
> 
> Fixes: 951d1e94801f ("blk-cgroup: Flush stats at blkgs destruction path")
> Reported-by: Yi Zhang <yi.zhang@redhat.com>
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  block/blk-cgroup.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 57941d2a8ba3..74fefc8cbcdf 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -1088,7 +1088,12 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
>  
>  	might_sleep();
>  
> -	css_get(&blkcg->css);
> +	/*
> +	 * If css_tryget() fails, there is no blkg to destroy.
> +	 */
> +	if (!css_tryget(&blkcg->css))
> +		return;
> +
>  	spin_lock_irq(&blkcg->lock);
>  	while (!hlist_empty(&blkcg->blkg_list)) {
>  		struct blkcg_gq *blkg = hlist_entry(blkcg->blkg_list.first,

This doesn't seem safe to me, but maybe I'm missing something. A tryget
operation can be fine if we're under RCU lock and the entity is freed
appropriately, but what makes it safe here? Could blkcg already be gone
at this point?

-- 
Jens Axboe

Re: [PATCH-block] blk-cgroup: Use css_tryget() in blkcg_destroy_blkgs()

[Waiman Long]

On 11/28/22 09:14, Jens Axboe wrote:
> On 11/27/22 8:30 PM, Waiman Long wrote:
>> Commit 951d1e94801f ("blk-cgroup: Flush stats at blkgs destruction
>> path") incorrectly assumes that css_get() will always succeed. That may
>> not be true if there is no blkg associated with the blkcg. If css_get()
>> fails, the subsequent css_put() call may lead to data corruption as
>> was illustrated in a test system that it crashed on bootup when that
>> commit was included. Also blkcg may be freed at any time leading to
>> use-after-free. Fix it by using css_tryget() instead and bail out if
>> the tryget fails.
>>
>> Fixes: 951d1e94801f ("blk-cgroup: Flush stats at blkgs destruction path")
>> Reported-by: Yi Zhang <yi.zhang@redhat.com>
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>>   block/blk-cgroup.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>> index 57941d2a8ba3..74fefc8cbcdf 100644
>> --- a/block/blk-cgroup.c
>> +++ b/block/blk-cgroup.c
>> @@ -1088,7 +1088,12 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
>>   
>>   	might_sleep();
>>   
>> -	css_get(&blkcg->css);
>> +	/*
>> +	 * If css_tryget() fails, there is no blkg to destroy.
>> +	 */
>> +	if (!css_tryget(&blkcg->css))
>> +		return;
>> +
>>   	spin_lock_irq(&blkcg->lock);
>>   	while (!hlist_empty(&blkcg->blkg_list)) {
>>   		struct blkcg_gq *blkg = hlist_entry(blkcg->blkg_list.first,
> This doesn't seem safe to me, but maybe I'm missing something. A tryget
> operation can be fine if we're under RCU lock and the entity is freed
> appropriately, but what makes it safe here? Could blkcg already be gone
> at this point?

The actual freeing of the blkcg structure is under RCU protection. So 
the structure won't be freed immediately even if css_tryget() fails. I 
suspect what Michal found may be the root cause of this problem. If so, 
this is an existing bug which gets exposed by my patch.

Cheers,
Longman

Re: [PATCH-block] blk-cgroup: Use css_tryget() in blkcg_destroy_blkgs()

[Jens Axboe]

On 11/28/22 8:38?AM, Waiman Long wrote:
> On 11/28/22 09:14, Jens Axboe wrote:
>> On 11/27/22 8:30?PM, Waiman Long wrote:
>>> Commit 951d1e94801f ("blk-cgroup: Flush stats at blkgs destruction
>>> path") incorrectly assumes that css_get() will always succeed. That may
>>> not be true if there is no blkg associated with the blkcg. If css_get()
>>> fails, the subsequent css_put() call may lead to data corruption as
>>> was illustrated in a test system that it crashed on bootup when that
>>> commit was included. Also blkcg may be freed at any time leading to
>>> use-after-free. Fix it by using css_tryget() instead and bail out if
>>> the tryget fails.
>>>
>>> Fixes: 951d1e94801f ("blk-cgroup: Flush stats at blkgs destruction path")
>>> Reported-by: Yi Zhang <yi.zhang@redhat.com>
>>> Signed-off-by: Waiman Long <longman@redhat.com>
>>> ---
>>> ? block/blk-cgroup.c | 7 ++++++-
>>> ? 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>>> index 57941d2a8ba3..74fefc8cbcdf 100644
>>> --- a/block/blk-cgroup.c
>>> +++ b/block/blk-cgroup.c
>>> @@ -1088,7 +1088,12 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
>>> ? ????? might_sleep();
>>> ? -??? css_get(&blkcg->css);
>>> +??? /*
>>> +???? * If css_tryget() fails, there is no blkg to destroy.
>>> +???? */
>>> +??? if (!css_tryget(&blkcg->css))
>>> +??????? return;
>>> +
>>> ????? spin_lock_irq(&blkcg->lock);
>>> ????? while (!hlist_empty(&blkcg->blkg_list)) {
>>> ????????? struct blkcg_gq *blkg = hlist_entry(blkcg->blkg_list.first,
>> This doesn't seem safe to me, but maybe I'm missing something. A tryget
>> operation can be fine if we're under RCU lock and the entity is freed
>> appropriately, but what makes it safe here? Could blkcg already be gone
>> at this point?
> 
> The actual freeing of the blkcg structure is under RCU protection. So
> the structure won't be freed immediately even if css_tryget() fails. I
> suspect what Michal found may be the root cause of this problem. If
> so, this is an existing bug which gets exposed by my patch.

But what prevents it from going away here since you're not under RCU
lock for the tryget? Doesn't help that the freeing side is done in an
RCU safe manner, if the ref attempt is not.

-- 
Jens Axboe

Re: [PATCH-block] blk-cgroup: Use css_tryget() in blkcg_destroy_blkgs()

[Waiman Long]

On 11/28/22 10:42, Jens Axboe wrote:
> On 11/28/22 8:38?AM, Waiman Long wrote:
>> On 11/28/22 09:14, Jens Axboe wrote:
>>> On 11/27/22 8:30?PM, Waiman Long wrote:
>>>> Commit 951d1e94801f ("blk-cgroup: Flush stats at blkgs destruction
>>>> path") incorrectly assumes that css_get() will always succeed. That may
>>>> not be true if there is no blkg associated with the blkcg. If css_get()
>>>> fails, the subsequent css_put() call may lead to data corruption as
>>>> was illustrated in a test system that it crashed on bootup when that
>>>> commit was included. Also blkcg may be freed at any time leading to
>>>> use-after-free. Fix it by using css_tryget() instead and bail out if
>>>> the tryget fails.
>>>>
>>>> Fixes: 951d1e94801f ("blk-cgroup: Flush stats at blkgs destruction path")
>>>> Reported-by: Yi Zhang <yi.zhang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>> Signed-off-by: Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>> ? block/blk-cgroup.c | 7 ++++++-
>>>> ? 1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>>>> index 57941d2a8ba3..74fefc8cbcdf 100644
>>>> --- a/block/blk-cgroup.c
>>>> +++ b/block/blk-cgroup.c
>>>> @@ -1088,7 +1088,12 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
>>>> ? ????? might_sleep();
>>>> ? -??? css_get(&blkcg->css);
>>>> +??? /*
>>>> +???? * If css_tryget() fails, there is no blkg to destroy.
>>>> +???? */
>>>> +??? if (!css_tryget(&blkcg->css))
>>>> +??????? return;
>>>> +
>>>> ????? spin_lock_irq(&blkcg->lock);
>>>> ????? while (!hlist_empty(&blkcg->blkg_list)) {
>>>> ????????? struct blkcg_gq *blkg = hlist_entry(blkcg->blkg_list.first,
>>> This doesn't seem safe to me, but maybe I'm missing something. A tryget
>>> operation can be fine if we're under RCU lock and the entity is freed
>>> appropriately, but what makes it safe here? Could blkcg already be gone
>>> at this point?
>> The actual freeing of the blkcg structure is under RCU protection. So
>> the structure won't be freed immediately even if css_tryget() fails. I
>> suspect what Michal found may be the root cause of this problem. If
>> so, this is an existing bug which gets exposed by my patch.
> But what prevents it from going away here since you're not under RCU
> lock for the tryget? Doesn't help that the freeing side is done in an
> RCU safe manner, if the ref attempt is not.

You are right. blkcg_destroy_blkgs() shouldn't be called with all the 
blkcg references gone. Will work on a revised patch.

Cheers,
Longman

Re: [PATCH-block] blk-cgroup: Use css_tryget() in blkcg_destroy_blkgs()

[Bart Van Assche]

On 11/27/22 19:30, Waiman Long wrote:
> Fixes: 951d1e94801f ("blk-cgroup: Flush stats at blkgs destruction path")

Has Jens' for-next branch perhaps been rebased? I see the following 
commit ID for that patch:

dae590a6c96c ("blk-cgroup: Flush stats at blkgs destruction path")

Thanks,

Bart.

Re: [PATCH-block] blk-cgroup: Use css_tryget() in blkcg_destroy_blkgs()

[Jens Axboe]

On 11/28/22 11:56 AM, Bart Van Assche wrote:
> On 11/27/22 19:30, Waiman Long wrote:
>> Fixes: 951d1e94801f ("blk-cgroup: Flush stats at blkgs destruction path")
> 
> Has Jens' for-next branch perhaps been rebased? I see the following commit ID for that patch:
> 
> dae590a6c96c ("blk-cgroup: Flush stats at blkgs destruction path")

I don't know that sha is from, not from me. for-6.2/block has not been
rebased, for-next gets rebased whenever I need to do so as linux-next is
continually rebased anyway. But the sha for that commit would not change
as a result.

I don't even have that sha in my tree, so...

-- 
Jens Axboe

Re: [PATCH-block] blk-cgroup: Use css_tryget() in blkcg_destroy_blkgs()

[Andy Shevchenko]

On Mon, Nov 28, 2022 at 12:00:55PM -0700, Jens Axboe wrote:
> On 11/28/22 11:56 AM, Bart Van Assche wrote:
> > On 11/27/22 19:30, Waiman Long wrote:
> >> Fixes: 951d1e94801f ("blk-cgroup: Flush stats at blkgs destruction path")
> > 
> > Has Jens' for-next branch perhaps been rebased? I see the following commit ID for that patch:
> > 
> > dae590a6c96c ("blk-cgroup: Flush stats at blkgs destruction path")
> 
> I don't know that sha is from, not from me. for-6.2/block has not been
> rebased, for-next gets rebased whenever I need to do so as linux-next is
> continually rebased anyway. But the sha for that commit would not change
> as a result.
> 
> I don't even have that sha in my tree, so...

$ git tag --contains dae590a6c96c
next-20221117
next-20221118
next-20221121
next-20221122
next-20221123
next-20221124
next-20221125
next-20221128

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH-block] blk-cgroup: Use css_tryget() in blkcg_destroy_blkgs()

[Jens Axboe]

On 11/28/22 12:07 PM, Andy Shevchenko wrote:
> On Mon, Nov 28, 2022 at 12:00:55PM -0700, Jens Axboe wrote:
>> On 11/28/22 11:56 AM, Bart Van Assche wrote:
>>> On 11/27/22 19:30, Waiman Long wrote:
>>>> Fixes: 951d1e94801f ("blk-cgroup: Flush stats at blkgs destruction path")
>>>
>>> Has Jens' for-next branch perhaps been rebased? I see the following commit ID for that patch:
>>>
>>> dae590a6c96c ("blk-cgroup: Flush stats at blkgs destruction path")
>>
>> I don't know that sha is from, not from me. for-6.2/block has not been
>> rebased, for-next gets rebased whenever I need to do so as linux-next is
>> continually rebased anyway. But the sha for that commit would not change
>> as a result.
>>
>> I don't even have that sha in my tree, so...
> 
> $ git tag --contains dae590a6c96c
> next-20221117
> next-20221118
> next-20221121
> next-20221122
> next-20221123
> next-20221124
> next-20221125
> next-20221128

That is the right sha, I'm talking about the fixes line in the
patch you're replying to:

Fixes: 951d1e94801f ("blk-cgroup: Flush stats at blkgs destruction path")

which is certainly not from my tree.

-- 
Jens Axboe

Re: [PATCH-block] blk-cgroup: Use css_tryget() in blkcg_destroy_blkgs()

[Andy Shevchenko]

On Mon, Nov 28, 2022 at 12:08:47PM -0700, Jens Axboe wrote:
> On 11/28/22 12:07 PM, Andy Shevchenko wrote:
> > On Mon, Nov 28, 2022 at 12:00:55PM -0700, Jens Axboe wrote:
> >> On 11/28/22 11:56 AM, Bart Van Assche wrote:
> >>> On 11/27/22 19:30, Waiman Long wrote:
> >>>> Fixes: 951d1e94801f ("blk-cgroup: Flush stats at blkgs destruction path")
> >>>
> >>> Has Jens' for-next branch perhaps been rebased? I see the following commit ID for that patch:
> >>>
> >>> dae590a6c96c ("blk-cgroup: Flush stats at blkgs destruction path")
> >>
> >> I don't know that sha is from, not from me. for-6.2/block has not been
> >> rebased, for-next gets rebased whenever I need to do so as linux-next is
> >> continually rebased anyway. But the sha for that commit would not change
> >> as a result.
> >>
> >> I don't even have that sha in my tree, so...
> > 
> > $ git tag --contains dae590a6c96c
> > next-20221117
> > next-20221118
> > next-20221121
> > next-20221122
> > next-20221123
> > next-20221124
> > next-20221125
> > next-20221128
> 
> That is the right sha, I'm talking about the fixes line in the
> patch you're replying to:
> 
> Fixes: 951d1e94801f ("blk-cgroup: Flush stats at blkgs destruction path")
> 
> which is certainly not from my tree.

Ah, I see. That one is local / wrong. I don't see it either.

-- 
With Best Regards,
Andy Shevchenko