[PATCH v4 0/2] Fix libaio cancellation support

[PATCH v4 0/2] Fix libaio cancellation support

[Bart Van Assche]

Hi Christian,

This patch series fixes cancellation support in libaio as follows:
 - Restore the code for completing cancelled I/O.
 - Ignore requests to support cancellation for I/O not submitted by libaio.

Please consider this patch series for the next merge window.

Thanks,

Bart.

Changes between v3 and v4:
 - Restored libaio cancellation support.
 - Changed the approach to marking libaio requests with a flag in struct kiocb
   instead of adding a new operation in struct file_operations.
 
Changes between v2 and v3:
 - Removed libaio cancellation support instead of trying to fix it.

Changes between v1 and v2:
 - Fixed a race between request completion and addition to the list of
   active requests.
 - Changed the return type of .cancel_kiocb() from int into void.
 - Simplified the .cancel_kiocb() implementations.
 - Introduced the ki_opcode member in struct aio_kiocb.
 - aio_cancel_and_del() now checks .ki_opcode before accessing union members.
 - Left out the include/include/mm changes.

Bart Van Assche (2):
  fs/aio: Restrict kiocb_set_cancel_fn() to I/O submitted via libaio
  fs/aio: Make io_cancel() generate completions again

 fs/aio.c           | 36 +++++++++++++++++++-----------------
 include/linux/fs.h |  2 ++
 2 files changed, 21 insertions(+), 17 deletions(-)

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

[Bart Van Assche]

If kiocb_set_cancel_fn() is called for I/O submitted via io_uring, the
following kernel warning appears:

WARNING: CPU: 3 PID: 368 at fs/aio.c:598 kiocb_set_cancel_fn+0x9c/0xa8
Call trace:
 kiocb_set_cancel_fn+0x9c/0xa8
 ffs_epfile_read_iter+0x144/0x1d0
 io_read+0x19c/0x498
 io_issue_sqe+0x118/0x27c
 io_submit_sqes+0x25c/0x5fc
 __arm64_sys_io_uring_enter+0x104/0xab0
 invoke_syscall+0x58/0x11c
 el0_svc_common+0xb4/0xf4
 do_el0_svc+0x2c/0xb0
 el0_svc+0x2c/0xa4
 el0t_64_sync_handler+0x68/0xb4
 el0t_64_sync+0x1a4/0x1a8

Fix this by setting the IOCB_AIO_RW flag for read and write I/O that is
submitted by libaio.

Suggested-by: Jens Axboe <axboe@kernel.dk>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Avi Kivity <avi@scylladb.com>
Cc: Sandeep Dhavale <dhavale@google.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: <stable@vger.kernel.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 fs/aio.c           | 9 ++++++++-
 include/linux/fs.h | 2 ++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/aio.c b/fs/aio.c
index bb2ff48991f3..da18dbcfcb22 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -593,6 +593,13 @@ void kiocb_set_cancel_fn(struct kiocb *iocb, kiocb_cancel_fn *cancel)
 	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;
 
@@ -1509,7 +1516,7 @@ static int aio_prep_rw(struct kiocb *req, const struct iocb *iocb)
 	req->ki_complete = aio_complete_rw;
 	req->private = NULL;
 	req->ki_pos = iocb->aio_offset;
-	req->ki_flags = req->ki_filp->f_iocb_flags;
+	req->ki_flags = req->ki_filp->f_iocb_flags | IOCB_AIO_RW;
 	if (iocb->aio_flags & IOCB_FLAG_RESFD)
 		req->ki_flags |= IOCB_EVENTFD;
 	if (iocb->aio_flags & IOCB_FLAG_IOPRIO) {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c9deac59c29a..d47306fe1121 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -352,6 +352,8 @@ enum rw_hint {
  * unrelated IO (like cache flushing, new IO generation, etc).
  */
 #define IOCB_DIO_CALLER_COMP	(1 << 22)
+/* kiocb is a read or write operation submitted by fs/aio.c. */
+#define IOCB_AIO_RW		(1 << 23)
 
 /* for use in trace events */
 #define TRACE_IOCB_STRINGS \

[PATCH v4 2/2] fs/aio: Make io_cancel() generate completions again

[Bart Van Assche]

The following patch accidentally removed the code for delivering
completions for cancelled reads and writes to user space: "[PATCH 04/33]
aio: remove retry-based AIO"
(https://lore.kernel.org/all/1363883754-27966-5-git-send-email-koverstreet@google.com/)
From that patch:

-	if (kiocbIsCancelled(iocb)) {
-		ret = -EINTR;
-		aio_complete(iocb, ret, 0);
-		/* must not access the iocb after this */
-		goto out;
-	}

This leads to a leak in user space of a struct iocb. Hence this patch
that restores the code that reports to user space that a read or write
has been cancelled successfully.

Fixes: 41003a7bcfed ("aio: remove retry-based AIO")
Cc: Christoph Hellwig <hch@lst.de>
Cc: Avi Kivity <avi@scylladb.com>
Cc: Sandeep Dhavale <dhavale@google.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: <stable@vger.kernel.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 fs/aio.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index da18dbcfcb22..28223f511931 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -2165,14 +2165,11 @@ COMPAT_SYSCALL_DEFINE3(io_submit, compat_aio_context_t, ctx_id,
 #endif
 
 /* sys_io_cancel:
- *	Attempts to cancel an iocb previously passed to io_submit.  If
- *	the operation is successfully cancelled, the resulting event is
- *	copied into the memory pointed to by result without being placed
- *	into the completion queue and 0 is returned.  May fail with
- *	-EFAULT if any of the data structures pointed to are invalid.
- *	May fail with -EINVAL if aio_context specified by ctx_id is
- *	invalid.  May fail with -EAGAIN if the iocb specified was not
- *	cancelled.  Will fail with -ENOSYS if not implemented.
+ *	Attempts to cancel an iocb previously passed to io_submit(). If the
+ *	operation is successfully cancelled 0 is returned. May fail with
+ *	-EFAULT if any of the data structures pointed to are invalid. May
+ *	fail with -EINVAL if aio_context specified by ctx_id is invalid. Will
+ *	fail with -ENOSYS if not implemented.
  */
 SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, struct iocb __user *, iocb,
 		struct io_event __user *, result)
@@ -2203,14 +2200,12 @@ SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, struct iocb __user *, iocb,
 	}
 	spin_unlock_irq(&ctx->ctx_lock);
 
-	if (!ret) {
-		/*
-		 * The result argument is no longer used - the io_event is
-		 * always delivered via the ring buffer. -EINPROGRESS indicates
-		 * cancellation is progress:
-		 */
-		ret = -EINPROGRESS;
-	}
+	/*
+	 * The result argument is no longer used - the io_event is always
+	 * delivered via the ring buffer.
+	 */
+	if (ret == 0 && kiocb->rw.ki_flags & IOCB_AIO_RW)
+		aio_complete_rw(&kiocb->rw, -EINTR);
 
 	percpu_ref_put(&ctx->users);
 

Re: [PATCH v4 2/2] fs/aio: Make io_cancel() generate completions again

[Christoph Hellwig]

On Thu, Feb 15, 2024 at 12:47:39PM -0800, Bart Van Assche wrote:
> The following patch accidentally removed the code for delivering
> completions for cancelled reads and writes to user space: "[PATCH 04/33]
> aio: remove retry-based AIO"
> (https://lore.kernel.org/all/1363883754-27966-5-git-send-email-koverstreet@google.com/)

Umm, that was more than 10 years ago.  What code do you have that
is this old, and only noticed that it needs the completions now?

I'd much prefer your older patch to simply always fail the cancelations.

Re: [PATCH v4 2/2] fs/aio: Make io_cancel() generate completions again

[Christian Brauner]

On Fri, Feb 16, 2024 at 08:13:25AM +0100, Christoph Hellwig wrote:
> On Thu, Feb 15, 2024 at 12:47:39PM -0800, Bart Van Assche wrote:
> > The following patch accidentally removed the code for delivering
> > completions for cancelled reads and writes to user space: "[PATCH 04/33]
> > aio: remove retry-based AIO"
> > (https://lore.kernel.org/all/1363883754-27966-5-git-send-email-koverstreet@google.com/)
> 
> Umm, that was more than 10 years ago.  What code do you have that
> is this old, and only noticed that it needs the completions now?
> 
> I'd much prefer your older patch to simply always fail the cancelations.

Hm, if no one noticed for that long then I agree that we should probably
try and get rid of this.

Re: [PATCH v4 2/2] fs/aio: Make io_cancel() generate completions again

[Bart Van Assche]

On 2/15/24 23:13, Christoph Hellwig wrote:
> On Thu, Feb 15, 2024 at 12:47:39PM -0800, Bart Van Assche wrote:
>> The following patch accidentally removed the code for delivering
>> completions for cancelled reads and writes to user space: "[PATCH 04/33]
>> aio: remove retry-based AIO"
>> (https://lore.kernel.org/all/1363883754-27966-5-git-send-email-koverstreet@google.com/)
> 
> Umm, that was more than 10 years ago.  What code do you have that
> is this old, and only noticed that it needs the completions now?

USB cancellation is being used and still works. It's only the completions
that are missing.

This patch was submitted less than two years ago and fixes a bug in the adb
daemon: "adbd: Dequeue pending USB write requests upon receiving CNXN"
(https://android.googlesource.com/platform/packages/modules/adb/+/4dd4da41e6b004fa0d49575ac87e8db877f3a116).
My questions about that patch are as follows (I have not yet found an adbd
expert who can help me with answering these questions):
* Are the io_cancel() calls racy? If the io_cancel() calls are delayed, will
   this break the fix for the bug described in the patch description?
* Should the reported bug perhaps be fixed by improving the adb protocol,
   e.g. by including a session ID in the adb protocol header?

Thanks,

Bart.

Re: [PATCH v4 0/2] Fix libaio cancellation support

[Christian Brauner]

On Thu, 15 Feb 2024 12:47:37 -0800, Bart Van Assche wrote:
> This patch series fixes cancellation support in libaio as follows:
>  - Restore the code for completing cancelled I/O.
>  - Ignore requests to support cancellation for I/O not submitted by libaio.

And the libaio cancellation code itself is safe?

> 
> Please consider this patch series for the next merge window.
> 
> Thanks,
> 
> [...]

Cancellation function appropriately named ffs_aio_cancel(). ;/

--

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/2] fs/aio: Restrict kiocb_set_cancel_fn() to I/O submitted via libaio
      https://git.kernel.org/vfs/vfs/c/34c6ea2e3aea
[2/2] fs/aio: Make io_cancel() generate completions again
      https://git.kernel.org/vfs/vfs/c/ee347c5af5be

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

[Jens Axboe]

On 2/15/24 1:47 PM, Bart Van Assche wrote:
> If kiocb_set_cancel_fn() is called for I/O submitted via io_uring, the
> following kernel warning appears:
> 
> WARNING: CPU: 3 PID: 368 at fs/aio.c:598 kiocb_set_cancel_fn+0x9c/0xa8
> Call trace:
>  kiocb_set_cancel_fn+0x9c/0xa8
>  ffs_epfile_read_iter+0x144/0x1d0
>  io_read+0x19c/0x498
>  io_issue_sqe+0x118/0x27c
>  io_submit_sqes+0x25c/0x5fc
>  __arm64_sys_io_uring_enter+0x104/0xab0
>  invoke_syscall+0x58/0x11c
>  el0_svc_common+0xb4/0xf4
>  do_el0_svc+0x2c/0xb0
>  el0_svc+0x2c/0xa4
>  el0t_64_sync_handler+0x68/0xb4
>  el0t_64_sync+0x1a4/0x1a8
> 
> Fix this by setting the IOCB_AIO_RW flag for read and write I/O that is
> submitted by libaio.

Like I said weeks ago, let's please get this fix in NOW and we can
debate what to do about cancelations in general for aio separately. This
patch 1 is a real fix, and it'd be silly to keep this stalled while
the other stuff is ongoing.

This isn't a critique at at you Bart, really just wanted to reply to teh
cover letter but there isn't one.

Christian, can you queue this up for 6.8 and mark it for stable?

-- 
Jens Axboe

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

[Christian Brauner]

On Thu, 15 Feb 2024 12:47:38 -0800, Bart Van Assche wrote:
> If kiocb_set_cancel_fn() is called for I/O submitted via io_uring, the
> following kernel warning appears:
> 
> WARNING: CPU: 3 PID: 368 at fs/aio.c:598 kiocb_set_cancel_fn+0x9c/0xa8
> Call trace:
>  kiocb_set_cancel_fn+0x9c/0xa8
>  ffs_epfile_read_iter+0x144/0x1d0
>  io_read+0x19c/0x498
>  io_issue_sqe+0x118/0x27c
>  io_submit_sqes+0x25c/0x5fc
>  __arm64_sys_io_uring_enter+0x104/0xab0
>  invoke_syscall+0x58/0x11c
>  el0_svc_common+0xb4/0xf4
>  do_el0_svc+0x2c/0xb0
>  el0_svc+0x2c/0xa4
>  el0t_64_sync_handler+0x68/0xb4
>  el0t_64_sync+0x1a4/0x1a8
> 
> [...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/2] fs/aio: Restrict kiocb_set_cancel_fn() to I/O submitted via libaio
      https://git.kernel.org/vfs/vfs/c/b820de741ae4

Re: [PATCH v4 0/2] Fix libaio cancellation support

[Bart Van Assche]

On 2/21/24 01:26, Christian Brauner wrote:
> On Thu, 15 Feb 2024 12:47:37 -0800, Bart Van Assche wrote:
>> This patch series fixes cancellation support in libaio as follows:
>>   - Restore the code for completing cancelled I/O.
>>   - Ignore requests to support cancellation for I/O not submitted by libaio.
> 
> And the libaio cancellation code itself is safe?

As far as I can see the libaio cancellation code is safe.

Thanks,

Bart.

Re: [PATCH v4 0/2] Fix libaio cancellation support

[Bart Van Assche]

On 2/21/24 01:26, Christian Brauner wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> branch: vfs.misc
> 
> [1/2] fs/aio: Restrict kiocb_set_cancel_fn() to I/O submitted via libaio
>        https://git.kernel.org/vfs/vfs/c/34c6ea2e3aea
> [2/2] fs/aio: Make io_cancel() generate completions again
>        https://git.kernel.org/vfs/vfs/c/ee347c5af5be

Patch [1/2] ended up in Linus' tree as commit b820de741ae4, which is
great. However, I can't find patch [2/2] in any vfs branch nor in
linux-next. Did I perhaps overlook something?

$ git branch -r | grep vfs/ | while read b; do PAGER= git log 
--format=oneline origin/master..$b fs/aio.c; done
83c671617c943c7e369de96d95cc94739d42bcca eventfd: simplify eventfd_signal()
1c146b0406bddf3769705e4cd31a422a6564ab7b fs/aio: obey min_nr when doing 
wakeups
1808acc4fab22bed2f259dcdbc3138333caac676 eventfd: simplify eventfd_signal()
6e44f043186bf108c353353acc21adca4c3d73db fs: introduce fs_supers_lock

Thanks,

Bart.

Re: [PATCH v4 0/2] Fix libaio cancellation support

[Christian Brauner]

On Mon, Feb 26, 2024 at 12:50:46PM -0800, Bart Van Assche wrote:
> On 2/21/24 01:26, Christian Brauner wrote:
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> > branch: vfs.misc
> > 
> > [1/2] fs/aio: Restrict kiocb_set_cancel_fn() to I/O submitted via libaio
> >        https://git.kernel.org/vfs/vfs/c/34c6ea2e3aea
> > [2/2] fs/aio: Make io_cancel() generate completions again
> >        https://git.kernel.org/vfs/vfs/c/ee347c5af5be
> 
> Patch [1/2] ended up in Linus' tree as commit b820de741ae4, which is
> great. However, I can't find patch [2/2] in any vfs branch nor in
> linux-next. Did I perhaps overlook something?

1/2 was supposed to be a bugfix that we had to do right away while 2/2
wasn't that urgent iirc. @Jens, can I get an Ack on the second patch?

Re: (subset) [PATCH v4 2/2] fs/aio: Make io_cancel() generate completions again

[Christian Brauner]

On Thu, 15 Feb 2024 12:47:39 -0800, Bart Van Assche wrote:
> The following patch accidentally removed the code for delivering
> completions for cancelled reads and writes to user space: "[PATCH 04/33]
> aio: remove retry-based AIO"
> (https://lore.kernel.org/all/1363883754-27966-5-git-send-email-koverstreet@google.com/)
> >From that patch:
> 
> -	if (kiocbIsCancelled(iocb)) {
> -		ret = -EINTR;
> -		aio_complete(iocb, ret, 0);
> -		/* must not access the iocb after this */
> -		goto out;
> -	}
> 
> [...]

@Jens, please take another look at this.

---

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[2/2] fs/aio: Make io_cancel() generate completions again
      https://git.kernel.org/vfs/vfs/c/ff365e6bc31c

Re: [PATCH v4 0/2] Fix libaio cancellation support

[Eric Biggers]

On Thu, Feb 15, 2024 at 12:47:37PM -0800, Bart Van Assche wrote:
> [PATCH v4 0/2] Fix libaio cancellation support

libaio is a userspace library (https://pagure.io/libaio).  In the future please
make it clear when actually talking about the kernel.  This patchset didn't
catch my attention quite as much as it should have, because it just said libaio.

- Eric

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

[Eric Biggers]

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;

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?

- Eric

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

[Bart Van Assche]

On 3/4/24 11:10, Eric Biggers 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;

Hi Eric,

Thanks for having reported this. I agree that this needs to be fixed.

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

As far as I know no Linux user space interface for submitting I/O 
supports cancellation of read or write requests other than the AIO
io_cancel() system call.

It would make it easier to maintain the kernel if I/O cancellation
support would be removed. However, there is existing user space code
that depends on USB I/O cancellation so I'm not sure how to proceed to 
remove AIO io_cancel() support from the kernel.

Thanks,

Bart.

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

[Jens Axboe]

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

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

[Jens Axboe]

On 3/4/24 12:43 PM, Bart Van Assche wrote:
>> 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?
> 
> As far as I know no Linux user space interface for submitting I/O
> supports cancellation of read or write requests other than the AIO
> io_cancel() system call.

Not true, see previous reply (on both points in this email). The kernel
in general does not support cancelation of regular file/storage IO that
has submitted. That includes aio. There are many reasons for this.

For anything but that, you can most certainly cancel inflight IO with
io_uring, be it to a socket, pipe, whatever.

The problem here isn't that only aio supports cancelations, it's that
the code to do so is a bad hack.

-- 
Jens Axboe

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

[Eric Biggers]

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.

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

- Eric

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

[Jens Axboe]

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

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

[Bart Van Assche]

On 3/4/24 12:21, Jens Axboe wrote:
> On 3/4/24 12:43 PM, Bart Van Assche wrote:
>> As far as I know no Linux user space interface for submitting I/O
>> supports cancellation of read or write requests other than the AIO
>> io_cancel() system call.
> 
> Not true, see previous reply (on both points in this email). The kernel
> in general does not support cancelation of regular file/storage IO that
> has submitted. That includes aio. There are many reasons for this.
> 
> For anything but that, you can most certainly cancel inflight IO with
> io_uring, be it to a socket, pipe, whatever.
> 
> The problem here isn't that only aio supports cancelations, it's that
> the code to do so is a bad hack.

What I meant is that the AIO code is the only code I know of that
supports cancelling I/O from user space after the I/O has been submitted
to the driver that will process the I/O request (e.g. a USB driver). Is
my understanding correct that io_uring cancellation involves setting the
IO_WQ_WORK_CANCEL flag and also that that flag is ignored by
io_wq_submit_work() after io_assign_file() has been called? The AIO code
supports cancelling I/O after call_read_iter() or call_write_iter() has
been called.

Thanks,

Bart.

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

[Jens Axboe]

On 3/5/24 1:43 PM, Bart Van Assche wrote:
> On 3/4/24 12:21, Jens Axboe wrote:
>> On 3/4/24 12:43 PM, Bart Van Assche wrote:
>>> As far as I know no Linux user space interface for submitting I/O
>>> supports cancellation of read or write requests other than the AIO
>>> io_cancel() system call.
>>
>> Not true, see previous reply (on both points in this email). The kernel
>> in general does not support cancelation of regular file/storage IO that
>> has submitted. That includes aio. There are many reasons for this.
>>
>> For anything but that, you can most certainly cancel inflight IO with
>> io_uring, be it to a socket, pipe, whatever.
>>
>> The problem here isn't that only aio supports cancelations, it's that
>> the code to do so is a bad hack.
> 
> What I meant is that the AIO code is the only code I know of that
> supports cancelling I/O from user space after the I/O has been submitted
> to the driver that will process the I/O request (e.g. a USB driver). Is

Right, we never offered that in general in the kernel. Like I said, it's
just a hack what is there for that.

> my understanding correct that io_uring cancellation involves setting the
> IO_WQ_WORK_CANCEL flag and also that that flag is ignored by
> io_wq_submit_work() after io_assign_file() has been called?

No, that's only for requests that go via io-wq, which is generally not
the fast path and most workloads will never see that.

For anything else, cancelation can very much happen at any time.

> The AIO code
> supports cancelling I/O after call_read_iter() or call_write_iter() has
> been called.

...for the above hacky case, and that's it, not as a generic thing.

-- 
Jens Axboe