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

[Jan Kara]

On Wed 09-09-26 21:30:34, Karl Mehltretter wrote:
> From: Christian Brauner <brauner@xxxxxxxxxx>
> 
> 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 iterator 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.
> 
> Move removal from fs_supers to put_super(), alongside removal from
> super_blocks, so a passive reference keeps both list nodes linked. Keep
> the filesystem module reference until then, since unlinking s_instances
> may touch type->fs_supers.
> 
> Make sget_fc() skip SB_DEAD superblocks before invoking test(), and set
> SB_DEAD under sb_lock to serialize with those callbacks. This allows
> kernfs to free its private information after kill_anon_super() returns.
> Keep matching SB_DYING superblocks until SB_DEAD is set so concurrent
> mounts still wait for teardown before retrying.
> 
> Fixes: 43e15cdbefea ("new helper: iterate_supers_type()")
> Reported-by: Karl Mehltretter <kmehltretter@xxxxxxxxx>
> Closes: https://lore.kernel.org/r/20260903013336.92081-1-kmehltretter@xxxxxxxxx
> Suggested-by: Jan Kara <jack@xxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Christian Brauner (Amutable) <brauner@xxxxxxxxxx>
> Tested-by: Karl Mehltretter <kmehltretter@xxxxxxxxx>
> Assisted-by: LLM
> [kmehltretter: supplied the commit message]
> Signed-off-by: Karl Mehltretter <kmehltretter@xxxxxxxxx>

Looks good to me! Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
> Changes in v2:
> - Use Christian's implementation of Jan's suggestion: keep s_instances
>   linked until the last passive reference is dropped and make sget_fc()
>   skip SB_DEAD superblocks under sb_lock.
> - Retain the filesystem module reference until put_super().
> - Attribute authorship to Christian; retain my commit message.
> 
> Testing: All tests run on v1 also passed with Christian's draft. These
> included an x86_64 QEMU deterministic KUnit test using the actual CIFS
> lookup callback and a Samba DFS reconnect test with concurrent CIFS
> mounts and unmounts in the same guest. The deterministic test exercised
> the missed-match case; the network stress did not reproduce the narrow
> race on baseline.
> 
> Link to v1: https://lore.kernel.org/r/20260903013336.92081-1-kmehltretter@xxxxxxxxx
> 
> Christian's draft:
> https://lore.kernel.org/r/20260904-rockkonzert-bergtour-dahin-23c9c5c87d0f@brauner
> 
>  fs/kernfs/mount.c |  4 ++--
>  fs/super.c        | 39 +++++++++++++++++++--------------------
>  2 files changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
> index f183a96778b9..a57399021c8b 100644
> --- a/fs/kernfs/mount.c
> +++ b/fs/kernfs/mount.c
> @@ -434,8 +434,8 @@ void kernfs_kill_sb(struct super_block *sb)
>  	up_write(&root->kernfs_supers_rwsem);
>  
>  	/*
> -	 * Remove the superblock from fs_supers/s_instances
> -	 * so we can't find it, before freeing kernfs_super_info.
> +	 * Mark the superblock dead so sget_fc() can't find it,
> +	 * before freeing kernfs_super_info.
>  	 */
>  	kill_anon_super(sb);
>  	kfree(info);
> diff --git a/fs/super.c b/fs/super.c
> index 05e443173038..0f9e13eedb4f 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -433,15 +433,19 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
>  void put_super(struct super_block *s)
>  {
>  	if (refcount_dec_and_test(&s->s_passive)) {
> +		struct file_system_type *type = s->s_type;
>  
>  		spin_lock(&sb_lock);
>  		list_del_init(&s->s_list);
> +		hlist_del_init(&s->s_instances);
>  		spin_unlock(&sb_lock);
>  
>  		WARN_ON(s->s_dentry_lru.node);
>  		WARN_ON(s->s_inode_lru.node);
>  		WARN_ON(s->s_mounts);
>  		call_rcu(&s->rcu, destroy_super_rcu);
> +		/* The unlink above may touch type->fs_supers, so drop it last. */
> +		put_filesystem(type);
>  	}
>  }
>  
> @@ -558,17 +562,6 @@ static void kill_super_notify(struct super_block *sb)
>  	if (sb->s_flags & SB_DEAD)
>  		return;
>  
> -	/*
> -	 * Remove it from @fs_supers so it isn't found by new
> -	 * sget_fc() walkers anymore. Any concurrent mounter still
> -	 * managing to grab a temporary reference is guaranteed to
> -	 * already see SB_DYING and will wait until we notify them about
> -	 * SB_DEAD.
> -	 */
> -	spin_lock(&sb_lock);
> -	hlist_del_init(&sb->s_instances);
> -	spin_unlock(&sb_lock);
> -
>  	/* Drop sget_fc()'s claim; a never-registered entry stays with the sb. */
>  	if (sb->s_super_dev->sd_dev) {
>  		super_dev_put(sb->s_super_dev);
> @@ -577,11 +570,15 @@ static void kill_super_notify(struct super_block *sb)
>  
>  	/*
>  	 * Let concurrent mounts know that this thing is really dead.
> -	 * We don't need @sb->s_umount here as every concurrent caller
> -	 * will see SB_DYING and either discard the superblock or wait
> -	 * for SB_DEAD.
> +	 * sget_fc() skips SB_DEAD superblocks and calls test() under
> +	 * sb_lock, so set it under sb_lock: once we return no test()
> +	 * runs on this superblock anymore and none will start. Everyone
> +	 * else already saw SB_DYING and either discarded the superblock
> +	 * or waits for SB_DEAD.
>  	 */
> +	spin_lock(&sb_lock);
>  	super_wake(sb, SB_DEAD);
> +	spin_unlock(&sb_lock);
>  }
>  
>  /**
> @@ -608,7 +605,6 @@ void deactivate_locked_super(struct super_block *s)
>  		list_lru_destroy(&s->s_dentry_lru);
>  		list_lru_destroy(&s->s_inode_lru);
>  
> -		put_filesystem(fs);
>  		put_super(s);
>  	} else {
>  		super_unlock_excl(s);
> @@ -795,12 +791,12 @@ void generic_shutdown_super(struct super_block *sb)
>  	}
>  	/*
>  	 * Broadcast to everyone that grabbed a temporary reference to this
> -	 * superblock before we removed it from @fs_supers that the superblock
> -	 * is dying. Every walker of @fs_supers outside of sget_fc() will now
> -	 * discard this superblock and treat it as dead.
> +	 * superblock that it is dying. Every walker of @fs_supers outside
> +	 * of sget_fc() will now discard this superblock and treat it as
> +	 * dead.
>  	 *
> -	 * We leave the superblock on @fs_supers so it can be found by
> -	 * sget_fc() until we passed sb->kill_sb().
> +	 * sget_fc() keeps finding the superblock until SB_DEAD is set, so
> +	 * a concurrent mounter waits until we passed sb->kill_sb().
>  	 */
>  	super_wake(sb, SB_DYING);
>  	super_unlock_excl(sb);
> @@ -879,6 +875,9 @@ struct super_block *sget_fc(struct fs_context *fc,
>  	spin_lock(&sb_lock);
>  	if (test) {
>  		hlist_for_each_entry(old, &fc->fs_type->fs_supers, s_instances) {
> +			/* Only unlinked at the last passive reference. */
> +			if (super_flags(old, SB_DEAD))
> +				continue;
>  			if (test(old, fc))
>  				goto share_extant_sb;
>  		}
> -- 
> 2.53.0
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR