Messages in this thread |  | | Date | Wed, 6 May 2026 14:45:16 -0700 | | Subject | Re: [PATCH] fs/resctrl: Fix use-after-free in resctrl_offline_mon_domain() | | From | Reinette Chatre <> |
| |
Hi Tony,
On 5/6/26 12:48 PM, Luck, Tony wrote: > On Wed, May 06, 2026 at 11:24:30AM -0700, Reinette Chatre wrote: > > ... trimmed discussion on how we got here ... > >> schedule_delayed_work_on() will schedule the work but will do so on CPU going >> offline. Does not seem as though schedule_delayed_work_on() should be used at all >> if the worker is currently running. As an alternative, when it finds that it cannot >> cancel the work resctrl can avoid attempting to reschedule the work and instead just >> set rdt_l3_mon_domain::mbm_work_cpu to nr_cpu_ids to signal that this domain needs a >> worker to be scheduled and that to be done by the exiting work. >> >> Combining the previous ideas with the results from experiments I think the following >> may address the problem for MBM overflow handler, not expanded to include limbo handler >> and untested: > > Initial testing seems good. I added a big mdelay() in mbm_handle_overflow() > before cpus_read_lock() to make it easy to hit the case where cancel_delayed_work() > fails. Tested both the "still have remaining CPUs in the domain" and "this is > last cpu" case for both success and fail of cancel_delayed_work().
Thank you very much for the testing.
> > It looks to me that resctrl_offline_cpu() handles this completely and > the additional cancel_delayed_work() calls from resctrl_offline_mon_domain() > aren't needed. > > Do you agree that those can be deleted?
Good catch. I am not able to think of a scenario where this is still needed. The new flow opens up some new scenarios, for example when the last *two* CPUs of a domain go offline while the worker is blocked on cpus_read_lock() and worker not getting opportunity to transition. Even then, when the MBM overflow handling code in resctrl_offline_cpu() is totally skipped for one CPU the cancel_delayed_work() in resctrl_offline_mon_domain() seems unnecessary to me.
Could you perhaps ask the AI agent that assisted with original patch if it can find any corner cases?
Unrelated to this question but may be worth a mention in the fix is that this work focuses and fixes resctrl to not access freed memory from the worker self. To complement this it may be worthwhile to highlight that it is safe for the work_struct self to be deleted while the work is running (but blocked on cpus_read_lock()) based on the following comment from kernel/workqueue.c:process_one_work(): "It is permissible to free the struct work_struct from inside the function that is called from it ..."
> > I'll look at fixing the cqm_limbo path in the same style.
Thank you very much.
Reinette
|  |