[PATCH 5.10 1/1] io_uring: fix double free in the deferred/cancelled path

[PATCH 5.10 1/1] io_uring: fix double free in the deferred/cancelled path

[Lee Jones]

792bb6eb86233 ("io_uring: don't take uring_lock during iowq cancel")
inadvertently fixed this issue in v5.12.  This patch cherry-picks the
hunk of commit which does so.

Syzbot vomit (snipped):

  ==================================================================
  BUG: KASAN: double-free or invalid-free in slab_free mm/slub.c:3195 [inline]
  BUG: KASAN: double-free or invalid-free in kfree+0xca/0x310 mm/slub.c:4183

  CPU: 1 PID: 431 Comm: syz-executor823 Not tainted 5.10.75-syzkaller-01082-g234d53d2bb60 #0
  Hardware name: Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
  Call Trace:
   __dump_stack lib/dump_stack.c:77 [inline]
   dump_stack_lvl+0x1e2/0x24b lib/dump_stack.c:118
   print_address_description+0x8d/0x3d0 mm/kasan/report.c:233
   kasan_report_invalid_free+0x58/0x130 mm/kasan/report.c:358
   ____kasan_slab_free+0x14b/0x170 mm/kasan/common.c:362
   __kasan_slab_free+0x11/0x20 mm/kasan/common.c:368
   kasan_slab_free include/linux/kasan.h:235 [inline]
   slab_free_hook mm/slub.c:1596 [inline]
   slab_free_freelist_hook+0xb2/0x180 mm/slub.c:1621
   slab_free mm/slub.c:3195 [inline]
   kfree+0xca/0x310 mm/slub.c:4183
   __io_queue_deferred fs/io_uring.c:1541 [inline]
   io_commit_cqring+0x76a/0xa00 fs/io_uring.c:1587
   io_iopoll_complete fs/io_uring.c:2378 [inline]
   io_do_iopoll+0x1e18/0x23f0 fs/io_uring.c:2431
   io_iopoll_try_reap_events+0x116/0x290 fs/io_uring.c:2470
   io_ring_ctx_wait_and_kill+0x295/0x670 fs/io_uring.c:8575
   io_uring_release+0x5b/0x70 fs/io_uring.c:8602
   __fput+0x348/0x7d0 fs/file_table.c:281
   ____fput+0x15/0x20 fs/file_table.c:314
   task_work_run+0x147/0x1b0 kernel/task_work.c:154
   exit_task_work include/linux/task_work.h:30 [inline]
   do_exit+0x70e/0x23a0 kernel/exit.c:813
   do_group_exit+0x16a/0x2d0 kernel/exit.c:910
   __do_sys_exit_group+0x17/0x20 kernel/exit.c:921
   __se_sys_exit_group+0x14/0x20 kernel/exit.c:919
   __x64_sys_exit_group+0x3b/0x40 kernel/exit.c:919
   do_syscall_64+0x31/0x70 arch/x86/entry/common.c:46
   entry_SYSCALL_64_after_hwframe+0x44/0xa9

Cc: stable <stable@vger.kernel.org> # 5.10.x
Reported-by: syzbot+59d8a1f4e60c20c066cf@syzkaller.appspotmail.com
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 fs/io_uring.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 26753d0cb4312..361f8ae96c36f 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2075,7 +2075,9 @@ static void io_req_task_cancel(struct callback_head *cb)
 	struct io_kiocb *req = container_of(cb, struct io_kiocb, task_work);
 	struct io_ring_ctx *ctx = req->ctx;
 
+	mutex_lock(&ctx->uring_lock);
 	__io_req_task_cancel(req, -ECANCELED);
+	mutex_unlock(&ctx->uring_lock);
 	percpu_ref_put(&ctx->refs);
 }
 
-- 
2.33.0.1079.g6e70778dc9-goog

Re: [PATCH 5.10 1/1] io_uring: fix double free in the deferred/cancelled path

[Greg KH]

On Wed, Oct 27, 2021 at 09:01:28AM +0100, Lee Jones wrote:
> 792bb6eb86233 ("io_uring: don't take uring_lock during iowq cancel")
> inadvertently fixed this issue in v5.12.  This patch cherry-picks the
> hunk of commit which does so.

Why can't we take all of that commit?  Why only part of it?

thanks,

greg k-h

Re: [PATCH 5.10 1/1] io_uring: fix double free in the deferred/cancelled path

[Lee Jones]

On Wed, 27 Oct 2021, Greg KH wrote:

> On Wed, Oct 27, 2021 at 09:01:28AM +0100, Lee Jones wrote:
> > 792bb6eb86233 ("io_uring: don't take uring_lock during iowq cancel")
> > inadvertently fixed this issue in v5.12.  This patch cherry-picks the
> > hunk of commit which does so.
> 
> Why can't we take all of that commit?  Why only part of it?

I don't know.

Why didn't the Stable team take it further than v5.11.y?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 5.10 1/1] io_uring: fix double free in the deferred/cancelled path

[Greg KH]

On Wed, Oct 27, 2021 at 09:37:59AM +0100, Lee Jones wrote:
> On Wed, 27 Oct 2021, Greg KH wrote:
> 
> > On Wed, Oct 27, 2021 at 09:01:28AM +0100, Lee Jones wrote:
> > > 792bb6eb86233 ("io_uring: don't take uring_lock during iowq cancel")
> > > inadvertently fixed this issue in v5.12.  This patch cherry-picks the
> > > hunk of commit which does so.
> > 
> > Why can't we take all of that commit?  Why only part of it?
> 
> I don't know.
> 
> Why didn't the Stable team take it further than v5.11.y?

Look in the archives?  Did it not apply cleanly?

/me goes off and looks...

Looks like I asked for a backport, but no one did it, I only received a
5.11 version:
	https://lore.kernel.org/r/1839646480a26a2461eccc38a75e98998d2d6e11.1615375332.git.asml.silence@gmail.com

so a 5.10 version would be nice, as I said it failed as-is:
	https://lore.kernel.org/all/161460075611654@kroah.com/

lore archives are your friend :)

thanks,

greg k-h

Re: [PATCH 5.10 1/1] io_uring: fix double free in the deferred/cancelled path

[Lee Jones]

On Wed, 27 Oct 2021, Greg KH wrote:

> On Wed, Oct 27, 2021 at 09:37:59AM +0100, Lee Jones wrote:
> > On Wed, 27 Oct 2021, Greg KH wrote:
> > 
> > > On Wed, Oct 27, 2021 at 09:01:28AM +0100, Lee Jones wrote:
> > > > 792bb6eb86233 ("io_uring: don't take uring_lock during iowq cancel")
> > > > inadvertently fixed this issue in v5.12.  This patch cherry-picks the
> > > > hunk of commit which does so.
> > > 
> > > Why can't we take all of that commit?  Why only part of it?
> > 
> > I don't know.
> > 
> > Why didn't the Stable team take it further than v5.11.y?
> 
> Look in the archives?  Did it not apply cleanly?
> 
> /me goes off and looks...
> 
> Looks like I asked for a backport, but no one did it, I only received a
> 5.11 version:
> 	https://lore.kernel.org/r/1839646480a26a2461eccc38a75e98998d2d6e11.1615375332.git.asml.silence@gmail.com
> 
> so a 5.10 version would be nice, as I said it failed as-is:
> 	https://lore.kernel.org/all/161460075611654@kroah.com/

Precisely.  This is the answer to your question:

  > > > Why can't we take all of that commit?  Why only part of it?

Same reason the Stable team didn't back-port it - it doesn't apply.

The second hunk is only relevant to v5.11+.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 5.10 1/1] io_uring: fix double free in the deferred/cancelled path

[Greg KH]

On Wed, Oct 27, 2021 at 10:03:01AM +0100, Lee Jones wrote:
> On Wed, 27 Oct 2021, Greg KH wrote:
> 
> > On Wed, Oct 27, 2021 at 09:37:59AM +0100, Lee Jones wrote:
> > > On Wed, 27 Oct 2021, Greg KH wrote:
> > > 
> > > > On Wed, Oct 27, 2021 at 09:01:28AM +0100, Lee Jones wrote:
> > > > > 792bb6eb86233 ("io_uring: don't take uring_lock during iowq cancel")
> > > > > inadvertently fixed this issue in v5.12.  This patch cherry-picks the
> > > > > hunk of commit which does so.
> > > > 
> > > > Why can't we take all of that commit?  Why only part of it?
> > > 
> > > I don't know.
> > > 
> > > Why didn't the Stable team take it further than v5.11.y?
> > 
> > Look in the archives?  Did it not apply cleanly?
> > 
> > /me goes off and looks...
> > 
> > Looks like I asked for a backport, but no one did it, I only received a
> > 5.11 version:
> > 	https://lore.kernel.org/r/1839646480a26a2461eccc38a75e98998d2d6e11.1615375332.git.asml.silence@gmail.com
> > 
> > so a 5.10 version would be nice, as I said it failed as-is:
> > 	https://lore.kernel.org/all/161460075611654@kroah.com/
> 
> Precisely.  This is the answer to your question:
> 
>   > > > Why can't we take all of that commit?  Why only part of it?
> 
> Same reason the Stable team didn't back-port it - it doesn't apply.
> 
> The second hunk is only relevant to v5.11+.

Great, then use the "normal" stable style, but down in the s-o-b area
say "dropped second chunk as it is not relevant to 5.10.y".

thanks,

greg k-h

Re: [PATCH 5.10 1/1] io_uring: fix double free in the deferred/cancelled path

[Lee Jones]

On Wed, 27 Oct 2021, Greg KH wrote:

> On Wed, Oct 27, 2021 at 10:03:01AM +0100, Lee Jones wrote:
> > On Wed, 27 Oct 2021, Greg KH wrote:
> > 
> > > On Wed, Oct 27, 2021 at 09:37:59AM +0100, Lee Jones wrote:
> > > > On Wed, 27 Oct 2021, Greg KH wrote:
> > > > 
> > > > > On Wed, Oct 27, 2021 at 09:01:28AM +0100, Lee Jones wrote:
> > > > > > 792bb6eb86233 ("io_uring: don't take uring_lock during iowq cancel")
> > > > > > inadvertently fixed this issue in v5.12.  This patch cherry-picks the
> > > > > > hunk of commit which does so.
> > > > > 
> > > > > Why can't we take all of that commit?  Why only part of it?
> > > > 
> > > > I don't know.
> > > > 
> > > > Why didn't the Stable team take it further than v5.11.y?
> > > 
> > > Look in the archives?  Did it not apply cleanly?
> > > 
> > > /me goes off and looks...
> > > 
> > > Looks like I asked for a backport, but no one did it, I only received a
> > > 5.11 version:
> > > 	https://lore.kernel.org/r/1839646480a26a2461eccc38a75e98998d2d6e11.1615375332.git.asml.silence@gmail.com
> > > 
> > > so a 5.10 version would be nice, as I said it failed as-is:
> > > 	https://lore.kernel.org/all/161460075611654@kroah.com/
> > 
> > Precisely.  This is the answer to your question:
> > 
> >   > > > Why can't we take all of that commit?  Why only part of it?
> > 
> > Same reason the Stable team didn't back-port it - it doesn't apply.
> > 
> > The second hunk is only relevant to v5.11+.
> 
> Great, then use the "normal" stable style, but down in the s-o-b area
> say "dropped second chunk as it is not relevant to 5.10.y".

Just to clarify, by "normal", you mean:

 - Take the original patch
 - Apply an "[ Upstream commit <id> ]" tag (or similar)
 - Remove the hunk that doesn't apply
 - Make a note of the aforementioned action
 - Submit to Stable

Rather than submitting a bespoke patch.  Right?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 5.10 1/1] io_uring: fix double free in the deferred/cancelled path

[Greg KH]

On Wed, Oct 27, 2021 at 03:00:20PM +0100, Lee Jones wrote:
> On Wed, 27 Oct 2021, Greg KH wrote:
> 
> > On Wed, Oct 27, 2021 at 10:03:01AM +0100, Lee Jones wrote:
> > > On Wed, 27 Oct 2021, Greg KH wrote:
> > > 
> > > > On Wed, Oct 27, 2021 at 09:37:59AM +0100, Lee Jones wrote:
> > > > > On Wed, 27 Oct 2021, Greg KH wrote:
> > > > > 
> > > > > > On Wed, Oct 27, 2021 at 09:01:28AM +0100, Lee Jones wrote:
> > > > > > > 792bb6eb86233 ("io_uring: don't take uring_lock during iowq cancel")
> > > > > > > inadvertently fixed this issue in v5.12.  This patch cherry-picks the
> > > > > > > hunk of commit which does so.
> > > > > > 
> > > > > > Why can't we take all of that commit?  Why only part of it?
> > > > > 
> > > > > I don't know.
> > > > > 
> > > > > Why didn't the Stable team take it further than v5.11.y?
> > > > 
> > > > Look in the archives?  Did it not apply cleanly?
> > > > 
> > > > /me goes off and looks...
> > > > 
> > > > Looks like I asked for a backport, but no one did it, I only received a
> > > > 5.11 version:
> > > > 	https://lore.kernel.org/r/1839646480a26a2461eccc38a75e98998d2d6e11.1615375332.git.asml.silence@gmail.com
> > > > 
> > > > so a 5.10 version would be nice, as I said it failed as-is:
> > > > 	https://lore.kernel.org/all/161460075611654@kroah.com/
> > > 
> > > Precisely.  This is the answer to your question:
> > > 
> > >   > > > Why can't we take all of that commit?  Why only part of it?
> > > 
> > > Same reason the Stable team didn't back-port it - it doesn't apply.
> > > 
> > > The second hunk is only relevant to v5.11+.
> > 
> > Great, then use the "normal" stable style, but down in the s-o-b area
> > say "dropped second chunk as it is not relevant to 5.10.y".
> 
> Just to clarify, by "normal", you mean:
> 
>  - Take the original patch
>  - Apply an "[ Upstream commit <id> ]" tag (or similar)
>  - Remove the hunk that doesn't apply
>  - Make a note of the aforementioned action
>  - Submit to Stable

Yes.

> Rather than submitting a bespoke patch.  Right?

Correct.

thanks,

greg k-h

Re: [PATCH 5.10 1/1] io_uring: fix double free in the deferred/cancelled path

[Lee Jones]

On Wed, 27 Oct 2021, Greg KH wrote:

> On Wed, Oct 27, 2021 at 03:00:20PM +0100, Lee Jones wrote:
> > On Wed, 27 Oct 2021, Greg KH wrote:
> > 
> > > On Wed, Oct 27, 2021 at 10:03:01AM +0100, Lee Jones wrote:
> > > > On Wed, 27 Oct 2021, Greg KH wrote:
> > > > 
> > > > > On Wed, Oct 27, 2021 at 09:37:59AM +0100, Lee Jones wrote:
> > > > > > On Wed, 27 Oct 2021, Greg KH wrote:
> > > > > > 
> > > > > > > On Wed, Oct 27, 2021 at 09:01:28AM +0100, Lee Jones wrote:
> > > > > > > > 792bb6eb86233 ("io_uring: don't take uring_lock during iowq cancel")
> > > > > > > > inadvertently fixed this issue in v5.12.  This patch cherry-picks the
> > > > > > > > hunk of commit which does so.
> > > > > > > 
> > > > > > > Why can't we take all of that commit?  Why only part of it?
> > > > > > 
> > > > > > I don't know.
> > > > > > 
> > > > > > Why didn't the Stable team take it further than v5.11.y?
> > > > > 
> > > > > Look in the archives?  Did it not apply cleanly?
> > > > > 
> > > > > /me goes off and looks...
> > > > > 
> > > > > Looks like I asked for a backport, but no one did it, I only received a
> > > > > 5.11 version:
> > > > > 	https://lore.kernel.org/r/1839646480a26a2461eccc38a75e98998d2d6e11.1615375332.git.asml.silence@gmail.com
> > > > > 
> > > > > so a 5.10 version would be nice, as I said it failed as-is:
> > > > > 	https://lore.kernel.org/all/161460075611654@kroah.com/
> > > > 
> > > > Precisely.  This is the answer to your question:
> > > > 
> > > >   > > > Why can't we take all of that commit?  Why only part of it?
> > > > 
> > > > Same reason the Stable team didn't back-port it - it doesn't apply.
> > > > 
> > > > The second hunk is only relevant to v5.11+.
> > > 
> > > Great, then use the "normal" stable style, but down in the s-o-b area
> > > say "dropped second chunk as it is not relevant to 5.10.y".
> > 
> > Just to clarify, by "normal", you mean:
> > 
> >  - Take the original patch
> >  - Apply an "[ Upstream commit <id> ]" tag (or similar)
> >  - Remove the hunk that doesn't apply
> >  - Make a note of the aforementioned action
> >  - Submit to Stable
> 
> Yes.
> 
> > Rather than submitting a bespoke patch.  Right?
> 
> Correct.

Got it, thanks.  Wilco.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog