* Re: [PATCH] blktrace: Revert "blktrace: remove debugfs file dentries from struct blk_trace"
2022-02-26 9:53 [PATCH] blktrace: Revert "blktrace: remove debugfs file dentries from struct blk_trace" Yu Kuai
@ 2022-02-26 9:43 ` Greg KH
2022-02-26 9:57 ` yukuai (C)
0 siblings, 1 reply; 4+ messages in thread
From: Greg KH @ 2022-02-26 9:43 UTC (permalink / raw)
To: Yu Kuai; +Cc: axboe, rostedt, mingo, linux-block, linux-kernel, yi.zhang
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] blktrace: Revert "blktrace: remove debugfs file dentries from struct blk_trace"
@ 2022-02-26 9:53 Yu Kuai
2022-02-26 9:43 ` Greg KH
0 siblings, 1 reply; 4+ messages in thread
From: Yu Kuai @ 2022-02-26 9:53 UTC (permalink / raw)
To: axboe, rostedt, mingo, gregkh
Cc: linux-block, linux-kernel, yukuai3, yi.zhang
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(-)
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);
--
2.31.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] blktrace: Revert "blktrace: remove debugfs file dentries from struct blk_trace"
2022-02-26 9:43 ` Greg KH
@ 2022-02-26 9:57 ` yukuai (C)
2022-02-26 13:00 ` Greg KH
0 siblings, 1 reply; 4+ messages in thread
From: yukuai (C) @ 2022-02-26 9:57 UTC (permalink / raw)
To: Greg KH; +Cc: axboe, rostedt, mingo, linux-block, linux-kernel, yi.zhang
在 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
> .
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] blktrace: Revert "blktrace: remove debugfs file dentries from struct blk_trace"
2022-02-26 9:57 ` yukuai (C)
@ 2022-02-26 13:00 ` Greg KH
0 siblings, 0 replies; 4+ messages in thread
From: Greg KH @ 2022-02-26 13:00 UTC (permalink / raw)
To: yukuai (C); +Cc: axboe, rostedt, mingo, linux-block, linux-kernel, yi.zhang
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-02-26 13:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-26 9:53 [PATCH] blktrace: Revert "blktrace: remove debugfs file dentries from struct blk_trace" Yu Kuai
2022-02-26 9:43 ` Greg KH
2022-02-26 9:57 ` yukuai (C)
2022-02-26 13:00 ` Greg KH
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).