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

["Luck, Tony" <tony.luck@intel.com>]

On Wed, May 06, 2026 at 08:42:09PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 5/6/26 4:14 PM, Luck, Tony wrote:
> >>>> 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.
> >>
> >> Right. I interpret this to mean that judging the safety of work_struct removal should consider not
> >> only the workqueue API itself but also external agents that may access the work_struct after its
> >> removal. The current fix addresses access to removed work_struct from within worker itself while I
> >> interpret the workqueue API to guarantee that there will be no access to work_struct during or
> >> after worker execution. The fix under development thus makes it possible to safely remove the
> >> domain even if a worker belonging to it is executing and blocked on cpus_read_lock(). Do you
> >> see any remaining issues here?
> > 
> > OK. I'll add something to the commit message.
> > 
> > I asked my original AI about this fix. It claimed to find problems relating to kernel using the work_struct
> > after return from the function. Pasting in that comment you gave me from process_one_work() about
> > it being OK to free the work_struct made it reconsider and retract.
> > 
> > Another AI (using a copy of the sashiko rules) has found an issue with our reliance on is_percpu_thread()
> > 
> > The problem is the ordering of hotplug callbacks.
> > 
> > resctrl_arch_offline_cpu() runs early because it is in the CPUHP_AP_ONLINE_DYN class. AI claims
> > that cpus_write_lock() is released after running this, but before running workqueue_offline_cpu() in the
> > CPUHP_AP_WORKQUEUE_ONLINE class.
> > 
> > So our worker may obtain cpus_read_lock() and not yet lost its_percpu_thread() status.
> 
> Your message is not clear to me. Do you agree with AI here and thus claim that there remains an issue? 

I was suspicious about the AI claim, but I should have dug into this
before bothering you. Your analysis and tracing experiments below make
it clear that this AI hallucinated this issue.

> Are you suggesting that the original race explained in
> https://lore.kernel.org/lkml/afoesuWB8RezVLrN@agluck-desk3/ is not accurate? 
> 
> I am not able to see how CPU hotplug write lock is released in the middle of all the AP cleanup
> handlers. When looking at _cpu_down() I see:
> 
> 	_cpu_down()
> 	{
> 		...
> 		cpus_write_lock();
> 
> 		/*
> 		 * Run all the AP handlers on CPU going down - this includes
> 		 * everything > CPUHP_TEARDOWN_CPU that includes CPUHP_AP_WORKQUEUE_ONLINE
> 		 * and CPUHP_AP_ONLINE_DYN.
> 		 */
> 
> 		/* 
> 		 * Run rest of cleanups on other CPU
> 		 */
> 
> 		cpus_write_unlock();
> 	}
> 
> You claim that cpus_write_lock() is dropped in this flow.
> 
> To test this I enabled tracing and see the following when offlining CPU #38 running the
> overflow handler:
> 
> offline triggered on CPU#1 and it takes CPU hotplug write lock
>   1)               |  _cpu_down() {
>   1)               |    percpu_down_write() {  <<<<<<<<<<======== CPU hotplug write lock acquired here
>   1) # 9155.999 us |    }
>   1)               |  /* cpuhp_enter: cpu: 0038 target: 144 step: 236 (cpuhp_kick_ap_work) */
> 
> ...
> executing moves to CPU being offlined (#38) from where the different AP offline callbacks are called:
>  38)               |  cpuhp_thread_fun() {
>  38)               |  /* cpuhp_enter: cpu: 0038 target: 144 step: 235 (sched_cpu_deactivate) */
>  38)               |  /* cpuhp_exit:  cpu: 0038  state: 234 step: 235 ret: 0 */
>  38) * 20632.54 us |  }
>  38)               |  cpuhp_thread_fun() {
>  38)               |  /* cpuhp_enter: cpu: 0038 target: 144 step: 214 (rapl_cpu_down_prep [intel_rapl_msr]) */
>  38)               |  /* cpuhp_exit:  cpu: 0038  state: 213 step: 214 ret: 0 */
>  38)   3.171 us    |  }
>  38)               |  cpuhp_thread_fun() {
>  38)               |  /* cpuhp_enter: cpu: 0038 target: 144 step: 213 (pkg_thermal_cpu_offline [x86_pkg_temp_thermal]) */
>  38)               |  /* cpuhp_exit:  cpu: 0038  state: 212 step: 213 ret: 0 */
>  38)   2.378 us    |  }
> 
> ... this includes resctrl ...
>  38)               |  cpuhp_thread_fun() {
>  38)               |  /* cpuhp_enter: cpu: 0038 target: 144 step: 209 (resctrl_arch_offline_cpu) */
>  38)               |    resctrl_arch_offline_cpu() {
>  38)               |      resctrl_offline_cpu() {
>  38)               |        /* workqueue_queue_work: work struct=00000000ed014eff function=mbm_handle_overflow workqueue=events req_cpu=39 cpu=39 */
>  38) # 5920.866 us |      }
>  38) # 5927.396 us |    }
>  38)               |  /* cpuhp_exit:  cpu: 0038  state: 208 step: 209 ret: 0 */
>  38) # 5929.182 us |  }
> 
> ... and the workqueues ...
>  38)               |  cpuhp_thread_fun() {
>  38)               |  /* cpuhp_enter: cpu: 0038 target: 144 step: 187 (workqueue_offline_cpu) */
>  38)               |    workqueue_offline_cpu() {
>  38)   3.724 us    |      unbind_worker();
>  38)   2.312 us    |      unbind_worker();
>  38)   1.701 us    |      unbind_worker();
>  38)   1.681 us    |      unbind_worker();
>  38) ! 226.852 us  |    }
>  38)               |  /* cpuhp_exit:  cpu: 0038  state: 186 step: 187 ret: 0 */
>  38) ! 229.393 us  |  }
> 
> ....
> eventually this all finishes and _cpu_down() completes, releasing the CPU hotplug write lock:
> 
>  73)               |  /* cpuhp_exit:  cpu: 0038  state:   6 step:   7 ret: 0 */
>  73)               |  /* cpuhp_enter: cpu: 0038 target:   0 step:   2 (x86_pmu_dead_cpu) */
>  73)               |  /* cpuhp_exit:  cpu: 0038  state:   1 step:   2 ret: 0 */
>  73)   5.038 us    |    percpu_up_write(); <<<<<<<<<<======== CPU hotplug write lock released here
>  73)               |    cpus_read_lock() {
>  73)   0.474 us    |      __percpu_down_read();
>  73)   1.420 us    |    }
>  73) * 62023.47 us |  } /* _cpu_down */
> 
> In the trace that included all CPUs I only see one instance of percpu_down_write() called when
> _cpu_down() starts and one instance of percpu_up_write() when _cpu_down() exits.
> 
> You claim that CPU hotplug write lock is released before workqueue_offline_cpu() is called.
> I am not able to verify this by looking at the code nor the traces generated when offlining a CPU.
> Could you please help me understand your claim?

As above - AI hallucination. I need to be less trusting of AI pronouncements. My
experience so far has been that AI is right often enough to instill a false
sense of confidence in content produced by AI. I need to set expectations
much lower and spend more time checking.
> 
> Reinette

I'll finish tidying up and post next version. I've got it set with you
as author (with a commented out Signed-off-by). I've applied a
Co-developed-by tag for myself.

If there are just minor issues with commit message, then perhaps you
should just fix and post a "final" version for upstream consideration
since you provided all the key components of this patch?

-Tony