public inbox for [email protected]
 help / color / mirror / Atom feed
* Re: KASAN: use-after-free Read in percpu_ref_switch_to_atomic_rcu
       [not found] <[email protected]>
@ 2020-03-04  7:59 ` Dmitry Vyukov
  2020-03-04 14:40   ` Jens Axboe
  0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Vyukov @ 2020-03-04  7:59 UTC (permalink / raw)
  To: syzbot, Al Viro, io-uring, linux-fsdevel, Jens Axboe
  Cc: Borislav Petkov, H. Peter Anvin, LKML, Ingo Molnar,
	Peter Zijlstra, syzkaller-bugs, Thomas Gleixner, tony.luck,
	the arch/x86 maintainers

On Fri, Feb 7, 2020 at 9:14 AM syzbot
<[email protected]> wrote:
>
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:    4c7d00cc Merge tag 'pwm/for-5.6-rc1' of git://git.kernel.o..
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=12fec785e00000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=e162021ddededa72
> dashboard link: https://syzkaller.appspot.com/bug?extid=e017e49c39ab484ac87a
> compiler:       clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
>
> Unfortunately, I don't have any reproducer for this crash yet.
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: [email protected]

+io_uring maintainers

Here is a repro:
https://gist.githubusercontent.com/dvyukov/6b340beab6483a036f4186e7378882ce/raw/cd1922185516453c201df8eded1d4b006a6d6a3a/gistfile1.txt

> ==================================================================
> BUG: KASAN: use-after-free in percpu_ref_call_confirm_rcu lib/percpu-refcount.c:126 [inline]
> BUG: KASAN: use-after-free in percpu_ref_switch_to_atomic_rcu+0x3f7/0x400 lib/percpu-refcount.c:165
> Read of size 1 at addr ffff8880a8d91830 by task swapper/0/0
>
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.5.0-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
>  <IRQ>
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x1fb/0x318 lib/dump_stack.c:118
>  print_address_description+0x74/0x5c0 mm/kasan/report.c:374
>  __kasan_report+0x149/0x1c0 mm/kasan/report.c:506
>  kasan_report+0x26/0x50 mm/kasan/common.c:641
>  __asan_report_load1_noabort+0x14/0x20 mm/kasan/generic_report.c:132
>  percpu_ref_call_confirm_rcu lib/percpu-refcount.c:126 [inline]
>  percpu_ref_switch_to_atomic_rcu+0x3f7/0x400 lib/percpu-refcount.c:165
>  rcu_do_batch kernel/rcu/tree.c:2186 [inline]
>  rcu_core+0x81b/0x10c0 kernel/rcu/tree.c:2410
>  rcu_core_si+0x9/0x10 kernel/rcu/tree.c:2419
>  __do_softirq+0x283/0x7bd kernel/softirq.c:292
>  invoke_softirq kernel/softirq.c:373 [inline]
>  irq_exit+0x227/0x230 kernel/softirq.c:413
>  exiting_irq arch/x86/include/asm/apic.h:536 [inline]
>  smp_apic_timer_interrupt+0x113/0x280 arch/x86/kernel/apic/apic.c:1137
>  apic_timer_interrupt+0xf/0x20 arch/x86/entry/entry_64.S:829
>  </IRQ>
> RIP: 0010:native_safe_halt+0x12/0x20 arch/x86/include/asm/irqflags.h:61
> Code: 89 d9 80 e1 07 80 c1 03 38 c1 7c ba 48 89 df e8 e4 5f 9d f9 eb b0 cc cc 55 48 89 e5 e9 07 00 00 00 0f 00 2d 62 17 4c 00 fb f4 <5d> c3 66 90 66 2e 0f 1f 84 00 00 00 00 00 55 48 89 e5 e9 07 00 00
> RSP: 0018:ffffffff89207db8 EFLAGS: 00000282 ORIG_RAX: ffffffffffffff13
> RAX: 1ffffffff1255a25 RBX: ffffffff89275b00 RCX: dffffc0000000000
> RDX: 0000000000000000 RSI: ffffffff812c06aa RDI: ffffffff89276344
> RBP: ffffffff89207db8 R08: ffffffff89276358 R09: fffffbfff124eb61
> R10: fffffbfff124eb61 R11: 0000000000000000 R12: 1ffffffff124eb60
> R13: dffffc0000000000 R14: dffffc0000000000 R15: 1ffffffff1255a23
>  arch_safe_halt arch/x86/include/asm/paravirt.h:144 [inline]
>  default_idle+0x50/0x70 arch/x86/kernel/process.c:695
>  arch_cpu_idle+0xa/0x10 arch/x86/kernel/process.c:686
>  default_idle_call+0x59/0xa0 kernel/sched/idle.c:94
>  cpuidle_idle_call kernel/sched/idle.c:154 [inline]
>  do_idle+0x1ec/0x630 kernel/sched/idle.c:269
>  cpu_startup_entry+0x25/0x30 kernel/sched/idle.c:361
>  rest_init+0x29d/0x2b0 init/main.c:450
>  arch_call_rest_init+0xe/0x10
>  start_kernel+0x676/0x777 init/main.c:784
>  x86_64_start_reservations+0x18/0x2e arch/x86/kernel/head64.c:490
>  x86_64_start_kernel+0x7a/0x7d arch/x86/kernel/head64.c:471
>  secondary_startup_64+0xa4/0xb0 arch/x86/kernel/head_64.S:242
>
> Allocated by task 25166:
>  save_stack mm/kasan/common.c:72 [inline]
>  set_track mm/kasan/common.c:80 [inline]
>  __kasan_kmalloc+0x118/0x1c0 mm/kasan/common.c:515
>  kasan_kmalloc+0x9/0x10 mm/kasan/common.c:529
>  kmem_cache_alloc_trace+0x221/0x2f0 mm/slab.c:3551
>  kmalloc include/linux/slab.h:555 [inline]
>  kzalloc include/linux/slab.h:669 [inline]
>  io_sqe_files_register fs/io_uring.c:5528 [inline]
>  __io_uring_register fs/io_uring.c:6875 [inline]
>  __do_sys_io_uring_register fs/io_uring.c:6955 [inline]
>  __se_sys_io_uring_register+0x1df4/0x3260 fs/io_uring.c:6937
>  __x64_sys_io_uring_register+0x9b/0xb0 fs/io_uring.c:6937
>  do_syscall_64+0xf7/0x1c0 arch/x86/entry/common.c:294
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Freed by task 25160:
>  save_stack mm/kasan/common.c:72 [inline]
>  set_track mm/kasan/common.c:80 [inline]
>  kasan_set_free_info mm/kasan/common.c:337 [inline]
>  __kasan_slab_free+0x12e/0x1e0 mm/kasan/common.c:476
>  kasan_slab_free+0xe/0x10 mm/kasan/common.c:485
>  __cache_free mm/slab.c:3426 [inline]
>  kfree+0x10d/0x220 mm/slab.c:3757
>  io_sqe_files_unregister+0x238/0x2b0 fs/io_uring.c:5250
>  io_ring_ctx_free fs/io_uring.c:6229 [inline]
>  io_ring_ctx_wait_and_kill+0x343d/0x3b00 fs/io_uring.c:6310
>  io_uring_release+0x5d/0x70 fs/io_uring.c:6318
>  __fput+0x2e4/0x740 fs/file_table.c:280
>  ____fput+0x15/0x20 fs/file_table.c:313
>  task_work_run+0x176/0x1b0 kernel/task_work.c:113
>  tracehook_notify_resume include/linux/tracehook.h:188 [inline]
>  exit_to_usermode_loop arch/x86/entry/common.c:164 [inline]
>  prepare_exit_to_usermode+0x480/0x5b0 arch/x86/entry/common.c:195
>  syscall_return_slowpath+0x113/0x4a0 arch/x86/entry/common.c:278
>  do_syscall_64+0x11f/0x1c0 arch/x86/entry/common.c:304
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> The buggy address belongs to the object at ffff8880a8d91800
>  which belongs to the cache kmalloc-256 of size 256
> The buggy address is located 48 bytes inside of
>  256-byte region [ffff8880a8d91800, ffff8880a8d91900)
> The buggy address belongs to the page:
> page:ffffea0002a36440 refcount:1 mapcount:0 mapping:ffff8880aa4008c0 index:0x0
> flags: 0xfffe0000000200(slab)
> raw: 00fffe0000000200 ffffea000265d2c8 ffffea00022f7948 ffff8880aa4008c0
> raw: 0000000000000000 ffff8880a8d91000 0000000100000008 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
>  ffff8880a8d91700: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>  ffff8880a8d91780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >ffff8880a8d91800: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>                                      ^
>  ffff8880a8d91880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>  ffff8880a8d91900: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ==================================================================
>
>
> ---
> This bug is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at [email protected].
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/00000000000067c6df059df7f9f5%40google.com.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: KASAN: use-after-free Read in percpu_ref_switch_to_atomic_rcu
  2020-03-04  7:59 ` KASAN: use-after-free Read in percpu_ref_switch_to_atomic_rcu Dmitry Vyukov
@ 2020-03-04 14:40   ` Jens Axboe
  2020-03-06 14:35     ` Dan Carpenter
  2020-03-06 14:57     ` Jann Horn
  0 siblings, 2 replies; 12+ messages in thread
