Re: [PATCH v5] smb: client: avoid ctx corruption on failed multichannel remounts

[Henrique Carvalho <henrique.carvalho@suse.com>]

On Fri, May 01, 2026 at 03:09:07PM +0900, DaeMyung Kang wrote:
> smb3_reconfigure() moves string fields out of the live mount context
> (cifs_sb->ctx) before updating multichannel state. If the remount fails
> after that point, the live context can be left with NULL strings or with
> options that do not match the session state.
> 
> Snapshot cifs_sb->ctx on entry, build the new context separately, and
> replace cifs_sb->ctx only after the staged reconfigure work succeeds.
> Restore the snapshot on failure paths before committing session-side
> state.
> 
> Also make smb3_sync_session_ctx_passwords() all-or-nothing, and keep the
> ses->password commit before smb3_update_ses_channels() so newly added
> channels authenticate with the new credentials.
> 
> Fixes: ef529f655a2c ("cifs: client: allow changing multichannel mount options on remount")
> Reported-by: RAJASI MANDAL <rajasimandalos@gmail.com>
> Closes: https://lore.kernel.org/lkml/CAEY6_V1+dzW3OD5zqXhsWyXwrDTrg5tAMGZ1AJ7_GAuRE+aevA@mail.gmail.com/
> Link: https://lore.kernel.org/lkml/xkr2dlvgibq5j6gkcxd3yhhnj4atgxw2uy4eug2pxm7wy7nbms@iq6cf5taa65v/
> Signed-off-by: DaeMyung Kang <charsyam@gmail.com>

Thanks, this looks good to me now.

Just two things.

First, we don't need the update_password and update_password2
booleans in smb3_sync_session_ctx_passwords, I believe? I suggest
cleaning that up.

Minor nit, restore_new_ctx is a bit misleading, cleanup_new_ctx is
clearer, what do you think?

Other than that, you can add

Reviewed-by: Henrique Carvalho <henrique.carvalho@suse.com>

> ---
> v5: (feedback from Henrique Carvalho)
>  - Drop mchan_rc / scale_busy; the early failure paths now restore
>    cifs_sb->ctx from a snapshot taken on entry.
>  - Build new_ctx separately and replace cifs_sb->ctx only on success.
>  - Make smb3_sync_session_ctx_passwords() all-or-nothing: allocate
>    both copies first, commit only when both succeed.
>  - Switch smb3_sync_ses_chan_max() max_channels parameter to size_t
>    to match struct cifs_ses::chan_max.
>  - Shorten the commit message and follow nearby CIFS log style.
> 
>  fs/smb/client/fs_context.c | 163 +++++++++++++++++++++++++------------
>  1 file changed, 110 insertions(+), 53 deletions(-)
> 
> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> index b9544eb0381b..1f0cbcf78e17 100644
> --- a/fs/smb/client/fs_context.c
> +++ b/fs/smb/client/fs_context.c
> @@ -767,7 +767,7 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
>  static int smb3_fs_context_parse_monolithic(struct fs_context *fc,
>  					    void *data);
>  static int smb3_get_tree(struct fs_context *fc);
> -static void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int max_channels);
> +static void smb3_sync_ses_chan_max(struct cifs_ses *ses, size_t max_channels);
>  static int smb3_reconfigure(struct fs_context *fc);
>  
>  static const struct fs_context_operations smb3_fs_context_ops = {
> @@ -1041,24 +1041,35 @@ do {									\
>  
>  int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses)
>  {
> +	char *password = NULL, *password2 = NULL;
> +	bool update_password = false, update_password2 = false;
> +
>  	if (ses->password &&
>  	    cifs_sb->ctx->password &&
>  	    strcmp(ses->password, cifs_sb->ctx->password)) {
> -		kfree_sensitive(cifs_sb->ctx->password);
> -		cifs_sb->ctx->password = kstrdup(ses->password, GFP_KERNEL);
> -		if (!cifs_sb->ctx->password)
> +		password = kstrdup(ses->password, GFP_KERNEL);
> +		if (!password)
>  			return -ENOMEM;
> +		update_password = true;
>  	}
>  	if (ses->password2 &&
>  	    cifs_sb->ctx->password2 &&
>  	    strcmp(ses->password2, cifs_sb->ctx->password2)) {
> -		kfree_sensitive(cifs_sb->ctx->password2);
> -		cifs_sb->ctx->password2 = kstrdup(ses->password2, GFP_KERNEL);
> -		if (!cifs_sb->ctx->password2) {
> -			kfree_sensitive(cifs_sb->ctx->password);
> -			cifs_sb->ctx->password = NULL;
> +		password2 = kstrdup(ses->password2, GFP_KERNEL);
> +		if (!password2) {
> +			kfree_sensitive(password);
>  			return -ENOMEM;
>  		}
> +		update_password2 = true;
> +	}
> +
> +	if (update_password) {
> +		kfree_sensitive(cifs_sb->ctx->password);
> +		cifs_sb->ctx->password = password;
> +	}
> +	if (update_password2) {
> +		kfree_sensitive(cifs_sb->ctx->password2);
> +		cifs_sb->ctx->password2 = password2;
>  	}
>  	return 0;
>  }
> @@ -1072,7 +1083,7 @@ int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_se
>   * with the session's channel lock. This should be called whenever the maximum
>   * allowed channels for a session changes (e.g., after a remount or reconfigure).
>   */
> -static void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int max_channels)
> +static void smb3_sync_ses_chan_max(struct cifs_ses *ses, size_t max_channels)
>  {
>  	spin_lock(&ses->chan_lock);
>  	ses->chan_max = max_channels;
> @@ -1082,12 +1093,15 @@ static void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int max_channe
>  static int smb3_reconfigure(struct fs_context *fc)
>  {
>  	struct smb3_fs_context *ctx = smb3_fc2context(fc);
> +	struct smb3_fs_context *new_ctx = NULL;
> +	struct smb3_fs_context *old_ctx = NULL;
>  	struct dentry *root = fc->root;
>  	struct cifs_sb_info *cifs_sb = CIFS_SB(root->d_sb);
>  	struct cifs_ses *ses = cifs_sb_master_tcon(cifs_sb)->ses;
>  	unsigned int rsize = ctx->rsize, wsize = ctx->wsize;
>  	char *new_password = NULL, *new_password2 = NULL;
>  	bool need_recon = false;
> +	bool need_mchan_update;
>  	int rc;
>  
>  	if (ses->expired_pwd)
> @@ -1097,6 +1111,16 @@ static int smb3_reconfigure(struct fs_context *fc)
>  	if (rc)
>  		return rc;
>  
> +	old_ctx = kzalloc_obj(*old_ctx);
> +	if (!old_ctx)
> +		return -ENOMEM;
> +
> +	rc = smb3_fs_context_dup(old_ctx, cifs_sb->ctx);
> +	if (rc) {
> +		kfree(old_ctx);
> +		return rc;
> +	}
> +
>  	/*
>  	 * We can not change UNC/username/password/domainname/
>  	 * workstation_name/nodename/iocharset
> @@ -1106,16 +1130,22 @@ static int smb3_reconfigure(struct fs_context *fc)
>  	STEAL_STRING(cifs_sb, ctx, UNC);
>  	STEAL_STRING(cifs_sb, ctx, source);
>  	STEAL_STRING(cifs_sb, ctx, username);
> +	STEAL_STRING(cifs_sb, ctx, domainname);
> +	STEAL_STRING(cifs_sb, ctx, nodename);
> +	STEAL_STRING(cifs_sb, ctx, iocharset);
>  
> -	if (need_recon == false)
> +	if (!need_recon) {
>  		STEAL_STRING_SENSITIVE(cifs_sb, ctx, password);
> -	else  {
> +	} else {
>  		if (ctx->password) {
>  			new_password = kstrdup(ctx->password, GFP_KERNEL);
> -			if (!new_password)
> -				return -ENOMEM;
> -		} else
> +			if (!new_password) {
> +				rc = -ENOMEM;
> +				goto restore_ctx;
> +			}
> +		} else {
>  			STEAL_STRING_SENSITIVE(cifs_sb, ctx, password);
> +		}
>  	}
>  
>  	/*
> @@ -1125,11 +1155,29 @@ static int smb3_reconfigure(struct fs_context *fc)
>  	if (ctx->password2) {
>  		new_password2 = kstrdup(ctx->password2, GFP_KERNEL);
>  		if (!new_password2) {
> -			kfree_sensitive(new_password);
> -			return -ENOMEM;
> +			rc = -ENOMEM;
> +			goto restore_ctx;
>  		}
> -	} else
> +	} else {
>  		STEAL_STRING_SENSITIVE(cifs_sb, ctx, password2);
> +	}
> +
> +	/* if rsize or wsize not passed in on remount, use previous values */
> +	ctx->rsize = rsize ? CIFS_ALIGN_RSIZE(fc, rsize) : cifs_sb->ctx->rsize;
> +	ctx->wsize = wsize ? CIFS_ALIGN_WSIZE(fc, wsize) : cifs_sb->ctx->wsize;
> +
> +	new_ctx = kzalloc_obj(*new_ctx);
> +	if (!new_ctx) {
> +		rc = -ENOMEM;
> +		goto restore_ctx;
> +	}
> +
> +	rc = smb3_fs_context_dup(new_ctx, ctx);
> +	if (rc)
> +		goto restore_ctx;
> +
> +	need_mchan_update = ctx->multichannel != cifs_sb->ctx->multichannel ||
> +			    ctx->max_channels != cifs_sb->ctx->max_channels;
>  
>  	/*
>  	 * we may update the passwords in the ses struct below. Make sure we do
> @@ -1140,54 +1188,55 @@ static int smb3_reconfigure(struct fs_context *fc)
>  	/*
>  	 * smb2_reconnect may swap password and password2 in case session setup
>  	 * failed. First get ctx passwords in sync with ses passwords. It should
> -	 * be okay to do this even if this function were to return an error at a
> -	 * later stage
> +	 * be done before committing new passwords.
>  	 */
>  	rc = smb3_sync_session_ctx_passwords(cifs_sb, ses);
>  	if (rc) {
>  		mutex_unlock(&ses->session_mutex);
> -		kfree_sensitive(new_password);
> -		kfree_sensitive(new_password2);
> -		return rc;
> +		goto restore_new_ctx;
>  	}
>  
>  	/*
> -	 * now that allocations for passwords are done, commit them
> +	 * If multichannel or max_channels has changed, update the session's channels accordingly.
> +	 * This may add or remove channels to match the new configuration.
> +	 */
> +	if (need_mchan_update) {
> +		/* Prevent concurrent scaling operations */
> +		spin_lock(&ses->ses_lock);
> +		if (ses->flags & CIFS_SES_FLAG_SCALE_CHANNELS) {
> +			spin_unlock(&ses->ses_lock);
> +			mutex_unlock(&ses->session_mutex);
> +			rc = -EINVAL;
> +			goto restore_new_ctx;
> +		}
> +		ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
> +		spin_unlock(&ses->ses_lock);
> +	}
> +
> +	/*
> +	 * Commit session passwords before any channel work so newly added
> +	 * channels authenticate with the new credentials.
>  	 */
>  	if (new_password) {
>  		kfree_sensitive(ses->password);
>  		ses->password = new_password;
> +		new_password = NULL;
>  	}
>  	if (new_password2) {
>  		kfree_sensitive(ses->password2);
>  		ses->password2 = new_password2;
> +		new_password2 = NULL;
>  	}
>  
> -	/*
> -	 * If multichannel or max_channels has changed, update the session's channels accordingly.
> -	 * This may add or remove channels to match the new configuration.
> -	 */
> -	if ((ctx->multichannel != cifs_sb->ctx->multichannel) ||
> -	    (ctx->max_channels != cifs_sb->ctx->max_channels)) {
> -
> +	if (need_mchan_update) {
>  		/* Synchronize ses->chan_max with the new mount context */
>  		smb3_sync_ses_chan_max(ses, ctx->max_channels);
> -		/* Now update the session's channels to match the new configuration */
> -		/* Prevent concurrent scaling operations */
> -		spin_lock(&ses->ses_lock);
> -		if (ses->flags & CIFS_SES_FLAG_SCALE_CHANNELS) {
> -			spin_unlock(&ses->ses_lock);
> -			mutex_unlock(&ses->session_mutex);
> -			return -EINVAL;
> -		}
> -		ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
> -		spin_unlock(&ses->ses_lock);
>  
>  		mutex_unlock(&ses->session_mutex);
>  
> -		rc = smb3_update_ses_channels(ses, ses->server,
> -					       false /* from_reconnect */,
> -					       false /* disable_mchan */);
> +		smb3_update_ses_channels(ses, ses->server,
> +					 false /* from_reconnect */,
> +					 false /* disable_mchan */);
>  
>  		/* Clear scaling flag after operation */
>  		spin_lock(&ses->ses_lock);
> @@ -1197,22 +1246,30 @@ static int smb3_reconfigure(struct fs_context *fc)
>  		mutex_unlock(&ses->session_mutex);
>  	}
>  
> -	STEAL_STRING(cifs_sb, ctx, domainname);
> -	STEAL_STRING(cifs_sb, ctx, nodename);
> -	STEAL_STRING(cifs_sb, ctx, iocharset);
> -
> -	/* if rsize or wsize not passed in on remount, use previous values */
> -	ctx->rsize = rsize ? CIFS_ALIGN_RSIZE(fc, rsize) : cifs_sb->ctx->rsize;
> -	ctx->wsize = wsize ? CIFS_ALIGN_WSIZE(fc, wsize) : cifs_sb->ctx->wsize;
> -
>  	smb3_cleanup_fs_context_contents(cifs_sb->ctx);
> -	rc = smb3_fs_context_dup(cifs_sb->ctx, ctx);
> +	memcpy(cifs_sb->ctx, new_ctx, sizeof(*new_ctx));
> +	kfree(new_ctx);
> +	new_ctx = NULL;
> +	smb3_cleanup_fs_context(old_ctx);
> +	old_ctx = NULL;
>  	smb3_update_mnt_flags(cifs_sb);
>  #ifdef CONFIG_CIFS_DFS_UPCALL
>  	if (!rc)
>  		rc = dfs_cache_remount_fs(cifs_sb);
>  #endif
>  
> +	return rc;
> +
> +restore_new_ctx:
> +	smb3_cleanup_fs_context_contents(new_ctx);
> +restore_ctx:
> +	kfree(new_ctx);
> +	kfree_sensitive(new_password);
> +	kfree_sensitive(new_password2);
> +	smb3_cleanup_fs_context_contents(cifs_sb->ctx);
> +	memcpy(cifs_sb->ctx, old_ctx, sizeof(*old_ctx));
> +	kfree(old_ctx);
> +
>  	return rc;
>  }
>  
> -- 
> 2.43.0
>