[PATCH v1] io_uring/kbuf: use WRITE_ONCE() for userspace-shared buffer ring fields

[PATCH v1] io_uring/kbuf: use WRITE_ONCE() for userspace-shared buffer ring fields

[Joanne Koong]

buf->addr and buf->len reside in memory shared with userspace. They
should be written with WRITE_ONCE() to guarantee atomic stores and
prevent tearing or other unsafe compiler optimizations.

Fixes: ae98dbf43d75 ("io_uring/kbuf: add support for incremental buffer consumption")
Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
Cc: Caleb Sander Mateos <csander@purestorage.com>
---
 io_uring/kbuf.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
index 52b636d00a6b..796d131107dd 100644
--- a/io_uring/kbuf.c
+++ b/io_uring/kbuf.c
@@ -44,11 +44,11 @@ static bool io_kbuf_inc_commit(struct io_buffer_list *bl, int len)
 		buf_len -= this_len;
 		/* Stop looping for invalid buffer length of 0 */
 		if (buf_len || !this_len) {
-			buf->addr = READ_ONCE(buf->addr) + this_len;
-			buf->len = buf_len;
+			WRITE_ONCE(buf->addr, READ_ONCE(buf->addr) + this_len);
+			WRITE_ONCE(buf->len, buf_len);
 			return false;
 		}
-		buf->len = 0;
+		WRITE_ONCE(buf->len, 0);
 		bl->head++;
 		len -= this_len;
 	}
@@ -291,7 +291,7 @@ static int io_ring_buffers_peek(struct io_kiocb *req, struct buf_sel_arg *arg,
 				arg->partial_map = 1;
 				if (iov != arg->iovs)
 					break;
-				buf->len = len;
+				WRITE_ONCE(buf->len, len);
 			}
 		}
 
-- 
2.47.3

Re: [PATCH v1] io_uring/kbuf: use WRITE_ONCE() for userspace-shared buffer ring fields

[Jens Axboe]

On 12/4/25 4:54 PM, Joanne Koong wrote:
> buf->addr and buf->len reside in memory shared with userspace. They
> should be written with WRITE_ONCE() to guarantee atomic stores and
> prevent tearing or other unsafe compiler optimizations.

It's not a real concern, but I think we should still do it purely
for documentation purposes. But because of that, I think we should drop:

> Fixes: ae98dbf43d75 ("io_uring/kbuf: add support for incremental buffer consumption")

to save me the trouble of ensuring older stable kernels get this applied.

-- 
Jens Axboe

Re: [PATCH v1] io_uring/kbuf: use WRITE_ONCE() for userspace-shared buffer ring fields

[Jens Axboe]

On Thu, 04 Dec 2025 15:54:50 -0800, Joanne Koong wrote:
> buf->addr and buf->len reside in memory shared with userspace. They
> should be written with WRITE_ONCE() to guarantee atomic stores and
> prevent tearing or other unsafe compiler optimizations.
> 
> 

Applied, thanks!

[1/1] io_uring/kbuf: use WRITE_ONCE() for userspace-shared buffer ring fields
      commit: a4c694bfc2455e82b7caf6045ca893d123e0ed11

Best regards,
-- 
Jens Axboe

Re: [PATCH v1] io_uring/kbuf: use WRITE_ONCE() for userspace-shared buffer ring fields

[Caleb Sander Mateos]

On Thu, Dec 4, 2025 at 3:55 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> buf->addr and buf->len reside in memory shared with userspace. They
> should be written with WRITE_ONCE() to guarantee atomic stores and
> prevent tearing or other unsafe compiler optimizations.

I considered this too, but I'm not sure it's necessary. A correctly
written userspace program won't access these fields concurrently with
the kernel, right? I'm not too familiar with the buffer ring UAPI, but
I would assume userspace is notified somehow (via a posted CQE?) that
the io_uring_buf slots have been consumed and are available to reuse.
In that case, a torn store here would only be observable to a buggy
userspace program, so I don't think we need to add WRITE_ONCE().

Best,
Caleb

>
> Fixes: ae98dbf43d75 ("io_uring/kbuf: add support for incremental buffer consumption")
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> Cc: Caleb Sander Mateos <csander@purestorage.com>
> ---
>  io_uring/kbuf.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
> index 52b636d00a6b..796d131107dd 100644
> --- a/io_uring/kbuf.c
> +++ b/io_uring/kbuf.c
> @@ -44,11 +44,11 @@ static bool io_kbuf_inc_commit(struct io_buffer_list *bl, int len)
>                 buf_len -= this_len;
>                 /* Stop looping for invalid buffer length of 0 */
>                 if (buf_len || !this_len) {
> -                       buf->addr = READ_ONCE(buf->addr) + this_len;
> -                       buf->len = buf_len;
> +                       WRITE_ONCE(buf->addr, READ_ONCE(buf->addr) + this_len);
> +                       WRITE_ONCE(buf->len, buf_len);
>                         return false;
>                 }
> -               buf->len = 0;
> +               WRITE_ONCE(buf->len, 0);
>                 bl->head++;
>                 len -= this_len;
>         }
> @@ -291,7 +291,7 @@ static int io_ring_buffers_peek(struct io_kiocb *req, struct buf_sel_arg *arg,
>                                 arg->partial_map = 1;
>                                 if (iov != arg->iovs)
>                                         break;
> -                               buf->len = len;
> +                               WRITE_ONCE(buf->len, len);
>                         }
>                 }
>
> --
> 2.47.3
>

Re: [PATCH v1] io_uring/kbuf: use WRITE_ONCE() for userspace-shared buffer ring fields

[Jens Axboe]

On 12/5/25 9:55 AM, Caleb Sander Mateos wrote:
> On Thu, Dec 4, 2025 at 3:55?PM Joanne Koong <joannelkoong@gmail.com> wrote:
>>
>> buf->addr and buf->len reside in memory shared with userspace. They
>> should be written with WRITE_ONCE() to guarantee atomic stores and
>> prevent tearing or other unsafe compiler optimizations.
> 
> I considered this too, but I'm not sure it's necessary. A correctly
> written userspace program won't access these fields concurrently with
> the kernel, right? I'm not too familiar with the buffer ring UAPI, but
> I would assume userspace is notified somehow (via a posted CQE?) that
> the io_uring_buf slots have been consumed and are available to reuse.
> In that case, a torn store here would only be observable to a buggy
> userspace program, so I don't think we need to add WRITE_ONCE().

I agree, see my reply from a few minutes ago. But I do think it serves a
documentation purpose ("shared memory is always read/write once"), which
is why I applied it.

-- 
Jens Axboe