diff mbox series

blktrace: Revert "blktrace: remove debugfs file dentries from struct blk_trace"

Message ID 20220226095343.1121256-1-yukuai3@huawei.com (mailing list archive)
State New, archived
Headers show
Series blktrace: Revert "blktrace: remove debugfs file dentries from struct blk_trace" | expand

Commit Message

Yu Kuai Feb. 26, 2022, 9:53 a.m. UTC
This reverts commit c0ea57608b691d6cde8aff23e11f9858a86b5918.

When tracing the whole disk, 'dropped' and 'msg' will be created
under 'q->debugfs_dir' and 'bt->dir' is NULL, thus blk_trace_free()
won't remove those files. What's worse, the following UAF can be
triggered because of stale 'dropped' and 'msg':

==================================================================
BUG: KASAN: use-after-free in blk_dropped_read+0x89/0x100
Read of size 4 at addr ffff88816912f3d8 by task blktrace/1188

CPU: 27 PID: 1188 Comm: blktrace Not tainted 5.17.0-rc4-next-20220217+ #469
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-4
Call Trace:
 <TASK>
 dump_stack_lvl+0x34/0x44
 print_address_description.constprop.0.cold+0xab/0x381
 ? blk_dropped_read+0x89/0x100
 ? blk_dropped_read+0x89/0x100
 kasan_report.cold+0x83/0xdf
 ? blk_dropped_read+0x89/0x100
 kasan_check_range+0x140/0x1b0
 blk_dropped_read+0x89/0x100
 ? blk_create_buf_file_callback+0x20/0x20
 ? kmem_cache_free+0xa1/0x500
 ? do_sys_openat2+0x258/0x460
 full_proxy_read+0x8f/0xc0
 vfs_read+0xc6/0x260
 ksys_read+0xb9/0x150
 ? vfs_write+0x3d0/0x3d0
 ? fpregs_assert_state_consistent+0x55/0x60
 ? exit_to_user_mode_prepare+0x39/0x1e0
 do_syscall_64+0x35/0x80
 entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7fbc080d92fd
Code: ce 20 00 00 75 10 b8 00 00 00 00 0f 05 48 3d 01 f0 ff ff 73 31 c3 48 83 1
RSP: 002b:00007fbb95ff9cb0 EFLAGS: 00000293 ORIG_RAX: 0000000000000000
RAX: ffffffffffffffda RBX: 00007fbb95ff9dc0 RCX: 00007fbc080d92fd
RDX: 0000000000000100 RSI: 00007fbb95ff9cc0 RDI: 0000000000000045
RBP: 0000000000000045 R08: 0000000000406299 R09: 00000000fffffffd
R10: 000000000153afa0 R11: 0000000000000293 R12: 00007fbb780008c0
R13: 00007fbb78000938 R14: 0000000000608b30 R15: 00007fbb780029c8
 </TASK>

Allocated by task 1050:
 kasan_save_stack+0x1e/0x40
 __kasan_kmalloc+0x81/0xa0
 do_blk_trace_setup+0xcb/0x410
 __blk_trace_setup+0xac/0x130
 blk_trace_ioctl+0xe9/0x1c0
 blkdev_ioctl+0xf1/0x390
 __x64_sys_ioctl+0xa5/0xe0
 do_syscall_64+0x35/0x80
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Freed by task 1050:
 kasan_save_stack+0x1e/0x40
 kasan_set_track+0x21/0x30
 kasan_set_free_info+0x20/0x30
 __kasan_slab_free+0x103/0x180
 kfree+0x9a/0x4c0
 __blk_trace_remove+0x53/0x70
 blk_trace_ioctl+0x199/0x1c0
 blkdev_common_ioctl+0x5e9/0xb30
 blkdev_ioctl+0x1a5/0x390
 __x64_sys_ioctl+0xa5/0xe0
 do_syscall_64+0x35/0x80
 entry_SYSCALL_64_after_hwframe+0x44/0xae

The buggy address belongs to the object at ffff88816912f380
 which belongs to the cache kmalloc-96 of size 96
