On 13-06-2023 13:02, Kevin Brodsky wrote:
On 12/05/2023 19:24, Kevin Brodsky wrote:
- For IORING_REGISTER_PBUF_RING and friends, the situation is different.
They can be used with operations specifying IOSQE_BUFFER_SELECT (4 allowed, see above), but here the operation does not specify the addr / len. We could still do the checking when processing the operation, since io_buffer_select() returns the original user pointer. It could be better to do it at the point of registration instead though (which is possible permission-wise because we know the operation will write to the buffer, as per above).
I'm sure I've missed something [...]
... and indeed I have. A pretty big thing in fact! IORING_REGISTER_PBUF_RING is different from IORING_OP_PROVIDE_BUFFERS (addressed further down the thread) in an important way: while the latter specifies N consecutive buffers "synchronously", the former specifies in fact *a ring* of struct io_uring_buf, each specifying a buffer, and the contents of the ring are *not* read at the point of registration. A glance at io_buffer_select() makes the difference between the two paths very clear.
Ah, indeed the whole point of registering a ring is so that the userspace can add and manipulate registered buffers in the later stages as well. Great find!
So for IORING_REGISTER_PBUF_RING, there are really two levels of checks. The first is checking whether we can read the ring via struct io_uring_buf_reg::ring_addr. This is straightforward and should be done at the point of registration. The second is checking whether the selected struct io_uring_buf::addr allows accessing the buffer, and clearly this one can only be done at the point of use, as the ring can be modified at any point by userspace.
This leads me to another confusion I created: there is in fact no need to explicitly check the pointers to the registered buffers. This is because such pointers returned by io_buffer_select() are still user pointers (void __user *), and explicit checking is only needed in the cases where we have to obtain the address for GUP or similar. It does happen in some cases, but it is orthogonal to these registration mechanisms.
This means that there is no good reason to check the pointer provided to IORING_OP_PROVIDE_BUFFERS either. We can just register the pointer, and then whoever calls io_buffer_select() might get back this pointer, which will eventually be checked like any other pointer provided by SQE::addr or similar.
The nice thing with this new approach is that we no longer have an inconsistency between the two approaches ({READ,WRITE}_FIXED and PROVIDE_BUFFERS / PBUF_RING): we now always check when processing the operation and never when registering buffers. Does that sound sensible?
It sounds good to me! Well done! :)
Tudor
Kevin