[PATCH net] ptp: Report clock removal to poll waiters

[PATCH net] ptp: Report clock removal to poll waiters

[Avi Weiss]

ptp_clock_unregister() marks the clock defunct and wake
timestamp waiters before posix_clock_unregister() marks
it inactive. A poll waiter can consume the removal wakeup
while the event queue is empty, return no events, and remain
blocked until timeout after device removal.

Identified while reviewing the PTP polling and removal paths.
Using netdevsim's mock PTP clock in an x86_64 QEMU/KVM guest,
the unpatched kernel missed 8 of 12 removal notifications,
while the patched kernel reported all 12. No kernel
instrumentation or forced fault injection was required.

W=1, ptp_chardev.o compiled under allyesconfig and
allmodconfig on net/main.

Check the defunct state after registering the poll wait
queue and return EPOLLERR when the clock is being removed.

Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.")
Signed-off-by: Avi Weiss <thnkslprpt@gmail.com>
---
 drivers/ptp/ptp_chardev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index f63b45f5ee6a..01adc70ad97b 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -593,6 +593,9 @@ __poll_t ptp_poll(struct posix_clock_context *pccontext, struct file *fp,
 
 	poll_wait(fp, &ptp->tsev_wq, wait);
 
+	if (READ_ONCE(ptp->defunct))
+		return EPOLLERR;
+
 	return queue_cnt(queue) ? EPOLLIN : 0;
 }
 

base-commit: cb284914e749388eef402938bdb3115fe1f6d337
-- 
2.53.0

Re: [PATCH net] ptp: Report clock removal to poll waiters

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102122340.1160733-1-thnkslprpt@gmail.com?part=1
---
ptp: Report clock removal to poll waiters

The patch adds a check of ptp->defunct to ptp_poll(), placed after
poll_wait() has registered on tsev_wq. ptp_poll() now returns EPOLLERR
when the clock is being removed, so a poll waiter that consumes the
removal wakeup no longer stays blocked until timeout.

> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index f63b45f5ee6ae..01adc70ad97b4 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -593,6 +593,9 @@ __poll_t ptp_poll(struct posix_clock_context *pccontext, struct file *fp,
>
>  	poll_wait(fp, &ptp->tsev_wq, wait);
>
> +	if (READ_ONCE(ptp->defunct))
> +		return EPOLLERR;
> +

[Severity: Low]
This isn't a bug, but should the other accesses to ptp->defunct be
annotated to match this new READ_ONCE()?

The only writer still uses a plain store, so this load has no
WRITE_ONCE() to pair with:

drivers/ptp/ptp_clock.c:ptp_clock_unregister() {
    ...
	ptp->defunct = 1;
	wake_up_interruptible(&ptp->tsev_wq);
    ...
}

The other lockless reader in this file, ptp_read(), still uses plain
loads:

drivers/ptp/ptp_chardev.c:ptp_read() {
    ...
	if (wait_event_interruptible(ptp->tsev_wq, ptp->defunct || queue_cnt(queue)))
		return -ERESTARTSYS;

	if (ptp->defunct)
		return -ENODEV;
    ...
}

After this patch, ptp_poll() and ptp_read() read the same flag in two
different ways. Other code in this subsystem pairs its annotations
explicitly. For example, queue_cnt() in ptp_private.h notes that its
READ_ONCE() is "Paired with WRITE_ONCE() in
enqueue_external_timestamp(), ptp_read(), extts_fifo_show()".

Would it be more consistent to use WRITE_ONCE(ptp->defunct, 1) in
ptp_clock_unregister() and READ_ONCE() in ptp_read()? The other option
is to leave the new check unannotated like the existing ones.

>  	return queue_cnt(queue) ? EPOLLIN : 0;
>  }
-- 
pw-bot: changes-requested