Re: [PATCH AUTOSEL 6.18-6.1] perf: marvell: Cancel CN10K DDR PMU hrtimer on device remove

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] perf: marvell: premature hrtimer_cancel() in cn10k_ddr_perf_remove()
--

commit ee388a976febdb25de30edee55cec9fcf0a7184a
Author: Geetha sowjanya <gakula@marvell.com>

perf: marvell: Cancel CN10K DDR PMU hrtimer on device remove

This commit cancels the poll hrtimer in cn10k_ddr_perf_remove() so the
timer callback cannot run after the device is unbound and its resources
are freed.

> diff --git a/drivers/perf/marvell_cn10k_ddr_pmu.c b/drivers/perf/marvell_cn10k_ddr_pmu.c
> index 72ac17efd846a..9974b62541f0d 100644
> --- a/drivers/perf/marvell_cn10k_ddr_pmu.c
> +++ b/drivers/perf/marvell_cn10k_ddr_pmu.c
> @@ -1088,6 +1088,12 @@ static void cn10k_ddr_perf_remove(struct platform_device *pdev)
>  {
>  	struct cn10k_ddr_pmu *ddr_pmu = platform_get_drvdata(pdev);
>  
> +	/*
> +	 * Cancel the poll timer before further teardown so the handler
> +	 * cannot run after this function returns.
> +	 */
> +	hrtimer_cancel(&ddr_pmu->hrtimer);
> +
>  	cpuhp_state_remove_instance_nocalls(
>  				CPUHP_AP_PERF_ARM_MARVELL_CN10K_DDR_ONLINE,
>  				&ddr_pmu->node);

[Severity: High]
Can ddr_pmu->hrtimer be re-armed after hrtimer_cancel() returns here?

In cn10k_ddr_perf_remove(), hrtimer_cancel(&ddr_pmu->hrtimer) is called
before cpuhp_state_remove_instance_nocalls() and perf_pmu_unregister():

drivers/perf/marvell_cn10k_ddr_pmu.c:cn10k_ddr_perf_remove() {
	hrtimer_cancel(&ddr_pmu->hrtimer);

	cpuhp_state_remove_instance_nocalls(
				CPUHP_AP_PERF_ARM_MARVELL_CN10K_DDR_ONLINE,
				&ddr_pmu->node);

	perf_pmu_unregister(&ddr_pmu->pmu);
}

Until cpuhp_state_remove_instance_nocalls() removes the hotplug instance
and perf_pmu_unregister() finishes synchronize_srcu() / synchronize_rcu()
and detaches events via pmu_detach_events(), PMU and CPU hotplug callbacks
remain active:

kernel/events/core.c:perf_pmu_unregister() {
	...
	synchronize_srcu(&pmus_srcu);
	synchronize_rcu();
	...
	pmu_detach_events(pmu);
	...
}

During that window, cn10k_ddr_perf_event_add() can still run on another
CPU (via an in-flight perf_event_open(), ioctl(PERF_EVENT_IOC_ENABLE),
perf_rotate_context(), or cn10k_ddr_pmu_offline_cpu() ->
perf_pmu_migrate_context()) and call hrtimer_start() when
pmu->active_events becomes 1:

drivers/perf/marvell_cn10k_ddr_pmu.c:cn10k_ddr_perf_event_add() {
	...
	pmu->active_events++;
	hwc->idx = counter;

	if (pmu->active_events == 1)
		hrtimer_start(&pmu->hrtimer, cn10k_ddr_pmu_timer_period(),
			      HRTIMER_MODE_REL_PINNED);

	if (counter < DDRC_PERF_NUM_GEN_COUNTERS) {
		/* Generic counters, configure event id */
		reg_offset = DDRC_PERF_CFG(p_data->cfg_base, counter);
		ret = ddr_perf_get_event_bitmap(config, &val, pmu);
		if (ret)
			return ret;
	...
}

If ddr_perf_get_event_bitmap() then returns -EINVAL (or if
pmu->active_events was previously skewed by a failed add),
cn10k_ddr_perf_event_add() returns without decrementing pmu->active_events
or stopping pmu->hrtimer. Because event_sched_in() leaves the event in
PERF_EVENT_STATE_INACTIVE on add failure, pmu_detach_events() ->
event_sched_out() never calls cn10k_ddr_perf_event_del() for that event
(and even if other active events are detached, pmu->active_events never
reaches 0 in cn10k_ddr_perf_event_del()).

This leaves ddr_pmu->hrtimer armed when cn10k_ddr_perf_remove() returns,
allowing cn10k_ddr_pmu_timer_handler() to run after devm frees struct
cn10k_ddr_pmu and unmaps ddr_pmu->base.

Should hrtimer_cancel(&ddr_pmu->hrtimer) be called after
perf_pmu_unregister() and cpuhp_state_remove_instance_nocalls() have
completed (matching the reverse of the setup order in
cn10k_ddr_perf_probe())?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-185-sashal@kernel.org?part=1