The buggy address is located 88 bytes inside of
 96-byte region [ffff88816912f380, ffff88816912f3e0)
The buggy address belongs to the page:
page:000000009a1b4e7c refcount:1 mapcount:0 mapping:0000000000000000 index:0x0f
flags: 0x17ffffc0000200(slab|node=0|zone=2|lastcpupid=0x1fffff)
raw: 0017ffffc0000200 ffffea00044f1100 dead000000000002 ffff88810004c780
raw: 0000000000000000 0000000000200020 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff88816912f280: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
 ffff88816912f300: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
>ffff88816912f380: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
                                                    ^
 ffff88816912f400: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
 ffff88816912f480: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
==================================================================

Fixes: c0ea57608b69 ("blktrace: remove debugfs file dentries from struct blk_trace")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 include/linux/blktrace_api.h | 2 ++
 kernel/trace/blktrace.c      | 8 ++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

Comments

Greg Kroah-Hartman Feb. 26, 2022, 9:43 a.m. UTC | #1
On Sat, Feb 26, 2022 at 05:53:43PM +0800, Yu Kuai wrote:
> This reverts commit c0ea57608b691d6cde8aff23e11f9858a86b5918.
> 
> When tracing the whole disk, 'dropped' and 'msg' will be created
> under 'q->debugfs_dir' and 'bt->dir' is NULL, thus blk_trace_free()
> won't remove those files. What's worse, the following UAF can be
> triggered because of stale 'dropped' and 'msg':

Only root has access to these files, right?

