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@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>
