Re: [PATCH] rtc: test: cancel the alarm timer on remove

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rtc: test: timer_delete_sync() in test_remove() races with alarm re-arming
--

commit 4195cfecb06de73a63ad94418a4fd408e93e1c16
Author: mimo-4@ilands.app <mimo-4@ilands.app>

rtc: test: cancel the alarm timer on remove

This commit adds a platform driver remove callback to cancel the emulated
alarm timer when the rtc-test device is removed.

> diff --git a/drivers/rtc/rtc-test.c b/drivers/rtc/rtc-test.c
> index 94f995febe5b4..1095fa2de4fb1 100644
> --- a/drivers/rtc/rtc-test.c
> +++ b/drivers/rtc/rtc-test.c
> @@ -141,8 +141,16 @@ static int test_probe(struct platform_device *plat_dev)
>  	return devm_rtc_register_device(rtd->rtc);
>  }
>  
> +static void test_remove(struct platform_device *plat_dev)
> +{
> +	struct rtc_test_data *rtd = platform_get_drvdata(plat_dev);
> +
> +	timer_delete_sync(&rtd->alarm);
> +}

[Severity: High]
Can rtd->alarm be re-armed after timer_delete_sync() returns in
test_remove()?

In test_probe(), the RTC device is registered via
devm_rtc_register_device(rtd->rtc), which queues devm_rtc_unregister_device()
as a devres action. During driver detach, device_remove() runs test_remove()
before device_unbind_cleanup() runs devres_release_all():

__device_release_driver()
  device_remove()
    test_remove()
      timer_delete_sync(&rtd->alarm);
  device_unbind_cleanup()
    devres_release_all()
      devm_rtc_unregister_device()

When test_remove() calls timer_delete_sync(&rtd->alarm),
devm_rtc_unregister_device() has not yet cleared rtd->rtc->ops or removed the
character device and sysfs attributes:

drivers/rtc/class.c:devm_rtc_unregister_device() {
	mutex_lock(&rtc->ops_lock);
	rtc_proc_del_device(rtc);
	if (!test_bit(RTC_NO_CDEV, &rtc->flags))
		cdev_device_del(&rtc->char_dev, &rtc->dev);
	rtc->ops = NULL;
	mutex_unlock(&rtc->ops_lock);
}

In addition, if test_rtc_alarm_handler() fired just before
timer_delete_sync(), rtc_update_irq() schedules rtd->rtc->irqwork
(rtc_timer_do_work()), which can still be pending or running and call
__rtc_set_alarm() -> test_rtc_set_alarm():

drivers/rtc/rtc-test.c:test_rtc_set_alarm() {
	...
	if (alrm->enabled)
		add_timer(&rtd->alarm);
	...
}

Because timer_delete_sync() does not prevent the timer from being re-armed, a
concurrent ioctl on /dev/rtcN, a write to sysfs wakealarm, or
rtc_timer_do_work() reprogramming an alarm before devm_rtc_unregister_device()
runs can call test_rtc_set_alarm() or test_rtc_alarm_irq_enable() and re-queue
rtd->alarm just before devres frees rtd.

Would registering the timer cleanup via devm_add_action_or_reset() before
devm_rtc_register_device(rtd->rtc), or using timer_shutdown_sync(), avoid
this race?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/010101a1ab5812b4-c8329a38-bb41-40db-99e9-b4661c9c7c66-000000@us-west-2.amazonses.com?part=1