> 
> ==================================================================
> BUG: KASAN: use-after-free in blk_dropped_read+0x89/0x100
> Read of size 4 at addr ffff88816912f3d8 by task blktrace/1188
> 
> CPU: 27 PID: 1188 Comm: blktrace Not tainted 5.17.0-rc4-next-20220217+ #469
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-4
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x34/0x44
>  print_address_description.constprop.0.cold+0xab/0x381
>  ? blk_dropped_read+0x89/0x100
>  ? blk_dropped_read+0x89/0x100
>  kasan_report.cold+0x83/0xdf
>  ? blk_dropped_read+0x89/0x100
>  kasan_check_range+0x140/0x1b0
>  blk_dropped_read+0x89/0x100
>  ? blk_create_buf_file_callback+0x20/0x20
>  ? kmem_cache_free+0xa1/0x500
>  ? do_sys_openat2+0x258/0x460
>  full_proxy_read+0x8f/0xc0
>  vfs_read+0xc6/0x260
>  ksys_read+0xb9/0x150
>  ? vfs_write+0x3d0/0x3d0
>  ? fpregs_assert_state_consistent+0x55/0x60
>  ? exit_to_user_mode_prepare+0x39/0x1e0
>  do_syscall_64+0x35/0x80
>  entry_SYSCALL_64_after_hwframe+0x44/0xae
> RIP: 0033:0x7fbc080d92fd
> Code: ce 20 00 00 75 10 b8 00 00 00 00 0f 05 48 3d 01 f0 ff ff 73 31 c3 48 83 1
> RSP: 002b:00007fbb95ff9cb0 EFLAGS: 00000293 ORIG_RAX: 0000000000000000
> RAX: ffffffffffffffda RBX: 00007fbb95ff9dc0 RCX: 00007fbc080d92fd
> RDX: 0000000000000100 RSI: 00007fbb95ff9cc0 RDI: 0000000000000045
> RBP: 0000000000000045 R08: 0000000000406299 R09: 00000000fffffffd
> R10: 000000000153afa0 R11: 0000000000000293 R12: 00007fbb780008c0
> R13: 00007fbb78000938 R14: 0000000000608b30 R15: 00007fbb780029c8
>  </TASK>
> 
> Allocated by task 1050:
>  kasan_save_stack+0x1e/0x40
>  __kasan_kmalloc+0x81/0xa0
>  do_blk_trace_setup+0xcb/0x410
>  __blk_trace_setup+0xac/0x130
>  blk_trace_ioctl+0xe9/0x1c0
>  blkdev_ioctl+0xf1/0x390
>  __x64_sys_ioctl+0xa5/0xe0
>  do_syscall_64+0x35/0x80
>  entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> Freed by task 1050:
>  kasan_save_stack+0x1e/0x40
>  kasan_set_track+0x21/0x30
>  kasan_set_free_info+0x20/0x30
>  __kasan_slab_free+0x103/0x180
>  kfree+0x9a/0x4c0
>  __blk_trace_remove+0x53/0x70
>  blk_trace_ioctl+0x199/0x1c0
>  blkdev_common_ioctl+0x5e9/0xb30
>  blkdev_ioctl+0x1a5/0x390
>  __x64_sys_ioctl+0xa5/0xe0
>  do_syscall_64+0x35/0x80
>  entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> The buggy address belongs to the object at ffff88816912f380
>  which belongs to the cache kmalloc-96 of size 96
> The buggy address is located 88 bytes inside of
>  96-byte region [ffff88816912f380, ffff88816912f3e0)
> The buggy address belongs to the page:
> page:000000009a1b4e7c refcount:1 mapcount:0 mapping:0000000000000000 index:0x0f
> flags: 0x17ffffc0000200(slab|node=0|zone=2|lastcpupid=0x1fffff)
> raw: 0017ffffc0000200 ffffea00044f1100 dead000000000002 ffff88810004c780
> raw: 0000000000000000 0000000000200020 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
> 
> Memory state around the buggy address:
>  ffff88816912f280: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
>  ffff88816912f300: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
> >ffff88816912f380: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
>                                                     ^
>  ffff88816912f400: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
>  ffff88816912f480: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
> ==================================================================
> 
> Fixes: c0ea57608b69 ("blktrace: remove debugfs file dentries from struct blk_trace")
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  include/linux/blktrace_api.h | 2 ++
>  kernel/trace/blktrace.c      | 8 ++++++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
> index 22501a293fa5..f288d229727c 100644
> --- a/include/linux/blktrace_api.h
> +++ b/include/linux/blktrace_api.h
> @@ -23,6 +23,8 @@ struct blk_trace {
>  	u32 pid;
>  	u32 dev;
>  	struct dentry *dir;
> +	struct dentry *dropped_file;
> +	struct dentry *msg_file;

No need to save these dentries.  Just look them up when you want to
remove the files.

>  	struct list_head running_list;
>  	atomic_t dropped;
>  };
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 19514edc44f7..13152a17fdb3 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -312,6 +312,8 @@ static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
>  
>  static void blk_trace_free(struct blk_trace *bt)
>  {
> +	debugfs_remove(bt->msg_file);
> +	debugfs_remove(bt->dropped_file);
>  	relay_close(bt->rchan);
>  	debugfs_remove(bt->dir);

Why not just move this line up above relay_close()?

Then you the whole directory is properly removed, along with the files
in it.

So this can just be a 1 line change :)

thanks,

greg k-h
Yu Kuai Feb. 26, 2022, 9:57 a.m. UTC | #2
在 2022/02/26 17:43, Greg KH 写道:
> On Sat, Feb 26, 2022 at 05:53:43PM +0800, Yu Kuai wrote:
>> This reverts commit c0ea57608b691d6cde8aff23e11f9858a86b5918.
>>
>> When tracing the whole disk, 'dropped' and 'msg' will be created
>> under 'q->debugfs_dir' and 'bt->dir' is NULL, thus blk_trace_free()
>> won't remove those files. What's worse, the following UAF can be
>> triggered because of stale 'dropped' and 'msg':
> 
> Only root has access to these files, right?

Hi,