From: Jens Axboe @ 2020-03-04 14:40 UTC (permalink / raw)
  To: Dmitry Vyukov, syzbot, Al Viro, io-uring, linux-fsdevel
  Cc: Borislav Petkov, H. Peter Anvin, LKML, Ingo Molnar,
	Peter Zijlstra, syzkaller-bugs, Thomas Gleixner, tony.luck,
	the arch/x86 maintainers

On 3/4/20 12:59 AM, Dmitry Vyukov wrote:
> On Fri, Feb 7, 2020 at 9:14 AM syzbot
> <[email protected]> wrote:
>>
>> Hello,
>>
>> syzbot found the following crash on:
>>
>> HEAD commit:    4c7d00cc Merge tag 'pwm/for-5.6-rc1' of git://git.kernel.o..
>> git tree:       upstream
>> console output: https://syzkaller.appspot.com/x/log.txt?x=12fec785e00000
>> kernel config:  https://syzkaller.appspot.com/x/.config?x=e162021ddededa72
>> dashboard link: https://syzkaller.appspot.com/bug?extid=e017e49c39ab484ac87a
>> compiler:       clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
>>
>> Unfortunately, I don't have any reproducer for this crash yet.
>>
>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>> Reported-by: [email protected]
> 
> +io_uring maintainers
> 
> Here is a repro:
> https://gist.githubusercontent.com/dvyukov/6b340beab6483a036f4186e7378882ce/raw/cd1922185516453c201df8eded1d4b006a6d6a3a/gistfile1.txt

I've queued up a fix for this:

https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.6&id=9875fe3dc4b8cff1f1b440fb925054a5124403c3

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: KASAN: use-after-free Read in percpu_ref_switch_to_atomic_rcu
  2020-03-04 14:40   ` Jens Axboe
@ 2020-03-06 14:35     ` Dan Carpenter
  2020-03-06 14:57       ` Jens Axboe
  2020-03-06 14:57     ` Jann Horn
  1 sibling, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2020-03-06 14:35 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Dmitry Vyukov, syzbot, Al Viro, io-uring, linux-fsdevel,
	Borislav Petkov, H. Peter Anvin, LKML, Ingo Molnar,
	Peter Zijlstra, syzkaller-bugs, Thomas Gleixner, tony.luck,
	the arch/x86 maintainers


There a bunch of similar bugs.  It's seems a common anti-pattern.

block/blk-cgroup.c:85 blkg_free() warn: freeing 'blkg' which has percpu_ref_exit()
block/blk-core.c:558 blk_alloc_queue_node() warn: freeing 'q' which has percpu_ref_exit()
drivers/md/md.c:5528 md_free() warn: freeing 'mddev' which has percpu_ref_exit()
drivers/target/target_core_transport.c:583 transport_free_session() warn: freeing 'se_sess' which has percpu_ref_exit()
fs/aio.c:592 free_ioctx() warn: freeing 'ctx' which has percpu_ref_exit()
fs/aio.c:806 ioctx_alloc() warn: freeing 'ctx' which has percpu_ref_exit()
fs/io_uring.c:6115 io_sqe_files_unregister() warn: freeing 'data' which has percpu_ref_exit()
fs/io_uring.c:6431 io_sqe_files_register() warn: freeing 'ctx->file_data' which has percpu_ref_exit()
fs/io_uring.c:7134 io_ring_ctx_free() warn: freeing 'ctx' which has percpu_ref_exit()
kernel/cgroup/cgroup.c:4948 css_free_rwork_fn() warn: freeing 'css' which has percpu_ref_exit()
mm/backing-dev.c:615 cgwb_create() warn: freeing 'wb' which has percpu_ref_exit()

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: KASAN: use-after-free Read in percpu_ref_switch_to_atomic_rcu
  2020-03-06 14:35     ` Dan Carpenter
@ 2020-03-06 14:57       ` Jens Axboe
  0 siblings, 0 replies; 12+ messages in thread
