| Message ID | 20221128033057.1279383-1-longman@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCH-block] blk-cgroup: Use css_tryget() in blkcg_destroy_blkgs() | expand |
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
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?
On 11/28/22 09:06, Michal Koutný wrote: > 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? 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
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
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.
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@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. You are right. blkcg_destroy_blkgs() shouldn't be called with all the blkcg references gone. Will work on a revised patch. Cheers, Longman
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.
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...
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
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.
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.
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,
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(-)