[PATCH] printk: mark racy data block id accesses in the ringbuffer

[PATCH] printk: mark racy data block id accesses in the ringbuffer

[Nguyen Ngoc Thang]

data_alloc() stores the descriptor ID into a newly reserved data block
while data_make_reusable(), running from data_push_tail() on another CPU,
may still be loading the ID from the same area. The code relies on this
race being harmless: a stale or garbage ID simply fails the descriptor
lookup or the blk_lpos check. But the accesses are plain, so KCSAN
reports them, and the compiler is free to tear or refetch the ID.

Use READ_ONCE()/WRITE_ONCE() for every access to the block ID that can
overlap with the lockless reader. No functional change intended.

Reported-by: syzbot+8324b8fd984b86905a66@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=8324b8fd984b86905a66
Fixes: 896fbe20b4e2 ("printk: use the lockless ringbuffer")
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
 kernel/printk/printk_ringbuffer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index 85c0c854b3ce..86fe71211de9 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -605,7 +605,7 @@ static bool data_make_reusable(struct printk_ringbuffer *rb,
 		 * sure it points back to this data block. If the check fails,
 		 * the data area has been recycled by another writer.
 		 */
-		id = blk->id; /* LMM(data_make_reusable:A) */
+		id = READ_ONCE(blk->id); /* LMM(data_make_reusable:A) */
 
 		d_state = desc_read(desc_ring, id, &desc,
 				    NULL, NULL); /* LMM(data_make_reusable:B) */
@@ -1113,7 +1113,7 @@ static char *data_alloc(struct printk_ringbuffer *rb, unsigned int size,
 					  next_lpos)); /* LMM(data_alloc:A) */
 
 	blk = to_block(data_ring, begin_lpos);
-	blk->id = id; /* LMM(data_alloc:B) */
+	WRITE_ONCE(blk->id, id); /* LMM(data_alloc:B) */
 
 	if (is_blk_wrapped(data_ring, begin_lpos, next_lpos)) {
 		/* Wrapping data blocks store their data at the beginning. */
@@ -1123,7 +1123,7 @@ static char *data_alloc(struct printk_ringbuffer *rb, unsigned int size,
 		 * Store the ID on the wrapped block for consistency.
 		 * The printk_ringbuffer does not actually use it.
 		 */
-		blk->id = id;
+		WRITE_ONCE(blk->id, id);
 	}
 
 	blk_lpos->begin = begin_lpos;
@@ -1212,7 +1212,7 @@ static char *data_realloc(struct printk_ringbuffer *rb, unsigned int size,
 		 * Store the ID on the wrapped block for consistency.
 		 * The printk_ringbuffer does not actually use it.
 		 */
-		blk->id = id;
+		WRITE_ONCE(blk->id, id);
 
 		if (!wrapped) {
 			/*
-- 
2.43.0

Re: [PATCH] printk: mark racy data block id accesses in the ringbuffer

[John Ogness]

On 2026-10-11, Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com> wrote:
> data_alloc() stores the descriptor ID into a newly reserved data block
> while data_make_reusable(), running from data_push_tail() on another CPU,
> may still be loading the ID from the same area. The code relies on this
> race being harmless: a stale or garbage ID simply fails the descriptor
> lookup or the blk_lpos check. But the accesses are plain, so KCSAN
> reports them, and the compiler is free to tear or refetch the ID.
>
> Use READ_ONCE()/WRITE_ONCE() for every access to the block ID that can
> overlap with the lockless reader. No functional change intended.

READ_ONCE()/WRITE_ONCE() could cause minor side-effects as it prohibits
optimizations that do not need to be prohibited. These would not cause a
problem, but they are unnecessary. The data race is allowed.

How about using data_race() instead?

	id = data_race(blk->id);

	data_race(blk->id = id);

John Ogness

Re: [PATCH] printk: mark racy data block id accesses in the ringbuffer

[Petr Mladek]

On Mon 2026-10-12 12:06:47, John Ogness wrote:
> On 2026-10-11, Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com> wrote:
> > data_alloc() stores the descriptor ID into a newly reserved data block
> > while data_make_reusable(), running from data_push_tail() on another CPU,
> > may still be loading the ID from the same area. The code relies on this
> > race being harmless: a stale or garbage ID simply fails the descriptor
> > lookup or the blk_lpos check. But the accesses are plain, so KCSAN
> > reports them, and the compiler is free to tear or refetch the ID.
> >
> > Use READ_ONCE()/WRITE_ONCE() for every access to the block ID that can
> > overlap with the lockless reader. No functional change intended.
> 
> READ_ONCE()/WRITE_ONCE() could cause minor side-effects as it prohibits
> optimizations that do not need to be prohibited. These would not cause a
> problem, but they are unnecessary. The data race is allowed.
> 
> How about using data_race() instead?
> 
> 	id = data_race(blk->id);
> 
> 	data_race(blk->id = id);

I prefer data_race() because it makes it clear that the race is
expected. Using READ_ONCE()/READ_ONCE() would create false expectations
that they are needed to prevent some races.

Best Regards,
Petr