Re: [v6,2/7] ptp: vmclock: support device notifications

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3813 bytes --]

On Thu, 2026-01-22 at 19:11 -0800, Jakub Kicinski wrote:
> 
> > +	status = acpi_install_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY,
> > +					     vmclock_acpi_notification_handler,
> > +					     dev);
> 
> Since acpi_install_notify_handler() is called here, should there be a
> corresponding acpi_remove_notify_handler() in vmclock_remove()? Currently
> the handler remains registered after device removal, which could lead to
> a use-after-free if the hypervisor sends a notification after the driver
> state has been freed.
> 
> [ ... ]
> 
> > @@ -549,6 +653,11 @@ static int vmclock_probe(struct platform_device *pdev)
> >   	if (ret)
> >   		return ret;
> > 
> > +	init_waitqueue_head(&st->disrupt_wait);
> > +	ret = vmclock_setup_notification(dev, st);
> > +	if (ret)
> > +		return ret;
> 
> [ ... ]
> 
> > @@ -581,6 +690,8 @@ static int vmclock_probe(struct platform_device *pdev)
> >   		return -ENODEV;
> >   	}
> > 
> > +	dev->driver_data = st;
> 
> Is there a race window here? The ACPI notification handler is installed
> in vmclock_setup_notification() which runs earlier in vmclock_probe(),
> but the handler accesses device->driver_data. If the hypervisor sends a
> notification between vmclock_setup_notification() and this assignment,
> vmclock_acpi_notification_handler() would dereference NULL or garbage.
> 
> I see this is fixed later in the series by commit 47fdd294385e which
> moves dev->driver_data = st before vmclock_setup_notification().

With the incremental diff below, I've fixed both of these in
https://git.infradead.org/?p=users/dwmw2/linux.git;a=shortlog;h=refs/heads/vmclock

Takahiro and Babis have the test environment set up to actually
exercise the notifications, so I'll let one of them actually do the
testing and repost v7.

Takahiro, please remember to add your own 'Signed-off-by:' as you do.

Thanks!


--- a/drivers/ptp/ptp_vmclock.c
+++ b/drivers/ptp/ptp_vmclock.c
@@ -490,17 +490,6 @@ static const struct file_operations vmclock_miscdev_fops = {
 
 /* module operations */
 
-static void vmclock_remove(void *data)
-{
-	struct vmclock_state *st = data;
-
-	if (st->ptp_clock)
-		ptp_clock_unregister(st->ptp_clock);
-
-	if (st->miscdev.minor != MISC_DYNAMIC_MINOR)
-		misc_deregister(&st->miscdev);
-}
-
 #if IS_ENABLED(CONFIG_ACPI)
 static acpi_status vmclock_acpi_resources(struct acpi_resource *ares, void *data)
 {
@@ -636,6 +625,32 @@ static int vmclock_setup_notification(struct device *dev,
 	return vmclock_setup_of_notification(dev);
 }
 
+static void vmclock_remove(void *data)
+{
+	struct device *dev = data;
+	struct vmclock_state *st = dev->driver_data;
+
+	if (!st) {
+		dev_err(dev, "vmclock_remove() called with NULL driver_data");
+		return;
+	}
+
+#if IS_ENABLED(CONFIG_ACPI)
+	if (has_acpi_companion(dev))
+		acpi_remove_notify_handler(ACPI_COMPANION(dev)->handle,
+					   ACPI_DEVICE_NOTIFY,
+					   vmclock_acpi_notification_handler);
+#endif
+
+	if (st->ptp_clock)
+		ptp_clock_unregister(st->ptp_clock);
+
+	if (st->miscdev.minor != MISC_DYNAMIC_MINOR)
+		misc_deregister(&st->miscdev);
+
+	dev->driver_data = NULL;
+}
+
 static void vmclock_put_idx(void *data)
 {
 	struct vmclock_state *st = data;
@@ -701,12 +716,13 @@ static int vmclock_probe(struct platform_device *pdev)
 
 	st->miscdev.minor = MISC_DYNAMIC_MINOR;
 
-	ret = devm_add_action_or_reset(&pdev->dev, vmclock_remove, st);
+	init_waitqueue_head(&st->disrupt_wait);
+	dev->driver_data = st;
+
+	ret = devm_add_action_or_reset(&pdev->dev, vmclock_remove, dev);
 	if (ret)
 		return ret;
 
-	init_waitqueue_head(&st->disrupt_wait);
-	dev->driver_data = st;
 	ret = vmclock_setup_notification(dev, st);
 	if (ret)
 		return ret;



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]