From: Jens Axboe @ 2020-03-06 14:57 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Dmitry Vyukov, syzbot, Al Viro, io-uring, linux-fsdevel,
	Borislav Petkov, H. Peter Anvin, LKML, Ingo Molnar,
	Peter Zijlstra, syzkaller-bugs, Thomas Gleixner, tony.luck,
	the arch/x86 maintainers

On 3/6/20 7:35 AM, Dan Carpenter wrote:
> 
> There a bunch of similar bugs.  It's seems a common anti-pattern.
> 
> block/blk-cgroup.c:85 blkg_free() warn: freeing 'blkg' which has percpu_ref_exit()
> block/blk-core.c:558 blk_alloc_queue_node() warn: freeing 'q' which has percpu_ref_exit()
> drivers/md/md.c:5528 md_free() warn: freeing 'mddev' which has percpu_ref_exit()
> drivers/target/target_core_transport.c:583 transport_free_session() warn: freeing 'se_sess' which has percpu_ref_exit()
> fs/aio.c:592 free_ioctx() warn: freeing 'ctx' which has percpu_ref_exit()
> fs/aio.c:806 ioctx_alloc() warn: freeing 'ctx' which has percpu_ref_exit()
> fs/io_uring.c:6115 io_sqe_files_unregister() warn: freeing 'data' which has percpu_ref_exit()
> fs/io_uring.c:6431 io_sqe_files_register() warn: freeing 'ctx->file_data' which has percpu_ref_exit()
> fs/io_uring.c:7134 io_ring_ctx_free() warn: freeing 'ctx' which has percpu_ref_exit()
> kernel/cgroup/cgroup.c:4948 css_free_rwork_fn() warn: freeing 'css' which has percpu_ref_exit()
> mm/backing-dev.c:615 cgwb_create() warn: freeing 'wb' which has percpu_ref_exit()

The file table io_uring issue is using the ref in a funky way, switching
in and out of atomic if we need to quiesce it. That's different from
other use cases, that just use it as a "normal" reference. Hence for the
funky use case, you can potentially have a switch in progress when you
exit the ref. You really want to wait for that, the easiest solution is
to punt the exit + free to an RCU callback, if there's nothing else you
need to handle once the switch is done.

So I would not be so quick to assume that similar patterns (exit + free)
have similar issues.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: KASAN: use-after-free Read in percpu_ref_switch_to_atomic_rcu
  2020-03-04 14:40   ` Jens Axboe
  2020-03-06 14:35     ` Dan Carpenter
@ 2020-03-06 14:57     ` Jann Horn
  2020-03-06 15:34       ` Jens Axboe
  1 sibling, 1 reply; 12+ messages in thread
From: Jann Horn @ 2020-03-06 14:57 UTC (permalink / raw)
  To: Jens Axboe, Paul E . McKenney
  Cc: Dmitry Vyukov, syzbot, Al Viro, io-uring, linux-fsdevel,
	Borislav Petkov, H. Peter Anvin, LKML, Ingo Molnar,
	Peter Zijlstra, syzkaller-bugs, Thomas Gleixner, tony.luck,
	the arch/x86 maintainers, Dan Carpenter

+paulmck

On Wed, Mar 4, 2020 at 3:40 PM Jens Axboe <[email protected]> wrote:
> On 3/4/20 12:59 AM, Dmitry Vyukov wrote:
> > On Fri, Feb 7, 2020 at 9:14 AM syzbot
> > <[email protected]> wrote:
> >>
> >> Hello,
> >>
> >> syzbot found the following crash on:
> >>
> >> HEAD commit:    4c7d00cc Merge tag 'pwm/for-5.6-rc1' of git://git.kernel.o..
> >> git tree:       upstream
> >> console output: https://syzkaller.appspot.com/x/log.txt?x=12fec785e00000
> >> kernel config:  https://syzkaller.appspot.com/x/.config?x=e162021ddededa72
> >> dashboard link: https://syzkaller.appspot.com/bug?extid=e017e49c39ab484ac87a
> >> compiler:       clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
> >>
> >> Unfortunately, I don't have any reproducer for this crash yet.
> >>
> >> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> >> Reported-by: [email protected]
> >
> > +io_uring maintainers
> >
> > Here is a repro:
> > https://gist.githubusercontent.com/dvyukov/6b340beab6483a036f4186e7378882ce/raw/cd1922185516453c201df8eded1d4b006a6d6a3a/gistfile1.txt
>
> I've queued up a fix for this:
>
> https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.6&id=9875fe3dc4b8cff1f1b440fb925054a5124403c3

I believe that this fix relies on call_rcu() having FIFO ordering; but
<https://www.kernel.org/doc/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.html#Callback%20Registry>
says:

| call_rcu() normally acts only on CPU-local state[...] It simply
enqueues the rcu_head structure on a per-CPU list,

