[PATCH 0/3] io_uring: fixes for provided buffer ring

[PATCH 0/3] io_uring: fixes for provided buffer ring

[Dylan Yudaken]

This fixes two problems in the new provided buffer ring feature. One
is a simple arithmetic bug (I think this came out from a refactor).
The other is due to type differences between head & tail, which causes
it to sometimes reuse an old buffer incorrectly.

Patch 1&2 fix bugs
Patch 3 limits the size of the ring as it's not
possible to address more entries with 16 bit head/tail

I will send test cases for liburing shortly.

One question might be if we should change the type of ring_entries
to uint16_t in struct io_uring_buf_reg?

Dylan Yudaken (3):
  io_uring: fix index calculation
  io_uring: fix types in provided buffer ring
  io_uring: limit size of provided buffer ring

 fs/io_uring.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)


base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
-- 
2.30.2

[PATCH 1/3] io_uring: fix index calculation

[Dylan Yudaken]

When indexing into a provided buffer ring, do not subtract 1 from the
index.

Fixes: c7fb19428d67 ("io_uring: add support for ring mapped supplied buffers")
Signed-off-by: Dylan Yudaken <dylany@fb.com>
---
 fs/io_uring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 3aab4182fd89..9cf9aff51b70 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3888,7 +3888,7 @@ static void __user *io_ring_buffer_select(struct io_kiocb *req, size_t *len,
 		buf = &br->bufs[head];
 	} else {
 		int off = head & (IO_BUFFER_LIST_BUF_PER_PAGE - 1);
-		int index = head / IO_BUFFER_LIST_BUF_PER_PAGE - 1;
+		int index = head / IO_BUFFER_LIST_BUF_PER_PAGE;
 		buf = page_address(bl->buf_pages[index]);
 		buf += off;
 	}
-- 
2.30.2

[PATCH 2/3] io_uring: fix types in provided buffer ring

[Dylan Yudaken]

The type of head needs to match that of tail in order for rollover and
comparisons to work correctly.

Without this change the comparison of tail to head might incorrectly allow
io_uring to use a buffer that userspace had not given it.

Fixes: c7fb19428d67 ("io_uring: add support for ring mapped supplied buffers")
Signed-off-by: Dylan Yudaken <dylany@fb.com>
---
 fs/io_uring.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 9cf9aff51b70..6eea18e8330c 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -298,8 +298,8 @@ struct io_buffer_list {
 	/* below is for ring provided buffers */
 	__u16 buf_nr_pages;
 	__u16 nr_entries;
-	__u32 head;
-	__u32 mask;
+	__u16 head;
+	__u16 mask;
 };
 
 struct io_buffer {
@@ -3876,7 +3876,7 @@ static void __user *io_ring_buffer_select(struct io_kiocb *req, size_t *len,
 {
 	struct io_uring_buf_ring *br = bl->buf_ring;
 	struct io_uring_buf *buf;
-	__u32 head = bl->head;
+	__u16 head = bl->head;
 
 	if (unlikely(smp_load_acquire(&br->tail) == head)) {
 		io_ring_submit_unlock(req->ctx, issue_flags);
-- 
2.30.2

[PATCH 3/3] io_uring: limit size of provided buffer ring

[Dylan Yudaken]

The type of head and tail do not allow more than 2^15 entries in a
provided buffer ring, so do not allow this.
At 2^16 while each entry can be indexed, there is no way to
disambiguate full vs empty.

Signed-off-by: Dylan Yudaken <dylany@fb.com>
---
 fs/io_uring.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 6eea18e8330c..85b116ddfd2a 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -13002,6 +13002,10 @@ static int io_register_pbuf_ring(struct io_ring_ctx *ctx, void __user *arg)
 	if (!is_power_of_2(reg.ring_entries))
 		return -EINVAL;
 
+	/* cannot disambiguate full vs empty due to head/tail size */
+	if (reg.ring_entries >= 65536)
+		return -EINVAL;
+
 	if (unlikely(reg.bgid < BGID_ARRAY && !ctx->io_bl)) {
 		int ret = io_init_bl_list(ctx);
 		if (ret)
-- 
2.30.2

Re: [PATCH 0/3] io_uring: fixes for provided buffer ring

[Hao Xu]

On 6/13/22 18:11, Dylan Yudaken wrote:
> This fixes two problems in the new provided buffer ring feature. One
> is a simple arithmetic bug (I think this came out from a refactor).
> The other is due to type differences between head & tail, which causes
> it to sometimes reuse an old buffer incorrectly.
> 
> Patch 1&2 fix bugs
> Patch 3 limits the size of the ring as it's not
> possible to address more entries with 16 bit head/tail

Reviewed-by: Hao Xu <howeyxu@tencent.com>

> 
> I will send test cases for liburing shortly.
> 
> One question might be if we should change the type of ring_entries
> to uint16_t in struct io_uring_buf_reg?

Why not? 5.19 is just rc2 now. So we can assume there is no users using
it right now I think?

Re: [PATCH 0/3] io_uring: fixes for provided buffer ring

[Pavel Begunkov]

On 6/13/22 12:08, Hao Xu wrote:
> On 6/13/22 18:11, Dylan Yudaken wrote:
>> This fixes two problems in the new provided buffer ring feature. One
>> is a simple arithmetic bug (I think this came out from a refactor).
>> The other is due to type differences between head & tail, which causes
>> it to sometimes reuse an old buffer incorrectly.
>>
>> Patch 1&2 fix bugs
>> Patch 3 limits the size of the ring as it's not
>> possible to address more entries with 16 bit head/tail
> 
> Reviewed-by: Hao Xu <howeyxu@tencent.com>
> 
>>
>> I will send test cases for liburing shortly.
>>
>> One question might be if we should change the type of ring_entries
>> to uint16_t in struct io_uring_buf_reg?
> 
> Why not? 5.19 is just rc2 now. So we can assume there is no users using
> it right now I think?

It's fine to change, but might be better if we want to extend it
in the future. Do other pbuf bits allow more than 2^16 buffers?

-- 
Pavel Begunkov

Re: [PATCH 0/3] io_uring: fixes for provided buffer ring

[Dylan Yudaken]

On Mon, 2022-06-13 at 13:59 +0100, Pavel Begunkov wrote:
> On 6/13/22 12:08, Hao Xu wrote:
> > On 6/13/22 18:11, Dylan Yudaken wrote:
> > > This fixes two problems in the new provided buffer ring feature.
> > > One
> > > is a simple arithmetic bug (I think this came out from a
> > > refactor).
> > > The other is due to type differences between head & tail, which
> > > causes
> > > it to sometimes reuse an old buffer incorrectly.
> > > 
> > > Patch 1&2 fix bugs
> > > Patch 3 limits the size of the ring as it's not
> > > possible to address more entries with 16 bit head/tail
> > 
> > Reviewed-by: Hao Xu <howeyxu@tencent.com>
> > 
> > > 
> > > I will send test cases for liburing shortly.
> > > 
> > > One question might be if we should change the type of
> > > ring_entries
> > > to uint16_t in struct io_uring_buf_reg?
> > 
> > Why not? 5.19 is just rc2 now. So we can assume there is no users
> > using
> > it right now I think?
> 
> It's fine to change, but might be better if we want to extend it
> in the future. Do other pbuf bits allow more than 2^16 buffers?
> 

I guess with

+	if (reg.ring_entries >= 65536)
+		return -EINVAL;

it doesn't matter either way. we can always use those bits later if we
need?

Re: [PATCH 0/3] io_uring: fixes for provided buffer ring

[Pavel Begunkov]

On 6/13/22 14:16, Dylan Yudaken wrote:
> On Mon, 2022-06-13 at 13:59 +0100, Pavel Begunkov wrote:
>> On 6/13/22 12:08, Hao Xu wrote:
>>> On 6/13/22 18:11, Dylan Yudaken wrote:
>>>> This fixes two problems in the new provided buffer ring feature.
>>>> One
>>>> is a simple arithmetic bug (I think this came out from a
>>>> refactor).
>>>> The other is due to type differences between head & tail, which
>>>> causes
>>>> it to sometimes reuse an old buffer incorrectly.
>>>>
>>>> Patch 1&2 fix bugs
>>>> Patch 3 limits the size of the ring as it's not
>>>> possible to address more entries with 16 bit head/tail
>>>
>>> Reviewed-by: Hao Xu <howeyxu@tencent.com>
>>>
>>>>
>>>> I will send test cases for liburing shortly.
>>>>
>>>> One question might be if we should change the type of
>>>> ring_entries
>>>> to uint16_t in struct io_uring_buf_reg?
>>>
>>> Why not? 5.19 is just rc2 now. So we can assume there is no users
>>> using
>>> it right now I think?
>>
>> It's fine to change, but might be better if we want to extend it
>> in the future. Do other pbuf bits allow more than 2^16 buffers?
>>

might be better to leave it u32 *

> I guess with
> 
> +	if (reg.ring_entries >= 65536)
> +		return -EINVAL;
> 
> it doesn't matter either way. we can always use those bits later if we
> need?

I think so as well.

I was also wondering whether pbufs can potentially allow >16 bits,
but taking a quick look IORING_CQE_BUFFER_SHIFT=16, so we only have
16 bits in cqe::flags for indexes we return.

-- 
Pavel Begunkov

Re: [PATCH 0/3] io_uring: fixes for provided buffer ring

[Hao Xu]

On 6/13/22 22:05, Pavel Begunkov wrote:
> On 6/13/22 14:16, Dylan Yudaken wrote:
>> On Mon, 2022-06-13 at 13:59 +0100, Pavel Begunkov wrote:
>>> On 6/13/22 12:08, Hao Xu wrote:
>>>> On 6/13/22 18:11, Dylan Yudaken wrote:
>>>>> This fixes two problems in the new provided buffer ring feature.
>>>>> One
>>>>> is a simple arithmetic bug (I think this came out from a
>>>>> refactor).
>>>>> The other is due to type differences between head & tail, which
>>>>> causes
>>>>> it to sometimes reuse an old buffer incorrectly.
>>>>>
>>>>> Patch 1&2 fix bugs
>>>>> Patch 3 limits the size of the ring as it's not
>>>>> possible to address more entries with 16 bit head/tail
>>>>
>>>> Reviewed-by: Hao Xu <howeyxu@tencent.com>
>>>>
>>>>>
>>>>> I will send test cases for liburing shortly.
>>>>>
>>>>> One question might be if we should change the type of
>>>>> ring_entries
>>>>> to uint16_t in struct io_uring_buf_reg?
>>>>
>>>> Why not? 5.19 is just rc2 now. So we can assume there is no users
>>>> using
>>>> it right now I think?
>>>
>>> It's fine to change, but might be better if we want to extend it
>>> in the future. Do other pbuf bits allow more than 2^16 buffers?
>>>
> 
> might be better to leave it u32 *
> 
>> I guess with
>>
>> +    if (reg.ring_entries >= 65536)
>> +        return -EINVAL;
>>
>> it doesn't matter either way. we can always use those bits later if we
>> need?
> 
> I think so as well.
> 
> I was also wondering whether pbufs can potentially allow >16 bits,
> but taking a quick look IORING_CQE_BUFFER_SHIFT=16, so we only have
> 16 bits in cqe::flags for indexes we return.
> 

Yea, the 16 bits return index in cqe->flags is a hard limit for
pbuf ring feature, but I do think it's ok since 1<<16 is already big

Re: [PATCH 0/3] io_uring: fixes for provided buffer ring

[Jens Axboe]

On Mon, 13 Jun 2022 03:11:54 -0700, Dylan Yudaken wrote:
> This fixes two problems in the new provided buffer ring feature. One
> is a simple arithmetic bug (I think this came out from a refactor).
> The other is due to type differences between head & tail, which causes
> it to sometimes reuse an old buffer incorrectly.
> 
> Patch 1&2 fix bugs
> Patch 3 limits the size of the ring as it's not
> possible to address more entries with 16 bit head/tail
> 
> [...]

Applied, thanks!

[1/3] io_uring: fix index calculation
      commit: 97da4a537924d87e2261773f3ac9365abb191fc9
[2/3] io_uring: fix types in provided buffer ring
      commit: c6e9fa5c0ab811f4bec36a96337f4b1bb77d142c
[3/3] io_uring: limit size of provided buffer ring
      commit: f9437ac0f851cea2374d53594f52fbbefdd977bd

Best regards,
-- 
Jens Axboe