* [syzbot] KASAN: use-after-free Read in remove_wait_queue (3)
@ 2021-11-16 9:23 syzbot
2021-12-10 22:42 ` syzbot
[not found] ` <20211211015620.1793-1-hdanton@sina.com>
0 siblings, 2 replies; 16+ messages in thread
From: syzbot @ 2021-11-16 9:23 UTC (permalink / raw)
To: linux-fsdevel, linux-kernel, netdev, syzkaller-bugs, viro
Hello,
syzbot found the following issue on:
HEAD commit: cc0356d6a02e Merge tag 'x86_core_for_v5.16_rc1' of git://g..
git tree: net-next
console output: https://syzkaller.appspot.com/x/log.txt?x=102e7ce6b00000
kernel config: https://syzkaller.appspot.com/x/.config?x=a5d447cdc3ae81d9
dashboard link: https://syzkaller.appspot.com/bug?extid=cdb5dd11c97cc532efad
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
Unfortunately, I don't have any reproducer for this issue yet.
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+cdb5dd11c97cc532efad@syzkaller.appspotmail.com
==================================================================
BUG: KASAN: use-after-free in __lock_acquire+0x3d86/0x54a0 kernel/locking/lockdep.c:4885
Read of size 8 at addr ffff888032563540 by task syz-executor.2/8869
CPU: 1 PID: 8869 Comm: syz-executor.2 Not tainted 5.15.0-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
print_address_description.constprop.0.cold+0x6c/0x309 mm/kasan/report.c:256
__kasan_report mm/kasan/report.c:442 [inline]
kasan_report.cold+0x83/0xdf mm/kasan/report.c:459
__lock_acquire+0x3d86/0x54a0 kernel/locking/lockdep.c:4885
lock_acquire kernel/locking/lockdep.c:5625 [inline]
lock_acquire+0x1ab/0x510 kernel/locking/lockdep.c:5590
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x39/0x50 kernel/locking/spinlock.c:162
remove_wait_queue+0x1d/0x180 kernel/sched/wait.c:55
ep_remove_wait_queue+0x88/0x1a0 fs/eventpoll.c:545
ep_unregister_pollwait fs/eventpoll.c:561 [inline]
ep_free+0x18a/0x390 fs/eventpoll.c:756
ep_eventpoll_release+0x41/0x60 fs/eventpoll.c:788
__fput+0x286/0x9f0 fs/file_table.c:280
task_work_run+0xdd/0x1a0 kernel/task_work.c:164
exit_task_work include/linux/task_work.h:32 [inline]
do_exit+0xbaa/0x2a20 kernel/exit.c:826
do_group_exit+0x125/0x310 kernel/exit.c:923
get_signal+0x47d/0x21d0 kernel/signal.c:2855
arch_do_signal_or_restart+0x2a9/0x1c40 arch/x86/kernel/signal.c:868
handle_signal_work kernel/entry/common.c:148 [inline]
exit_to_user_mode_loop kernel/entry/common.c:172 [inline]
exit_to_user_mode_prepare+0x17d/0x290 kernel/entry/common.c:207
__syscall_exit_to_user_mode_work kernel/entry/common.c:289 [inline]
syscall_exit_to_user_mode+0x19/0x60 kernel/entry/common.c:300
do_syscall_64+0x42/0xb0 arch/x86/entry/common.c:86
entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f8b46801ae9
Code: Unable to access opcode bytes at RIP 0x7f8b46801abf.
RSP: 002b:00007f8b43d77218 EFLAGS: 00000246 ORIG_RAX: 00000000000000ca
RAX: 0000000000000000 RBX: 00007f8b46914f68 RCX: 00007f8b46801ae9
RDX: 0000000000000000 RSI: 0000000000000080 RDI: 00007f8b46914f68
RBP: 00007f8b46914f60 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f8b46914f6c
R13: 00007fff0432647f R14: 00007f8b43d77300 R15: 0000000000022000
</TASK>
Allocated by task 8869:
kasan_save_stack+0x1b/0x40 mm/kasan/common.c:38
kasan_set_track mm/kasan/common.c:46 [inline]
set_alloc_info mm/kasan/common.c:434 [inline]
____kasan_kmalloc mm/kasan/common.c:513 [inline]
____kasan_kmalloc mm/kasan/common.c:472 [inline]
__kasan_kmalloc+0xa4/0xd0 mm/kasan/common.c:522
kmalloc include/linux/slab.h:591 [inline]
psi_trigger_create.part.0+0x15e/0x7f0 kernel/sched/psi.c:1141
cgroup_pressure_write+0x15d/0x6b0 kernel/cgroup/cgroup.c:3622
cgroup_file_write+0x1ec/0x780 kernel/cgroup/cgroup.c:3829
kernfs_fop_write_iter+0x342/0x500 fs/kernfs/file.c:296
call_write_iter include/linux/fs.h:2161 [inline]
new_sync_write+0x429/0x660 fs/read_write.c:503
vfs_write+0x7cd/0xae0 fs/read_write.c:590
ksys_write+0x12d/0x250 fs/read_write.c:643
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
Freed by task 8869:
kasan_save_stack+0x1b/0x40 mm/kasan/common.c:38
kasan_set_track+0x1c/0x30 mm/kasan/common.c:46
kasan_set_free_info+0x20/0x30 mm/kasan/generic.c:360
____kasan_slab_free mm/kasan/common.c:366 [inline]
____kasan_slab_free mm/kasan/common.c:328 [inline]
__kasan_slab_free+0xff/0x130 mm/kasan/common.c:374
kasan_slab_free include/linux/kasan.h:235 [inline]
slab_free_hook mm/slub.c:1700 [inline]
slab_free_freelist_hook+0x8b/0x1c0 mm/slub.c:1726
slab_free mm/slub.c:3492 [inline]
kfree+0xf3/0x550 mm/slub.c:4552
cgroup_pressure_write+0x18d/0x6b0 kernel/cgroup/cgroup.c:3628
cgroup_file_write+0x1ec/0x780 kernel/cgroup/cgroup.c:3829
kernfs_fop_write_iter+0x342/0x500 fs/kernfs/file.c:296
call_write_iter include/linux/fs.h:2161 [inline]
new_sync_write+0x429/0x660 fs/read_write.c:503
vfs_write+0x7cd/0xae0 fs/read_write.c:590
ksys_write+0x12d/0x250 fs/read_write.c:643
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
Last potentially related work creation:
kasan_save_stack+0x1b/0x40 mm/kasan/common.c:38
kasan_record_aux_stack+0xe9/0x110 mm/kasan/generic.c:348
insert_work+0x48/0x370 kernel/workqueue.c:1353
__queue_work+0x5ca/0xee0 kernel/workqueue.c:1519
queue_work_on+0xee/0x110 kernel/workqueue.c:1546
queue_work include/linux/workqueue.h:501 [inline]
call_usermodehelper_exec+0x1f0/0x4c0 kernel/umh.c:435
kobject_uevent_env+0xf8f/0x1650 lib/kobject_uevent.c:618
rx_queue_add_kobject net/core/net-sysfs.c:1069 [inline]
net_rx_queue_update_kobjects+0xf8/0x500 net/core/net-sysfs.c:1109
register_queue_kobjects net/core/net-sysfs.c:1766 [inline]
netdev_register_kobject+0x275/0x430 net/core/net-sysfs.c:2014
register_netdevice+0xd31/0x1500 net/core/dev.c:10330
__ip_tunnel_create+0x398/0x5c0 net/ipv4/ip_tunnel.c:267
ip_tunnel_init_net+0x2e4/0x9d0 net/ipv4/ip_tunnel.c:1070
ops_init+0xaf/0x470 net/core/net_namespace.c:140
setup_net+0x40f/0xa30 net/core/net_namespace.c:326
copy_net_ns+0x319/0x760 net/core/net_namespace.c:470
create_new_namespaces+0x3f6/0xb20 kernel/nsproxy.c:110
unshare_nsproxy_namespaces+0xc1/0x1f0 kernel/nsproxy.c:226
ksys_unshare+0x445/0x920 kernel/fork.c:3076
__do_sys_unshare kernel/fork.c:3150 [inline]
__se_sys_unshare kernel/fork.c:3148 [inline]
__x64_sys_unshare+0x2d/0x40 kernel/fork.c:3148
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
Second to last potentially related work creation:
kasan_save_stack+0x1b/0x40 mm/kasan/common.c:38
kasan_record_aux_stack+0xe9/0x110 mm/kasan/generic.c:348
insert_work+0x48/0x370 kernel/workqueue.c:1353
__queue_work+0x5ca/0xee0 kernel/workqueue.c:1519
queue_work_on+0xee/0x110 kernel/workqueue.c:1546
queue_work include/linux/workqueue.h:501 [inline]
call_usermodehelper_exec+0x1f0/0x4c0 kernel/umh.c:435
kobject_uevent_env+0xf8f/0x1650 lib/kobject_uevent.c:618
rx_queue_add_kobject net/core/net-sysfs.c:1069 [inline]
net_rx_queue_update_kobjects+0xf8/0x500 net/core/net-sysfs.c:1109
register_queue_kobjects net/core/net-sysfs.c:1766 [inline]
netdev_register_kobject+0x275/0x430 net/core/net-sysfs.c:2014
register_netdevice+0xd31/0x1500 net/core/dev.c:10330
__ip_tunnel_create+0x398/0x5c0 net/ipv4/ip_tunnel.c:267
ip_tunnel_init_net+0x2e4/0x9d0 net/ipv4/ip_tunnel.c:1070
vti_init_net+0x2a/0x370 net/ipv4/ip_vti.c:504
ops_init+0xaf/0x470 net/core/net_namespace.c:140
setup_net+0x40f/0xa30 net/core/net_namespace.c:326
copy_net_ns+0x319/0x760 net/core/net_namespace.c:470
create_new_namespaces+0x3f6/0xb20 kernel/nsproxy.c:110
unshare_nsproxy_namespaces+0xc1/0x1f0 kernel/nsproxy.c:226
ksys_unshare+0x445/0x920 kernel/fork.c:3076
__do_sys_unshare kernel/fork.c:3150 [inline]
__se_sys_unshare kernel/fork.c:3148 [inline]
__x64_sys_unshare+0x2d/0x40 kernel/fork.c:3148
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
The buggy address belongs to the object at ffff888032563500
which belongs to the cache kmalloc-192 of size 192
The buggy address is located 64 bytes inside of
192-byte region [ffff888032563500, ffff8880325635c0)
The buggy address belongs to the page:
page:ffffea0000c958c0 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x32563
flags: 0xfff00000000200(slab|node=0|zone=1|lastcpupid=0x7ff)
raw: 00fff00000000200 ffffea0001ef2780 0000000600000002 ffff888010c41a00
raw: 0000000000000000 0000000000100010 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
page_owner tracks the page as allocated
page last allocated via order 0, migratetype Unmovable, gfp_mask 0x112cc0(GFP_USER|__GFP_NOWARN|__GFP_NORETRY), pid 3310, ts 611411618313, free_ts 611397869806
prep_new_page mm/page_alloc.c:2426 [inline]
get_page_from_freelist+0xa72/0x2f80 mm/page_alloc.c:4155
__alloc_pages+0x1b2/0x500 mm/page_alloc.c:5381
alloc_pages+0x1a7/0x300 mm/mempolicy.c:2191
alloc_slab_page mm/slub.c:1770 [inline]
allocate_slab mm/slub.c:1907 [inline]
new_slab+0x319/0x490 mm/slub.c:1970
___slab_alloc+0x921/0xfe0 mm/slub.c:3001
__slab_alloc.constprop.0+0x4d/0xa0 mm/slub.c:3088
slab_alloc_node mm/slub.c:3179 [inline]
slab_alloc mm/slub.c:3221 [inline]
__kmalloc_track_caller+0x2ef/0x310 mm/slub.c:4916
__do_krealloc mm/slab_common.c:1208 [inline]
krealloc+0x87/0xf0 mm/slab_common.c:1241
push_jmp_history kernel/bpf/verifier.c:2278 [inline]
is_state_visited kernel/bpf/verifier.c:10967 [inline]
do_check kernel/bpf/verifier.c:11107 [inline]
do_check_common+0x3521/0xcf50 kernel/bpf/verifier.c:13374
do_check_main kernel/bpf/verifier.c:13437 [inline]
bpf_check+0x87ca/0xbc90 kernel/bpf/verifier.c:14004
bpf_prog_load+0xf4c/0x21e0 kernel/bpf/syscall.c:2329
__sys_bpf+0x67e/0x5f00 kernel/bpf/syscall.c:4618
__do_sys_bpf kernel/bpf/syscall.c:4722 [inline]
__se_sys_bpf kernel/bpf/syscall.c:4720 [inline]
__x64_sys_bpf+0x75/0xb0 kernel/bpf/syscall.c:4720
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
page last free stack trace:
reset_page_owner include/linux/page_owner.h:24 [inline]
free_pages_prepare mm/page_alloc.c:1340 [inline]
free_pcp_prepare+0x326/0x810 mm/page_alloc.c:1391
free_unref_page_prepare mm/page_alloc.c:3317 [inline]
free_unref_page+0x19/0x690 mm/page_alloc.c:3396
__vunmap+0x781/0xb70 mm/vmalloc.c:2621
__vfree+0x3c/0xd0 mm/vmalloc.c:2669
vfree+0x5a/0x90 mm/vmalloc.c:2700
bpf_jit_free+0xbb/0x1c0
bpf_prog_free_deferred+0x5c1/0x790 kernel/bpf/core.c:2292
process_one_work+0x9b2/0x1690 kernel/workqueue.c:2297
worker_thread+0x658/0x11f0 kernel/workqueue.c:2444
kthread+0x405/0x4f0 kernel/kthread.c:327
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
Memory state around the buggy address:
ffff888032563400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffff888032563480: 04 fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff888032563500: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff888032563580: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
ffff888032563600: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================
---
This report 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 syzkaller@googlegroups.com.
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [syzbot] KASAN: use-after-free Read in remove_wait_queue (3)
2021-11-16 9:23 [syzbot] KASAN: use-after-free Read in remove_wait_queue (3) syzbot
@ 2021-12-10 22:42 ` syzbot
[not found] ` <20211211015620.1793-1-hdanton@sina.com>
1 sibling, 0 replies; 16+ messages in thread
From: syzbot @ 2021-12-10 22:42 UTC (permalink / raw)
To: linux-fsdevel, linux-kernel, netdev, syzkaller-bugs, viro
syzbot has found a reproducer for the following issue on:
HEAD commit: e5d75fc20b92 sh_eth: Use dev_err_probe() helper
git tree: net-next
console output: https://syzkaller.appspot.com/x/log.txt?x=1540cdceb00000
kernel config: https://syzkaller.appspot.com/x/.config?x=24fd48984584829b
dashboard link: https://syzkaller.appspot.com/bug?extid=cdb5dd11c97cc532efad
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15de00bab00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15ad646db00000
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+cdb5dd11c97cc532efad@syzkaller.appspotmail.com
==================================================================
BUG: KASAN: use-after-free in __lock_acquire+0x3d86/0x54a0 kernel/locking/lockdep.c:4897
Read of size 8 at addr ffff888015be3740 by task syz-executor161/3598
CPU: 1 PID: 3598 Comm: syz-executor161 Not tainted 5.16.0-rc4-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
print_address_description.constprop.0.cold+0x8d/0x320 mm/kasan/report.c:247
__kasan_report mm/kasan/report.c:433 [inline]
kasan_report.cold+0x83/0xdf mm/kasan/report.c:450
__lock_acquire+0x3d86/0x54a0 kernel/locking/lockdep.c:4897
lock_acquire kernel/locking/lockdep.c:5637 [inline]
lock_acquire+0x1ab/0x510 kernel/locking/lockdep.c:5602
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x39/0x50 kernel/locking/spinlock.c:162
remove_wait_queue+0x1d/0x180 kernel/sched/wait.c:55
ep_remove_wait_queue+0x88/0x1a0 fs/eventpoll.c:545
ep_unregister_pollwait fs/eventpoll.c:561 [inline]
ep_remove+0x106/0x9c0 fs/eventpoll.c:690
eventpoll_release_file+0xe1/0x130 fs/eventpoll.c:923
eventpoll_release include/linux/eventpoll.h:53 [inline]
__fput+0x87b/0x9f0 fs/file_table.c:271
task_work_run+0xdd/0x1a0 kernel/task_work.c:164
tracehook_notify_resume include/linux/tracehook.h:189 [inline]
exit_to_user_mode_loop kernel/entry/common.c:175 [inline]
exit_to_user_mode_prepare+0x27e/0x290 kernel/entry/common.c:207
__syscall_exit_to_user_mode_work kernel/entry/common.c:289 [inline]
syscall_exit_to_user_mode+0x19/0x60 kernel/entry/common.c:300
do_syscall_64+0x42/0xb0 arch/x86/entry/common.c:86
entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f3167c0def3
Code: c7 c2 c0 ff ff ff f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb ba 0f 1f 00 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 03 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 45 c3 0f 1f 40 00 48 83 ec 18 89 7c 24 0c e8
RSP: 002b:00007ffddef2e488 EFLAGS: 00000246 ORIG_RAX: 0000000000000003
RAX: 0000000000000000 RBX: 0000000000000005 RCX: 00007f3167c0def3
RDX: 000000000000002f RSI: 0000000020001340 RDI: 0000000000000004
RBP: 0000000000000000 R08: 0000000000000014 R09: 00007ffddef2e4b0
R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffddef2e4ac
R13: 00007ffddef2e4c0 R14: 00007ffddef2e500 R15: 0000000000000000
</TASK>
Allocated by task 3598:
kasan_save_stack+0x1e/0x50 mm/kasan/common.c:38
kasan_set_track mm/kasan/common.c:46 [inline]
set_alloc_info mm/kasan/common.c:434 [inline]
____kasan_kmalloc mm/kasan/common.c:513 [inline]
____kasan_kmalloc mm/kasan/common.c:472 [inline]
__kasan_kmalloc+0xa9/0xd0 mm/kasan/common.c:522
kmalloc include/linux/slab.h:590 [inline]
psi_trigger_create.part.0+0x15e/0x7f0 kernel/sched/psi.c:1141
cgroup_pressure_write+0x15d/0x6b0 kernel/cgroup/cgroup.c:3645
cgroup_file_write+0x1ec/0x780 kernel/cgroup/cgroup.c:3852
kernfs_fop_write_iter+0x342/0x500 fs/kernfs/file.c:296
call_write_iter include/linux/fs.h:2162 [inline]
new_sync_write+0x429/0x660 fs/read_write.c:503
vfs_write+0x7cd/0xae0 fs/read_write.c:590
ksys_write+0x12d/0x250 fs/read_write.c:643
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
Freed by task 3598:
kasan_save_stack+0x1e/0x50 mm/kasan/common.c:38
kasan_set_track+0x21/0x30 mm/kasan/common.c:46
kasan_set_free_info+0x20/0x30 mm/kasan/generic.c:370
____kasan_slab_free mm/kasan/common.c:366 [inline]
____kasan_slab_free mm/kasan/common.c:328 [inline]
__kasan_slab_free+0xff/0x130 mm/kasan/common.c:374
kasan_slab_free include/linux/kasan.h:235 [inline]
slab_free_hook mm/slub.c:1723 [inline]
slab_free_freelist_hook+0x8b/0x1c0 mm/slub.c:1749
slab_free mm/slub.c:3513 [inline]
kfree+0xf6/0x560 mm/slub.c:4561
cgroup_pressure_write+0x18d/0x6b0 kernel/cgroup/cgroup.c:3651
cgroup_file_write+0x1ec/0x780 kernel/cgroup/cgroup.c:3852
kernfs_fop_write_iter+0x342/0x500 fs/kernfs/file.c:296
call_write_iter include/linux/fs.h:2162 [inline]
new_sync_write+0x429/0x660 fs/read_write.c:503
vfs_write+0x7cd/0xae0 fs/read_write.c:590
ksys_write+0x12d/0x250 fs/read_write.c:643
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
The buggy address belongs to the object at ffff888015be3700
which belongs to the cache kmalloc-192 of size 192
The buggy address is located 64 bytes inside of
192-byte region [ffff888015be3700, ffff888015be37c0)
The buggy address belongs to the page:
page:ffffea000056f8c0 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x15be3
flags: 0xfff00000000200(slab|node=0|zone=1|lastcpupid=0x7ff)
raw: 00fff00000000200 0000000000000000 dead000000000001 ffff888010c41a00
raw: 0000000000000000 0000000080100010 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
page_owner tracks the page as allocated
page last allocated via order 0, migratetype Unmovable, gfp_mask 0x12cc0(GFP_KERNEL|__GFP_NOWARN|__GFP_NORETRY), pid 1, ts 1983850449, free_ts 0
prep_new_page mm/page_alloc.c:2418 [inline]
get_page_from_freelist+0xa72/0x2f50 mm/page_alloc.c:4149
__alloc_pages+0x1b2/0x500 mm/page_alloc.c:5369
alloc_page_interleave+0x1e/0x200 mm/mempolicy.c:2036
alloc_pages+0x29f/0x300 mm/mempolicy.c:2186
alloc_slab_page mm/slub.c:1793 [inline]
allocate_slab mm/slub.c:1930 [inline]
new_slab+0x32d/0x4a0 mm/slub.c:1993
___slab_alloc+0x918/0xfe0 mm/slub.c:3022
__slab_alloc.constprop.0+0x4d/0xa0 mm/slub.c:3109
slab_alloc_node mm/slub.c:3200 [inline]
slab_alloc mm/slub.c:3242 [inline]
kmem_cache_alloc_trace+0x289/0x2c0 mm/slub.c:3259
kmalloc include/linux/slab.h:590 [inline]
kzalloc include/linux/slab.h:724 [inline]
call_usermodehelper_setup+0x97/0x340 kernel/umh.c:365
kobject_uevent_env+0xf73/0x1650 lib/kobject_uevent.c:614
version_sysfs_builtin kernel/params.c:878 [inline]
param_sysfs_init+0x146/0x43b kernel/params.c:969
do_one_initcall+0x103/0x650 init/main.c:1297
do_initcall_level init/main.c:1370 [inline]
do_initcalls init/main.c:1386 [inline]
do_basic_setup init/main.c:1405 [inline]
kernel_init_freeable+0x6b1/0x73a init/main.c:1610
kernel_init+0x1a/0x1d0 init/main.c:1499
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
page_owner free stack trace missing
Memory state around the buggy address:
ffff888015be3600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffff888015be3680: 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc
>ffff888015be3700: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff888015be3780: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
ffff888015be3800: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
==================================================================
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [syzbot] KASAN: use-after-free Read in remove_wait_queue (3)
[not found] ` <20211211015620.1793-1-hdanton@sina.com>
@ 2021-12-11 3:00 ` Eric Biggers
2022-01-05 15:20 ` psi_trigger_poll() is completely broken Eric Biggers
0 siblings, 1 reply; 16+ messages in thread
From: Eric Biggers @ 2021-12-11 3:00 UTC (permalink / raw)
To: Johannes Weiner, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Ingo Molnar, Hillf Danton
Cc: syzbot, linux-fsdevel, linux-kernel, syzkaller-bugs, linux-mm,
Linus Torvalds
On Sat, Dec 11, 2021 at 09:56:20AM +0800, Hillf Danton wrote:
> On Fri, 10 Dec 2021 14:42:22 -0800
> > syzbot has found a reproducer for the following issue on:
> >
> > HEAD commit: e5d75fc20b92 sh_eth: Use dev_err_probe() helper
> > git tree: net-next
> > console output: https://syzkaller.appspot.com/x/log.txt?x=1540cdceb00000
> > kernel config: https://syzkaller.appspot.com/x/.config?x=24fd48984584829b
> > dashboard link: https://syzkaller.appspot.com/bug?extid=cdb5dd11c97cc532efad
> > compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15de00bab00000
> > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15ad646db00000
> >
> > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > Reported-by: syzbot+cdb5dd11c97cc532efad@syzkaller.appspotmail.com
> >
> > ==================================================================
> > BUG: KASAN: use-after-free in __lock_acquire+0x3d86/0x54a0 kernel/locking/lockdep.c:4897
> > Read of size 8 at addr ffff888015be3740 by task syz-executor161/3598
> >
> > CPU: 1 PID: 3598 Comm: syz-executor161 Not tainted 5.16.0-rc4-syzkaller #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > Call Trace:
> > <TASK>
> > __dump_stack lib/dump_stack.c:88 [inline]
> > dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
> > print_address_description.constprop.0.cold+0x8d/0x320 mm/kasan/report.c:247
> > __kasan_report mm/kasan/report.c:433 [inline]
> > kasan_report.cold+0x83/0xdf mm/kasan/report.c:450
> > __lock_acquire+0x3d86/0x54a0 kernel/locking/lockdep.c:4897
> > lock_acquire kernel/locking/lockdep.c:5637 [inline]
> > lock_acquire+0x1ab/0x510 kernel/locking/lockdep.c:5602
> > __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
> > _raw_spin_lock_irqsave+0x39/0x50 kernel/locking/spinlock.c:162
> > remove_wait_queue+0x1d/0x180 kernel/sched/wait.c:55
> > ep_remove_wait_queue+0x88/0x1a0 fs/eventpoll.c:545
> > ep_unregister_pollwait fs/eventpoll.c:561 [inline]
> > ep_remove+0x106/0x9c0 fs/eventpoll.c:690
> > eventpoll_release_file+0xe1/0x130 fs/eventpoll.c:923
> > eventpoll_release include/linux/eventpoll.h:53 [inline]
> > __fput+0x87b/0x9f0 fs/file_table.c:271
> > task_work_run+0xdd/0x1a0 kernel/task_work.c:164
> > tracehook_notify_resume include/linux/tracehook.h:189 [inline]
> > exit_to_user_mode_loop kernel/entry/common.c:175 [inline]
> > exit_to_user_mode_prepare+0x27e/0x290 kernel/entry/common.c:207
> > __syscall_exit_to_user_mode_work kernel/entry/common.c:289 [inline]
> > syscall_exit_to_user_mode+0x19/0x60 kernel/entry/common.c:300
> > do_syscall_64+0x42/0xb0 arch/x86/entry/common.c:86
> > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > RIP: 0033:0x7f3167c0def3
> > Code: c7 c2 c0 ff ff ff f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb ba 0f 1f 00 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 03 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 45 c3 0f 1f 40 00 48 83 ec 18 89 7c 24 0c e8
> > RSP: 002b:00007ffddef2e488 EFLAGS: 00000246 ORIG_RAX: 0000000000000003
> > RAX: 0000000000000000 RBX: 0000000000000005 RCX: 00007f3167c0def3
> > RDX: 000000000000002f RSI: 0000000020001340 RDI: 0000000000000004
> > RBP: 0000000000000000 R08: 0000000000000014 R09: 00007ffddef2e4b0
> > R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffddef2e4ac
> > R13: 00007ffddef2e4c0 R14: 00007ffddef2e500 R15: 0000000000000000
> > </TASK>
> >
> > Allocated by task 3598:
> > kasan_save_stack+0x1e/0x50 mm/kasan/common.c:38
> > kasan_set_track mm/kasan/common.c:46 [inline]
> > set_alloc_info mm/kasan/common.c:434 [inline]
> > ____kasan_kmalloc mm/kasan/common.c:513 [inline]
> > ____kasan_kmalloc mm/kasan/common.c:472 [inline]
> > __kasan_kmalloc+0xa9/0xd0 mm/kasan/common.c:522
> > kmalloc include/linux/slab.h:590 [inline]
> > psi_trigger_create.part.0+0x15e/0x7f0 kernel/sched/psi.c:1141
> > cgroup_pressure_write+0x15d/0x6b0 kernel/cgroup/cgroup.c:3645
> > cgroup_file_write+0x1ec/0x780 kernel/cgroup/cgroup.c:3852
> > kernfs_fop_write_iter+0x342/0x500 fs/kernfs/file.c:296
> > call_write_iter include/linux/fs.h:2162 [inline]
> > new_sync_write+0x429/0x660 fs/read_write.c:503
> > vfs_write+0x7cd/0xae0 fs/read_write.c:590
> > ksys_write+0x12d/0x250 fs/read_write.c:643
> > do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> > do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> > entry_SYSCALL_64_after_hwframe+0x44/0xae
> >
> > Freed by task 3598:
> > kasan_save_stack+0x1e/0x50 mm/kasan/common.c:38
> > kasan_set_track+0x21/0x30 mm/kasan/common.c:46
> > kasan_set_free_info+0x20/0x30 mm/kasan/generic.c:370
> > ____kasan_slab_free mm/kasan/common.c:366 [inline]
> > ____kasan_slab_free mm/kasan/common.c:328 [inline]
> > __kasan_slab_free+0xff/0x130 mm/kasan/common.c:374
> > kasan_slab_free include/linux/kasan.h:235 [inline]
> > slab_free_hook mm/slub.c:1723 [inline]
> > slab_free_freelist_hook+0x8b/0x1c0 mm/slub.c:1749
> > slab_free mm/slub.c:3513 [inline]
> > kfree+0xf6/0x560 mm/slub.c:4561
> > cgroup_pressure_write+0x18d/0x6b0 kernel/cgroup/cgroup.c:3651
> > cgroup_file_write+0x1ec/0x780 kernel/cgroup/cgroup.c:3852
> > kernfs_fop_write_iter+0x342/0x500 fs/kernfs/file.c:296
> > call_write_iter include/linux/fs.h:2162 [inline]
> > new_sync_write+0x429/0x660 fs/read_write.c:503
> > vfs_write+0x7cd/0xae0 fs/read_write.c:590
> > ksys_write+0x12d/0x250 fs/read_write.c:643
> > do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> > do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> > entry_SYSCALL_64_after_hwframe+0x44/0xae
> >
> > The buggy address belongs to the object at ffff888015be3700
> > which belongs to the cache kmalloc-192 of size 192
> > The buggy address is located 64 bytes inside of
> > 192-byte region [ffff888015be3700, ffff888015be37c0)
> > The buggy address belongs to the page:
> > page:ffffea000056f8c0 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x15be3
> > flags: 0xfff00000000200(slab|node=0|zone=1|lastcpupid=0x7ff)
> > raw: 00fff00000000200 0000000000000000 dead000000000001 ffff888010c41a00
> > raw: 0000000000000000 0000000080100010 00000001ffffffff 0000000000000000
> > page dumped because: kasan: bad access detected
> > page_owner tracks the page as allocated
> > page last allocated via order 0, migratetype Unmovable, gfp_mask 0x12cc0(GFP_KERNEL|__GFP_NOWARN|__GFP_NORETRY), pid 1, ts 1983850449, free_ts 0
> > prep_new_page mm/page_alloc.c:2418 [inline]
> > get_page_from_freelist+0xa72/0x2f50 mm/page_alloc.c:4149
> > __alloc_pages+0x1b2/0x500 mm/page_alloc.c:5369
> > alloc_page_interleave+0x1e/0x200 mm/mempolicy.c:2036
> > alloc_pages+0x29f/0x300 mm/mempolicy.c:2186
> > alloc_slab_page mm/slub.c:1793 [inline]
> > allocate_slab mm/slub.c:1930 [inline]
> > new_slab+0x32d/0x4a0 mm/slub.c:1993
> > ___slab_alloc+0x918/0xfe0 mm/slub.c:3022
> > __slab_alloc.constprop.0+0x4d/0xa0 mm/slub.c:3109
> > slab_alloc_node mm/slub.c:3200 [inline]
> > slab_alloc mm/slub.c:3242 [inline]
> > kmem_cache_alloc_trace+0x289/0x2c0 mm/slub.c:3259
> > kmalloc include/linux/slab.h:590 [inline]
> > kzalloc include/linux/slab.h:724 [inline]
> > call_usermodehelper_setup+0x97/0x340 kernel/umh.c:365
> > kobject_uevent_env+0xf73/0x1650 lib/kobject_uevent.c:614
> > version_sysfs_builtin kernel/params.c:878 [inline]
> > param_sysfs_init+0x146/0x43b kernel/params.c:969
> > do_one_initcall+0x103/0x650 init/main.c:1297
> > do_initcall_level init/main.c:1370 [inline]
> > do_initcalls init/main.c:1386 [inline]
> > do_basic_setup init/main.c:1405 [inline]
> > kernel_init_freeable+0x6b1/0x73a init/main.c:1610
> > kernel_init+0x1a/0x1d0 init/main.c:1499
> > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
> > page_owner free stack trace missing
> >
> > Memory state around the buggy address:
> > ffff888015be3600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > ffff888015be3680: 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc
> > >ffff888015be3700: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> > ^
> > ffff888015be3780: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
> > ffff888015be3800: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > ==================================================================
>
> Hey Eric
>
> Let us know if this uaf adds another call site for what you added [1].
>
> Hillf
>
> [1] https://lore.kernel.org/lkml/20211209010455.42744-2-ebiggers@kernel.org/
>
> +++ x/kernel/sched/psi.c
> @@ -1193,7 +1193,7 @@ static void psi_trigger_destroy(struct k
> * Wakeup waiters to stop polling. Can happen if cgroup is deleted
> * from under a polling process.
> */
> - wake_up_interruptible(&t->event_wait);
> + wake_up_pollfree(&t->event_wait);
>
> mutex_lock(&group->trigger_lock);
[added linux-mm and all maintainers for kernel/sched/psi.c]
Well, it is the same sort of issue, but POLLFREE is *not* enough here. POLLFREE
only works if the lifetime of waitqueue is tied to the polling task, as blocking
polls don't handle it -- only non-blocking polls do.
The kernel/sched/psi.c use case is just totally broken, since the lifetime of
its waitqueue is totally arbitrary; the open file descriptor can be written to
at any time by any process, which causes the waitqueue to be freed. So it will
cause a use-after-free even for regular blocking poll().
To fix this, I think the psi trigger stuff will need to be refactored to have
just one waitqueue per open file. We need to be removing uses of POLLFREE, not
adding new ones. (See Linus' comments on POLLFREE here:
https://lore.kernel.org/lkml/CAHk-=wgvt7PH+AU_29H95tJQZ9FnhS8vVmymbhpZ6NZ7yaAigw@mail.gmail.com/)
Here are some repros:
#include <fcntl.h>
#include <sys/epoll.h>
#include <unistd.h>
int main()
{
int fd = open("/proc/pressure/cpu", O_RDWR);
int epfd = epoll_create(1);
const char trigger[] = "some 100000 1000000";
struct epoll_event event = { .events = EPOLLIN };
write(fd, trigger, sizeof(trigger));
epoll_ctl(epfd, EPOLL_CTL_ADD, fd, &event);
write(fd, trigger, sizeof(trigger));
}
#include <fcntl.h>
#include <sys/poll.h>
#include <unistd.h>
int main()
{
int fd = open("/proc/pressure/cpu", O_RDWR);
const char trigger[] = "some 100000 1000000";
if (fork()) {
struct pollfd pfd = { .fd = fd, .events = POLLIN };
for (;;)
poll(&pfd, 1, -1);
} else {
for (;;)
write(fd, trigger, sizeof(trigger));
}
}
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: psi_trigger_poll() is completely broken
2021-12-11 3:00 ` Eric Biggers
@ 2022-01-05 15:20 ` Eric Biggers
2022-01-05 18:50 ` Linus Torvalds
0 siblings, 1 reply; 16+ messages in thread
From: Eric Biggers @ 2022-01-05 15:20 UTC (permalink / raw)
To: Johannes Weiner, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Ingo Molnar, Hillf Danton
Cc: syzbot, linux-fsdevel, linux-kernel, syzkaller-bugs, linux-mm,
Linus Torvalds
[changed subject line to hopefully get people to stop ignoring this]
Please see my message below where I explained the problem in detail. Any
response from the maintainers of kernel/sched/psi.c? There are a lot of you:
$ ./scripts/get_maintainer.pl kernel/sched/psi.c
Johannes Weiner <hannes@cmpxchg.org> (maintainer:PRESSURE STALL INFORMATION (PSI))
Ingo Molnar <mingo@redhat.com> (maintainer:SCHEDULER)
Peter Zijlstra <peterz@infradead.org> (maintainer:SCHEDULER)
Juri Lelli <juri.lelli@redhat.com> (maintainer:SCHEDULER)
Vincent Guittot <vincent.guittot@linaro.org> (maintainer:SCHEDULER)
Dietmar Eggemann <dietmar.eggemann@arm.com> (reviewer:SCHEDULER)
Steven Rostedt <rostedt@goodmis.org> (reviewer:SCHEDULER)
Ben Segall <bsegall@google.com> (reviewer:SCHEDULER)
Mel Gorman <mgorman@suse.de> (reviewer:SCHEDULER)
Daniel Bristot de Oliveira <bristot@redhat.com> (reviewer:SCHEDULER)
linux-kernel@vger.kernel.org (open list:SCHEDULER)
On Fri, Dec 10, 2021 at 07:00:26PM -0800, Eric Biggers wrote:
> On Sat, Dec 11, 2021 at 09:56:20AM +0800, Hillf Danton wrote:
> > On Fri, 10 Dec 2021 14:42:22 -0800
> > > syzbot has found a reproducer for the following issue on:
> > >
> > > HEAD commit: e5d75fc20b92 sh_eth: Use dev_err_probe() helper
> > > git tree: net-next
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=1540cdceb00000
> > > kernel config: https://syzkaller.appspot.com/x/.config?x=24fd48984584829b
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=cdb5dd11c97cc532efad
> > > compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15de00bab00000
> > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15ad646db00000
> > >
> > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > Reported-by: syzbot+cdb5dd11c97cc532efad@syzkaller.appspotmail.com
> > >
> > > ==================================================================
> > > BUG: KASAN: use-after-free in __lock_acquire+0x3d86/0x54a0 kernel/locking/lockdep.c:4897
> > > Read of size 8 at addr ffff888015be3740 by task syz-executor161/3598
> > >
> > > CPU: 1 PID: 3598 Comm: syz-executor161 Not tainted 5.16.0-rc4-syzkaller #0
> > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > > Call Trace:
> > > <TASK>
> > > __dump_stack lib/dump_stack.c:88 [inline]
> > > dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
> > > print_address_description.constprop.0.cold+0x8d/0x320 mm/kasan/report.c:247
> > > __kasan_report mm/kasan/report.c:433 [inline]
> > > kasan_report.cold+0x83/0xdf mm/kasan/report.c:450
> > > __lock_acquire+0x3d86/0x54a0 kernel/locking/lockdep.c:4897
> > > lock_acquire kernel/locking/lockdep.c:5637 [inline]
> > > lock_acquire+0x1ab/0x510 kernel/locking/lockdep.c:5602
> > > __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
> > > _raw_spin_lock_irqsave+0x39/0x50 kernel/locking/spinlock.c:162
> > > remove_wait_queue+0x1d/0x180 kernel/sched/wait.c:55
> > > ep_remove_wait_queue+0x88/0x1a0 fs/eventpoll.c:545
> > > ep_unregister_pollwait fs/eventpoll.c:561 [inline]
> > > ep_remove+0x106/0x9c0 fs/eventpoll.c:690
> > > eventpoll_release_file+0xe1/0x130 fs/eventpoll.c:923
> > > eventpoll_release include/linux/eventpoll.h:53 [inline]
> > > __fput+0x87b/0x9f0 fs/file_table.c:271
> > > task_work_run+0xdd/0x1a0 kernel/task_work.c:164
> > > tracehook_notify_resume include/linux/tracehook.h:189 [inline]
> > > exit_to_user_mode_loop kernel/entry/common.c:175 [inline]
> > > exit_to_user_mode_prepare+0x27e/0x290 kernel/entry/common.c:207
> > > __syscall_exit_to_user_mode_work kernel/entry/common.c:289 [inline]
> > > syscall_exit_to_user_mode+0x19/0x60 kernel/entry/common.c:300
> > > do_syscall_64+0x42/0xb0 arch/x86/entry/common.c:86
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > RIP: 0033:0x7f3167c0def3
> > > Code: c7 c2 c0 ff ff ff f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb ba 0f 1f 00 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 03 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 45 c3 0f 1f 40 00 48 83 ec 18 89 7c 24 0c e8
> > > RSP: 002b:00007ffddef2e488 EFLAGS: 00000246 ORIG_RAX: 0000000000000003
> > > RAX: 0000000000000000 RBX: 0000000000000005 RCX: 00007f3167c0def3
> > > RDX: 000000000000002f RSI: 0000000020001340 RDI: 0000000000000004
> > > RBP: 0000000000000000 R08: 0000000000000014 R09: 00007ffddef2e4b0
> > > R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffddef2e4ac
> > > R13: 00007ffddef2e4c0 R14: 00007ffddef2e500 R15: 0000000000000000
> > > </TASK>
> > >
> > > Allocated by task 3598:
> > > kasan_save_stack+0x1e/0x50 mm/kasan/common.c:38
> > > kasan_set_track mm/kasan/common.c:46 [inline]
> > > set_alloc_info mm/kasan/common.c:434 [inline]
> > > ____kasan_kmalloc mm/kasan/common.c:513 [inline]
> > > ____kasan_kmalloc mm/kasan/common.c:472 [inline]
> > > __kasan_kmalloc+0xa9/0xd0 mm/kasan/common.c:522
> > > kmalloc include/linux/slab.h:590 [inline]
> > > psi_trigger_create.part.0+0x15e/0x7f0 kernel/sched/psi.c:1141
> > > cgroup_pressure_write+0x15d/0x6b0 kernel/cgroup/cgroup.c:3645
> > > cgroup_file_write+0x1ec/0x780 kernel/cgroup/cgroup.c:3852
> > > kernfs_fop_write_iter+0x342/0x500 fs/kernfs/file.c:296
> > > call_write_iter include/linux/fs.h:2162 [inline]
> > > new_sync_write+0x429/0x660 fs/read_write.c:503
> > > vfs_write+0x7cd/0xae0 fs/read_write.c:590
> > > ksys_write+0x12d/0x250 fs/read_write.c:643
> > > do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> > > do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > Freed by task 3598:
> > > kasan_save_stack+0x1e/0x50 mm/kasan/common.c:38
> > > kasan_set_track+0x21/0x30 mm/kasan/common.c:46
> > > kasan_set_free_info+0x20/0x30 mm/kasan/generic.c:370
> > > ____kasan_slab_free mm/kasan/common.c:366 [inline]
> > > ____kasan_slab_free mm/kasan/common.c:328 [inline]
> > > __kasan_slab_free+0xff/0x130 mm/kasan/common.c:374
> > > kasan_slab_free include/linux/kasan.h:235 [inline]
> > > slab_free_hook mm/slub.c:1723 [inline]
> > > slab_free_freelist_hook+0x8b/0x1c0 mm/slub.c:1749
> > > slab_free mm/slub.c:3513 [inline]
> > > kfree+0xf6/0x560 mm/slub.c:4561
> > > cgroup_pressure_write+0x18d/0x6b0 kernel/cgroup/cgroup.c:3651
> > > cgroup_file_write+0x1ec/0x780 kernel/cgroup/cgroup.c:3852
> > > kernfs_fop_write_iter+0x342/0x500 fs/kernfs/file.c:296
> > > call_write_iter include/linux/fs.h:2162 [inline]
> > > new_sync_write+0x429/0x660 fs/read_write.c:503
> > > vfs_write+0x7cd/0xae0 fs/read_write.c:590
> > > ksys_write+0x12d/0x250 fs/read_write.c:643
> > > do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> > > do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > The buggy address belongs to the object at ffff888015be3700
> > > which belongs to the cache kmalloc-192 of size 192
> > > The buggy address is located 64 bytes inside of
> > > 192-byte region [ffff888015be3700, ffff888015be37c0)
> > > The buggy address belongs to the page:
> > > page:ffffea000056f8c0 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x15be3
> > > flags: 0xfff00000000200(slab|node=0|zone=1|lastcpupid=0x7ff)
> > > raw: 00fff00000000200 0000000000000000 dead000000000001 ffff888010c41a00
> > > raw: 0000000000000000 0000000080100010 00000001ffffffff 0000000000000000
> > > page dumped because: kasan: bad access detected
> > > page_owner tracks the page as allocated
> > > page last allocated via order 0, migratetype Unmovable, gfp_mask 0x12cc0(GFP_KERNEL|__GFP_NOWARN|__GFP_NORETRY), pid 1, ts 1983850449, free_ts 0
> > > prep_new_page mm/page_alloc.c:2418 [inline]
> > > get_page_from_freelist+0xa72/0x2f50 mm/page_alloc.c:4149
> > > __alloc_pages+0x1b2/0x500 mm/page_alloc.c:5369
> > > alloc_page_interleave+0x1e/0x200 mm/mempolicy.c:2036
> > > alloc_pages+0x29f/0x300 mm/mempolicy.c:2186
> > > alloc_slab_page mm/slub.c:1793 [inline]
> > > allocate_slab mm/slub.c:1930 [inline]
> > > new_slab+0x32d/0x4a0 mm/slub.c:1993
> > > ___slab_alloc+0x918/0xfe0 mm/slub.c:3022
> > > __slab_alloc.constprop.0+0x4d/0xa0 mm/slub.c:3109
> > > slab_alloc_node mm/slub.c:3200 [inline]
> > > slab_alloc mm/slub.c:3242 [inline]
> > > kmem_cache_alloc_trace+0x289/0x2c0 mm/slub.c:3259
> > > kmalloc include/linux/slab.h:590 [inline]
> > > kzalloc include/linux/slab.h:724 [inline]
> > > call_usermodehelper_setup+0x97/0x340 kernel/umh.c:365
> > > kobject_uevent_env+0xf73/0x1650 lib/kobject_uevent.c:614
> > > version_sysfs_builtin kernel/params.c:878 [inline]
> > > param_sysfs_init+0x146/0x43b kernel/params.c:969
> > > do_one_initcall+0x103/0x650 init/main.c:1297
> > > do_initcall_level init/main.c:1370 [inline]
> > > do_initcalls init/main.c:1386 [inline]
> > > do_basic_setup init/main.c:1405 [inline]
> > > kernel_init_freeable+0x6b1/0x73a init/main.c:1610
> > > kernel_init+0x1a/0x1d0 init/main.c:1499
> > > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
> > > page_owner free stack trace missing
> > >
> > > Memory state around the buggy address:
> > > ffff888015be3600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > ffff888015be3680: 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc
> > > >ffff888015be3700: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> > > ^
> > > ffff888015be3780: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
> > > ffff888015be3800: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > ==================================================================
> >
> > Hey Eric
> >
> > Let us know if this uaf adds another call site for what you added [1].
> >
> > Hillf
> >
> > [1] https://lore.kernel.org/lkml/20211209010455.42744-2-ebiggers@kernel.org/
> >
> > +++ x/kernel/sched/psi.c
> > @@ -1193,7 +1193,7 @@ static void psi_trigger_destroy(struct k
> > * Wakeup waiters to stop polling. Can happen if cgroup is deleted
> > * from under a polling process.
> > */
> > - wake_up_interruptible(&t->event_wait);
> > + wake_up_pollfree(&t->event_wait);
> >
> > mutex_lock(&group->trigger_lock);
>
> [added linux-mm and all maintainers for kernel/sched/psi.c]
>
> Well, it is the same sort of issue, but POLLFREE is *not* enough here. POLLFREE
> only works if the lifetime of waitqueue is tied to the polling task, as blocking
> polls don't handle it -- only non-blocking polls do.
>
> The kernel/sched/psi.c use case is just totally broken, since the lifetime of
> its waitqueue is totally arbitrary; the open file descriptor can be written to
> at any time by any process, which causes the waitqueue to be freed. So it will
> cause a use-after-free even for regular blocking poll().
>
> To fix this, I think the psi trigger stuff will need to be refactored to have
> just one waitqueue per open file. We need to be removing uses of POLLFREE, not
> adding new ones. (See Linus' comments on POLLFREE here:
> https://lore.kernel.org/lkml/CAHk-=wgvt7PH+AU_29H95tJQZ9FnhS8vVmymbhpZ6NZ7yaAigw@mail.gmail.com/)
>
> Here are some repros:
>
> #include <fcntl.h>
> #include <sys/epoll.h>
> #include <unistd.h>
> int main()
> {
> int fd = open("/proc/pressure/cpu", O_RDWR);
> int epfd = epoll_create(1);
> const char trigger[] = "some 100000 1000000";
> struct epoll_event event = { .events = EPOLLIN };
>
> write(fd, trigger, sizeof(trigger));
> epoll_ctl(epfd, EPOLL_CTL_ADD, fd, &event);
> write(fd, trigger, sizeof(trigger));
> }
>
>
> #include <fcntl.h>
> #include <sys/poll.h>
> #include <unistd.h>
> int main()
> {
> int fd = open("/proc/pressure/cpu", O_RDWR);
> const char trigger[] = "some 100000 1000000";
>
> if (fork()) {
> struct pollfd pfd = { .fd = fd, .events = POLLIN };
>
> for (;;)
> poll(&pfd, 1, -1);
> } else {
> for (;;)
> write(fd, trigger, sizeof(trigger));
> }
> }
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: psi_trigger_poll() is completely broken
2022-01-05 15:20 ` psi_trigger_poll() is completely broken Eric Biggers
@ 2022-01-05 18:50 ` Linus Torvalds
2022-01-05 19:07 ` Linus Torvalds
0 siblings, 1 reply; 16+ messages in thread
From: Linus Torvalds @ 2022-01-05 18:50 UTC (permalink / raw)
To: Eric Biggers
Cc: Johannes Weiner, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Ingo Molnar, Hillf Danton, syzbot, linux-fsdevel,
Linux Kernel Mailing List, syzkaller-bugs, Linux-MM
On Wed, Jan 5, 2022 at 7:21 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> [changed subject line to hopefully get people to stop ignoring this]
>
> Please see my message below where I explained the problem in detail. Any
> response from the maintainers of kernel/sched/psi.c? There are a lot of you:
Ok, this one is clearly a kernel/sched/psi.c bug, since the lifetime
isn't even maintained by the fiel reference.
I think the proper thing to do is to move the whole "get kref to
trigger pointer" in the open/close code, and keep the ref around that
way.
The natural thing to do would be to look up the trigger at open time,
save the pointer in seq->private, and release it at close time.
Sadly, right now the code actually uses that 'seq->private' as an
indirect rcu-pointer to the trigger data, instead of as the trigger
data itself. And that seems very much on purpose and inherent to that
'psi_write()' model, where it changes the trigger pointer very much on
purpose.
So I agree 100% - the PSI code is fundamentally broken. psi_write()
seems to be literally _designed_ to do the wrong thing.
I don't know who - if anybody - uses this. My preference would be to
just disable the completely broken poll support.
Another alternative is to just make 'psi_write()' return -EBUSY if
there are existing poll waiters (ie t->event_wait not being empty. At
least then the open file would keep the kref to the trigger.
That would require that 'psi_trigger_replace()' serialize with the
waitqueue lock (easy), but all callers would also have to check the
return value of it
The cgroup code does
psi_trigger_replace(&of->priv, NULL);
in the release function, but I guess that might work since at release
time there shouldn't be any pending polls anyway.
But it would also mean that anybody who can open the file for reading
(so that they can poll it) would be able to keep people from changing
it.
But yes, I think that unless we get some reply from the PSI
maintainers, we will have to just disable polling entirely.
I hope there are no users that would break, but considering that the
current code is clearly too broken to live, this may be one of those
"we have no choice" cases.
Linus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: psi_trigger_poll() is completely broken
2022-01-05 18:50 ` Linus Torvalds
@ 2022-01-05 19:07 ` Linus Torvalds
2022-01-05 19:13 ` Linus Torvalds
0 siblings, 1 reply; 16+ messages in thread
From: Linus Torvalds @ 2022-01-05 19:07 UTC (permalink / raw)
To: Eric Biggers, Tejun Heo, Zefan Li
Cc: Johannes Weiner, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Ingo Molnar, Hillf Danton, syzbot, linux-fsdevel,
Linux Kernel Mailing List, syzkaller-bugs, Linux-MM
On Wed, Jan 5, 2022 at 10:50 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> That would require that 'psi_trigger_replace()' serialize with the
> waitqueue lock (easy)
I take the "easy" back. The other side of that serialization would
require that the poll() side also re-lookup the trigger pointer under
that same lock.
And you can't do that with the waitqueue lock, because 'poll_wait()'
does the add_wait_queue() internally, and that will take the waitqueue
lock. So you can't take and hold the waitqueue lock in the caller in
poll, it would just deadlock.
And not holding the lock over the call would mean that you'd have a
small race between adding a new poll waiter, and checking that the
trigger is still the same one.
We could use another lock - the code in kernel/sched/psi.c already does
mutex_lock(&seq->lock);
psi_trigger_replace(&seq->private, new);
mutex_unlock(&seq->lock);
and could use that same lock around the poll sequence too.
But the cgroup_pressure_write() code doesn't even do that, and
concurrent writes aren't serialized at all (much less concurrent
poll() calls).
Side note: it looks like concurrent writes in the
cgroup_pressure_write() is literally broken. Because
psi_trigger_replace() is *not* handling concurrency, and does that
struct psi_trigger *old = *trigger_ptr;
....
if (old)
kref_put(&old->refcount, psi_trigger_destroy);
assuming that the caller holds some lock that makes '*trigger_ptr' a
stable thing.
Again, kernel/sched/psi.c itself does that already, but the cgroup
code doesn't seem to.
So the bugs in this area go deeper than "just" poll(). The whole
psi_trigger_replace() thing is literally broken even ignoring the
poll() interactions.
Whoever came up with that stupid "replace existing trigger with a
write()" model should feel bad. It's garbage, and it's actively buggy
in multiple ways.
Linus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: psi_trigger_poll() is completely broken
2022-01-05 19:07 ` Linus Torvalds
@ 2022-01-05 19:13 ` Linus Torvalds
2022-01-06 22:05 ` Tejun Heo
2022-01-10 13:45 ` Johannes Weiner
0 siblings, 2 replies; 16+ messages in thread
From: Linus Torvalds @ 2022-01-05 19:13 UTC (permalink / raw)
To: Eric Biggers, Tejun Heo, Zefan Li
Cc: Johannes Weiner, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Ingo Molnar, Hillf Danton, syzbot, linux-fsdevel,
Linux Kernel Mailing List, syzkaller-bugs, Linux-MM
On Wed, Jan 5, 2022 at 11:07 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Whoever came up with that stupid "replace existing trigger with a
> write()" model should feel bad. It's garbage, and it's actively buggy
> in multiple ways.
What are the users? Can we make the rule for -EBUSY simply be that you
can _install_ a trigger, but you can't replace an existing one (except
with NULL, when you close it).
That would fix the poll() lifetime issue, and would make the
psi_trigger_replace() races fairly easy to fix - just use
if (cmpxchg(trigger_ptr, NULL, new) != NULL) {
... free 'new', return -EBUSY ..
to install the new one, instead of
rcu_assign_pointer(*trigger_ptr, new);
or something like that. No locking necessary.
But I assume people actually end up re-writing triggers, because
people are perverse and have taken advantage of this completely broken
API.
Linus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: psi_trigger_poll() is completely broken
2022-01-05 19:13 ` Linus Torvalds
@ 2022-01-06 22:05 ` Tejun Heo
2022-01-06 22:59 ` Linus Torvalds
2022-01-10 13:45 ` Johannes Weiner
1 sibling, 1 reply; 16+ messages in thread
From: Tejun Heo @ 2022-01-06 22:05 UTC (permalink / raw)
To: Linus Torvalds
Cc: Eric Biggers, Zefan Li, Johannes Weiner, Peter Zijlstra,
Juri Lelli, Vincent Guittot, Ingo Molnar, Hillf Danton, syzbot,
linux-fsdevel, Linux Kernel Mailing List, syzkaller-bugs,
Linux-MM
Happy new year,
On Wed, Jan 05, 2022 at 11:13:30AM -0800, Linus Torvalds wrote:
> On Wed, Jan 5, 2022 at 11:07 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Whoever came up with that stupid "replace existing trigger with a
> > write()" model should feel bad. It's garbage, and it's actively buggy
> > in multiple ways.
>
> What are the users? Can we make the rule for -EBUSY simply be that you
> can _install_ a trigger, but you can't replace an existing one (except
> with NULL, when you close it).
I don't have enough context here and Johannes seems offline today. Let's
wait for him to chime in.
> That would fix the poll() lifetime issue, and would make the
> psi_trigger_replace() races fairly easy to fix - just use
>
> if (cmpxchg(trigger_ptr, NULL, new) != NULL) {
> ... free 'new', return -EBUSY ..
>
> to install the new one, instead of
>
> rcu_assign_pointer(*trigger_ptr, new);
>
> or something like that. No locking necessary.
>
> But I assume people actually end up re-writing triggers, because
> people are perverse and have taken advantage of this completely broken
> API.
IIRC, the rationale for the shared trigger at the time was around the
complexities of preventing it from devolving into O(N) trigger checks on
every pressure update. If the overriding behavior is something that can be
changed, I'd prefer going for per-opener triggers even if that involves
adding complexities (maybe a rbtree w/ prev/next links for faster sweeps?).
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: psi_trigger_poll() is completely broken
2022-01-06 22:05 ` Tejun Heo
@ 2022-01-06 22:59 ` Linus Torvalds
2022-01-07 0:13 ` Eric Biggers
0 siblings, 1 reply; 16+ messages in thread
From: Linus Torvalds @ 2022-01-06 22:59 UTC (permalink / raw)
To: Tejun Heo
Cc: Eric Biggers, Zefan Li, Johannes Weiner, Peter Zijlstra,
Juri Lelli, Vincent Guittot, Ingo Molnar, Hillf Danton, syzbot,
linux-fsdevel, Linux Kernel Mailing List, syzkaller-bugs,
Linux-MM
[-- Attachment #1: Type: text/plain, Size: 3614 bytes --]
On Thu, Jan 6, 2022 at 2:05 PM Tejun Heo <tj@kernel.org> wrote:
>
> On Wed, Jan 05, 2022 at 11:13:30AM -0800, Linus Torvalds wrote:
> >
> > What are the users? Can we make the rule for -EBUSY simply be that you
> > can _install_ a trigger, but you can't replace an existing one (except
> > with NULL, when you close it).
>
> I don't have enough context here and Johannes seems offline today. Let's
> wait for him to chime in.
Context here:
https://lore.kernel.org/all/YdW3WfHURBXRmn%2F6@sol.localdomain/
> IIRC, the rationale for the shared trigger at the time was around the
> complexities of preventing it from devolving into O(N) trigger checks on
> every pressure update. If the overriding behavior is something that can be
> changed, I'd prefer going for per-opener triggers even if that involves
> adding complexities (maybe a rbtree w/ prev/next links for faster sweeps?).
So here's a COMPLETELY UNTESTED patch to try to fix the lifetime and locking.
The locking was completely broken, in that psi_trigger_replace()
expected that the caller would hold some exclusive lock so that it
would release the correct previous trigger. The cgroup code doesn't
seem to have any such exclusion.
This (UNTESTED!) patch fixes that breakage by just using a cmpxchg loop.
And the lifetime was completely broken (and that's Eric's email)
because psi_trigger_replace() would drop the refcount to the old
trigger - assuming it got the right one - even though the old trigger
could still have active waiters on the waitqueue due to poll() or
select().
This (UNTESTED!) patch fixes _that_ breakage by making
psi_trigger_replace() instead just put the previous trigger on the
"stale_trigger" linked list, and never release it at all.
It now gets released by "psi_trigger_release()" instead, which walks
the list at file release time. Doing "psi_trigger_replace(.., NULL)"
is not valid any more.
And because the reference cannot go away, we now can throw away all
the incorrect temporary kref_get/put games from psi_trigger_poll(),
which didn't actually fix the race at all, only limited it to the poll
waitqueue.
That also means we can remove the "synchronize_rcu()" from
psi_trigger_destroy(), since that was trying to hide all the problems
with the "take rcu lock and then do kref_get()" thing not having
locking. The locking still doesn't exist, but since we don't release
the old one when replacing it, the issue is moot.
NOTE NOTE NOTE! Not only is this patch entirely untested, there are
optimizations you could do if there was some sane synchronization
between psi_trigger_poll() and psi_trigger_replace(). I put comments
about it in the code, but right now the code just assumes that
replacing a trigger is fairly rare (and since it requires write
permissions, it's not something random users can do).
I'm not proud of this patch, but I think it might fix the fundamental
bugs in the code for now.
It's not lovely, it has room for improvement, and I wish we didn't
need this kind of thing, but it looks superficially sane as a fix to
me.
Comments?
And once again: this is UNTESTED. I've compiled-tested it, it looks
kind of sane to me, but honestly, I don't know the code very well.
Also, I'm not super-happy with how that 'psi_disabled' static branch
works. If somebody switches it off after it has been on, that will
also disable the freeing code, so now you'll be leaking memory.
I couldn't find it in myself to care.
Eric - you have the test-case, and the eagle-eyes that found this
problem in the first place. As such, your opinion and comments count
more than most...
Linus
[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 5084 bytes --]
include/linux/psi.h | 1 +
include/linux/psi_types.h | 3 ++
kernel/cgroup/cgroup.c | 2 +-
kernel/sched/psi.c | 71 ++++++++++++++++++++++++++++++++---------------
4 files changed, 54 insertions(+), 23 deletions(-)
diff --git a/include/linux/psi.h b/include/linux/psi.h
index 65eb1476ac70..9ec9468d6068 100644
--- a/include/linux/psi.h
+++ b/include/linux/psi.h
@@ -33,6 +33,7 @@ void cgroup_move_task(struct task_struct *p, struct css_set *to);
struct psi_trigger *psi_trigger_create(struct psi_group *group,
char *buf, size_t nbytes, enum psi_res res);
void psi_trigger_replace(void **trigger_ptr, struct psi_trigger *t);
+void psi_trigger_release(void **trigger_ptr);
__poll_t psi_trigger_poll(void **trigger_ptr, struct file *file,
poll_table *wait);
diff --git a/include/linux/psi_types.h b/include/linux/psi_types.h
index 0a23300d49af..eab79e68bf56 100644
--- a/include/linux/psi_types.h
+++ b/include/linux/psi_types.h
@@ -132,6 +132,9 @@ struct psi_trigger {
/* Refcounting to prevent premature destruction */
struct kref refcount;
+
+ /* Previous trigger that this one replaced */
+ struct psi_trigger *stale_trigger;
};
struct psi_group {
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 919194de39c8..801d0aec0443 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -3684,7 +3684,7 @@ static __poll_t cgroup_pressure_poll(struct kernfs_open_file *of,
static void cgroup_pressure_release(struct kernfs_open_file *of)
{
- psi_trigger_replace(&of->priv, NULL);
+ psi_trigger_release(&of->priv);
}
bool cgroup_psi_enabled(void)
diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 1652f2bb54b7..10430f75f21a 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -1152,6 +1152,7 @@ struct psi_trigger *psi_trigger_create(struct psi_group *group,
t->last_event_time = 0;
init_waitqueue_head(&t->event_wait);
kref_init(&t->refcount);
+ t->stale_trigger = NULL;
mutex_lock(&group->trigger_lock);
@@ -1223,12 +1224,6 @@ static void psi_trigger_destroy(struct kref *ref)
mutex_unlock(&group->trigger_lock);
- /*
- * Wait for both *trigger_ptr from psi_trigger_replace and
- * poll_task RCUs to complete their read-side critical sections
- * before destroying the trigger and optionally the poll_task
- */
- synchronize_rcu();
/*
* Stop kthread 'psimon' after releasing trigger_lock to prevent a
* deadlock while waiting for psi_poll_work to acquire trigger_lock
@@ -1243,16 +1238,48 @@ static void psi_trigger_destroy(struct kref *ref)
kfree(t);
}
+/*
+ * Replacing a trigger must not throw away the old one, since it
+ * can still have pending waiters.
+ *
+ * Possible optimization: after successfully installing a new
+ * trigger, we could release the old one from the stale list
+ * early. Not done here yet - needs locking with psi_trigger_poll.
+ */
void psi_trigger_replace(void **trigger_ptr, struct psi_trigger *new)
{
- struct psi_trigger *old = *trigger_ptr;
+ if (static_branch_likely(&psi_disabled))
+ return;
+
+ for (;;) {
+ struct psi_trigger *old = *trigger_ptr;
+
+ new->stale_trigger = old;
+ if (try_cmpxchg(trigger_ptr, old, new))
+ break;
+ }
+
+ /*
+ * Now that the new one has been installed, we could
+ * check if the stale one has an empty wait-queue
+ * and release it early... But we'd need some locking
+ * with enw poll users to be sure.
+ */
+}
+
+/* No locking needed for final release */
+void psi_trigger_release(void **trigger_ptr)
+{
+ struct psi_trigger *trigger;
if (static_branch_likely(&psi_disabled))
return;
- rcu_assign_pointer(*trigger_ptr, new);
- if (old)
- kref_put(&old->refcount, psi_trigger_destroy);
+ while (trigger) {
+ struct psi_trigger *next = trigger->stale_trigger;
+ kref_put(&trigger->refcount, psi_trigger_destroy);
+ trigger = next;
+ }
}
__poll_t psi_trigger_poll(void **trigger_ptr,
@@ -1264,24 +1291,24 @@ __poll_t psi_trigger_poll(void **trigger_ptr,
if (static_branch_likely(&psi_disabled))
return DEFAULT_POLLMASK | EPOLLERR | EPOLLPRI;
- rcu_read_lock();
-
- t = rcu_dereference(*(void __rcu __force **)trigger_ptr);
- if (!t) {
- rcu_read_unlock();
+ /*
+ * See psi_trigger_replace(): finding a trigger means
+ * that it is guaranteed to have an elevated refcount
+ * for the lifetime of this file descriptor.
+ *
+ * If we had locking, we could release it early. As it
+ * is, we'll only release it with psi_trigger_release()
+ * at the very end.
+ */
+ t = READ_ONCE(*trigger_ptr);
+ if (!t)
return DEFAULT_POLLMASK | EPOLLERR | EPOLLPRI;
- }
- kref_get(&t->refcount);
-
- rcu_read_unlock();
poll_wait(file, &t->event_wait, wait);
if (cmpxchg(&t->event, 1, 0) == 1)
ret |= EPOLLPRI;
- kref_put(&t->refcount, psi_trigger_destroy);
-
return ret;
}
@@ -1347,7 +1374,7 @@ static int psi_fop_release(struct inode *inode, struct file *file)
{
struct seq_file *seq = file->private_data;
- psi_trigger_replace(&seq->private, NULL);
+ psi_trigger_release(&seq->private);
return single_release(inode, file);
}
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: psi_trigger_poll() is completely broken
2022-01-06 22:59 ` Linus Torvalds
@ 2022-01-07 0:13 ` Eric Biggers
2022-01-07 3:02 ` Linus Torvalds
0 siblings, 1 reply; 16+ messages in thread
From: Eric Biggers @ 2022-01-07 0:13 UTC (permalink / raw)
To: Linus Torvalds
Cc: Tejun Heo, Zefan Li, Johannes Weiner, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Ingo Molnar, Hillf Danton, syzbot,
linux-fsdevel, Linux Kernel Mailing List, syzkaller-bugs,
Linux-MM
On Thu, Jan 06, 2022 at 02:59:36PM -0800, Linus Torvalds wrote:
>
> So here's a COMPLETELY UNTESTED patch to try to fix the lifetime and locking.
>
> The locking was completely broken, in that psi_trigger_replace()
> expected that the caller would hold some exclusive lock so that it
> would release the correct previous trigger. The cgroup code doesn't
> seem to have any such exclusion.
>
> This (UNTESTED!) patch fixes that breakage by just using a cmpxchg loop.
>
> And the lifetime was completely broken (and that's Eric's email)
> because psi_trigger_replace() would drop the refcount to the old
> trigger - assuming it got the right one - even though the old trigger
> could still have active waiters on the waitqueue due to poll() or
> select().
>
> This (UNTESTED!) patch fixes _that_ breakage by making
> psi_trigger_replace() instead just put the previous trigger on the
> "stale_trigger" linked list, and never release it at all.
>
> It now gets released by "psi_trigger_release()" instead, which walks
> the list at file release time. Doing "psi_trigger_replace(.., NULL)"
> is not valid any more.
>
> And because the reference cannot go away, we now can throw away all
> the incorrect temporary kref_get/put games from psi_trigger_poll(),
> which didn't actually fix the race at all, only limited it to the poll
> waitqueue.
>
> That also means we can remove the "synchronize_rcu()" from
> psi_trigger_destroy(), since that was trying to hide all the problems
> with the "take rcu lock and then do kref_get()" thing not having
> locking. The locking still doesn't exist, but since we don't release
> the old one when replacing it, the issue is moot.
>
> NOTE NOTE NOTE! Not only is this patch entirely untested, there are
> optimizations you could do if there was some sane synchronization
> between psi_trigger_poll() and psi_trigger_replace(). I put comments
> about it in the code, but right now the code just assumes that
> replacing a trigger is fairly rare (and since it requires write
> permissions, it's not something random users can do).
>
> I'm not proud of this patch, but I think it might fix the fundamental
> bugs in the code for now.
>
> It's not lovely, it has room for improvement, and I wish we didn't
> need this kind of thing, but it looks superficially sane as a fix to
> me.
>
> Comments?
>
> And once again: this is UNTESTED. I've compiled-tested it, it looks
> kind of sane to me, but honestly, I don't know the code very well.
>
> Also, I'm not super-happy with how that 'psi_disabled' static branch
> works. If somebody switches it off after it has been on, that will
> also disable the freeing code, so now you'll be leaking memory.
>
> I couldn't find it in myself to care.
I had to make the following changes to Linus's patch:
diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 10430f75f21a..7d5afa89db44 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -1255,7 +1255,7 @@ void psi_trigger_replace(void **trigger_ptr, struct psi_trigger *new)
struct psi_trigger *old = *trigger_ptr;
new->stale_trigger = old;
- if (try_cmpxchg(trigger_ptr, old, new))
+ if (try_cmpxchg(trigger_ptr, &old, new))
break;
}
@@ -1270,7 +1270,7 @@ void psi_trigger_replace(void **trigger_ptr, struct psi_trigger *new)
/* No locking needed for final release */
void psi_trigger_release(void **trigger_ptr)
{
- struct psi_trigger *trigger;
+ struct psi_trigger *trigger = *trigger_ptr;
if (static_branch_likely(&psi_disabled))
return;
After that, the two reproducers I gave in
https://lore.kernel.org/r/YbQUSlq76Iv5L4cC@sol.localdomain (the ones at the end
of my mail, not the syzbot-generated ones which I didn't try) no longer crash
the kernel.
This is one way to fix the use-after-free, but the fact that it allows anyone
who can write to a /proc/pressure/* file to cause the kernel to allocate an
unbounded number of 'struct psi_trigger' structs is still really broken.
I think we really need an answer to Linus' question:
> What are the users? Can we make the rule for -EBUSY simply be that you
> can _install_ a trigger, but you can't replace an existing one (except
> with NULL, when you close it).
... since that would be a much better fix. The example in
Documentation/accounting/psi.rst only does a single write; that case wouldn't be
broken if we made multiple writes not work.
- Eric
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: psi_trigger_poll() is completely broken
2022-01-07 0:13 ` Eric Biggers
@ 2022-01-07 3:02 ` Linus Torvalds
0 siblings, 0 replies; 16+ messages in thread
From: Linus Torvalds @ 2022-01-07 3:02 UTC (permalink / raw)
To: Eric Biggers
Cc: Tejun Heo, Zefan Li, Johannes Weiner, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Ingo Molnar, Hillf Danton, syzbot,
linux-fsdevel, Linux Kernel Mailing List, syzkaller-bugs,
Linux-MM
On Thu, Jan 6, 2022 at 4:14 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> I had to make the following changes to Linus's patch:
Ack. Thanks.
> This is one way to fix the use-after-free, but the fact that it allows anyone
> who can write to a /proc/pressure/* file to cause the kernel to allocate an
> unbounded number of 'struct psi_trigger' structs is still really broken.
Yeah, I agree. Very non-optimal - that patch really was trying to just
keep the status quo, and fixing the immediate problems.
Modifying that patch to only allow a previous NULL value in
psi_trigger_replace() would be fairly simple - it would basically just
get rid of the "stale_trigger" list (and the loops it creates).
You'd still want the psi_trigger_release() model to separate that
whole "release" from "new trigger".
But that does require that nobody ever does more than a single write
to one file.
Debian code search finds those "/proc/pressure/xyz" files mentioned at
least by systemd and the chromium chrome browser sources. Whether they
actually write triggers to them, I can't say.
Maybe we just need to try.
Linus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: psi_trigger_poll() is completely broken
2022-01-05 19:13 ` Linus Torvalds
2022-01-06 22:05 ` Tejun Heo
@ 2022-01-10 13:45 ` Johannes Weiner
2022-01-10 17:25 ` Suren Baghdasaryan
1 sibling, 1 reply; 16+ messages in thread
From: Johannes Weiner @ 2022-01-10 13:45 UTC (permalink / raw)
To: Linus Torvalds
Cc: Eric Biggers, Tejun Heo, Zefan Li, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Ingo Molnar, Hillf Danton, syzbot,
linux-fsdevel, Linux Kernel Mailing List, syzkaller-bugs,
Linux-MM, Suren Baghdasaryan
On Wed, Jan 05, 2022 at 11:13:30AM -0800, Linus Torvalds wrote:
> On Wed, Jan 5, 2022 at 11:07 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Whoever came up with that stupid "replace existing trigger with a
> > write()" model should feel bad. It's garbage, and it's actively buggy
> > in multiple ways.
>
> What are the users? Can we make the rule for -EBUSY simply be that you
> can _install_ a trigger, but you can't replace an existing one (except
> with NULL, when you close it).
Apologies for the delay, I'm traveling right now.
The primary user of the poll interface is still Android userspace OOM
killing. I'm CCing Suren who is the most familiar with this usecase.
Suren, the way the refcounting is written right now assumes that
poll_wait() is the actual blocking wait. That's not true, it just
queues the waiter and saves &t->event_wait, and the *caller* of
psi_trigger_poll() continues to interact with it afterwards.
If at all possible, I would also prefer the simplicity of one trigger
setup per fd; if you need a new trigger, close the fd and open again.
Can you please take a look if that is workable from the Android side?
(I'm going to follow up on the static branch issue Linus pointed out,
later this week when I'm back home. I also think we should add Suren
as additional psi maintainer since the polling code is a good chunk of
the codebase and he shouldn't miss threads like these.)
> That would fix the poll() lifetime issue, and would make the
> psi_trigger_replace() races fairly easy to fix - just use
>
> if (cmpxchg(trigger_ptr, NULL, new) != NULL) {
> ... free 'new', return -EBUSY ..
>
> to install the new one, instead of
>
> rcu_assign_pointer(*trigger_ptr, new);
>
> or something like that. No locking necessary.
>
> But I assume people actually end up re-writing triggers, because
> people are perverse and have taken advantage of this completely broken
> API.
>
> Linus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: psi_trigger_poll() is completely broken
2022-01-10 13:45 ` Johannes Weiner
@ 2022-01-10 17:25 ` Suren Baghdasaryan
2022-01-10 17:42 ` Linus Torvalds
0 siblings, 1 reply; 16+ messages in thread
From: Suren Baghdasaryan @ 2022-01-10 17:25 UTC (permalink / raw)
To: Johannes Weiner
Cc: Linus Torvalds, Eric Biggers, Tejun Heo, Zefan Li,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Ingo Molnar,
Hillf Danton, syzbot, linux-fsdevel, Linux Kernel Mailing List,
syzkaller-bugs, Linux-MM
On Mon, Jan 10, 2022 at 5:45 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Wed, Jan 05, 2022 at 11:13:30AM -0800, Linus Torvalds wrote:
> > On Wed, Jan 5, 2022 at 11:07 AM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > Whoever came up with that stupid "replace existing trigger with a
> > > write()" model should feel bad. It's garbage, and it's actively buggy
> > > in multiple ways.
> >
> > What are the users? Can we make the rule for -EBUSY simply be that you
> > can _install_ a trigger, but you can't replace an existing one (except
> > with NULL, when you close it).
>
> Apologies for the delay, I'm traveling right now.
>
> The primary user of the poll interface is still Android userspace OOM
> killing. I'm CCing Suren who is the most familiar with this usecase.
>
> Suren, the way the refcounting is written right now assumes that
> poll_wait() is the actual blocking wait. That's not true, it just
> queues the waiter and saves &t->event_wait, and the *caller* of
> psi_trigger_poll() continues to interact with it afterwards.
Thanks for adding me, Johannes. I see where I made a mistake.
Terribly sorry for the trouble this caused. I do feel bad.
>
> If at all possible, I would also prefer the simplicity of one trigger
> setup per fd; if you need a new trigger, close the fd and open again.
>
> Can you please take a look if that is workable from the Android side?
Yes, one trigger per fd would work fine for Android. That's how we
intended to use it.
I'm still catching up on this email thread. Once I digest it, will try
to fix this with one-trigger-per-fd approach.
About the issue of serializing concurrent writes for
cgroup_pressure_write() similar to how psi_write() does. Doesn't
of->mutex inside kernfs_fop_write_iter() serialize the writes to the
same file: https://elixir.bootlin.com/linux/latest/source/fs/kernfs/file.c#L287
?
>
> (I'm going to follow up on the static branch issue Linus pointed out,
> later this week when I'm back home. I also think we should add Suren
> as additional psi maintainer since the polling code is a good chunk of
> the codebase and he shouldn't miss threads like these.)
That would help me not to miss these emails and respond promptly.
Thanks,
Suren.
>
> > That would fix the poll() lifetime issue, and would make the
> > psi_trigger_replace() races fairly easy to fix - just use
> >
> > if (cmpxchg(trigger_ptr, NULL, new) != NULL) {
> > ... free 'new', return -EBUSY ..
> >
> > to install the new one, instead of
> >
> > rcu_assign_pointer(*trigger_ptr, new);
> >
> > or something like that. No locking necessary.
> >
> > But I assume people actually end up re-writing triggers, because
> > people are perverse and have taken advantage of this completely broken
> > API.
> >
> > Linus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: psi_trigger_poll() is completely broken
2022-01-10 17:25 ` Suren Baghdasaryan
@ 2022-01-10 17:42 ` Linus Torvalds
2022-01-10 18:19 ` Suren Baghdasaryan
0 siblings, 1 reply; 16+ messages in thread
From: Linus Torvalds @ 2022-01-10 17:42 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: Johannes Weiner, Eric Biggers, Tejun Heo, Zefan Li,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Ingo Molnar,
Hillf Danton, syzbot, linux-fsdevel, Linux Kernel Mailing List,
syzkaller-bugs, Linux-MM
On Mon, Jan 10, 2022 at 9:25 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> About the issue of serializing concurrent writes for
> cgroup_pressure_write() similar to how psi_write() does. Doesn't
> of->mutex inside kernfs_fop_write_iter() serialize the writes to the
> same file?
Ahh, yes, it looks like that does solve the serialization issue.
Sorry, I missed that because I'm not actually all that familiar with
the kernfs 'of' code.
So the only issue is the trigger lifetime one, and if a single trigger
is sufficient and returning -EBUSY for trying to replace an existing
one is good, then I think that's the proper fix.
I'm very busy with the merge window (and some upcoming travel and
family events), so I'm hoping somebody will write and test such a
patch. Please?
Linus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: psi_trigger_poll() is completely broken
2022-01-10 17:42 ` Linus Torvalds
@ 2022-01-10 18:19 ` Suren Baghdasaryan
2022-01-11 3:02 ` Suren Baghdasaryan
0 siblings, 1 reply; 16+ messages in thread
From: Suren Baghdasaryan @ 2022-01-10 18:19 UTC (permalink / raw)
To: Linus Torvalds
Cc: Johannes Weiner, Eric Biggers, Tejun Heo, Zefan Li,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Ingo Molnar,
Hillf Danton, syzbot, linux-fsdevel, Linux Kernel Mailing List,
syzkaller-bugs, Linux-MM
On Mon, Jan 10, 2022 at 9:42 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, Jan 10, 2022 at 9:25 AM Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > About the issue of serializing concurrent writes for
> > cgroup_pressure_write() similar to how psi_write() does. Doesn't
> > of->mutex inside kernfs_fop_write_iter() serialize the writes to the
> > same file?
>
> Ahh, yes, it looks like that does solve the serialization issue.
> Sorry, I missed that because I'm not actually all that familiar with
> the kernfs 'of' code.
>
> So the only issue is the trigger lifetime one, and if a single trigger
> is sufficient and returning -EBUSY for trying to replace an existing
> one is good, then I think that's the proper fix.
>
> I'm very busy with the merge window (and some upcoming travel and
> family events), so I'm hoping somebody will write and test such a
> patch. Please?
Yes, definitely. I'm on it. Will try posting it later today or
tomorrow morning if testing reveals something unexpected.
Thanks!
>
> Linus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: psi_trigger_poll() is completely broken
2022-01-10 18:19 ` Suren Baghdasaryan
@ 2022-01-11 3:02 ` Suren Baghdasaryan
0 siblings, 0 replies; 16+ messages in thread
From: Suren Baghdasaryan @ 2022-01-11 3:02 UTC (permalink / raw)
To: Linus Torvalds
Cc: Johannes Weiner, Eric Biggers, Tejun Heo, Zefan Li,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Ingo Molnar,
Hillf Danton, syzbot, linux-fsdevel, Linux Kernel Mailing List,
syzkaller-bugs, Linux-MM
On Mon, Jan 10, 2022 at 10:19 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Mon, Jan 10, 2022 at 9:42 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Mon, Jan 10, 2022 at 9:25 AM Suren Baghdasaryan <surenb@google.com> wrote:
> > >
> > > About the issue of serializing concurrent writes for
> > > cgroup_pressure_write() similar to how psi_write() does. Doesn't
> > > of->mutex inside kernfs_fop_write_iter() serialize the writes to the
> > > same file?
> >
> > Ahh, yes, it looks like that does solve the serialization issue.
> > Sorry, I missed that because I'm not actually all that familiar with
> > the kernfs 'of' code.
> >
> > So the only issue is the trigger lifetime one, and if a single trigger
> > is sufficient and returning -EBUSY for trying to replace an existing
> > one is good, then I think that's the proper fix.
> >
> > I'm very busy with the merge window (and some upcoming travel and
> > family events), so I'm hoping somebody will write and test such a
> > patch. Please?
>
> Yes, definitely. I'm on it. Will try posting it later today or
> tomorrow morning if testing reveals something unexpected.
My first attempt to fix this issue is posted at:
https://lore.kernel.org/all/20220111025138.1071848-1-surenb@google.com/
Couple notes:
- I don't think we need psi_trigger::refcount anymore, therefore it's removed.
- synchronize_rcu is kept to ensure we do not free group->poll_task
while psi_schedule_poll_work is using it.
- Documentation needed minimal changes because it did not clearly
specify how trigger overwrite should work. Now it does.
I ran as many test cases as I could find/create. I'll work on adding
some kselftests for psi triggers to test different usage patterns.
Thanks,
Suren.
> Thanks!
>
> >
> > Linus
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-01-11 3:02 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-16 9:23 [syzbot] KASAN: use-after-free Read in remove_wait_queue (3) syzbot
2021-12-10 22:42 ` syzbot
[not found] ` <20211211015620.1793-1-hdanton@sina.com>
2021-12-11 3:00 ` Eric Biggers
2022-01-05 15:20 ` psi_trigger_poll() is completely broken Eric Biggers
2022-01-05 18:50 ` Linus Torvalds
2022-01-05 19:07 ` Linus Torvalds
2022-01-05 19:13 ` Linus Torvalds
2022-01-06 22:05 ` Tejun Heo
2022-01-06 22:59 ` Linus Torvalds
2022-01-07 0:13 ` Eric Biggers
2022-01-07 3:02 ` Linus Torvalds
2022-01-10 13:45 ` Johannes Weiner
2022-01-10 17:25 ` Suren Baghdasaryan
2022-01-10 17:42 ` Linus Torvalds
2022-01-10 18:19 ` Suren Baghdasaryan
2022-01-11 3:02 ` Suren Baghdasaryan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.