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

[Giorgi Tchankvetadze <giorgitchankvetadze1997@gmail.com>]

Hi there. Can we fix this by allowing callers to set work->done = NULL 
when no completion is desired?
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>
>