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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux