Re: [PATCH v4 1/2] fs/aio: Restrict kiocb_set_cancel_fn() to I/O submitted via libaio

[Jens Axboe <axboe@kernel.dk>]

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