Yes, usually user will use blktrace to access these files.
> 
>>
>> ==================================================================
>> BUG: KASAN: use-after-free in blk_dropped_read+0x89/0x100
>> Read of size 4 at addr ffff88816912f3d8 by task blktrace/1188
>>
>> CPU: 27 PID: 1188 Comm: blktrace Not tainted 5.17.0-rc4-next-20220217+ #469
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-4
>> Call Trace:
>>   <TASK>
>>   dump_stack_lvl+0x34/0x44
>>   print_address_description.constprop.0.cold+0xab/0x381
>>   ? blk_dropped_read+0x89/0x100
>>   ? blk_dropped_read+0x89/0x100
>>   kasan_report.cold+0x83/0xdf
>>   ? blk_dropped_read+0x89/0x100
>>   kasan_check_range+0x140/0x1b0
>>   blk_dropped_read+0x89/0x100
>>   ? blk_create_buf_file_callback+0x20/0x20
>>   ? kmem_cache_free+0xa1/0x500
>>   ? do_sys_openat2+0x258/0x460
>>   full_proxy_read+0x8f/0xc0
>>   vfs_read+0xc6/0x260
>>   ksys_read+0xb9/0x150
>>   ? vfs_write+0x3d0/0x3d0
>>   ? fpregs_assert_state_consistent+0x55/0x60
>>   ? exit_to_user_mode_prepare+0x39/0x1e0
>>   do_syscall_64+0x35/0x80
>>   entry_SYSCALL_64_after_hwframe+0x44/0xae
>> RIP: 0033:0x7fbc080d92fd
>> Code: ce 20 00 00 75 10 b8 00 00 00 00 0f 05 48 3d 01 f0 ff ff 73 31 c3 48 83 1
>> RSP: 002b:00007fbb95ff9cb0 EFLAGS: 00000293 ORIG_RAX: 0000000000000000
>> RAX: ffffffffffffffda RBX: 00007fbb95ff9dc0 RCX: 00007fbc080d92fd
>> RDX: 0000000000000100 RSI: 00007fbb95ff9cc0 RDI: 0000000000000045
>> RBP: 0000000000000045 R08: 0000000000406299 R09: 00000000fffffffd
>> R10: 000000000153afa0 R11: 0000000000000293 R12: 00007fbb780008c0
>> R13: 00007fbb78000938 R14: 0000000000608b30 R15: 00007fbb780029c8
>>   </TASK>
>>
>> Allocated by task 1050:
>>   kasan_save_stack+0x1e/0x40
>>   __kasan_kmalloc+0x81/0xa0
>>   do_blk_trace_setup+0xcb/0x410
>>   __blk_trace_setup+0xac/0x130
>>   blk_trace_ioctl+0xe9/0x1c0
>>   blkdev_ioctl+0xf1/0x390
>>   __x64_sys_ioctl+0xa5/0xe0
>>   do_syscall_64+0x35/0x80
>>   entry_SYSCALL_64_after_hwframe+0x44/0xae
>>
>> Freed by task 1050:
>>   kasan_save_stack+0x1e/0x40
>>   kasan_set_track+0x21/0x30
>>   kasan_set_free_info+0x20/0x30
>>   __kasan_slab_free+0x103/0x180
>>   kfree+0x9a/0x4c0
>>   __blk_trace_remove+0x53/0x70
>>   blk_trace_ioctl+0x199/0x1c0
>>   blkdev_common_ioctl+0x5e9/0xb30
>>   blkdev_ioctl+0x1a5/0x390
>>   __x64_sys_ioctl+0xa5/0xe0
>>   do_syscall_64+0x35/0x80
>>   entry_SYSCALL_64_after_hwframe+0x44/0xae
>>
>> The buggy address belongs to the object at ffff88816912f380
>>   which belongs to the cache kmalloc-96 of size 96
>> The buggy address is located 88 bytes inside of
>>   96-byte region [ffff88816912f380, ffff88816912f3e0)
>> The buggy address belongs to the page:
>> page:000000009a1b4e7c refcount:1 mapcount:0 mapping:0000000000000000 index:0x0f
>> flags: 0x17ffffc0000200(slab|node=0|zone=2|lastcpupid=0x1fffff)
>> raw: 0017ffffc0000200 ffffea00044f1100 dead000000000002 ffff88810004c780
>> raw: 0000000000000000 0000000000200020 00000001ffffffff 0000000000000000
>> page dumped because: kasan: bad access detected
>>
>> Memory state around the buggy address:
>>   ffff88816912f280: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
>>   ffff88816912f300: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
>>> ffff88816912f380: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
>>                                                      ^
>>   ffff88816912f400: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
>>   ffff88816912f480: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
>> ==================================================================
>>
>> Fixes: c0ea57608b69 ("blktrace: remove debugfs file dentries from struct blk_trace")
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   include/linux/blktrace_api.h | 2 ++
>>   kernel/trace/blktrace.c      | 8 ++++++--
>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
>> index 22501a293fa5..f288d229727c 100644
>> --- a/include/linux/blktrace_api.h
>> +++ b/include/linux/blktrace_api.h
>> @@ -23,6 +23,8 @@ struct blk_trace {
>>   	u32 pid;
>>   	u32 dev;
>>   	struct dentry *dir;
>> +	struct dentry *dropped_file;
>> +	struct dentry *msg_file;
> 
> No need to save these dentries.  Just look them up when you want to
> remove the files.

Ok, I'll do this in the v2 patch.
> 
>>   	struct list_head running_list;
>>   	atomic_t dropped;
>>   };
>> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
>> index 19514edc44f7..13152a17fdb3 100644
>> --- a/kernel/trace/blktrace.c
>> +++ b/kernel/trace/blktrace.c
>> @@ -312,6 +312,8 @@ static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
>>   
>>   static void blk_trace_free(struct blk_trace *bt)
>>   {
>> +	debugfs_remove(bt->msg_file);
>> +	debugfs_remove(bt->dropped_file);
>>   	relay_close(bt->rchan);
>>   	debugfs_remove(bt->dir);
> 
> Why not just move this line up above relay_close()?
> 
> Then you the whole directory is properly removed, along with the files
> in it.

In the case tracing the whole disk, bt->dir is NULL, if dentries are not
saved, they should be looked up from 'q->debugfs_dir'. Perhaps the
following:

if (bt->dir) {
	debugfs_remove(bt->dir);
} else {
	/* lookup from q->debugfs_dir and remove */
}

Thanks,
Kuai
> 
> So this can just be a 1 line change :)
> 
> thanks,
> 
> greg k-h
> .
>
Greg Kroah-Hartman Feb. 26, 2022, 1 p.m. UTC | #3
On Sat, Feb 26, 2022 at 05:57:44PM +0800, yukuai (C) wrote:
> 在 2022/02/26 17:43, Greg KH 写道:
> > On Sat, Feb 26, 2022 at 05:53:43PM +0800, Yu Kuai wrote:
> > > This reverts commit c0ea57608b691d6cde8aff23e11f9858a86b5918.
> > > 
> > > When tracing the whole disk, 'dropped' and 'msg' will be created
> > > under 'q->debugfs_dir' and 'bt->dir' is NULL, thus blk_trace_free()
> > > won't remove those files. What's worse, the following UAF can be
> > > triggered because of stale 'dropped' and 'msg':
> > 
> > Only root has access to these files, right?
> 
> Hi,
> 
> Yes, usually user will use blktrace to access these files.
> > 
> > > 
> > > ==================================================================
> > > BUG: KASAN: use-after-free in blk_dropped_read+0x89/0x100
> > > Read of size 4 at addr ffff88816912f3d8 by task blktrace/1188
> > > 
> > > CPU: 27 PID: 1188 Comm: blktrace Not tainted 5.17.0-rc4-next-20220217+ #469
> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-4
> > > Call Trace:
> > >   <TASK>
> > >   dump_stack_lvl+0x34/0x44
> > >   print_address_description.constprop.0.cold+0xab/0x381
> > >   ? blk_dropped_read+0x89/0x100
> > >   ? blk_dropped_read+0x89/0x100
> > >   kasan_report.cold+0x83/0xdf
> > >   ? blk_dropped_read+0x89/0x100
> > >   kasan_check_range+0x140/0x1b0
> > >   blk_dropped_read+0x89/0x100
> > >   ? blk_create_buf_file_callback+0x20/0x20
> > >   ? kmem_cache_free+0xa1/0x500
> > >   ? do_sys_openat2+0x258/0x460
> > >   full_proxy_read+0x8f/0xc0
> > >   vfs_read+0xc6/0x260
> > >   ksys_read+0xb9/0x150
> > >   ? vfs_write+0x3d0/0x3d0
> > >   ? fpregs_assert_state_consistent+0x55/0x60
> > >   ? exit_to_user_mode_prepare+0x39/0x1e0
> > >   do_syscall_64+0x35/0x80
> > >   entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > RIP: 0033:0x7fbc080d92fd
> > > Code: ce 20 00 00 75 10 b8 00 00 00 00 0f 05 48 3d 01 f0 ff ff 73 31 c3 48 83 1
> > > RSP: 002b:00007fbb95ff9cb0 EFLAGS: 00000293 ORIG_RAX: 0000000000000000
> > > RAX: ffffffffffffffda RBX: 00007fbb95ff9dc0 RCX: 00007fbc080d92fd
> > > RDX: 0000000000000100 RSI: 00007fbb95ff9cc0 RDI: 0000000000000045
> > > RBP: 0000000000000045 R08: 0000000000406299 R09: 00000000fffffffd
> > > R10: 000000000153afa0 R11: 0000000000000293 R12: 00007fbb780008c0
> > > R13: 00007fbb78000938 R14: 0000000000608b30 R15: 00007fbb780029c8
> > >   </TASK>
> > > 
> > > Allocated by task 1050:
> > >   kasan_save_stack+0x1e/0x40
> > >   __kasan_kmalloc+0x81/0xa0
> > >   do_blk_trace_setup+0xcb/0x410
> > >   __blk_trace_setup+0xac/0x130
> > >   blk_trace_ioctl+0xe9/0x1c0
> > >   blkdev_ioctl+0xf1/0x390
> > >   __x64_sys_ioctl+0xa5/0xe0
> > >   do_syscall_64+0x35/0x80
> > >   entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > 
> > > Freed by task 1050:
> > >   kasan_save_stack+0x1e/0x40
> > >   kasan_set_track+0x21/0x30
> > >   kasan_set_free_info+0x20/0x30
> > >   __kasan_slab_free+0x103/0x180
> > >   kfree+0x9a/0x4c0
> > >   __blk_trace_remove+0x53/0x70
> > >   blk_trace_ioctl+0x199/0x1c0
> > >   blkdev_common_ioctl+0x5e9/0xb30
> > >   blkdev_ioctl+0x1a5/0x390
> > >   __x64_sys_ioctl+0xa5/0xe0
> > >   do_syscall_64+0x35/0x80
> > >   entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > 
> > > The buggy address belongs to the object at ffff88816912f380
> > >   which belongs to the cache kmalloc-96 of size 96
> > > The buggy address is located 88 bytes inside of
> > >   96-byte region [ffff88816912f380, ffff88816912f3e0)
> > > The buggy address belongs to the page:
> > > page:000000009a1b4e7c refcount:1 mapcount:0 mapping:0000000000000000 index:0x0f
> > > flags: 0x17ffffc0000200(slab|node=0|zone=2|lastcpupid=0x1fffff)
> > > raw: 0017ffffc0000200 ffffea00044f1100 dead000000000002 ffff88810004c780
> > > raw: 0000000000000000 0000000000200020 00000001ffffffff 0000000000000000
> > > page dumped because: kasan: bad access detected
> > > 
> > > Memory state around the buggy address:
> > >   ffff88816912f280: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
> > >   ffff88816912f300: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
> > > > ffff88816912f380: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
> > >                                                      ^
> > >   ffff88816912f400: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
> > >   ffff88816912f480: fa fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
> > > ==================================================================
> > > 
> > > Fixes: c0ea57608b69 ("blktrace: remove debugfs file dentries from struct blk_trace")
> > > Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> > > ---
> > >   include/linux/blktrace_api.h | 2 ++
> > >   kernel/trace/blktrace.c      | 8 ++++++--
> > >   2 files changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
> > > index 22501a293fa5..f288d229727c 100644
> > > --- a/include/linux/blktrace_api.h
> > > +++ b/include/linux/blktrace_api.h
> > > @@ -23,6 +23,8 @@ struct blk_trace {
> > >   	u32 pid;
> > >   	u32 dev;
> > >   	struct dentry *dir;
> > > +	struct dentry *dropped_file;
> > > +	struct dentry *msg_file;
> > 
> > No need to save these dentries.  Just look them up when you want to
> > remove the files.
> 
> Ok, I'll do this in the v2 patch.
> > 
> > >   	struct list_head running_list;
> > >   	atomic_t dropped;
> > >   };
> > > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> > > index 19514edc44f7..13152a17fdb3 100644
> > > --- a/kernel/trace/blktrace.c
> > > +++ b/kernel/trace/blktrace.c
> > > @@ -312,6 +312,8 @@ static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
> > >   static void blk_trace_free(struct blk_trace *bt)
> > >   {
> > > +	debugfs_remove(bt->msg_file);
> > > +	debugfs_remove(bt->dropped_file);
> > >   	relay_close(bt->rchan);
> > >   	debugfs_remove(bt->dir);
> > 
> > Why not just move this line up above relay_close()?
> > 
> > Then you the whole directory is properly removed, along with the files
> > in it.
> 
> In the case tracing the whole disk, bt->dir is NULL, if dentries are not
> saved, they should be looked up from 'q->debugfs_dir'. Perhaps the
> following:
> 
> if (bt->dir) {
> 	debugfs_remove(bt->dir);
> } else {
> 	/* lookup from q->debugfs_dir and remove */
> }

The check for bt->dir is odd, as aren't the msg_file in the bt->dir
directory?

Ah, ick, that's not obvious at all that there is two different cases
happening here.

Yes, your code will work, but please comment the heck out of it as
normally when I see a "check if we have a file/directory before we
remove it" for debugfs I want to optimize it away to just call the
debugfs function as debugfs doesn't care about invalid parameters like
this.  But you are using it as a test if this is in full disk mode or
not, which isn't obvious at all (hence my confusion when I wrote the
original patch, sorry.)

