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@xxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxx> > > > > > > Hi, > > > > On Sat, Aug 23, 2025
at 1:56 AM Tejun Heo <tj@xxxxxxxxxx> 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@xxxxxxxxxxxxx> > > > >
Thanks,
--
Julian Sun <sunjunchao@xxxxxxxxxxxxx>
