IOSQE_BUFFER_SELECT buffer returned even in case of failure?

IOSQE_BUFFER_SELECT buffer returned even in case of failure?

[Olivier Langlois]

the man page says the following:

    If succesful, the resulting CQE will have IORING_CQE_F_BUFFER set
in the flags part of the struct, and the upper IORING_CQE_BUFFER_SHIFT
bits will contain the ID of the selected buffers.

based on my understanding of the kernel code (it could wrong. testing
is still ongoing to be 100% sure), the buffer will be returned even if
the underlying syscall fails. (I have only checked io_read() for
IORING_OP_READ).

At the minimum, the man page should be clarified to better reflect the
code behavior. (and there is a missing 's' in succesful)

ideally, imho, I believe the code should be modified to do what the man
page says because:

1. doing otherwise is counter-intuitive and error-prone (I cannot think
of a single example of a syscall failing and still require the user to
free the allocated resources)

2. it is inefficient because the buffer is unneeded since there is no
data to transfer back to the user and the buffer will need to be
returned back to io_uring to avoid a leak.

Greetings,
Olivier

Re: IOSQE_BUFFER_SELECT buffer returned even in case of failure?

[Olivier Langlois]

On Wed, 2021-06-09 at 15:19 -0400, Olivier Langlois wrote:
> the man page says the following:
> 
>     If succesful, the resulting CQE will have IORING_CQE_F_BUFFER set
> in the flags part of the struct, and the upper
> IORING_CQE_BUFFER_SHIFT
> bits will contain the ID of the selected buffers.
> 
> based on my understanding of the kernel code (it could wrong. testing
> is still ongoing to be 100% sure), the buffer will be returned even
> if
> the underlying syscall fails. (I have only checked io_read() for
> IORING_OP_READ).
> 
> At the minimum, the man page should be clarified to better reflect
> the
> code behavior. (and there is a missing 's' in succesful)
> 
> ideally, imho, I believe the code should be modified to do what the
> man
> page says because:
> 
> 1. doing otherwise is counter-intuitive and error-prone (I cannot
> think
> of a single example of a syscall failing and still require the user
> to
> free the allocated resources)
> 
> 2. it is inefficient because the buffer is unneeded since there is no
> data to transfer back to the user and the buffer will need to be
> returned back to io_uring to avoid a leak.
> 
I have confirmed that a buffer was returned and needed to be provided
back io_uring even when read() syscall was failing.

It failed with ECONNRESET with obviously no data and I still had to
return back the buffer to avoid a leak (which my app was suffering of).

Unless someone says that it works as designed, I'll figure out a way to
fix that with a patch. It should be trivial to do.

Greetings,
Olivier