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

[Giorgi Tchankvetadze <giorgitchankvetadze1997@gmail.com>]

Makes sense, yeah. What about using a shared, long-lived completion 
object (done_acc) inside cgwb_frn. All writeback jobs point to this same 
object via work->done = &frn->done_acc.

On 8/23/2025 12:23 PM, Julian Sun wrote:
> 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>
>