[PATCH net] ptp: annotate data-race around q->head and q->tail

[PATCH net] ptp: annotate data-race around q->head and q->tail

[Eric Dumazet]

As I was working on a syzbot report, I found that KCSAN would
probably complain that reading q->head or q->tail without
barriers could lead to invalid results.

Add corresponding READ_ONCE() and WRITE_ONCE() to avoid
load-store tearing.

Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Richard Cochran <richardcochran@gmail.com>
---
 drivers/ptp/ptp_chardev.c | 3 ++-
 drivers/ptp/ptp_clock.c   | 5 +++--
 drivers/ptp/ptp_private.h | 8 ++++++--
 drivers/ptp/ptp_sysfs.c   | 3 ++-
 4 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 3f7a7478880240a2d256caf624b61dcc8e7054af..7513018c9f9ac72d5c1b0055b55ae9ff36e710b0 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -572,7 +572,8 @@ ssize_t ptp_read(struct posix_clock_context *pccontext, uint rdflags,
 
 	for (i = 0; i < cnt; i++) {
 		event[i] = queue->buf[queue->head];
-		queue->head = (queue->head + 1) % PTP_MAX_TIMESTAMPS;
+		/* Paired with READ_ONCE() in queue_cnt() */
+		WRITE_ONCE(queue->head, (queue->head + 1) % PTP_MAX_TIMESTAMPS);
 	}
 
 	spin_unlock_irqrestore(&queue->lock, flags);
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 3134568af622d396f6ab15049cd1a3ace3243269..15b804ba48685ee11a34b88df1ae738a136d17a1 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -57,10 +57,11 @@ static void enqueue_external_timestamp(struct timestamp_event_queue *queue,
 	dst->t.sec = seconds;
 	dst->t.nsec = remainder;
 
+	/* Both WRITE_ONCE() are paired with READ_ONCE() in queue_cnt() */
 	if (!queue_free(queue))
-		queue->head = (queue->head + 1) % PTP_MAX_TIMESTAMPS;
+		WRITE_ONCE(queue->head, (queue->head + 1) % PTP_MAX_TIMESTAMPS);
 
-	queue->tail = (queue->tail + 1) % PTP_MAX_TIMESTAMPS;
+	WRITE_ONCE(queue->tail, (queue->tail + 1) % PTP_MAX_TIMESTAMPS);
 
 	spin_unlock_irqrestore(&queue->lock, flags);
 }
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index 35fde0a0574606a04d6bdf0ab42a204da5fa6532..45f9002a5dcaea2c588c001fa83317fc318500ee 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -85,9 +85,13 @@ struct ptp_vclock {
  * that a writer might concurrently increment the tail does not
  * matter, since the queue remains nonempty nonetheless.
  */
-static inline int queue_cnt(struct timestamp_event_queue *q)
+static inline int queue_cnt(const struct timestamp_event_queue *q)
 {
-	int cnt = q->tail - q->head;
+	/*
+	 * Paired with WRITE_ONCE() in enqueue_external_timestamp(),
+	 * ptp_read(), extts_fifo_show().
+	 */
+	int cnt = READ_ONCE(q->tail) - READ_ONCE(q->head);
 	return cnt < 0 ? PTP_MAX_TIMESTAMPS + cnt : cnt;
 }
 
diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index 7d023d9d0acbfb3d128be09578753588fa59e84d..f7a499a1bd39ec22edf6c77407a48736e137f277 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c
@@ -94,7 +94,8 @@ static ssize_t extts_fifo_show(struct device *dev,
 	qcnt = queue_cnt(queue);
 	if (qcnt) {
 		event = queue->buf[queue->head];
-		queue->head = (queue->head + 1) % PTP_MAX_TIMESTAMPS;
+		/* Paired with READ_ONCE() in queue_cnt() */
+		WRITE_ONCE(queue->head, (queue->head + 1) % PTP_MAX_TIMESTAMPS);
 	}
 	spin_unlock_irqrestore(&queue->lock, flags);
 
-- 
2.42.0.869.gea05f2083d-goog

Re: [PATCH net] ptp: annotate data-race around q->head and q->tail

[Richard Cochran]

On Thu, Nov 09, 2023 at 05:48:59PM +0000, Eric Dumazet wrote:
> As I was working on a syzbot report, I found that KCSAN would
> probably complain that reading q->head or q->tail without
> barriers could lead to invalid results.
> 
> Add corresponding READ_ONCE() and WRITE_ONCE() to avoid
> load-store tearing.

Acked-by: Richard Cochran <richardcochran@gmail.com>

Re: [PATCH net] ptp: annotate data-race around q->head and q->tail

[Eric Dumazet]

On Fri, Nov 10, 2023 at 5:23 AM Richard Cochran
<richardcochran@gmail.com> wrote:
>
> On Thu, Nov 09, 2023 at 05:48:59PM +0000, Eric Dumazet wrote:
> > As I was working on a syzbot report, I found that KCSAN would
> > probably complain that reading q->head or q->tail without
> > barriers could lead to invalid results.
> >
> > Add corresponding READ_ONCE() and WRITE_ONCE() to avoid
> > load-store tearing.
>
> Acked-by: Richard Cochran <richardcochran@gmail.com>

Note the syzbot report I am looking at point to bugs added in

commit 8f5de6fb245326704f37d91780b9a10253a8a100    ptp: support
multiple timestamp event readers

For instance ptp_poll() can crash.

I saw the following patch being merged (without me being CC ?)

commit 8a4f030dbced6fc255cbe67b2d0a129947e18493
Author: Yuran Pereira <yuran.pereira@hotmail.com>
Date:   Wed Nov 8 02:18:36 2023 +0530

    ptp: Fixes a null pointer dereference in ptp_ioctl

I do not see how races are solved... Shouldn't
pccontext->private_clkdata be protected by RCU ?

Re: [PATCH net] ptp: annotate data-race around q->head and q->tail

[Richard Cochran]

On Fri, Nov 10, 2023 at 10:42:01AM +0100, Eric Dumazet wrote:

> I do not see how races are solved... Shouldn't
> pccontext->private_clkdata be protected by RCU ?

Yeah, the test is useless, because the memory is allocated in open()
and later freed in release().  During ioctl() the pointer must be
valid.

However, there was a bogus call to ptp_release() in the read() method,
but that has now been removed, and so the test is now bogus.

Thanks,
Richard

Re: [PATCH net] ptp: annotate data-race around q->head and q->tail

[Jakub Kicinski]

On Fri, 10 Nov 2023 09:09:45 -0800 Richard Cochran wrote:
> > I do not see how races are solved... Shouldn't
> > pccontext->private_clkdata be protected by RCU ?  
> 
> Yeah, the test is useless, because the memory is allocated in open()
> and later freed in release().  During ioctl() the pointer must be
> valid.
> 
> However, there was a bogus call to ptp_release() in the read() method,
> but that has now been removed, and so the test is now bogus.

Meaning we should revert 8a4f030dbced ("ptp: Fixes a null pointer
dereference in ptp_ioctl") ?

Re: [PATCH net] ptp: annotate data-race around q->head and q->tail

[Richard Cochran]

On Fri, Nov 10, 2023 at 11:52:24AM -0800, Jakub Kicinski wrote:

> Meaning we should revert 8a4f030dbced ("ptp: Fixes a null pointer
> dereference in ptp_ioctl") ?

Yes, I think that would be ideal.

The test itself is harmless, but keeping it will make people think, "oh
this pointer can be invalid."

In fact the core stack ensures that ioctl() can't be invoked after
release(), otherwise Bad Stuff happens.

Thanks,
Richard

Re: [PATCH net] ptp: annotate data-race around q->head and q->tail

[Jakub Kicinski]

On Sat, 11 Nov 2023 16:44:46 -0800 Richard Cochran wrote:
> > Meaning we should revert 8a4f030dbced ("ptp: Fixes a null pointer
> > dereference in ptp_ioctl") ?  
> 
> Yes, I think that would be ideal.

Done!

BTW Eric patch is series id 800000 in patchwork.
Nice and round number :)

Re: [PATCH net] ptp: annotate data-race around q->head and q->tail

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  9 Nov 2023 17:48:59 +0000 you wrote:
> As I was working on a syzbot report, I found that KCSAN would
> probably complain that reading q->head or q->tail without
> barriers could lead to invalid results.
> 
> Add corresponding READ_ONCE() and WRITE_ONCE() to avoid
> load-store tearing.
> 
> [...]

Here is the summary with links:
  - [net] ptp: annotate data-race around q->head and q->tail
    https://git.kernel.org/netdev/net/c/73bde5a32948

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html