Linux CIFS filesystem development
 help / color / mirror / Atom feed
* [PATCH v5] smb: client: avoid ctx corruption on failed multichannel remounts
@ 2026-05-01  6:09 DaeMyung Kang
  2026-05-12 22:31 ` Henrique Carvalho
  2026-05-13 13:26 ` [PATCH v6] cifs: client: stage smb3_reconfigure() updates and restore ctx on failure DaeMyung Kang
  0 siblings, 2 replies; 5+ messages in thread
From: DaeMyung Kang @ 2026-05-01  6:09 UTC (permalink / raw)
  To: Steve French, Paulo Alcantara
  Cc: Ronnie Sahlberg, Shyam Prasad N, Tom Talpey, Bharath SM,
	Henrique Carvalho, Rajasi Mandal, Rajasi Mandal, linux-cifs,
	linux-kernel, DaeMyung Kang

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>
---
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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v5] smb: client: avoid ctx corruption on failed multichannel remounts
  2026-05-01  6:09 [PATCH v5] smb: client: avoid ctx corruption on failed multichannel remounts DaeMyung Kang
@ 2026-05-12 22:31 ` Henrique Carvalho
  2026-05-12 22:58   ` Steve French
  2026-05-13 13:26 ` [PATCH v6] cifs: client: stage smb3_reconfigure() updates and restore ctx on failure DaeMyung Kang
  1 sibling, 1 reply; 5+ messages in thread
From: Henrique Carvalho @ 2026-05-12 22:31 UTC (permalink / raw)
  To: DaeMyung Kang
  Cc: Steve French, Paulo Alcantara, Ronnie Sahlberg, Shyam Prasad N,
	Tom Talpey, Bharath SM, Rajasi Mandal, Rajasi Mandal, linux-cifs,
	linux-kernel

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
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v5] smb: client: avoid ctx corruption on failed multichannel remounts
  2026-05-12 22:31 ` Henrique Carvalho
@ 2026-05-12 22:58   ` Steve French
  0 siblings, 0 replies; 5+ messages in thread
From: Steve French @ 2026-05-12 22:58 UTC (permalink / raw)
  To: Henrique Carvalho
  Cc: DaeMyung Kang, Paulo Alcantara, Ronnie Sahlberg, Shyam Prasad N,
	Tom Talpey, Bharath SM, Rajasi Mandal, Rajasi Mandal, linux-cifs,
	linux-kernel

On Tue, May 12, 2026 at 5:37 PM Henrique Carvalho
<henrique.carvalho@suse.com> wrote:
>
> 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.

They are needed to check for what memory needs to be freed so seem
logical, unless I am missing something
or are you saying that the kstrdup of ses->password (and
ses->password2) to cifs_sb->ctx->password  (see below) is not needed?

        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;
        }


> 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
> >
>


-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v6] cifs: client: stage smb3_reconfigure() updates and restore ctx on failure
  2026-05-01  6:09 [PATCH v5] smb: client: avoid ctx corruption on failed multichannel remounts DaeMyung Kang
  2026-05-12 22:31 ` Henrique Carvalho
@ 2026-05-13 13:26 ` DaeMyung Kang
  2026-05-13 18:03   ` Steve French
  1 sibling, 1 reply; 5+ messages in thread
From: DaeMyung Kang @ 2026-05-13 13:26 UTC (permalink / raw)
  To: Steve French
  Cc: Paulo Alcantara, Ronnie Sahlberg, Shyam Prasad N, Tom Talpey,
	Bharath SM, Henrique Carvalho, Rajasi Mandal, Rajasi Mandal,
	linux-cifs, linux-kernel, DaeMyung Kang

smb3_reconfigure() moves strings out of cifs_sb->ctx before the
multichannel update, so a later failure can leave the live context
with NULL strings or options that do not match the session.

Stage the new ctx separately, commit it only on success, and restore
the snapshot on failure. Also make smb3_sync_session_ctx_passwords()
all-or-nothing.

Commit session passwords before channel updates so newly added channels
authenticate with the staged 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/
Reviewed-by: Henrique Carvalho <henrique.carvalho@suse.com>
Signed-off-by: DaeMyung Kang <charsyam@gmail.com>
---
v6: (feedback from Henrique Carvalho)
 - Drop the update_password / update_password2 booleans in
   smb3_sync_session_ctx_passwords(); commit when the local pointer
   itself is non-NULL.
 - Rename the failure label restore_new_ctx -> cleanup_new_ctx, since
   new_ctx is cleaned up (not restored) on the staged-failure path.
 - Shorten the commit message further per review.

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 | 161 +++++++++++++++++++++++++------------
 1 file changed, 108 insertions(+), 53 deletions(-)

diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index b9544eb0381b..cf6e18876c59 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,25 +1041,34 @@ do {									\
 
 int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses)
 {
+	char *password = NULL, *password2 = NULL;
+
 	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;
 	}
 	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;
 		}
 	}
+
+	if (password) {
+		kfree_sensitive(cifs_sb->ctx->password);
+		cifs_sb->ctx->password = password;
+	}
+	if (password2) {
+		kfree_sensitive(cifs_sb->ctx->password2);
+		cifs_sb->ctx->password2 = password2;
+	}
+
 	return 0;
 }
 
@@ -1072,7 +1081,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 +1091,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 +1109,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 +1128,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 +1153,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 +1186,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 cleanup_new_ctx;
+	}
+
+	/*
+	 * 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 cleanup_new_ctx;
+		}
+		ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
+		spin_unlock(&ses->ses_lock);
 	}
 
 	/*
-	 * now that allocations for passwords are done, commit them
+	 * 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 +1244,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;
+
+cleanup_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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v6] cifs: client: stage smb3_reconfigure() updates and restore ctx on failure
  2026-05-13 13:26 ` [PATCH v6] cifs: client: stage smb3_reconfigure() updates and restore ctx on failure DaeMyung Kang
@ 2026-05-13 18:03   ` Steve French
  0 siblings, 0 replies; 5+ messages in thread
From: Steve French @ 2026-05-13 18:03 UTC (permalink / raw)
  To: DaeMyung Kang
  Cc: Paulo Alcantara, Ronnie Sahlberg, Shyam Prasad N, Tom Talpey,
	Bharath SM, Henrique Carvalho, Rajasi Mandal, Rajasi Mandal,
	linux-cifs, linux-kernel

Merged into cifs-2.6.git for-next pending additional testing, but
Rajasi will need to update some of her remount series due to this
patch (e.g. "smb: client: sync tcon-level options on remount")

On Wed, May 13, 2026 at 8:54 AM DaeMyung Kang <charsyam@gmail.com> wrote:
>
> smb3_reconfigure() moves strings out of cifs_sb->ctx before the
> multichannel update, so a later failure can leave the live context
> with NULL strings or options that do not match the session.
>
> Stage the new ctx separately, commit it only on success, and restore
> the snapshot on failure. Also make smb3_sync_session_ctx_passwords()
> all-or-nothing.
>
> Commit session passwords before channel updates so newly added channels
> authenticate with the staged 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/
> Reviewed-by: Henrique Carvalho <henrique.carvalho@suse.com>
> Signed-off-by: DaeMyung Kang <charsyam@gmail.com>
> ---
> v6: (feedback from Henrique Carvalho)
>  - Drop the update_password / update_password2 booleans in
>    smb3_sync_session_ctx_passwords(); commit when the local pointer
>    itself is non-NULL.
>  - Rename the failure label restore_new_ctx -> cleanup_new_ctx, since
>    new_ctx is cleaned up (not restored) on the staged-failure path.
>  - Shorten the commit message further per review.
>
> 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 | 161 +++++++++++++++++++++++++------------
>  1 file changed, 108 insertions(+), 53 deletions(-)
>
> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> index b9544eb0381b..cf6e18876c59 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,25 +1041,34 @@ do {                                                                    \
>
>  int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses)
>  {
> +       char *password = NULL, *password2 = NULL;
> +
>         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;
>         }
>         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;
>                 }
>         }
> +
> +       if (password) {
> +               kfree_sensitive(cifs_sb->ctx->password);
> +               cifs_sb->ctx->password = password;
> +       }
> +       if (password2) {
> +               kfree_sensitive(cifs_sb->ctx->password2);
> +               cifs_sb->ctx->password2 = password2;
> +       }
> +
>         return 0;
>  }
>
> @@ -1072,7 +1081,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 +1091,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 +1109,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 +1128,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 +1153,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 +1186,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 cleanup_new_ctx;
> +       }
> +
> +       /*
> +        * 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 cleanup_new_ctx;
> +               }
> +               ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
> +               spin_unlock(&ses->ses_lock);
>         }
>
>         /*
> -        * now that allocations for passwords are done, commit them
> +        * 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 +1244,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;
> +
> +cleanup_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
>
>


-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-05-13 18:04 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-01  6:09 [PATCH v5] smb: client: avoid ctx corruption on failed multichannel remounts DaeMyung Kang
2026-05-12 22:31 ` Henrique Carvalho
2026-05-12 22:58   ` Steve French
2026-05-13 13:26 ` [PATCH v6] cifs: client: stage smb3_reconfigure() updates and restore ctx on failure DaeMyung Kang
2026-05-13 18:03   ` Steve French

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox