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@go...) 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);
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@go...)
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.
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@go...)
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.
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@go...)
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/+/4dd4da41e6b...). 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.
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@go...)
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
linux-stable-mirror@lists.linaro.org