On 16/03/2023 14:39, Tudor Cretu wrote:
[...]
@@ -604,7 +641,9 @@ void io_cq_unlock_post(struct io_ring_ctx *ctx) static bool __io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force) { bool all_flushed; - size_t cqe_size = sizeof(struct io_uring_cqe); + size_t cqe_size = ctx->compat ? + sizeof(struct compat_io_uring_cqe) : + sizeof(struct io_uring_cqe);
I'm not sure this is enough to get the overflow machinery to work in compat64. struct io_overflow_cqe gets filled in io_cqring_event_overflow() but AFAICT that code is not compat-aware, so it always initialises the native cqe and not the compat one. I suppose we could make use of __io_fill_cqe_any() in io_cqring_event_overflow()?
This makes me think, are there tests that trigger this overflow mechanism? Issues there might easily go unnoticed otherwise.
I had a better look at this and realised that struct io_overflow_cqe is always filled with a native cqe, so I removed the union from it. Also this change is not needed anymore.
Right I guess that's easier this way, the overflow list is not visible to userspace so it makes sense to always store in the native format and do the conversion in this function like you did in v4.
Yes, there are tests in liburing, and they pass now for compat as well.
Nice!
[...]
@@ -508,19 +596,19 @@ int io_register_pbuf_ring(struct io_ring_ctx *ctx, void __user *arg) return -ENOMEM; } - pages = io_pin_pages(reg.ring_addr, - struct_size(br, bufs, reg.ring_entries),
AFAICT the new code is not quite equivalent to this line, even in the native case, because struct_size() is equivalent to sizeof(struct) + n * sizeof(flex_array_member). struct io_uring_buf_ring is a bit of a strange union, in the end its size is the size of struct io_uring_buf. IOW I think this line actually yields (n + 1) * sizeof(struct io_uring_buf).
I suspect this is not what was actually intended, and that your change fixes that off-by-one, though I may well be wrong as the way the union is used is not exactly trivial. If that's the case then it should be a separate patch, and it would be a good candidate for upstreaming.
Indeed, it is a off-by-one error. The man pages states that: The size of the ring is the product of ring_entries and the size of struct io_uring_buf, so I just did that. The union is there, just so that we store the tail of the ring in the same place as the resv field of the first io_uring_buf. ring_entries must be a power of two, otherwise the syscalls errs early, so it can't be 0, so the change should be fine.
Makes sense.
diff --git a/io_uring/net.c b/io_uring/net.c index c586278858e7e..4c133bc6f9d1d 100644 --- a/io_uring/net.c +++ b/io_uring/net.c @@ -4,6 +4,7 @@ #include <linux/file.h> #include <linux/slab.h> #include <linux/net.h> +#include <linux/uio.h> #include <linux/compat.h> #include <net/compat.h> #include <linux/io_uring.h> @@ -435,7 +436,9 @@ static int __io_recvmsg_copy_hdr(struct io_kiocb *req, } else if (msg.msg_iovlen > 1) { return -EINVAL; } else { - if (copy_from_user(iomsg->fast_iov, msg.msg_iov, sizeof(*msg.msg_iov))) + void *iov = iovec_from_user(msg.msg_iov, 1, 1, iomsg->fast_iov, + req->ctx->compat); + if (IS_ERR(iov))
This looks correct, but do you have any idea why the compat code looks so different? It does not set fast_iov at all in that case, which makes me wonder if it's really equivalent...
I don't know why one uses copy_from_user and the other uses __get_user. This has been like this from the very beginning and I don't think there is a good reason for it. __get_user would suffice in both cases really. The difference in the setting of fast_iov comes from the commit 5476dfed29ad ("io_uring: clean iov usage for recvmsg buf select") which cleanups only the __io_recvmsg_copy_hdr and ignores __io_compat_recvmsg_copy_hdr.
I don't think that commit fundamentally changed the situation, which boils down to this: in native, the iov in iomsg is set (now to fast_iov), while in compat it is not. The only rational explanation that I can think of is that setting the iov is not actually needed, but I didn't manage to convince myself this is true.
In any case the situation is the same whether using compat32 or compat64, so it's not essential to investigate this further.
Kevin