Is this fix really correct?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: KASAN: use-after-free Read in percpu_ref_switch_to_atomic_rcu
  2020-03-06 14:57     ` Jann Horn
@ 2020-03-06 15:34       ` Jens Axboe
  2020-03-06 15:36         ` Jann Horn
  0 siblings, 1 reply; 12+ messages in thread
From: Jens Axboe @ 2020-03-06 15:34 UTC (permalink / raw)
  To: Jann Horn, Paul E . McKenney
  Cc: Dmitry Vyukov, syzbot, Al Viro, io-uring, linux-fsdevel,
	Borislav Petkov, H. Peter Anvin, LKML, Ingo Molnar,
	Peter Zijlstra, syzkaller-bugs, Thomas Gleixner, tony.luck,
	the arch/x86 maintainers, Dan Carpenter

On 3/6/20 7:57 AM, Jann Horn wrote:
> +paulmck
> 
> On Wed, Mar 4, 2020 at 3:40 PM Jens Axboe <[email protected]> wrote:
>> On 3/4/20 12:59 AM, Dmitry Vyukov wrote:
>>> On Fri, Feb 7, 2020 at 9:14 AM syzbot
>>> <[email protected]> wrote:
>>>>
>>>> Hello,
>>>>
>>>> syzbot found the following crash on:
>>>>
>>>> HEAD commit:    4c7d00cc Merge tag 'pwm/for-5.6-rc1' of git://git.kernel.o..
>>>> git tree:       upstream
>>>> console output: https://syzkaller.appspot.com/x/log.txt?x=12fec785e00000
>>>> kernel config:  https://syzkaller.appspot.com/x/.config?x=e162021ddededa72
>>>> dashboard link: https://syzkaller.appspot.com/bug?extid=e017e49c39ab484ac87a
>>>> compiler:       clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
>>>>
>>>> Unfortunately, I don't have any reproducer for this crash yet.
>>>>
>>>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>>>> Reported-by: [email protected]
>>>
>>> +io_uring maintainers
>>>
>>> Here is a repro:
>>> https://gist.githubusercontent.com/dvyukov/6b340beab6483a036f4186e7378882ce/raw/cd1922185516453c201df8eded1d4b006a6d6a3a/gistfile1.txt
>>
>> I've queued up a fix for this:
>>
>> https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.6&id=9875fe3dc4b8cff1f1b440fb925054a5124403c3
> 
> I believe that this fix relies on call_rcu() having FIFO ordering; but
> <https://www.kernel.org/doc/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.html#Callback%20Registry>
> says:
> 
> | call_rcu() normally acts only on CPU-local state[...] It simply
> enqueues the rcu_head structure on a per-CPU list,
> 
> Is this fix really correct?

That's a good point, there's a potentially stronger guarantee we need
here that isn't "nobody is inside an RCU critical section", but rather
that we're depending on a previous call_rcu() to have happened. Hence I
think you are right - it'll shrink the window drastically, since the
previous callback is already queued up, but it's not a full close.

Hmm...

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: KASAN: use-after-free Read in percpu_ref_switch_to_atomic_rcu
  2020-03-06 15:34       ` Jens Axboe
@ 2020-03-06 15:36         ` Jann Horn
  2020-03-06 16:44           ` Paul E. McKenney
  0 siblings, 1 reply; 12+ messages in thread
From: Jann Horn @ 2020-03-06 15:36 UTC (permalink / raw)
  To: Jens Axboe, Paul E . McKenney
  Cc: Dmitry Vyukov, syzbot, Al Viro, io-uring, linux-fsdevel,
	Borislav Petkov, H. Peter Anvin, LKML, Ingo Molnar,
	Peter Zijlstra, syzkaller-bugs, Thomas Gleixner, tony.luck,
	the arch/x86 maintainers, Dan Carpenter

On Fri, Mar 6, 2020 at 4:34 PM Jens Axboe <[email protected]> wrote:
> On 3/6/20 7:57 AM, Jann Horn wrote:
> > +paulmck
> >
> > On Wed, Mar 4, 2020 at 3:40 PM Jens Axboe <[email protected]> wrote:
> >> On 3/4/20 12:59 AM, Dmitry Vyukov wrote:
> >>> On Fri, Feb 7, 2020 at 9:14 AM syzbot
> >>> <[email protected]> wrote:
> >>>>
> >>>> Hello,
> >>>>
> >>>> syzbot found the following crash on:
> >>>>
> >>>> HEAD commit:    4c7d00cc Merge tag 'pwm/for-5.6-rc1' of git://git.kernel.o..
> >>>> git tree:       upstream
> >>>> console output: https://syzkaller.appspot.com/x/log.txt?x=12fec785e00000
> >>>> kernel config:  https://syzkaller.appspot.com/x/.config?x=e162021ddededa72
> >>>> dashboard link: https://syzkaller.appspot.com/bug?extid=e017e49c39ab484ac87a
> >>>> compiler:       clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
> >>>>
> >>>> Unfortunately, I don't have any reproducer for this crash yet.
> >>>>
> >>>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> >>>> Reported-by: [email protected]
> >>>
> >>> +io_uring maintainers
> >>>
> >>> Here is a repro:
> >>> https://gist.githubusercontent.com/dvyukov/6b340beab6483a036f4186e7378882ce/raw/cd1922185516453c201df8eded1d4b006a6d6a3a/gistfile1.txt
> >>
> >> I've queued up a fix for this:
> >>
> >> https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.6&id=9875fe3dc4b8cff1f1b440fb925054a5124403c3
> >
> > I believe that this fix relies on call_rcu() having FIFO ordering; but
> > <https://www.kernel.org/doc/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.html#Callback%20Registry>
> > says:
> >
> > | call_rcu() normally acts only on CPU-local state[...] It simply
> > enqueues the rcu_head structure on a per-CPU list,
> >
> > Is this fix really correct?
>
> That's a good point, there's a potentially stronger guarantee we need
> here that isn't "nobody is inside an RCU critical section", but rather
> that we're depending on a previous call_rcu() to have happened. Hence I
> think you are right - it'll shrink the window drastically, since the
> previous callback is already queued up, but it's not a full close.
>
> Hmm...

