[PATCH] writeback: fix obtain a reference to a freeing memcg css

[PATCH] writeback: fix obtain a reference to a freeing memcg css

[Muchun Song]

The caller of wb_get_create() should pin the memcg, because
wb_get_create() relies on this guarantee. The rcu read lock
only can guarantee that the memcg css returned by css_from_id()
cannot be released, but the reference of the memcg can be zero.
Fix it by holding a reference to the css before calling
wb_get_create(). This is not a problem I encountered in the
real world. Just the result of a code review.

Fixes: 682aa8e1a6a1 ("writeback: implement unlocked_inode_to_wb transaction and use it for stat updates")
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 fs/fs-writeback.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 3ac002561327..afa658ffc09f 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -506,8 +506,10 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)
 	/* find and pin the new wb */
 	rcu_read_lock();
 	memcg_css = css_from_id(new_wb_id, &memory_cgrp_subsys);
-	if (memcg_css)
+	if (memcg_css && css_tryget(memcg_css)) {
 		isw->new_wb = wb_get_create(bdi, memcg_css, GFP_ATOMIC);
+		css_put(memcg_css);
+	}
 	rcu_read_unlock();
 	if (!isw->new_wb)
 		goto out_free;
-- 
2.11.0

Re: [PATCH] writeback: fix obtain a reference to a freeing memcg css

[Matthew Wilcox]

On Tue, Mar 30, 2021 at 05:29:33PM +0800, Muchun Song wrote:
> +++ b/fs/fs-writeback.c
> @@ -506,8 +506,10 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)
>  	/* find and pin the new wb */
>  	rcu_read_lock();
>  	memcg_css = css_from_id(new_wb_id, &memory_cgrp_subsys);
> -	if (memcg_css)
> +	if (memcg_css && css_tryget(memcg_css)) {
>  		isw->new_wb = wb_get_create(bdi, memcg_css, GFP_ATOMIC);
> +		css_put(memcg_css);
> +	}
>  	rcu_read_unlock();
>  	if (!isw->new_wb)
>  		goto out_free;

This seems like an unnecessary use of GFP_ATOMIC.  Why not:

	rcu_read_lock();
	memcg_css = css_from_id(new_wb_id, &memory_cgrp_subsys);
	if (memcg_css && !css_tryget(memcg_css))
		memcg_css = NULL;
	rcu_read_unlock();
	if (!memcg_css)
		goto out_free;
	isw->new_wb = wb_get_create(bdi, memcg_css, GFP_NOIO);
	css_put(memcg_css);
	if (!isw->new_wb)
		goto out_free;

(inode_switch_wbs can't be called in interrupt context because it takes
inode->i_lock, which is not interrupt-safe.  it's not clear to me whether
it is allowed to start IO or do FS reclaim, given where it is in the
I/O path, so i went with GFP_NOIO rather than GFP_KERNEL)

(also there's another use of GFP_ATOMIC in that function, which is
probably wrong)

Re: [External] Re: [PATCH] writeback: fix obtain a reference to a freeing memcg css

[Muchun Song]

On Tue, Mar 30, 2021 at 7:34 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Mar 30, 2021 at 05:29:33PM +0800, Muchun Song wrote:
> > +++ b/fs/fs-writeback.c
> > @@ -506,8 +506,10 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)
> >       /* find and pin the new wb */
> >       rcu_read_lock();
> >       memcg_css = css_from_id(new_wb_id, &memory_cgrp_subsys);
> > -     if (memcg_css)
> > +     if (memcg_css && css_tryget(memcg_css)) {
> >               isw->new_wb = wb_get_create(bdi, memcg_css, GFP_ATOMIC);
> > +             css_put(memcg_css);
> > +     }
> >       rcu_read_unlock();
> >       if (!isw->new_wb)
> >               goto out_free;
>
> This seems like an unnecessary use of GFP_ATOMIC.  Why not:
>
>         rcu_read_lock();
>         memcg_css = css_from_id(new_wb_id, &memory_cgrp_subsys);
>         if (memcg_css && !css_tryget(memcg_css))
>                 memcg_css = NULL;
>         rcu_read_unlock();
>         if (!memcg_css)
>                 goto out_free;
>         isw->new_wb = wb_get_create(bdi, memcg_css, GFP_NOIO);
>         css_put(memcg_css);
>         if (!isw->new_wb)
>                 goto out_free;

Thanks. I will reuse this.

>
> (inode_switch_wbs can't be called in interrupt context because it takes
> inode->i_lock, which is not interrupt-safe.  it's not clear to me whether
> it is allowed to start IO or do FS reclaim, given where it is in the
> I/O path, so i went with GFP_NOIO rather than GFP_KERNEL)
>
> (also there's another use of GFP_ATOMIC in that function, which is
> probably wrong)

Do you mean the allocation of struct inode_switch_wbs_context in
inode_switch_wbs?