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

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

[Avi Weiss]

ptp_clock_unregister() marks the clock defunct and wakes timestamp
waiters before posix_clock_unregister() marks it a zombie. 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.

Using netdevsim's mock PTP clock in an x86_64 QEMU/KVM guest,
the unpatched kernel missed all eight epoll removal notifications.
The four poll waits returned POLLERR only at timeout. With this
patch, all 12 poll and epoll waits reported clock removal promptly.

Check the defunct state after registering the poll wait queue and
return EPOLLERR when the clock is being removed. Use READ_ONCE() for
the lockless defunct checks and WRITE_ONCE() for the unregister store.

Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.")
Signed-off-by: Avi Weiss <thnkslprpt@gmail.com>
---

v2:
  - ddress Sashiko's v1 review, forwarded by Simon Horman, by pairing
    the new READ_ONCE() with WRITE_ONCE() in ptp_clock_unregister() and
    using READ_ONCE() for both ptp_read() checks
  - the v1 poll fix already addressed the observed removal scenario,
    these changes are really just to make the lockless accesses consistent

v1: https://lore.kernel.org/r/20261102122340.1160733-1-thnkslprpt@gmail.com/
 drivers/ptp/ptp_chardev.c | 8 ++++++--
 drivers/ptp/ptp_clock.c   | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index f63b45f5ee6a..dc5865e95f77 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;
 }
 
@@ -616,10 +619,11 @@ ssize_t ptp_read(struct posix_clock_context *pccontext, uint rdflags,
 	if (cnt > EXTTS_BUFSIZE)
 		cnt = EXTTS_BUFSIZE;
 
-	if (wait_event_interruptible(ptp->tsev_wq, ptp->defunct || queue_cnt(queue)))
+	if (wait_event_interruptible(ptp->tsev_wq,
+				     READ_ONCE(ptp->defunct) || queue_cnt(queue)))
 		return -ERESTARTSYS;
 
-	if (ptp->defunct)
+	if (READ_ONCE(ptp->defunct))
 		return -ENODEV;
 
 	event = kmalloc(EXTTS_BUFSIZE, GFP_KERNEL);
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 4111342d64f0..d2640325abf7 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -518,7 +518,7 @@ int ptp_clock_unregister(struct ptp_clock *ptp)
 	get_device(&ptp->dev);
 
 	/* Wake up any userspace waiting for an event. */
-	ptp->defunct = 1;
+	WRITE_ONCE(ptp->defunct, 1);
 	wake_up_interruptible(&ptp->tsev_wq);
 
 	/* Tear down the POSIX clock, which removes the user interface. */
-- 
2.43.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch is incomplete for epoll waiters registered with
  EPOLLEXCLUSIVE.

--

ptp: Report clock removal to poll waiters

This patch makes ptp_poll() return EPOLLERR once the clock is marked
defunct. Before, a poll waiter could take the removal wakeup, see an
empty queue and stay blocked until timeout. It also changes the defunct
accesses in ptp_read() to READ_ONCE() and the store in
ptp_clock_unregister() to WRITE_ONCE().

> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index f63b45f5ee6ae..dc5865e95f775 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;
>  }
>  

[ ... ]

> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index 47ffc773065a3..67f53ab4df61d 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -523,7 +523,7 @@ int ptp_clock_unregister(struct ptp_clock *ptp)
>  	get_device(&ptp->dev);
>  
>  	/* Wake up any userspace waiting for an event. */
> -	ptp->defunct = 1;
> +	WRITE_ONCE(ptp->defunct, 1);
>  	wake_up_interruptible(&ptp->tsev_wq);

[Severity: Medium]
Is this fix complete for epoll waiters that registered with EPOLLEXCLUSIVE?

wake_up_interruptible() expands to __wake_up(x, TASK_INTERRUPTIBLE, 1,
NULL), so it wakes at most one exclusive entry and passes a NULL key. An
EPOLLEXCLUSIVE epitem is added to tsev_wq with add_wait_queue_exclusive()
in ep_ptable_queue_proc().

With a NULL key, pollflags is 0, and ep_poll_callback() takes this branch
whenever its epoll instance has a thread blocked in epoll_wait():

fs/eventpoll.c:ep_poll_callback() {
    ...
	if (waitqueue_active(&ep->wq)) {
		if ((epi->event.events & EPOLLEXCLUSIVE) &&
					!(pollflags & POLLFREE)) {
			switch (pollflags & EPOLLINOUT_BITS) {
			...
			case 0:
				ewake = 1;
				break;
    ...
}

__wake_up_common() then stops walking the queue:

	if (ret && (flags & WQ_FLAG_EXCLUSIVE) && !--nr_exclusive)
		break;

The remaining exclusive epitems on tsev_wq are never called, so their
epoll instances never re-poll the file. They never reach the new defunct
check in ptp_poll() or the zombie check in posix_clock_poll().

After this wakeup, ptp_clock_unregister() calls ptp_disable_all_events()
and tears down the kworker. posix_clock_unregister() does no wakeup of
its own. Nothing wakes tsev_wq again, so the other EPOLLEXCLUSIVE waiters
stay blocked until their timeout, or forever with a timeout of -1 unless
a signal arrives. That is the same symptom the commit message describes
as fixed:

  "remain blocked until timeout after device removal."

Plain poll/select, non-exclusive epoll, and the wait_event_interruptible()
in ptp_read() all use non-exclusive entries, so they are always woken.
That would explain why all 12 test waits passed.

Would wake_up_interruptible_all(&ptp->tsev_wq) be better for this final
removal wakeup, as sock_def_wakeup() does? A keyed wake-one such as
wake_up_interruptible_poll(..., EPOLLERR) would not help. EPOLLERR has no
EPOLLIN or EPOLLOUT bits, so it still lands in case 0 and sets ewake.

>  
>  	/* Tear down the POSIX clock, which removes the user interface. */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108064928.1931917-2-thnkslprpt%40gmail.com