RE: [PATCH] fs/resctrl: Fix use-after-free in resctrl_offline_mon_domain()

[Luck, Tony]

> > ... 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?

I've got a couple of AI's spinning now. I'll resume context for the one that offered the v1
patch and ask it what it thinks of this new version.

>
> 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 ..."

Scope increased from just the use-after-free when the domain was deleted. The case
for taking the current worker CPU offline doesn't involve a use-after-free. It just results
in running the workier on the wrong CPU for one iteration.

Deleting the work_struct inside the called function is different from some agent deleting
the work_struct while the worker is running.

>
> >
> > I'll look at fixing the cqm_limbo path in the same style.

-Tony