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

-- 
Jens Axboe