[v1] hfs: fix missing hfs_bnode_get() in __hfs_bnode_create

[PATCH] hfs: fix missing hfs_bnode_get() in __hfs_bnode_create

Posted by Liu Shixin 2 years, 9 months ago

Syzbot found a kernel BUG in hfs_bnode_put():

 kernel BUG at fs/hfs/bnode.c:466!
 invalid opcode: 0000 [#1] PREEMPT SMP KASAN
 CPU: 0 PID: 3634 Comm: kworker/u4:5 Not tainted 6.1.0-rc7-syzkaller-00190-g97ee9d1c1696 #0
 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022
 Workqueue: writeback wb_workfn (flush-7:0)
 RIP: 0010:hfs_bnode_put+0x46f/0x480 fs/hfs/bnode.c:466
 Code: 8a 80 ff e9 73 fe ff ff 89 d9 80 e1 07 80 c1 03 38 c1 0f 8c a0 fe ff ff 48 89 df e8 db 8a 80 ff e9 93 fe ff ff e8 a1 68 2c ff <0f> 0b e8 9a 68 2c ff 0f 0b 0f 1f 84 00 00 00 00 00 55 41 57 41 56
 RSP: 0018:ffffc90003b4f258 EFLAGS: 00010293
 RAX: ffffffff825e318f RBX: 0000000000000000 RCX: ffff8880739dd7c0
 RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
 RBP: ffffc90003b4f430 R08: ffffffff825e2d9b R09: ffffed10045157d1
 R10: ffffed10045157d1 R11: 1ffff110045157d0 R12: ffff8880228abe80
 R13: ffff88807016c000 R14: dffffc0000000000 R15: ffff8880228abe00
 FS:  0000000000000000(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00007fa6ebe88718 CR3: 000000001e93d000 CR4: 00000000003506f0
 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 Call Trace:
  <TASK>
  hfs_write_inode+0x1bc/0xb40
  write_inode fs/fs-writeback.c:1440 [inline]
  __writeback_single_inode+0x4d6/0x670 fs/fs-writeback.c:1652
  writeback_sb_inodes+0xb3b/0x18f0 fs/fs-writeback.c:1878
  __writeback_inodes_wb+0x125/0x420 fs/fs-writeback.c:1949
  wb_writeback+0x440/0x7b0 fs/fs-writeback.c:2054
  wb_check_start_all fs/fs-writeback.c:2176 [inline]
  wb_do_writeback fs/fs-writeback.c:2202 [inline]
  wb_workfn+0x827/0xef0 fs/fs-writeback.c:2235
  process_one_work+0x877/0xdb0 kernel/workqueue.c:2289
  worker_thread+0xb14/0x1330 kernel/workqueue.c:2436
  kthread+0x266/0x300 kernel/kthread.c:376
  ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
  </TASK>

By tracing the refcnt, I found the node is find by hfs_bnode_findhash() in
__hfs_bnode_create(). There is a missing of hfs_bnode_get() after find the
node.

Reported-by: syzbot+5b04b49a7ec7226c7426@syzkaller.appspotmail.com
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
 fs/hfs/bnode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
index 2015e42e752a..6add6ebfef89 100644
--- a/fs/hfs/bnode.c
+++ b/fs/hfs/bnode.c
@@ -274,6 +274,7 @@ static struct hfs_bnode *__hfs_bnode_create(struct hfs_btree *tree, u32 cnid)
 		tree->node_hash[hash] = node;
 		tree->node_hash_cnt++;
 	} else {
+		hfs_bnode_get(node2);
 		spin_unlock(&tree->hash_lock);
 		kfree(node);
 		wait_event(node2->lock_wq, !test_bit(HFS_BNODE_NEW, &node2->flags));
-- 
2.25.1

Re: [PATCH] hfs: fix missing hfs_bnode_get() in __hfs_bnode_create

Posted by Viacheslav Dubeyko 2 years, 9 months ago


> On Dec 9, 2022, at 1:10 AM, Liu Shixin <liushixin2@huawei.com> wrote:
> 
> Syzbot found a kernel BUG in hfs_bnode_put():
> 
> kernel BUG at fs/hfs/bnode.c:466!
> invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> CPU: 0 PID: 3634 Comm: kworker/u4:5 Not tainted 6.1.0-rc7-syzkaller-00190-g97ee9d1c1696 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022
> Workqueue: writeback wb_workfn (flush-7:0)
> RIP: 0010:hfs_bnode_put+0x46f/0x480 fs/hfs/bnode.c:466
> Code: 8a 80 ff e9 73 fe ff ff 89 d9 80 e1 07 80 c1 03 38 c1 0f 8c a0 fe ff ff 48 89 df e8 db 8a 80 ff e9 93 fe ff ff e8 a1 68 2c ff <0f> 0b e8 9a 68 2c ff 0f 0b 0f 1f 84 00 00 00 00 00 55 41 57 41 56
> RSP: 0018:ffffc90003b4f258 EFLAGS: 00010293
> RAX: ffffffff825e318f RBX: 0000000000000000 RCX: ffff8880739dd7c0
> RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> RBP: ffffc90003b4f430 R08: ffffffff825e2d9b R09: ffffed10045157d1
> R10: ffffed10045157d1 R11: 1ffff110045157d0 R12: ffff8880228abe80
> R13: ffff88807016c000 R14: dffffc0000000000 R15: ffff8880228abe00
> FS:  0000000000000000(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fa6ebe88718 CR3: 000000001e93d000 CR4: 00000000003506f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  <TASK>
>  hfs_write_inode+0x1bc/0xb40
>  write_inode fs/fs-writeback.c:1440 [inline]
>  __writeback_single_inode+0x4d6/0x670 fs/fs-writeback.c:1652
>  writeback_sb_inodes+0xb3b/0x18f0 fs/fs-writeback.c:1878
>  __writeback_inodes_wb+0x125/0x420 fs/fs-writeback.c:1949
>  wb_writeback+0x440/0x7b0 fs/fs-writeback.c:2054
>  wb_check_start_all fs/fs-writeback.c:2176 [inline]
>  wb_do_writeback fs/fs-writeback.c:2202 [inline]
>  wb_workfn+0x827/0xef0 fs/fs-writeback.c:2235
>  process_one_work+0x877/0xdb0 kernel/workqueue.c:2289
>  worker_thread+0xb14/0x1330 kernel/workqueue.c:2436
>  kthread+0x266/0x300 kernel/kthread.c:376
>  ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
>  </TASK>
> 
> By tracing the refcnt, I found the node is find by hfs_bnode_findhash() in
> __hfs_bnode_create(). There is a missing of hfs_bnode_get() after find the
> node.
> 

The patch looks good. But could you add more detailed explanation
of the place of issue? I mean of adding source code of issue place
into comment section. Because, this place fs/hfs/bnode.c:466 is already
not consistent for the latest kernel version. And it will be not easy to find
in the future. But its is important to see the code that trigger the issue
to understand the fix.

/* Dispose of resources used by a node */
void hfs_bnode_put(struct hfs_bnode *node)
{	
  if (node) {
       <skipped>	
       BUG_ON(!atomic_read(&node->refcnt)); <— we  have issue here!!!!
       <skipped>
  }
}

Am I correct?

I believe it will be great to have more detail explanation how the
issue is working. I mean the explanation how the issue happens
and for what use-case. Could you please add it?

Thanks,
Slava.

> Reported-by: syzbot+5b04b49a7ec7226c7426@syzkaller.appspotmail.com
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Liu Shixin <liushixin2@huawei.com>
> ---
> fs/hfs/bnode.c | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
> index 2015e42e752a..6add6ebfef89 100644
> --- a/fs/hfs/bnode.c
> +++ b/fs/hfs/bnode.c
> @@ -274,6 +274,7 @@ static struct hfs_bnode *__hfs_bnode_create(struct hfs_btree *tree, u32 cnid)
> 		tree->node_hash[hash] = node;
> 		tree->node_hash_cnt++;
> 	} else {
> +		hfs_bnode_get(node2);
> 		spin_unlock(&tree->hash_lock);
> 		kfree(node);
> 		wait_event(node2->lock_wq, !test_bit(HFS_BNODE_NEW, &node2->flags));
> -- 
> 2.25.1
>

Re: [PATCH] hfs: fix missing hfs_bnode_get() in __hfs_bnode_create

Posted by Liu Shixin 2 years, 9 months ago


On 2022/12/10 3:46, Viacheslav Dubeyko wrote:
>
>> On Dec 9, 2022, at 1:10 AM, Liu Shixin <liushixin2@huawei.com> wrote:
>>
>> Syzbot found a kernel BUG in hfs_bnode_put():
>>
>> kernel BUG at fs/hfs/bnode.c:466!
>> invalid opcode: 0000 [#1] PREEMPT SMP KASAN
>> CPU: 0 PID: 3634 Comm: kworker/u4:5 Not tainted 6.1.0-rc7-syzkaller-00190-g97ee9d1c1696 #0
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022
>> Workqueue: writeback wb_workfn (flush-7:0)
>> RIP: 0010:hfs_bnode_put+0x46f/0x480 fs/hfs/bnode.c:466
>> Code: 8a 80 ff e9 73 fe ff ff 89 d9 80 e1 07 80 c1 03 38 c1 0f 8c a0 fe ff ff 48 89 df e8 db 8a 80 ff e9 93 fe ff ff e8 a1 68 2c ff <0f> 0b e8 9a 68 2c ff 0f 0b 0f 1f 84 00 00 00 00 00 55 41 57 41 56
>> RSP: 0018:ffffc90003b4f258 EFLAGS: 00010293
>> RAX: ffffffff825e318f RBX: 0000000000000000 RCX: ffff8880739dd7c0
>> RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
>> RBP: ffffc90003b4f430 R08: ffffffff825e2d9b R09: ffffed10045157d1
>> R10: ffffed10045157d1 R11: 1ffff110045157d0 R12: ffff8880228abe80
>> R13: ffff88807016c000 R14: dffffc0000000000 R15: ffff8880228abe00
>> FS:  0000000000000000(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 00007fa6ebe88718 CR3: 000000001e93d000 CR4: 00000000003506f0
>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> Call Trace:
>>  <TASK>
>>  hfs_write_inode+0x1bc/0xb40
>>  write_inode fs/fs-writeback.c:1440 [inline]
>>  __writeback_single_inode+0x4d6/0x670 fs/fs-writeback.c:1652
>>  writeback_sb_inodes+0xb3b/0x18f0 fs/fs-writeback.c:1878
>>  __writeback_inodes_wb+0x125/0x420 fs/fs-writeback.c:1949
>>  wb_writeback+0x440/0x7b0 fs/fs-writeback.c:2054
>>  wb_check_start_all fs/fs-writeback.c:2176 [inline]
>>  wb_do_writeback fs/fs-writeback.c:2202 [inline]
>>  wb_workfn+0x827/0xef0 fs/fs-writeback.c:2235
>>  process_one_work+0x877/0xdb0 kernel/workqueue.c:2289
>>  worker_thread+0xb14/0x1330 kernel/workqueue.c:2436
>>  kthread+0x266/0x300 kernel/kthread.c:376
>>  ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
>>  </TASK>
>>
>> By tracing the refcnt, I found the node is find by hfs_bnode_findhash() in
>> __hfs_bnode_create(). There is a missing of hfs_bnode_get() after find the
>> node.
>>
> The patch looks good. But could you add more detailed explanation
> of the place of issue? I mean of adding source code of issue place
> into comment section. Because, this place fs/hfs/bnode.c:466 is already
> not consistent for the latest kernel version. And it will be not easy to find
> in the future. But its is important to see the code that trigger the issue
> to understand the fix.
>
> /* Dispose of resources used by a node */
> void hfs_bnode_put(struct hfs_bnode *node)
> {	
>   if (node) {
>        <skipped>	
>        BUG_ON(!atomic_read(&node->refcnt)); <— we  have issue here!!!!
>        <skipped>
>   }
> }
>
> Am I correct?
Yes, that is where trigger the BUG_ON().
>
> I believe it will be great to have more detail explanation how the
> issue is working. I mean the explanation how the issue happens
> and for what use-case. Could you please add it?
Thanks for your advice, I will add more detail explanation.
>
> Thanks,
> Slava.
>
>> Reported-by: syzbot+5b04b49a7ec7226c7426@syzkaller.appspotmail.com
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Signed-off-by: Liu Shixin <liushixin2@huawei.com>
>> ---
>> fs/hfs/bnode.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
>> index 2015e42e752a..6add6ebfef89 100644
>> --- a/fs/hfs/bnode.c
>> +++ b/fs/hfs/bnode.c
>> @@ -274,6 +274,7 @@ static struct hfs_bnode *__hfs_bnode_create(struct hfs_btree *tree, u32 cnid)
>> 		tree->node_hash[hash] = node;
>> 		tree->node_hash_cnt++;
>> 	} else {
>> +		hfs_bnode_get(node2);
>> 		spin_unlock(&tree->hash_lock);
>> 		kfree(node);
>> 		wait_event(node2->lock_wq, !test_bit(HFS_BNODE_NEW, &node2->flags));
>> -- 
>> 2.25.1
>>
>
> .
>