Re: [External] Re: [PATCH] memcg: Don't wait writeback completion when release memcg.

[Julian Sun <sunjunchao@bytedance.com>]

Hi,

On Sat, Aug 23, 2025 at 4:08 PM Giorgi Tchankvetadze
<giorgitchankvetadze1997@gmail.com> wrote:
>
> Hi there. Can we fix this by allowing callers to set work->done = NULL
> when no completion is desired?

No, we can't do that. Because cgwb_frn needs to track the state of wb
work by work->done.cnt, if we set work->done = Null, then we can not
know whether the wb work finished or not. See
mem_cgroup_track_foreign_dirty_slowpath() and
mem_cgroup_flush_foreign() for details.

> The already-existing "if (done)" check in finish_writeback_work()
> already provides the necessary protection, so the change is purely
> mechanical.
>
>
>
> On 8/23/2025 10:18 AM, Julian Sun wrote:
> > Hi,
> >
> > On Sat, Aug 23, 2025 at 1:56 AM Tejun Heo <tj@kernel.org> wrote:
> >> > Hello, > > On Fri, Aug 22, 2025 at 04:22:09PM +0800, Julian Sun
> > wrote: > > +struct wb_wait_queue_head { > > + wait_queue_head_t waitq; >
> >  > + wb_wait_wakeup_func_t wb_wakeup_func; > > +}; > > wait_queue_head_t
> > itself already allows overriding the wakeup function. > Please look for
> > init_wait_func() usages in the tree. Hopefully, that should > contain
> > the changes within memcg.
> > Well... Yes, I checked this function before, but it can't do the same
> > thing as in the previous email. There are some differences—please
> > check the code in the last email.
> >
> > First, let's clarify: the key point here is that if we want to remove
> > wb_wait_for_completion() and avoid self-freeing, we must not access
> > "done" in finish_writeback_work(), otherwise it will cause a UAF.
> > However, init_wait_func() can't achieve this. Of course, I also admit
> > that the method in the previous email seems a bit odd.
> >
> > To summarize again, the root causes of the problem here are:
> > 1. When memcg is released, it calls wb_wait_for_completion() to
> > prevent UAF, which is completely unnecessary—cgwb_frn only needs to
> > issue wb work and no need to wait writeback finished.
> > 2. The current finish_writeback_work() will definitely dereference
> > "done", which may lead to UAF.
> >
> > Essentially, cgwb_frn introduces a new scenario where no wake-up is
> > needed. Therefore, we just need to make finish_writeback_work() not
> > dereference "done" and not wake up the waiting thread. However, this
> > cannot keep the modifications within memcg...
> >
> > Please correct me if my understanding is incorrect.
> >> > Thanks. > > -- > tejun
> >
> > Thanks,
> > --
> > Julian Sun <sunjunchao@bytedance.com>
> >
>
>
> > Hi,
> >
> > On Sat, Aug 23, 2025 at 1:56 AM Tejun Heo <tj@kernel.org> wrote:
> >> > Hello, > > On Fri, Aug 22, 2025 at 04:22:09PM +0800, Julian Sun
> > wrote: > > +struct wb_wait_queue_head { > > + wait_queue_head_t waitq; >
> >  > + wb_wait_wakeup_func_t wb_wakeup_func; > > +}; > > wait_queue_head_t
> > itself already allows overriding the wakeup function. > Please look for
> > init_wait_func() usages in the tree. Hopefully, that should > contain
> > the changes within memcg.
> > Well... Yes, I checked this function before, but it can't do the same
> > thing as in the previous email. There are some differences—please
> > check the code in the last email.
> >
> > First, let's clarify: the key point here is that if we want to remove
> > wb_wait_for_completion() and avoid self-freeing, we must not access
> > "done" in finish_writeback_work(), otherwise it will cause a UAF.
> > However, init_wait_func() can't achieve this. Of course, I also admit
> > that the method in the previous email seems a bit odd.
> >
> > To summarize again, the root causes of the problem here are:
> > 1. When memcg is released, it calls wb_wait_for_completion() to
> > prevent UAF, which is completely unnecessary—cgwb_frn only needs to
> > issue wb work and no need to wait writeback finished.
> > 2. The current finish_writeback_work() will definitely dereference
> > "done", which may lead to UAF.
> >
> > Essentially, cgwb_frn introduces a new scenario where no wake-up is
> > needed. Therefore, we just need to make finish_writeback_work() not
> > dereference "done" and not wake up the waiting thread. However, this
> > cannot keep the modifications within memcg...
> >
> > Please correct me if my understanding is incorrect.
> >> > Thanks. > > -- > tejun
> >
> > Thanks,
> > --
> > Julian Sun <sunjunchao@bytedance.com>
> >
>
>

Thanks,
-- 
Julian Sun <sunjunchao@bytedance.com>