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

[Liu Shixin]

On 2022/12/10 3:46, Viacheslav Dubeyko wrote:
>
>> On Dec 9, 2022, at 1:10 AM, Liu Shixin <liushixin2@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Signed-off-by: Liu Shixin <liushixin2@xxxxxxxxxx>
>> ---
>> 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
>>
>
> .
>