thanks,

greg k-h
diff mbox series

Patch

diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
index 22501a293fa5..f288d229727c 100644
--- a/include/linux/blktrace_api.h
+++ b/include/linux/blktrace_api.h
@@ -23,6 +23,8 @@  struct blk_trace {
 	u32 pid;
 	u32 dev;
 	struct dentry *dir;
+	struct dentry *dropped_file;
+	struct dentry *msg_file;
 	struct list_head running_list;
 	atomic_t dropped;
 };
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 19514edc44f7..13152a17fdb3 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -312,6 +312,8 @@  static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
 
 static void blk_trace_free(struct blk_trace *bt)
 {
+	debugfs_remove(bt->msg_file);
+	debugfs_remove(bt->dropped_file);
 	relay_close(bt->rchan);
 	debugfs_remove(bt->dir);
 	free_percpu(bt->sequence);
@@ -543,8 +545,10 @@  static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 	INIT_LIST_HEAD(&bt->running_list);
 
 	ret = -EIO;
-	debugfs_create_file("dropped", 0444, dir, bt, &blk_dropped_fops);
-	debugfs_create_file("msg", 0222, dir, bt, &blk_msg_fops);
+	bt->dropped_file = debugfs_create_file("dropped", 0444, dir, bt,
+					       &blk_dropped_fops);
+
+	bt->msg_file = debugfs_create_file("msg", 0222, dir, bt, &blk_msg_fops);
 
 	bt->rchan = relay_open("trace", dir, buts->buf_size,
 				buts->buf_nr, &blk_relay_callbacks, bt);