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. -- Jens Axboe
