Re: [PATCH v2] ocfs2: drain fs_state readers before journal teardown

[Joseph Qi <joseph.qi@linux.alibaba.com>]

On 10/31/26 1:13 PM, Cen Zhang wrote:
> The per-mount fs_state open callback formats journal fields through
> osb->journal. Its debugfs active-user guard protects the open callback,
> but does not keep the journal allocated after journal shutdown.
> 
> Both unmount and the mount-error path in ocfs2_mount_volume() can free
> and clear the journal before the debugfs subtree is removed:
> 
>   fs_state reader                       Mount cleanup
>   ---------------                       -------------
>   enter ocfs2_osb_debug_open()
>     ocfs2_osb_dump()
>       load osb->journal
>                                         ocfs2_journal_shutdown()
>                                           free journal; clear pointer
>       read journal fields
>   leave the open callback
>                                         remove the debugfs subtree
> 
> Remove fs_state before cleanup releases its backing state in both
> callers of ocfs2_journal_shutdown(). Debugfs removal prevents new opens
> and waits for admitted open callbacks to finish. Later reads and release
> use only the per-open snapshot buffer, so they do not need the journal.
> 
> Keep removal in the caller paths while the per-mount debugfs parent is
> still present. The remaining debug files retain their teardown order.
> 
> KASAN report as below:
> 
>     ==================================================================
>     BUG: KASAN: slab-use-after-free in ocfs2_osb_debug_open+0x967/0x15e0
>     Read of size 4 at addr ffff888116a56028 by task cat/502
> 
>     Call Trace:
>      <TASK>
>      dump_stack_lvl+0x93/0xd0
>      print_report+0xce/0x630
>      kasan_report+0xe0/0x110
>      kasan_check_range+0x105/0x1b0
>      ocfs2_osb_debug_open+0x967/0x15e0
>      full_proxy_open_regular+0x193/0x310
>      do_dentry_open+0x595/0x12d0
>      vfs_open_consume+0xd1/0x400
>      path_openat+0x18d0/0x2020
>      do_file_open+0x21e/0x470
>      do_sys_openat2+0xf4/0x1b0
>      __x64_sys_openat+0x136/0x1e0
>      do_syscall_64+0x114/0x620
>      entry_SYSCALL_64_after_hwframe+0x77/0x7f
>      </TASK>
> 
>     Allocated by task 495:
>      kasan_save_stack+0x33/0x60
>      kasan_save_track+0x14/0x30
>      __kasan_kmalloc+0xaa/0xb0
>      __kmalloc_cache_noprof+0x28d/0x610
>      ocfs2_journal_alloc+0x91/0x450
>      ocfs2_initialize_super.isra.0+0x11b8/0x3070
>      ocfs2_fill_super+0x801/0x38f0
>      get_tree_bdev_flags+0x35d/0x5f0
>      vfs_get_tree+0x93/0x320
>      fc_mount+0x1d/0x200
>      path_mount+0x592/0x1e00
>      __x64_sys_mount+0x24b/0x2d0
>      do_syscall_64+0x114/0x620
>      entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
>     Freed by task 504:
>      kasan_save_stack+0x33/0x60
>      kasan_save_track+0x14/0x30
>      kasan_save_free_info+0x3b/0x60
>      __kasan_slab_free+0x5f/0x80
>      kfree+0x308/0x580
>      ocfs2_journal_shutdown+0x10a/0xc20
>      ocfs2_dismount_volume+0x315/0x940
>      generic_shutdown_super+0x15f/0x490
>      kill_block_super+0x40/0xa0
>      deactivate_locked_super+0xae/0x120
>      deactivate_super+0xa2/0xc0
>      cleanup_mnt+0x217/0x450
>      task_work_run+0x144/0x230
>      exit_to_user_mode_loop+0x17d/0x570
>      do_syscall_64+0x4d1/0x620
>      entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
>     The buggy address belongs to the object at ffff888116a56000
>      which belongs to the cache kmalloc-512 of size 512
>     The buggy address is located 40 bytes inside of
>      freed 512-byte region [ffff888116a56000, ffff888116a56200)
>     ==================================================================
> 
> Fixes: da5e7c87827e ("ocfs2: cleanup journal init and shutdown")
> Cc: stable@vger.kernel.org
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> ---
> Changes in v2:
> - Address the osb dereference before BUG_ON(!osb) raised by Sashiko:
>   https://sashiko.dev/#/patchset/98583
> - Remove fs_state after BUG_ON(!osb) in ocfs2_dismount_volume().
> - Trim the KASAN report.
> 
> Assisted-by: LLM
> 
> Link to v1: https://lore.kernel.org/r/pm-ocfs2-objects-candidate-0003-v6-2f69dbb7417fcd537be6@gmail.com
> 
>  fs/ocfs2/super.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index 0af2245ae303..c02896e127e5 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -1783,6 +1783,9 @@ static int ocfs2_mount_volume(struct super_block *sb)
>  	return 0;
>  
>  out_system_inodes:
> +	/* Drain fs_state readers before releasing their backing state. */
> +	debugfs_lookup_and_remove("fs_state", osb->osb_debug_root);
> +
>  	if (osb->local_alloc_state == OCFS2_LA_ENABLED)
>  		ocfs2_shutdown_local_alloc(osb);
>  	/* Drain pending suballoc reclaim work before the journal goes away */
> @@ -1812,6 +1815,9 @@ static void ocfs2_dismount_volume(struct super_block *sb, int mnt_err)
>  	osb = OCFS2_SB(sb);
>  	BUG_ON(!osb);
>  
> +	/* Drain fs_state readers before releasing their backing state. */
> +	debugfs_lookup_and_remove("fs_state", osb->osb_debug_root);
> +
>  	/* Remove file check sysfs related directories/files,
>  	 * and wait for the pending file check operations */
>  	ocfs2_filecheck_remove_sysfs(osb);

'fs_state' is created in ocfs2_fill_super(), so it looks weird to clean
here.
I see now ocfs2_mount_volume() also calls ocfs2_release_system_inodes()
and ocfs2_free_slot_info(), so it seems we have to do more refactor.

Thanks,
Joseph