On 3/4/24 1:49 PM, Eric Biggers wrote:
On Mon, Mar 04, 2024 at 01:09:15PM -0700, Jens Axboe wrote:
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.
It dereferences the pointer.
Oops yes, was too focused on the container_of(). We should move them down, one for clarity and one for not dereferencing it.
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.
Maybe kiocb_set_cancel_fn() should have a comment that explains this?
It should have a big fat comment that nobody should be using it. At least the gadget stuff is the only one doing it, and we haven't grown a new one in decades, thankfully.