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

[netdev-bot+sashiko@kernel.org]

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