Re: [PATCH v2] ocfs2: validate group add input before caching

[Joseph Qi]

On 4/9/26 11:36 PM, ZhengYuan Huang wrote:
> [BUG]
> OCFS2_IOC_GROUP_ADD can trigger a BUG_ON in
> ocfs2_set_new_buffer_uptodate():
> 
> kernel BUG at fs/ocfs2/uptodate.c:509!
> Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
> RIP: 0010:ocfs2_set_new_buffer_uptodate+0x194/0x1e0 fs/ocfs2/uptodate.c:509
> Code: ffffe88f 42b9fe4c 89e64889 dfe8b4df
> Call Trace:
>  ocfs2_group_add+0x3f1/0x1510 fs/ocfs2/resize.c:507
>  ocfs2_ioctl+0x309/0x6e0 fs/ocfs2/ioctl.c:887
>  vfs_ioctl fs/ioctl.c:51 [inline]
>  __do_sys_ioctl fs/ioctl.c:597 [inline]
>  __se_sys_ioctl fs/ioctl.c:583 [inline]
>  __x64_sys_ioctl+0x197/0x1e0 fs/ioctl.c:583
>  x64_sys_call+0x1144/0x26a0 arch/x86/include/generated/asm/syscalls_64.h:17
>  do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
>  do_syscall_64+0x93/0xf80 arch/x86/entry/syscall_64.c:94
>  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> RIP: 0033:0x7bbfb55a966d
> 
> [CAUSE]
> ocfs2_group_add() calls ocfs2_set_new_buffer_uptodate() on a
> user-controlled group block before ocfs2_verify_group_and_input()
> validates that block number. That helper is only valid for newly
> allocated metadata and asserts that the block is not already present in
> the chosen metadata cache. The code also uses INODE_CACHE(inode) even
> though the group descriptor belongs to main_bm_inode and later journal
> accesses use that cache context instead.
> 
> [FIX]
> Validate the on-disk group descriptor before caching it, then add it to
> the metadata cache tracked by INODE_CACHE(main_bm_inode). Keep the
> validation failure path separate from the later cleanup path so we only
> remove the buffer from that cache after it has actually been inserted.
> This keeps the group buffer lifetime consistent across validation,
> journaling, and cleanup.
> 
> Fixes: 7909f2bf8353 ("[PATCH 2/2] ocfs2: Implement group add for online resize")
> Signed-off-by: ZhengYuan Huang <gality369@xxxxxxxxx>
> ---
> v2:
> - add the missing Fixes tag for the group-add introduction commit
> - keep the validation failure path separate so cache removal only happens
>   after the buffer has been inserted into main_bm_inode's cache
> ---
> 
>  fs/ocfs2/resize.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ocfs2/resize.c b/fs/ocfs2/resize.c
> index b0733c08ed13..6bb91b091b29 100644
> --- a/fs/ocfs2/resize.c
> +++ b/fs/ocfs2/resize.c
> @@ -504,14 +504,14 @@ int ocfs2_group_add(struct inode *inode, struct ocfs2_new_group_input *input)
>  		goto out_unlock;
>  	}
>  
> -	ocfs2_set_new_buffer_uptodate(INODE_CACHE(inode), group_bh);
> -
>  	ret = ocfs2_verify_group_and_input(main_bm_inode, fe, input, group_bh);
>  	if (ret) {
>  		mlog_errno(ret);
> -		goto out_free_group_bh;
> +		goto out_brelse_group_bh;
>  	}
>  
> +	ocfs2_set_new_buffer_uptodate(INODE_CACHE(main_bm_inode), group_bh);
> +
>  	trace_ocfs2_group_add((unsigned long long)input->group,
>  			       input->chain, input->clusters, input->frees);
>  
> @@ -575,7 +575,9 @@ int ocfs2_group_add(struct inode *inode, struct ocfs2_new_group_input *input)
>  
>  out_free_group_bh:

Now this label seems improper.
So I'd like keep it for brelse(), and move ocfs2_remove_from_cache() out
to a new label, e.g. out_remove_cache, and if ocfs2_start_trans() fails,
goto here.

Thanks,
Joseph
 
>  	if (ret < 0)
> -		ocfs2_remove_from_cache(INODE_CACHE(inode), group_bh);
> +		ocfs2_remove_from_cache(INODE_CACHE(main_bm_inode), group_bh);
> +
> +out_brelse_group_bh:
>  	brelse(group_bh);
>  
>  out_unlock: