Re: [PATCH] super: make iterate_supers_type() deletion-safe

[Jan Kara]

On Thu 03-09-26 03:33:36, Karl Mehltretter wrote:
> iterate_supers_type() drops sb_lock while invoking the callback and keeps
> only a passive reference to the current superblock. That reference keeps
> the object allocated, but does not keep its s_instances node linked.
> 
> After the callback releases s_umount, final teardown can unlink the current
> s_instances node. The iterator then advances through a reinitialized node.
> With the current hlist it stops without visiting the remaining superblocks.
> The unlink moved from generic_shutdown_super() to kill_super_notify(), but
> the cursor lifetime has been unsafe since the helper was introduced.
> 
> The CIFS DFS lookup can consequently miss a matching superblock and return
> -EINVAL.
> 
> Walk the global superblock list in reverse and filter it by filesystem
> type. A passive reference keeps its s_list node linked, and reverse
> traversal preserves newest-first visitation. Superblocks removed from
> fs_supers remain on the global list, but teardown marks them SB_DYING
> before unlinking them, so the existing filter excludes them.
> 
> This broadens the scan from superblocks of one type to all superblocks.
> The only in-tree caller is the CIFS DFS lookup, so the broader scan is
> limited to that path.
> 
> Fixes: 43e15cdbefea ("new helper: iterate_supers_type()")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <kmehltretter@xxxxxxxxx>

Good spotting! But what I'm wondering about is whether we just shouldn't
move where we delete sb from fs_supers. Currently we do that in
kill_super_notify() to hide the sb from sget_fc() (which would otherwise
permanently retry and call test() for S_DEAD sb which can cause problems).
But if we just skipped S_DEAD superblocks in sget_fc(), we could move
removal from fs_supers list to put_super() (make it symmetric with the
handling of super_blocks list) and that would also fix
iterate_supers_type(). Christian, what do you think?

								Honza

> ---
> Testing: x86_64 QEMU deterministic KUnit A/B using the actual CIFS lookup
> callback over temporary VFS superblocks. Baseline skipped the surviving
> match after teardown of the preceding nonmatch and returned -EINVAL. With
> only this patch applied, the same test passed.
> 
> A real Samba DFS server and the kernel CIFS client were then run in the
> same QEMU guest over loopback. Baseline and patch-only kernels both followed
> the referrals and completed 20 reconnects while two workers repeatedly
> mounted and unmounted independent CIFS superblocks. The patch-only run
> successfully mounted, read from, and unmounted CIFS shares 95 times. Kprobes
> recorded 110 DFS lookup calls and 60 iterate_supers_type() calls. There were
> no unmount failures, kernel warnings, oopses, or sanitizer reports. The
> natural network stress did not hit the narrow race on baseline.
> 
> Stable is requested because concurrent DFS automount teardown can make a
> reconnect lookup miss a live matching superblock.
> 
> Backport note: before dc3216b14160 ("super: ensure valid info"), the
> s_instances unlink is in generic_shutdown_super(). Before 3ec9800c2d33
> ("super: convert s_count to refcount_t s_passive"), the passive reference is
> named s_count.
> 
>  fs/super.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 05e4431730387..c8accd144bad2 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1026,11 +1026,18 @@ void iterate_supers_type(struct file_system_type *type,
>  	struct super_block *sb, *p = NULL;
>  
>  	spin_lock(&sb_lock);
> -	hlist_for_each_entry(sb, &type->fs_supers, s_instances) {
> +	/*
> +	 * The passive reference keeps the s_list cursor valid while sb_lock
> +	 * is dropped. Entries are added at the tail. Walk backwards to retain
> +	 * newest-first visitation.
> +	 */
> +	list_for_each_entry_reverse(sb, &super_blocks, s_list) {
>  		bool locked;
>  
>  		if (super_flags(sb, SB_DYING))
>  			continue;
> +		if (sb->s_type != type)
> +			continue;
>  
>  		if (!refcount_inc_not_zero(&sb->s_passive))
>  			continue;
> -- 
> 2.53.0
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR