On 3/4/24 12:10 PM, Eric Biggers wrote:
On Thu, Feb 15, 2024 at 12:47:38PM -0800, Bart Van Assche wrote:
void kiocb_set_cancel_fn(struct kiocb *iocb, kiocb_cancel_fn *cancel) { struct aio_kiocb *req = container_of(iocb, struct aio_kiocb, rw); struct kioctx *ctx = req->ki_ctx; unsigned long flags;
- /*
* kiocb didn't come from aio or is neither a read nor a write, hence* ignore it.*/- if (!(iocb->ki_flags & IOCB_AIO_RW))
return;- if (WARN_ON_ONCE(!list_empty(&req->ki_list))) return;
If I understand correctly, this patch is supposed to fix a memory safety bug when kiocb_set_cancel_fn() is called on a kiocb that is owned by io_uring instead of legacy AIO. However, the kiocb still gets accessed as an aio_kiocb at the very beginning of the function, so it's still broken:
struct aio_kiocb *req = container_of(iocb, struct aio_kiocb, rw); struct kioctx *ctx = req->ki_ctx;
Doesn't matter, they are both just pointer math. But it'd look cleaner if it was below.
I'm also wondering why "ignore" is the right fix. The USB gadget driver sees that it has asynchronous I/O (kiocb::ki_complete != NULL) and then tries to set a cancellation function. What is the expected behavior when the I/O is owned by io_uring? Should it perhaps call into io_uring to set a cancellation function with io_uring? Or is the concept of cancellation functions indeed specific to legacy AIO, and nothing should be done with io_uring I/O?
Because the ->ki_cancel() is a hack, as demonstrated by this issue in teh first place, which is a gross layering violation. io_uring supports proper cancelations, invoked from userspace. It would never have worked with this scheme.