You could potentially hack up the semantics you want by doing a
call_rcu() whose callback does another call_rcu(), or something like
that - but I'd like to hear paulmck's opinion on this first.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: KASAN: use-after-free Read in percpu_ref_switch_to_atomic_rcu
  2020-03-06 15:36         ` Jann Horn
@ 2020-03-06 16:44           ` Paul E. McKenney
  2020-03-06 17:00             ` Jens Axboe
  0 siblings, 1 reply; 12+ messages in thread
From: Paul E. McKenney @ 2020-03-06 16:44 UTC (permalink / raw)
  To: Jann Horn
  Cc: Jens Axboe, Dmitry Vyukov, syzbot, Al Viro, io-uring,
	linux-fsdevel, Borislav Petkov, H. Peter Anvin, LKML, Ingo Molnar,
	Peter Zijlstra, syzkaller-bugs, Thomas Gleixner, tony.luck,
	the arch/x86 maintainers, Dan Carpenter

On Fri, Mar 06, 2020 at 04:36:20PM +0100, Jann Horn wrote:
> On Fri, Mar 6, 2020 at 4:34 PM Jens Axboe <[email protected]> wrote:
> > On 3/6/20 7:57 AM, Jann Horn wrote:
> > > +paulmck
> > >
> > > On Wed, Mar 4, 2020 at 3:40 PM Jens Axboe <[email protected]> wrote:
> > >> On 3/4/20 12:59 AM, Dmitry Vyukov wrote:
> > >>> On Fri, Feb 7, 2020 at 9:14 AM syzbot
> > >>> <[email protected]> wrote:
> > >>>>
> > >>>> Hello,
> > >>>>
> > >>>> syzbot found the following crash on:
> > >>>>
> > >>>> HEAD commit:    4c7d00cc Merge tag 'pwm/for-5.6-rc1' of git://git.kernel.o..
> > >>>> git tree:       upstream
> > >>>> console output: https://syzkaller.appspot.com/x/log.txt?x=12fec785e00000
> > >>>> kernel config:  https://syzkaller.appspot.com/x/.config?x=e162021ddededa72
> > >>>> dashboard link: https://syzkaller.appspot.com/bug?extid=e017e49c39ab484ac87a
> > >>>> compiler:       clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
> > >>>>
> > >>>> Unfortunately, I don't have any reproducer for this crash yet.
> > >>>>
> > >>>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > >>>> Reported-by: [email protected]
> > >>>
> > >>> +io_uring maintainers
> > >>>
> > >>> Here is a repro:
> > >>> https://gist.githubusercontent.com/dvyukov/6b340beab6483a036f4186e7378882ce/raw/cd1922185516453c201df8eded1d4b006a6d6a3a/gistfile1.txt
> > >>
> > >> I've queued up a fix for this:
> > >>
> > >> https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.6&id=9875fe3dc4b8cff1f1b440fb925054a5124403c3
> > >
> > > I believe that this fix relies on call_rcu() having FIFO ordering; but
> > > <https://www.kernel.org/doc/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.html#Callback%20Registry>
> > > says:
> > >
> > > | call_rcu() normally acts only on CPU-local state[...] It simply
> > > enqueues the rcu_head structure on a per-CPU list,

Indeed.  For but one example, if there was a CPU-to-CPU migration between
the two call_rcu() invocations, it would not be at all surprising for
the two callbacks to execute out of order.

> > > Is this fix really correct?
> >
> > That's a good point, there's a potentially stronger guarantee we need
> > here that isn't "nobody is inside an RCU critical section", but rather
> > that we're depending on a previous call_rcu() to have happened. Hence I
> > think you are right - it'll shrink the window drastically, since the
> > previous callback is already queued up, but it's not a full close.
> >
> > Hmm...
> 
> You could potentially hack up the semantics you want by doing a
> call_rcu() whose callback does another call_rcu(), or something like
> that - but I'd like to hear paulmck's opinion on this first.

That would work!

Or, alternatively, do an rcu_barrier() between the two calls to
call_rcu(), assuming that the use case can tolerate rcu_barrier()
overhead and latency.

							Thanx, Paul

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: KASAN: use-after-free Read in percpu_ref_switch_to_atomic_rcu
  2020-03-06 16:44           ` Paul E. McKenney
@ 2020-03-06 17:00             ` Jens Axboe
  2020-03-06 17:08               ` Jens Axboe
  2020-03-06 17:19               ` Paul E. McKenney
  0 siblings, 2 replies; 12+ messages in thread
From: Jens Axboe @ 2020-03-06 17:00 UTC (permalink / raw)
  To: paulmck, Jann Horn
  Cc: Dmitry Vyukov, syzbot, Al Viro, io-uring, linux-fsdevel,
	Borislav Petkov, H. Peter Anvin, LKML, Ingo Molnar,
	Peter Zijlstra, syzkaller-bugs, Thomas Gleixner, tony.luck,
	the arch/x86 maintainers, Dan Carpenter

On 3/6/20 9:44 AM, Paul E. McKenney wrote:
> On Fri, Mar 06, 2020 at 04:36:20PM +0100, Jann Horn wrote:
>> On Fri, Mar 6, 2020 at 4:34 PM Jens Axboe <[email protected]> wrote:
>>> On 3/6/20 7:57 AM, Jann Horn wrote:
>>>> +paulmck
>>>>
>>>> On Wed, Mar 4, 2020 at 3:40 PM Jens Axboe <[email protected]> wrote:
>>>>> On 3/4/20 12:59 AM, Dmitry Vyukov wrote:
>>>>>> On Fri, Feb 7, 2020 at 9:14 AM syzbot
>>>>>> <[email protected]> wrote:
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> syzbot found the following crash on:
>>>>>>>
>>>>>>> HEAD commit:    4c7d00cc Merge tag 'pwm/for-5.6-rc1' of git://git.kernel.o..
>>>>>>> git tree:       upstream
>>>>>>> console output: https://syzkaller.appspot.com/x/log.txt?x=12fec785e00000
>>>>>>> kernel config:  https://syzkaller.appspot.com/x/.config?x=e162021ddededa72
>>>>>>> dashboard link: https://syzkaller.appspot.com/bug?extid=e017e49c39ab484ac87a
>>>>>>> compiler:       clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
>>>>>>>
>>>>>>> Unfortunately, I don't have any reproducer for this crash yet.
>>>>>>>
>>>>>>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>>>>>>> Reported-by: [email protected]
>>>>>>
>>>>>> +io_uring maintainers
>>>>>>
>>>>>> Here is a repro:
>>>>>> https://gist.githubusercontent.com/dvyukov/6b340beab6483a036f4186e7378882ce/raw/cd1922185516453c201df8eded1d4b006a6d6a3a/gistfile1.txt
>>>>>
>>>>> I've queued up a fix for this:
>>>>>
>>>>> https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.6&id=9875fe3dc4b8cff1f1b440fb925054a5124403c3
>>>>
>>>> I believe that this fix relies on call_rcu() having FIFO ordering; but
>>>> <https://www.kernel.org/doc/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.html#Callback%20Registry>
>>>> says:
>>>>
>>>> | call_rcu() normally acts only on CPU-local state[...] It simply
>>>> enqueues the rcu_head structure on a per-CPU list,
> 
> Indeed.  For but one example, if there was a CPU-to-CPU migration between
> the two call_rcu() invocations, it would not be at all surprising for
> the two callbacks to execute out of order.
> 
>>>> Is this fix really correct?
>>>
>>> That's a good point, there's a potentially stronger guarantee we need
>>> here that isn't "nobody is inside an RCU critical section", but rather
>>> that we're depending on a previous call_rcu() to have happened. Hence I
>>> think you are right - it'll shrink the window drastically, since the
>>> previous callback is already queued up, but it's not a full close.
>>>
>>> Hmm...
>>
>> You could potentially hack up the semantics you want by doing a
>> call_rcu() whose callback does another call_rcu(), or something like
>> that - but I'd like to hear paulmck's opinion on this first.
> 
> That would work!
> 
> Or, alternatively, do an rcu_barrier() between the two calls to
> call_rcu(), assuming that the use case can tolerate rcu_barrier()
> overhead and latency.

If the nested call_rcu() works, that seems greatly preferable to needing
the rcu_barrier(), even if that would not be a showstopper for me. The
nested call_rcu() is just a bit odd, but with a comment it should be OK.

Incremental here I'm going to test, would just fold in of course.


diff --git a/fs/io_uring.c b/fs/io_uring.c
index f3218fc81943..95ba95b4d8ec 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5330,7 +5330,7 @@ static void io_file_ref_kill(struct percpu_ref *ref)
 	complete(&data->done);
 }
 
-static void io_file_ref_exit_and_free(struct rcu_head *rcu)
+static void __io_file_ref_exit_and_free(struct rcu_head *rcu)
 {
 	struct fixed_file_data *data = container_of(rcu, struct fixed_file_data,
 							rcu);
@@ -5338,6 +5338,18 @@ static void io_file_ref_exit_and_free(struct rcu_head *rcu)
 	kfree(data);
 }
 
+static void io_file_ref_exit_and_free(struct rcu_head *rcu)
+{
+	/*
+	 * We need to order our exit+free call again the potentially
+	 * existing call_rcu() for switching to atomic. One way to do that
+	 * is to have this rcu callback queue the final put and free, as we
+	 * could otherwise a pre-existing atomic switch complete _after_
+	 * the free callback we queued.
+	 */
+	call_rcu(rcu, __io_file_ref_exit_and_free);
+}
+
 static int io_sqe_files_unregister(struct io_ring_ctx *ctx)
 {
 	struct fixed_file_data *data = ctx->file_data;

-- 
Jens Axboe


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: KASAN: use-after-free Read in percpu_ref_switch_to_atomic_rcu
  2020-03-06 17:00             ` Jens Axboe
@ 2020-03-06 17:08               ` Jens Axboe
  2020-03-06 17:19               ` Paul E. McKenney
  1 sibling, 0 replies; 12+ messages in thread
From: Jens Axboe @ 2020-03-06 17:08 UTC (permalink / raw)
  To: paulmck, Jann Horn
  Cc: Dmitry Vyukov, syzbot, Al Viro, io-uring, linux-fsdevel,
	Borislav Petkov, H. Peter Anvin, LKML, Ingo Molnar,
	Peter Zijlstra, syzkaller-bugs, Thomas Gleixner, tony.luck,
	the arch/x86 maintainers, Dan Carpenter

On 3/6/20 10:00 AM, Jens Axboe wrote:
> On 3/6/20 9:44 AM, Paul E. McKenney wrote:
>> On Fri, Mar 06, 2020 at 04:36:20PM +0100, Jann Horn wrote:
>>> On Fri, Mar 6, 2020 at 4:34 PM Jens Axboe <[email protected]> wrote:
>>>> On 3/6/20 7:57 AM, Jann Horn wrote:
>>>>> +paulmck
>>>>>
>>>>> On Wed, Mar 4, 2020 at 3:40 PM Jens Axboe <[email protected]> wrote:
>>>>>> On 3/4/20 12:59 AM, Dmitry Vyukov wrote:
>>>>>>> On Fri, Feb 7, 2020 at 9:14 AM syzbot
>>>>>>> <[email protected]> wrote:
>>>>>>>>
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> syzbot found the following crash on:
>>>>>>>>
>>>>>>>> HEAD commit:    4c7d00cc Merge tag 'pwm/for-5.6-rc1' of git://git.kernel.o..
>>>>>>>> git tree:       upstream
>>>>>>>> console output: https://syzkaller.appspot.com/x/log.txt?x=12fec785e00000
>>>>>>>> kernel config:  https://syzkaller.appspot.com/x/.config?x=e162021ddededa72
>>>>>>>> dashboard link: https://syzkaller.appspot.com/bug?extid=e017e49c39ab484ac87a
>>>>>>>> compiler:       clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
>>>>>>>>
>>>>>>>> Unfortunately, I don't have any reproducer for this crash yet.
>>>>>>>>
>>>>>>>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>>>>>>>> Reported-by: [email protected]
>>>>>>>
>>>>>>> +io_uring maintainers
>>>>>>>
>>>>>>> Here is a repro:
>>>>>>> https://gist.githubusercontent.com/dvyukov/6b340beab6483a036f4186e7378882ce/raw/cd1922185516453c201df8eded1d4b006a6d6a3a/gistfile1.txt
>>>>>>
>>>>>> I've queued up a fix for this:
>>>>>>
>>>>>> https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.6&id=9875fe3dc4b8cff1f1b440fb925054a5124403c3
>>>>>
>>>>> I believe that this fix relies on call_rcu() having FIFO ordering; but
>>>>> <https://www.kernel.org/doc/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.html#Callback%20Registry>
>>>>> says:
>>>>>
>>>>> | call_rcu() normally acts only on CPU-local state[...] It simply
>>>>> enqueues the rcu_head structure on a per-CPU list,
>>
>> Indeed.  For but one example, if there was a CPU-to-CPU migration between
>> the two call_rcu() invocations, it would not be at all surprising for
>> the two callbacks to execute out of order.
>>
>>>>> Is this fix really correct?
>>>>
>>>> That's a good point, there's a potentially stronger guarantee we need
>>>> here that isn't "nobody is inside an RCU critical section", but rather
>>>> that we're depending on a previous call_rcu() to have happened. Hence I
>>>> think you are right - it'll shrink the window drastically, since the
>>>> previous callback is already queued up, but it's not a full close.
>>>>
>>>> Hmm...
>>>
>>> You could potentially hack up the semantics you want by doing a
>>> call_rcu() whose callback does another call_rcu(), or something like
>>> that - but I'd like to hear paulmck's opinion on this first.
>>
>> That would work!
>>
>> Or, alternatively, do an rcu_barrier() between the two calls to
>> call_rcu(), assuming that the use case can tolerate rcu_barrier()
>> overhead and latency.
> 
> If the nested call_rcu() works, that seems greatly preferable to needing
> the rcu_barrier(), even if that would not be a showstopper for me. The
> nested call_rcu() is just a bit odd, but with a comment it should be OK.
> 
> Incremental here I'm going to test, would just fold in of course.

Been running for a few minutes just fine, I'm going to leave the
reproducer beating on it for a few hours. But here's the folded in
final:

https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.6&id=fae702294a6a0774ceb3cf250be79e7fe207250a

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: KASAN: use-after-free Read in percpu_ref_switch_to_atomic_rcu
  2020-03-06 17:00             ` Jens Axboe
  2020-03-06 17:08               ` Jens Axboe
@ 2020-03-06 17:19               ` Paul E. McKenney
  2020-03-06 17:21                 ` Jens Axboe
  1 sibling, 1 reply; 12+ messages in thread
From: Paul E. McKenney @ 2020-03-06 17:19 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Jann Horn, Dmitry Vyukov, syzbot, Al Viro, io-uring,
	linux-fsdevel, Borislav Petkov, H. Peter Anvin, LKML, Ingo Molnar,
	Peter Zijlstra, syzkaller-bugs, Thomas Gleixner, tony.luck,
	the arch/x86 maintainers, Dan Carpenter

On Fri, Mar 06, 2020 at 10:00:19AM -0700, Jens Axboe wrote:
> On 3/6/20 9:44 AM, Paul E. McKenney wrote:
> > On Fri, Mar 06, 2020 at 04:36:20PM +0100, Jann Horn wrote:
> >> On Fri, Mar 6, 2020 at 4:34 PM Jens Axboe <[email protected]> wrote:
> >>> On 3/6/20 7:57 AM, Jann Horn wrote:
> >>>> +paulmck
> >>>>
> >>>> On Wed, Mar 4, 2020 at 3:40 PM Jens Axboe <[email protected]> wrote:
> >>>>> On 3/4/20 12:59 AM, Dmitry Vyukov wrote:
> >>>>>> On Fri, Feb 7, 2020 at 9:14 AM syzbot
> >>>>>> <[email protected]> wrote:
> >>>>>>>
> >>>>>>> Hello,
> >>>>>>>
> >>>>>>> syzbot found the following crash on:
> >>>>>>>
> >>>>>>> HEAD commit:    4c7d00cc Merge tag 'pwm/for-5.6-rc1' of git://git.kernel.o..
> >>>>>>> git tree:       upstream
> >>>>>>> console output: https://syzkaller.appspot.com/x/log.txt?x=12fec785e00000
> >>>>>>> kernel config:  https://syzkaller.appspot.com/x/.config?x=e162021ddededa72
> >>>>>>> dashboard link: https://syzkaller.appspot.com/bug?extid=e017e49c39ab484ac87a
> >>>>>>> compiler:       clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
> >>>>>>>
> >>>>>>> Unfortunately, I don't have any reproducer for this crash yet.
> >>>>>>>
> >>>>>>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> >>>>>>> Reported-by: [email protected]
> >>>>>>
> >>>>>> +io_uring maintainers
> >>>>>>
> >>>>>> Here is a repro:
> >>>>>> https://gist.githubusercontent.com/dvyukov/6b340beab6483a036f4186e7378882ce/raw/cd1922185516453c201df8eded1d4b006a6d6a3a/gistfile1.txt
> >>>>>
> >>>>> I've queued up a fix for this:
> >>>>>
> >>>>> https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.6&id=9875fe3dc4b8cff1f1b440fb925054a5124403c3
> >>>>
> >>>> I believe that this fix relies on call_rcu() having FIFO ordering; but
> >>>> <https://www.kernel.org/doc/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.html#Callback%20Registry>
> >>>> says:
> >>>>
> >>>> | call_rcu() normally acts only on CPU-local state[...] It simply
> >>>> enqueues the rcu_head structure on a per-CPU list,
> > 
> > Indeed.  For but one example, if there was a CPU-to-CPU migration between
> > the two call_rcu() invocations, it would not be at all surprising for
> > the two callbacks to execute out of order.
> > 
> >>>> Is this fix really correct?
> >>>
> >>> That's a good point, there's a potentially stronger guarantee we need
> >>> here that isn't "nobody is inside an RCU critical section", but rather
> >>> that we're depending on a previous call_rcu() to have happened. Hence I
> >>> think you are right - it'll shrink the window drastically, since the
> >>> previous callback is already queued up, but it's not a full close.
> >>>
> >>> Hmm...
> >>
> >> You could potentially hack up the semantics you want by doing a
> >> call_rcu() whose callback does another call_rcu(), or something like
> >> that - but I'd like to hear paulmck's opinion on this first.
> > 
> > That would work!
> > 
> > Or, alternatively, do an rcu_barrier() between the two calls to
> > call_rcu(), assuming that the use case can tolerate rcu_barrier()
> > overhead and latency.
> 
> If the nested call_rcu() works, that seems greatly preferable to needing
> the rcu_barrier(), even if that would not be a showstopper for me. The
> nested call_rcu() is just a bit odd, but with a comment it should be OK.
> 
> Incremental here I'm going to test, would just fold in of course.
> 
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index f3218fc81943..95ba95b4d8ec 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -5330,7 +5330,7 @@ static void io_file_ref_kill(struct percpu_ref *ref)
>  	complete(&data->done);
>  }
>  
> -static void io_file_ref_exit_and_free(struct rcu_head *rcu)
> +static void __io_file_ref_exit_and_free(struct rcu_head *rcu)
>  {
>  	struct fixed_file_data *data = container_of(rcu, struct fixed_file_data,
>  							rcu);
> @@ -5338,6 +5338,18 @@ static void io_file_ref_exit_and_free(struct rcu_head *rcu)
>  	kfree(data);
>  }
>  
> +static void io_file_ref_exit_and_free(struct rcu_head *rcu)
> +{
> +	/*
> +	 * We need to order our exit+free call again the potentially
> +	 * existing call_rcu() for switching to atomic. One way to do that
> +	 * is to have this rcu callback queue the final put and free, as we
> +	 * could otherwise a pre-existing atomic switch complete _after_
> +	 * the free callback we queued.
> +	 */
> +	call_rcu(rcu, __io_file_ref_exit_and_free);
> +}
> +
>  static int io_sqe_files_unregister(struct io_ring_ctx *ctx)
>  {
>  	struct fixed_file_data *data = ctx->file_data;

Looks good to me!

							Thanx, Paul

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: KASAN: use-after-free Read in percpu_ref_switch_to_atomic_rcu
  2020-03-06 17:19               ` Paul E. McKenney
@ 2020-03-06 17:21                 ` Jens Axboe
  0 siblings, 0 replies; 12+ messages in thread
From: Jens Axboe @ 2020-03-06 17:21 UTC (permalink / raw)
  To: paulmck
  Cc: Jann Horn, Dmitry Vyukov, syzbot, Al Viro, io-uring,
	linux-fsdevel, Borislav Petkov, H. Peter Anvin, LKML, Ingo Molnar,
	Peter Zijlstra, syzkaller-bugs, Thomas Gleixner, tony.luck,
	the arch/x86 maintainers, Dan Carpenter

On 3/6/20 10:19 AM, Paul E. McKenney wrote:
> On Fri, Mar 06, 2020 at 10:00:19AM -0700, Jens Axboe wrote:
>> On 3/6/20 9:44 AM, Paul E. McKenney wrote:
>>> On Fri, Mar 06, 2020 at 04:36:20PM +0100, Jann Horn wrote:
>>>> On Fri, Mar 6, 2020 at 4:34 PM Jens Axboe <[email protected]> wrote:
>>>>> On 3/6/20 7:57 AM, Jann Horn wrote:
>>>>>> +paulmck
>>>>>>
>>>>>> On Wed, Mar 4, 2020 at 3:40 PM Jens Axboe <[email protected]> wrote:
>>>>>>> On 3/4/20 12:59 AM, Dmitry Vyukov wrote:
>>>>>>>> On Fri, Feb 7, 2020 at 9:14 AM syzbot
>>>>>>>> <[email protected]> wrote:
>>>>>>>>>
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> syzbot found the following crash on:
>>>>>>>>>
>>>>>>>>> HEAD commit:    4c7d00cc Merge tag 'pwm/for-5.6-rc1' of git://git.kernel.o..
>>>>>>>>> git tree:       upstream
>>>>>>>>> console output: https://syzkaller.appspot.com/x/log.txt?x=12fec785e00000
>>>>>>>>> kernel config:  https://syzkaller.appspot.com/x/.config?x=e162021ddededa72
>>>>>>>>> dashboard link: https://syzkaller.appspot.com/bug?extid=e017e49c39ab484ac87a
>>>>>>>>> compiler:       clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
>>>>>>>>>
>>>>>>>>> Unfortunately, I don't have any reproducer for this crash yet.
>>>>>>>>>
>>>>>>>>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>>>>>>>>> Reported-by: [email protected]
>>>>>>>>
>>>>>>>> +io_uring maintainers
>>>>>>>>
>>>>>>>> Here is a repro:
>>>>>>>> https://gist.githubusercontent.com/dvyukov/6b340beab6483a036f4186e7378882ce/raw/cd1922185516453c201df8eded1d4b006a6d6a3a/gistfile1.txt
>>>>>>>
>>>>>>> I've queued up a fix for this:
>>>>>>>
>>>>>>> https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.6&id=9875fe3dc4b8cff1f1b440fb925054a5124403c3
>>>>>>
>>>>>> I believe that this fix relies on call_rcu() having FIFO ordering; but
>>>>>> <https://www.kernel.org/doc/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.html#Callback%20Registry>
>>>>>> says:
>>>>>>
>>>>>> | call_rcu() normally acts only on CPU-local state[...] It simply
>>>>>> enqueues the rcu_head structure on a per-CPU list,
>>>
>>> Indeed.  For but one example, if there was a CPU-to-CPU migration between
>>> the two call_rcu() invocations, it would not be at all surprising for
>>> the two callbacks to execute out of order.
>>>
>>>>>> Is this fix really correct?
>>>>>
>>>>> That's a good point, there's a potentially stronger guarantee we need
>>>>> here that isn't "nobody is inside an RCU critical section", but rather
>>>>> that we're depending on a previous call_rcu() to have happened. Hence I
>>>>> think you are right - it'll shrink the window drastically, since the
>>>>> previous callback is already queued up, but it's not a full close.
>>>>>
>>>>> Hmm...
>>>>
>>>> You could potentially hack up the semantics you want by doing a
>>>> call_rcu() whose callback does another call_rcu(), or something like
>>>> that - but I'd like to hear paulmck's opinion on this first.
>>>
>>> That would work!
>>>
>>> Or, alternatively, do an rcu_barrier() between the two calls to
>>> call_rcu(), assuming that the use case can tolerate rcu_barrier()
>>> overhead and latency.
>>
>> If the nested call_rcu() works, that seems greatly preferable to needing
>> the rcu_barrier(), even if that would not be a showstopper for me. The
>> nested call_rcu() is just a bit odd, but with a comment it should be OK.
>>
>> Incremental here I'm going to test, would just fold in of course.
>>
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index f3218fc81943..95ba95b4d8ec 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -5330,7 +5330,7 @@ static void io_file_ref_kill(struct percpu_ref *ref)
>>  	complete(&data->done);
>>  }
>>  
>> -static void io_file_ref_exit_and_free(struct rcu_head *rcu)
>> +static void __io_file_ref_exit_and_free(struct rcu_head *rcu)
>>  {
>>  	struct fixed_file_data *data = container_of(rcu, struct fixed_file_data,
>>  							rcu);
>> @@ -5338,6 +5338,18 @@ static void io_file_ref_exit_and_free(struct rcu_head *rcu)
>>  	kfree(data);
>>  }
>>  
>> +static void io_file_ref_exit_and_free(struct rcu_head *rcu)
>> +{
>> +	/*
>> +	 * We need to order our exit+free call again the potentially
>> +	 * existing call_rcu() for switching to atomic. One way to do that
>> +	 * is to have this rcu callback queue the final put and free, as we
>> +	 * could otherwise a pre-existing atomic switch complete _after_
>> +	 * the free callback we queued.
>> +	 */
>> +	call_rcu(rcu, __io_file_ref_exit_and_free);
>> +}
>> +
>>  static int io_sqe_files_unregister(struct io_ring_ctx *ctx)
>>  {
>>  	struct fixed_file_data *data = ctx->file_data;
> 
> Looks good to me!

Thanks Paul! I talked to Paul in private, but here's the final version
with updated comment and attributions.

https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.6&id=c1e2148f8ecb26863b899d402a823dab8e26efd1

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-03-06 17:21 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <[email protected]>
2020-03-04  7:59 ` KASAN: use-after-free Read in percpu_ref_switch_to_atomic_rcu Dmitry Vyukov
2020-03-04 14:40   ` Jens Axboe
2020-03-06 14:35     ` Dan Carpenter
2020-03-06 14:57       ` Jens Axboe
2020-03-06 14:57     ` Jann Horn
2020-03-06 15:34       ` Jens Axboe
2020-03-06 15:36         ` Jann Horn
2020-03-06 16:44           ` Paul E. McKenney
2020-03-06 17:00             ` Jens Axboe
2020-03-06 17:08               ` Jens Axboe
2020-03-06 17:19               ` Paul E. McKenney
2020-03-06 17:21                 ` Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox