[PATCH] smb/client: fix chan_max race and rollback in smb3_reconfigure

[PATCH] smb/client: fix chan_max race and rollback in smb3_reconfigure

[DaeMyung Kang]

smb3_reconfigure() has two bugs when handling multichannel remount:

1) smb3_sync_ses_chan_max() is called before acquiring
   CIFS_SES_FLAG_SCALE_CHANNELS. If a concurrent operation (e.g.
   smb2_reconnect) holds the flag, the current thread takes the
   loser path and returns -EINVAL, but ses->chan_max has already
   been updated to the new value. This leaves chan_max inconsistent
   with the actual channel state.

2) When smb3_update_ses_channels() fails, chan_max is not rolled
   back to its previous value and the error is not propagated to
   the caller. The mount command returns success even though the
   channel configuration was not applied, and repeated failures
   cause chan_max to drift further from reality.

Fix both by moving smb3_sync_ses_chan_max() after the
CIFS_SES_FLAG_SCALE_CHANNELS acquisition so the loser path cannot
corrupt chan_max, and by restoring chan_max from old_chan_max on
update failure while still holding the scaling flag to prevent a
concurrent reconfigure from observing the intermediate state.
Propagate the error to the caller so userspace is informed.

Note: smb3_reconfigure() is not fully transactional — credentials
are committed before the multichannel block, so any early return
(including the existing CIFS_SES_FLAG_SCALE_CHANNELS loser path)
can leave ses->password updated while cifs_sb->ctx is not yet
synced. Making the entire function atomic is a larger refactor
and is left for a separate patch.

Tested with a QEMU VM (ksmbd + cifs) using module-parameter-based
fault injection:
 - Forced smb3_update_ses_channels() failure via module param and
   verified chan_max is preserved at the old value after remount
   failure (fault-injection test for rollback).
 - Pre-set CIFS_SES_FLAG_SCALE_CHANNELS before entering the
   scaling path and verified the loser path returns -EINVAL
   without corrupting chan_max (loser-path invariant test).
 - Repeated 19 forced-failure remounts with varying max_channels
   (range 2-8) and confirmed no chan_max drift (stress test).
All three scenarios pass with the patch and fail without it.

Fixes: ef529f655a2c ("cifs: client: allow changing multichannel mount options on remount")
Signed-off-by: DaeMyung Kang <charsyam@gmail.com>
---
 fs/smb/client/fs_context.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index b9544eb0381b..99e62c2dbf50 100644
--- a/fs/smb/client/fs_context.c
+++ b/fs/smb/client/fs_context.c
@@ -1085,6 +1085,7 @@ static int smb3_reconfigure(struct fs_context *fc)
 	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 old_chan_max;
 	unsigned int rsize = ctx->rsize, wsize = ctx->wsize;
 	char *new_password = NULL, *new_password2 = NULL;
 	bool need_recon = false;
@@ -1170,9 +1171,6 @@ static int smb3_reconfigure(struct fs_context *fc)
 	if ((ctx->multichannel != cifs_sb->ctx->multichannel) ||
 	    (ctx->max_channels != cifs_sb->ctx->max_channels)) {
 
-		/* 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) {
@@ -1183,16 +1181,31 @@ static int smb3_reconfigure(struct fs_context *fc)
 		ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
 		spin_unlock(&ses->ses_lock);
 
+		old_chan_max = ses->chan_max;
+		/* Synchronize ses->chan_max with the new mount context */
+		smb3_sync_ses_chan_max(ses, ctx->max_channels);
+
 		mutex_unlock(&ses->session_mutex);
 
 		rc = smb3_update_ses_channels(ses, ses->server,
 					       false /* from_reconnect */,
 					       false /* disable_mchan */);
 
+		/*
+		 * On failure, restore chan_max while still holding
+		 * CIFS_SES_FLAG_SCALE_CHANNELS so a concurrent reconfigure
+		 * cannot observe or race with the rollback.
+		 */
+		if (rc < 0)
+			smb3_sync_ses_chan_max(ses, old_chan_max);
+
 		/* Clear scaling flag after operation */
 		spin_lock(&ses->ses_lock);
 		ses->flags &= ~CIFS_SES_FLAG_SCALE_CHANNELS;
 		spin_unlock(&ses->ses_lock);
+
+		if (rc < 0)
+			return rc;
 	} else {
 		mutex_unlock(&ses->session_mutex);
 	}
-- 
2.43.0

Re: [PATCH] smb/client: fix chan_max race and rollback in smb3_reconfigure

[RAJASI MANDAL]

Hi DaeMyung,

I reviewed your patch. The two core fixes (moving
smb3_sync_ses_chan_max after the SCALE_CHANNELS check, and rolling
back chan_max on failure) are correct. However the error handling
paths corrupt the mount context leading to false future remount
failures.

Issue 1: return rc after STEAL_STRING nullifies UNC/source/username

Earlier in smb3_reconfigure(), STEAL_STRING moves UNC, source, and
username from cifs_sb->ctx into ctx, setting cifs_sb->ctx->UNC = NULL.
The new if (rc < 0) return rc at the end of the multichannel block
exits before smb3_fs_context_dup() can copy them back.

How to repro:
mount -t cifs //server/share /mnt -o multichannel,max_channels=2,...
mount -o remount,multichannel,max_channels=4 /mnt  # fails (server error)
grep /mnt /proc/mounts   # shows "none" instead of //server/share
mount -o remount,multichannel,max_channels=2 /mnt  # fails: "bad UNC (none)"


Issue 2: return -EINVAL on loser path has the same STEAL_STRING corruption

When CIFS_SES_FLAG_SCALE_CHANNELS is already set (concurrent remount),
the return -EINVAL exits after STEAL_STRING but before
smb3_fs_context_dup(), same permanent NULL UNC corruption.

How to repro: Tested with a module parameter that forces the
SCALE_CHANNELS flag check to take the loser path. After the failed
remount, /proc/mounts shows "none" and all future remounts fail.

Suggested fix:

--- a/fs/smb/client/fs_context.c
+++ b/fs/smb/client/fs_context.c
@@ -NNNN,7 +NNNN,7 @@ static int smb3_reconfigure(struct fs_context *fc)
     char *new_password = NULL, *new_password2 = NULL;
     bool need_recon = false;
- int rc;
+ int rc, mchan_rc = 0;

     if (ses->expired_pwd)
@@ -NNNN,8 +NNNN,9 @@ static int smb3_reconfigure(struct fs_context *fc)
         if (ses->flags & CIFS_SES_FLAG_SCALE_CHANNELS) {
             spin_unlock(&ses->ses_lock);
             mutex_unlock(&ses->session_mutex);
- return -EINVAL;
+ mchan_rc = -EINVAL;
+ goto out;
         }
         ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
@@ -NNNN,13 +NNNN,14 @@ static int smb3_reconfigure(struct fs_context *fc)
         ses->flags &= ~CIFS_SES_FLAG_SCALE_CHANNELS;
         spin_unlock(&ses->ses_lock);

- if (rc < 0)
- return rc;
+ if (rc < 0)
+ mchan_rc = rc;
     } else {
         mutex_unlock(&ses->session_mutex);
     }

+out:
     STEAL_STRING(cifs_sb, ctx, domainname);
     STEAL_STRING(cifs_sb, ctx, nodename);
     STEAL_STRING(cifs_sb, ctx, iocharset);
@@ -NNNN,9 +NNNN,22 @@ static int smb3_reconfigure(struct fs_context *fc)
     ctx->rsize = rsize ? CIFS_ALIGN_RSIZE(fc, rsize) : cifs_sb->ctx->rsize;
     ctx->wsize = wsize ? CIFS_ALIGN_WSIZE(fc, wsize) : cifs_sb->ctx->wsize;

+ /*
+ * On multichannel failure, restore the old multichannel settings in ctx
+ * so smb3_fs_context_dup does not desync cifs_sb->ctx from ses->chan_max.
+ */
+ if (mchan_rc) {
+ ctx->multichannel = cifs_sb->ctx->multichannel;
+ ctx->max_channels = cifs_sb->ctx->max_channels;
+ }
+
     smb3_cleanup_fs_context_contents(cifs_sb->ctx);
     rc = smb3_fs_context_dup(cifs_sb->ctx, ctx);
     smb3_update_mnt_flags(cifs_sb);
+
+ if (mchan_rc)
+ return mchan_rc;
+
 #ifdef CONFIG_CIFS_DFS_UPCALL
     if (!rc)
         rc = dfs_cache_remount_fs(cifs_sb);

On Wed, Apr 15, 2026 at 7:28 PM DaeMyung Kang <charsyam@gmail.com> wrote:
>
> smb3_reconfigure() has two bugs when handling multichannel remount:
>
> 1) smb3_sync_ses_chan_max() is called before acquiring
>    CIFS_SES_FLAG_SCALE_CHANNELS. If a concurrent operation (e.g.
>    smb2_reconnect) holds the flag, the current thread takes the
>    loser path and returns -EINVAL, but ses->chan_max has already
>    been updated to the new value. This leaves chan_max inconsistent
>    with the actual channel state.
>
> 2) When smb3_update_ses_channels() fails, chan_max is not rolled
>    back to its previous value and the error is not propagated to
>    the caller. The mount command returns success even though the
>    channel configuration was not applied, and repeated failures
>    cause chan_max to drift further from reality.
>
> Fix both by moving smb3_sync_ses_chan_max() after the
> CIFS_SES_FLAG_SCALE_CHANNELS acquisition so the loser path cannot
> corrupt chan_max, and by restoring chan_max from old_chan_max on
> update failure while still holding the scaling flag to prevent a
> concurrent reconfigure from observing the intermediate state.
> Propagate the error to the caller so userspace is informed.
>
> Note: smb3_reconfigure() is not fully transactional — credentials
> are committed before the multichannel block, so any early return
> (including the existing CIFS_SES_FLAG_SCALE_CHANNELS loser path)
> can leave ses->password updated while cifs_sb->ctx is not yet
> synced. Making the entire function atomic is a larger refactor
> and is left for a separate patch.
>
> Tested with a QEMU VM (ksmbd + cifs) using module-parameter-based
> fault injection:
>  - Forced smb3_update_ses_channels() failure via module param and
>    verified chan_max is preserved at the old value after remount
>    failure (fault-injection test for rollback).
>  - Pre-set CIFS_SES_FLAG_SCALE_CHANNELS before entering the
>    scaling path and verified the loser path returns -EINVAL
>    without corrupting chan_max (loser-path invariant test).
>  - Repeated 19 forced-failure remounts with varying max_channels
>    (range 2-8) and confirmed no chan_max drift (stress test).
> All three scenarios pass with the patch and fail without it.
>
> Fixes: ef529f655a2c ("cifs: client: allow changing multichannel mount options on remount")
> Signed-off-by: DaeMyung Kang <charsyam@gmail.com>
> ---
>  fs/smb/client/fs_context.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> index b9544eb0381b..99e62c2dbf50 100644
> --- a/fs/smb/client/fs_context.c
> +++ b/fs/smb/client/fs_context.c
> @@ -1085,6 +1085,7 @@ static int smb3_reconfigure(struct fs_context *fc)
>         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 old_chan_max;
>         unsigned int rsize = ctx->rsize, wsize = ctx->wsize;
>         char *new_password = NULL, *new_password2 = NULL;
>         bool need_recon = false;
> @@ -1170,9 +1171,6 @@ static int smb3_reconfigure(struct fs_context *fc)
>         if ((ctx->multichannel != cifs_sb->ctx->multichannel) ||
>             (ctx->max_channels != cifs_sb->ctx->max_channels)) {
>
> -               /* 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) {
> @@ -1183,16 +1181,31 @@ static int smb3_reconfigure(struct fs_context *fc)
>                 ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
>                 spin_unlock(&ses->ses_lock);
>
> +               old_chan_max = ses->chan_max;
> +               /* Synchronize ses->chan_max with the new mount context */
> +               smb3_sync_ses_chan_max(ses, ctx->max_channels);
> +
>                 mutex_unlock(&ses->session_mutex);
>
>                 rc = smb3_update_ses_channels(ses, ses->server,
>                                                false /* from_reconnect */,
>                                                false /* disable_mchan */);
>
> +               /*
> +                * On failure, restore chan_max while still holding
> +                * CIFS_SES_FLAG_SCALE_CHANNELS so a concurrent reconfigure
> +                * cannot observe or race with the rollback.
> +                */
> +               if (rc < 0)
> +                       smb3_sync_ses_chan_max(ses, old_chan_max);
> +
>                 /* Clear scaling flag after operation */
>                 spin_lock(&ses->ses_lock);
>                 ses->flags &= ~CIFS_SES_FLAG_SCALE_CHANNELS;
>                 spin_unlock(&ses->ses_lock);
> +
> +               if (rc < 0)
> +                       return rc;
>         } else {
>                 mutex_unlock(&ses->session_mutex);
>         }
> --
> 2.43.0
>
>

[PATCH v2] smb/client: fix chan_max and UNC state corruption in smb3_reconfigure

[DaeMyung Kang]

smb3_reconfigure() has four related bugs when handling a
multichannel remount that leave ses->chan_max or cifs_sb->ctx
inconsistent with the actual state.

1) smb3_sync_ses_chan_max() is called before acquiring
   CIFS_SES_FLAG_SCALE_CHANNELS. If a concurrent operation (e.g.
   smb2_reconnect) holds the flag, the current thread takes the
   loser path and returns -EINVAL, but ses->chan_max has already
   been updated to the new value. chan_max is then inconsistent
   with the actual channel state.

2) When smb3_update_ses_channels() fails, ses->chan_max is not
   rolled back and the error is not propagated to the caller.
   mount returns success even though the channel configuration
   was not applied, and repeated failures cause chan_max to
   drift further from reality.

3) Earlier in smb3_reconfigure(), STEAL_STRING moves UNC, source
   and username from cifs_sb->ctx into ctx, setting
   cifs_sb->ctx->UNC to NULL until smb3_fs_context_dup() copies
   them back near the end of the function. Both the existing
   CIFS_SES_FLAG_SCALE_CHANNELS loser-path 'return -EINVAL' and a
   naive 'if (rc < 0) return rc;' to propagate the failure above
   exit in this window. A single failed multichannel remount
   therefore permanently nulls cifs_sb->ctx->UNC; /proc/mounts
   shows the device as "none" and every subsequent
   mount.cifs-based remount is rejected by
   smb3_verify_reconfigure_ctx() because mount.cifs passes that
   "none" back as the new UNC.

4) If the multichannel update fails but smb3_fs_context_dup() is
   still allowed to run with the new ctx values, the rejected
   new max_channels is written into cifs_sb->ctx, undoing the
   ses->chan_max rollback from (2) and creating a fresh
   cifs_sb->ctx vs ses->chan_max mismatch.

Fix all four by:
 - Moving smb3_sync_ses_chan_max() after the SCALE_CHANNELS
   acquire so the loser path cannot corrupt chan_max.
 - Capturing old_chan_max before the sync and restoring it on
   failure while still holding SCALE_CHANNELS so a concurrent
   reconfigure cannot race with the rollback.
 - Recording any multichannel-path failure in a local mchan_rc
   and routing it through a common 'out:' label so every exit
   reaches smb3_fs_context_dup() and cifs_sb->ctx is restored.
 - Before the dup, restoring ctx->multichannel and
   ctx->max_channels from cifs_sb->ctx on mchan_rc so the dup
   does not desync cifs_sb->ctx from the already-rolled-back
   ses->chan_max.
 - Returning mchan_rc to userspace so a failed scaling remount
   surfaces as an error.
 - Surfacing smb3_fs_context_dup() failure in preference to
   mchan_rc when both occur. The dup is the final step of the
   ctx restore and runs after smb3_cleanup_fs_context_contents()
   has already freed cifs_sb->ctx's strings, so a dup failure
   (e.g. -ENOMEM) leaves cifs_sb->ctx partially restored with
   some strings still NULL. Reporting the scaling error in that
   case would hide the real recovery status from userspace;
   reporting the dup error exposes it so the caller can react
   accordingly.

Note: smb3_reconfigure() is not fully transactional. Credentials
(ses->password and ses->password2) are committed before the
multichannel block, so a failed multichannel remount can still
leave ses->password updated while reporting an error to
userspace. Making the entire function atomic is a larger
refactor and is left for a separate patch.

Tested with a QEMU VM (ksmbd + cifs) using module-parameter-based
fault injection:
 - Forced smb3_update_ses_channels() failure via module param
   and verified ses->chan_max is preserved at the old value
   after remount failure.
 - Pre-set CIFS_SES_FLAG_SCALE_CHANNELS before entering the
   scaling path and verified the loser path returns -EINVAL
   without corrupting ses->chan_max.
 - Repeated 19 forced-failure remounts with varying max_channels
   (range 2-8) and confirmed no chan_max drift.
 - After each failure path, verified /proc/mounts continues to
   show the original UNC (//127.0.0.1/share) so subsequent
   remounts are accepted.
All scenarios pass with the patch and fail without it.

Reported-by: RAJASI MANDAL <rajasimandalos@gmail.com>
Closes: https://lore.kernel.org/lkml/CAEY6_V1+dzW3OD5zqXhsWyXwrDTrg5tAMGZ1AJ7_GAuRE+aevA@mail.gmail.com/
Fixes: ef529f655a2c ("cifs: client: allow changing multichannel mount options on remount")
Signed-off-by: DaeMyung Kang <charsyam@gmail.com>
---
 fs/smb/client/fs_context.c | 48 ++++++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 7 deletions(-)

diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index b9544eb0381b..9efd73ce6910 100644
--- a/fs/smb/client/fs_context.c
+++ b/fs/smb/client/fs_context.c
@@ -1085,10 +1085,11 @@ static int smb3_reconfigure(struct fs_context *fc)
 	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 old_chan_max;
 	unsigned int rsize = ctx->rsize, wsize = ctx->wsize;
 	char *new_password = NULL, *new_password2 = NULL;
 	bool need_recon = false;
-	int rc;
+	int rc, mchan_rc = 0;
 
 	if (ses->expired_pwd)
 		need_recon = true;
@@ -1170,25 +1171,37 @@ static int smb3_reconfigure(struct fs_context *fc)
 	if ((ctx->multichannel != cifs_sb->ctx->multichannel) ||
 	    (ctx->max_channels != cifs_sb->ctx->max_channels)) {
 
-		/* 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;
+			mchan_rc = -EINVAL;
+			goto out;
 		}
 		ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
 		spin_unlock(&ses->ses_lock);
 
+		old_chan_max = ses->chan_max;
+		/* Synchronize ses->chan_max with the new mount context */
+		smb3_sync_ses_chan_max(ses, ctx->max_channels);
+
 		mutex_unlock(&ses->session_mutex);
 
 		rc = smb3_update_ses_channels(ses, ses->server,
 					       false /* from_reconnect */,
 					       false /* disable_mchan */);
 
+		/*
+		 * On failure, restore chan_max while still holding
+		 * CIFS_SES_FLAG_SCALE_CHANNELS so a concurrent reconfigure
+		 * cannot observe or race with the rollback.
+		 */
+		if (rc < 0) {
+			smb3_sync_ses_chan_max(ses, old_chan_max);
+			mchan_rc = rc;
+		}
+
 		/* Clear scaling flag after operation */
 		spin_lock(&ses->ses_lock);
 		ses->flags &= ~CIFS_SES_FLAG_SCALE_CHANNELS;
@@ -1197,6 +1210,7 @@ static int smb3_reconfigure(struct fs_context *fc)
 		mutex_unlock(&ses->session_mutex);
 	}
 
+out:
 	STEAL_STRING(cifs_sb, ctx, domainname);
 	STEAL_STRING(cifs_sb, ctx, nodename);
 	STEAL_STRING(cifs_sb, ctx, iocharset);
@@ -1205,12 +1219,32 @@ static int smb3_reconfigure(struct fs_context *fc)
 	ctx->rsize = rsize ? CIFS_ALIGN_RSIZE(fc, rsize) : cifs_sb->ctx->rsize;
 	ctx->wsize = wsize ? CIFS_ALIGN_WSIZE(fc, wsize) : cifs_sb->ctx->wsize;
 
+	/*
+	 * If the multichannel update failed, restore the old multichannel
+	 * settings in ctx so smb3_fs_context_dup() does not desync
+	 * cifs_sb->ctx from ses->chan_max (which was already rolled back).
+	 */
+	if (mchan_rc) {
+		ctx->multichannel = cifs_sb->ctx->multichannel;
+		ctx->max_channels = cifs_sb->ctx->max_channels;
+	}
+
 	smb3_cleanup_fs_context_contents(cifs_sb->ctx);
 	rc = smb3_fs_context_dup(cifs_sb->ctx, ctx);
 	smb3_update_mnt_flags(cifs_sb);
+
+	/*
+	 * If smb3_fs_context_dup() failed, cifs_sb->ctx is left in a
+	 * partially-restored state after the cleanup above. Surface that
+	 * failure to the caller before any mchan_rc, since it reflects the
+	 * real recovery status of cifs_sb->ctx rather than the scaling error.
+	 */
+	if (rc)
+		return rc;
+	if (mchan_rc)
+		return mchan_rc;
 #ifdef CONFIG_CIFS_DFS_UPCALL
-	if (!rc)
-		rc = dfs_cache_remount_fs(cifs_sb);
+	rc = dfs_cache_remount_fs(cifs_sb);
 #endif
 
 	return rc;
-- 
2.43.0

Re: [PATCH v2] smb/client: fix chan_max and UNC state corruption in smb3_reconfigure

[Henrique Carvalho]

Isn't this a policy change? If multichannel fails all the other changes
up to that point are persisted (i.e. password change). Despite that, we
will be returning an error to the user, but have a state that we have no
idea what actually happened. Did the password change fail? Did the
multichannel change fail?

Before we had a best-effort remount and after this change we will be
having a partial success reported as failure.

So I would say that, for this change, if this is indeed a bug,
no need for mchan_rc, roll back all the changes that happened during
smb3_reconfigure.

Futhermore, we need to handle mchan update ses channels just like we do
during mount, because remount could end up being delayed the same way as
mount, AFAICS. I have a patch for this with the series of that
serialization smb3_update_ses_channels patch.

-- 
Henrique
SUSE Labs

[PATCH v3] smb/client: fix state corruption in smb3_reconfigure multichannel path

[DaeMyung Kang]

smb3_reconfigure() has several state-consistency bugs when handling
a multichannel remount that leave ses->chan_max or cifs_sb->ctx
inconsistent with the actual state. This patch repairs internal
state only; the userspace-visible return value of
smb3_reconfigure() is preserved to match the pre-patch behaviour:
the concurrent-scale loser path still returns -EINVAL, and a
failed smb3_update_ses_channels() is not newly propagated to
userspace by this patch.

Bugs addressed:

1) smb3_sync_ses_chan_max() is called before acquiring
   CIFS_SES_FLAG_SCALE_CHANNELS. If a concurrent operation (e.g.
   smb2_reconnect) holds the flag, the current thread takes the
   loser path and returns -EINVAL, but ses->chan_max has already
   been updated to the new value. chan_max is then out of sync
   with the actual channel state.

2) When smb3_update_ses_channels() fails, ses->chan_max is not
   rolled back. Repeated failures cause chan_max to drift further
   from reality, and subsequent reconnect/reconfigure paths use
   the drifted target.

3) Earlier in smb3_reconfigure(), STEAL_STRING moves UNC, source
   and username from cifs_sb->ctx into ctx, setting
   cifs_sb->ctx->UNC to NULL until smb3_fs_context_dup() copies
   them back near the end of the function. The pre-existing
   CIFS_SES_FLAG_SCALE_CHANNELS loser-path 'return -EINVAL' exits
   inside this window. A loser-path failure therefore permanently
   nulls cifs_sb->ctx->UNC; /proc/mounts shows the device as
   "none" and every subsequent mount.cifs-based remount is
   rejected by smb3_verify_reconfigure_ctx() because mount.cifs
   passes that "none" back as the new UNC.

4) Once (2) rolls ses->chan_max back to the old value on update
   failure, smb3_fs_context_dup() would still copy the rejected
   new ctx->max_channels into cifs_sb->ctx, creating a fresh
   cifs_sb->ctx vs ses->chan_max mismatch. This must be handled
   together with (2) to keep the rollback complete.

Fix all four by:

 - Moving smb3_sync_ses_chan_max() after the SCALE_CHANNELS
   acquire so the loser path cannot corrupt chan_max.

 - Capturing old_chan_max before the sync and restoring it on
   failure while still holding SCALE_CHANNELS so a concurrent
   reconfigure cannot race with the rollback.

 - Recording any multichannel-path failure in a local mchan_rc
   and routing the loser path through a common 'out:' label so
   every exit reaches smb3_fs_context_dup() and cifs_sb->ctx is
   restored. mchan_rc is used only as internal control flow.

 - Before the dup, restoring ctx->multichannel and
   ctx->max_channels from cifs_sb->ctx on mchan_rc so the dup
   does not desync cifs_sb->ctx from the already-rolled-back
   ses->chan_max.

 - Tracking the concurrent-scale loser path with a separate
   scale_busy flag. After cifs_sb->ctx is fully restored, the
   return value is forced to -EINVAL for that path so userspace
   continues to see the pre-patch rejection. dup /
   dfs_cache_remount_fs() failures still take precedence because
   they reflect real state recovery errors.

Deliberately not changed by this patch: the return value of
smb3_reconfigure() on smb3_update_ses_channels() failure. That
path is not newly propagated to userspace here, matching the
asynchronous model used by the mount path (mchan_mount_work_fn).
Those return-semantics and deferred-handling questions are left
to follow-up discussion/patches.

smb3_reconfigure() is not fully transactional: ses->password and
ses->password2 are committed before the multichannel block, so
unrelated earlier state changes may still be visible after a
failed multichannel remount. That is a structural property of
the function and out of scope here.

Tested with a QEMU VM (ksmbd + cifs) using module-parameter
based fault injection:
 - Forced smb3_update_ses_channels() failure via module param
   and verified ses->chan_max is preserved at the old value
   after the remount path runs.
 - Pre-set CIFS_SES_FLAG_SCALE_CHANNELS before entering the
   scaling path and verified the loser path still returns
   -EINVAL, no longer corrupts ses->chan_max, and no longer nulls
   cifs_sb->ctx->UNC.
 - Repeated 19 forced-failure remounts with varying max_channels
   (range 2-8) and confirmed no chan_max drift.
 - After each failure path, verified /proc/mounts continues to
   show the original UNC (//127.0.0.1/share) so subsequent
   remounts are accepted.

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/
Fixes: ef529f655a2c ("cifs: client: allow changing multichannel mount options on remount")
Signed-off-by: DaeMyung Kang <charsyam@gmail.com>
---
v3: (feedback from Henrique Carvalho)
 - Drop propagation of smb3_update_ses_channels() failure to
   userspace; preserve the pre-patch best-effort semantics on that
   path.
 - Keep the pre-existing CIFS_SES_FLAG_SCALE_CHANNELS loser-path
   -EINVAL via a separate scale_busy flag so that return is not
   silently converted into success.
 - Reword commit message to describe accurately what userspace-
   visible semantics are preserved and what changed internally.

v2: (feedback from Rajasi Mandal)
 - Route loser-path and update-failure exits through a common
   'out:' label so smb3_fs_context_dup() always runs and
   cifs_sb->ctx->UNC is restored after STEAL_STRING.
 - Restore ctx->multichannel/ctx->max_channels from cifs_sb->ctx
   before the dup so dup does not re-desync cifs_sb->ctx from the
   rolled-back ses->chan_max.

 fs/smb/client/fs_context.c | 46 +++++++++++++++++++++++++++++++++-----
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index b9544eb0381b..aaa364a3f60d 100644
--- a/fs/smb/client/fs_context.c
+++ b/fs/smb/client/fs_context.c
@@ -1085,10 +1085,12 @@ static int smb3_reconfigure(struct fs_context *fc)
 	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 old_chan_max;
 	unsigned int rsize = ctx->rsize, wsize = ctx->wsize;
 	char *new_password = NULL, *new_password2 = NULL;
 	bool need_recon = false;
-	int rc;
+	bool scale_busy = false;
+	int rc, mchan_rc = 0;
 
 	if (ses->expired_pwd)
 		need_recon = true;
@@ -1170,25 +1172,38 @@ static int smb3_reconfigure(struct fs_context *fc)
 	if ((ctx->multichannel != cifs_sb->ctx->multichannel) ||
 	    (ctx->max_channels != cifs_sb->ctx->max_channels)) {
 
-		/* 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;
+			scale_busy = true;
+			mchan_rc = -EINVAL;
+			goto out;
 		}
 		ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
 		spin_unlock(&ses->ses_lock);
 
+		old_chan_max = ses->chan_max;
+		/* Synchronize ses->chan_max with the new mount context */
+		smb3_sync_ses_chan_max(ses, ctx->max_channels);
+
 		mutex_unlock(&ses->session_mutex);
 
 		rc = smb3_update_ses_channels(ses, ses->server,
 					       false /* from_reconnect */,
 					       false /* disable_mchan */);
 
+		/*
+		 * On failure, restore chan_max while still holding
+		 * CIFS_SES_FLAG_SCALE_CHANNELS so a concurrent reconfigure
+		 * cannot observe or race with the rollback.
+		 */
+		if (rc < 0) {
+			smb3_sync_ses_chan_max(ses, old_chan_max);
+			mchan_rc = rc;
+		}
+
 		/* Clear scaling flag after operation */
 		spin_lock(&ses->ses_lock);
 		ses->flags &= ~CIFS_SES_FLAG_SCALE_CHANNELS;
@@ -1197,6 +1212,7 @@ static int smb3_reconfigure(struct fs_context *fc)
 		mutex_unlock(&ses->session_mutex);
 	}
 
+out:
 	STEAL_STRING(cifs_sb, ctx, domainname);
 	STEAL_STRING(cifs_sb, ctx, nodename);
 	STEAL_STRING(cifs_sb, ctx, iocharset);
@@ -1205,6 +1221,16 @@ static int smb3_reconfigure(struct fs_context *fc)
 	ctx->rsize = rsize ? CIFS_ALIGN_RSIZE(fc, rsize) : cifs_sb->ctx->rsize;
 	ctx->wsize = wsize ? CIFS_ALIGN_WSIZE(fc, wsize) : cifs_sb->ctx->wsize;
 
+	/*
+	 * If the multichannel update failed, restore the old multichannel
+	 * settings in ctx so smb3_fs_context_dup() does not desync
+	 * cifs_sb->ctx from ses->chan_max (which was already rolled back).
+	 */
+	if (mchan_rc) {
+		ctx->multichannel = cifs_sb->ctx->multichannel;
+		ctx->max_channels = cifs_sb->ctx->max_channels;
+	}
+
 	smb3_cleanup_fs_context_contents(cifs_sb->ctx);
 	rc = smb3_fs_context_dup(cifs_sb->ctx, ctx);
 	smb3_update_mnt_flags(cifs_sb);
@@ -1213,6 +1239,16 @@ static int smb3_reconfigure(struct fs_context *fc)
 		rc = dfs_cache_remount_fs(cifs_sb);
 #endif
 
+	/*
+	 * Preserve the pre-existing loser-path semantics: a concurrent
+	 * scaling operation causes the remount to be rejected with
+	 * -EINVAL. smb3_fs_context_dup() / dfs_cache_remount_fs()
+	 * failures take precedence because they reflect real state
+	 * recovery errors. Other multichannel failures remain best-effort.
+	 */
+	if (!rc && scale_busy)
+		rc = -EINVAL;
+
 	return rc;
 }
 
-- 
2.43.0

Re: [PATCH v3] smb/client: fix state corruption in smb3_reconfigure multichannel path

[RAJASI MANDAL]

Hi DaeMyung,

Thanks for the v3. One minor suggestion

> + old_chan_max = ses->chan_max;
> + /* Synchronize ses->chan_max with the new mount context */
> + smb3_sync_ses_chan_max(ses, ctx->max_channels);

This reads ses->chan_max without holding chan_lock.
smb3_sync_ses_chan_max() itself takes chan_lock for the write, and
cifs_try_adding_channels() / cifs_chan_skip_or_disable() also access
chan_max under chan_lock. So there is a potential data race between
this unlocked read and a concurrent reconnect path writing chan_max.

Other than that, this  patch looks good to me.

Thanks,
Rajasi

[PATCH v4] smb: client: fix state-consistency bugs in smb3_reconfigure() multichannel path

[DaeMyung Kang]

smb3_reconfigure() has several state-consistency bugs when handling
a multichannel remount that leave ses->chan_max or cifs_sb->ctx
inconsistent with the actual state. This patch repairs internal
state only; the userspace-visible return value of
smb3_reconfigure() is preserved to match the pre-patch behaviour:
the concurrent-scale loser path still returns -EINVAL, and a
failed smb3_update_ses_channels() is not newly propagated to
userspace by this patch.

Bugs addressed:

1) Before this patch, smb3_sync_ses_chan_max() is called before
   acquiring CIFS_SES_FLAG_SCALE_CHANNELS. If a concurrent
   operation (e.g. smb2_reconnect) holds the flag, the current
   thread takes the loser path and returns -EINVAL, but
   ses->chan_max has already been updated to the new value.
   chan_max is then out of sync with the actual channel state.

2) When smb3_update_ses_channels() fails, ses->chan_max is not
   rolled back. Repeated failures cause chan_max to drift further
   from reality, and subsequent reconnect/reconfigure paths use
   the drifted target.

3) Earlier in smb3_reconfigure(), STEAL_STRING moves UNC, source
   and username from cifs_sb->ctx into ctx, setting
   cifs_sb->ctx->UNC to NULL until smb3_fs_context_dup() copies
   them back near the end of the function. The pre-existing
   CIFS_SES_FLAG_SCALE_CHANNELS loser-path 'return -EINVAL' exits
   inside this window. A loser-path failure therefore permanently
   nulls cifs_sb->ctx->UNC; /proc/mounts shows the device as
   "none" and every subsequent mount.cifs-based remount is
   rejected by smb3_verify_reconfigure_ctx() because mount.cifs
   passes that "none" back as the new UNC.

4) Once (2) rolls ses->chan_max back to the old value on update
   failure, smb3_fs_context_dup() would still copy the rejected
   new ctx->max_channels into cifs_sb->ctx, creating a fresh
   cifs_sb->ctx vs ses->chan_max mismatch. This must be handled
   together with (2) to keep the rollback complete.

Fix all four by:

 - Moving smb3_sync_ses_chan_max() after the SCALE_CHANNELS
   acquire so the loser path cannot corrupt chan_max.

 - Capturing old_chan_max atomically with the sync via the new
   smb3_sync_ses_chan_max() return value (under chan_lock), and
   restoring it on failure while still holding SCALE_CHANNELS so
   a concurrent reconfigure cannot race with the rollback.

 - Recording any multichannel-path failure in a local mchan_rc
   and routing the loser path through a common 'out:' label so
   every exit reaches smb3_fs_context_dup() and cifs_sb->ctx is
   restored. mchan_rc is used only as internal control flow.

 - Before the dup, restoring ctx->multichannel,
   ctx->multichannel_specified, ctx->max_channels and
   ctx->max_channels_specified from cifs_sb->ctx on mchan_rc so
   the dup does not desync cifs_sb->ctx from the already-rolled-
   back ses->chan_max (the *_specified bits must travel with the
   values to avoid a "user-specified" flag mismatching the
   restored value).

 - Tracking the concurrent-scale loser path with a separate
   scale_busy flag. After cifs_sb->ctx is fully restored, the
   return value is forced to -EINVAL for that path so userspace
   continues to see the pre-patch rejection. dup /
   dfs_cache_remount_fs() failures still take precedence because
   they reflect real state recovery errors.

Deliberately not changed by this patch: the return value of
smb3_reconfigure() on smb3_update_ses_channels() failure. That
path is not newly propagated to userspace here, matching the
asynchronous model used by the mount path (mchan_mount_work_fn).
Those return-semantics and deferred-handling questions are left
to follow-up discussion/patches.

smb3_reconfigure() is not fully transactional: ses->password and
ses->password2 are committed before the multichannel block, so
unrelated earlier state changes may still be visible after a
failed multichannel remount. That is a structural property of
the function and out of scope here.

Tested with a QEMU VM (ksmbd + cifs) using module-parameter
based fault injection:
 - Forced smb3_update_ses_channels() failure via module param
   and verified ses->chan_max is preserved at the old value
   after the remount path runs.
 - Pre-set CIFS_SES_FLAG_SCALE_CHANNELS before entering the
   scaling path and verified the loser path still returns
   -EINVAL, no longer corrupts ses->chan_max, and no longer nulls
   cifs_sb->ctx->UNC.
 - Repeated dozens of forced-failure remounts with varying
   max_channels (range 2-8) and confirmed no chan_max drift.
 - After each failure path, verified /proc/mounts continues to
   show the original UNC (//127.0.0.1/share) so subsequent
   remounts are accepted.

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/
Fixes: ef529f655a2c ("cifs: client: allow changing multichannel mount options on remount")
Signed-off-by: DaeMyung Kang <charsyam@gmail.com>
---
v4: (feedback from Rajasi Mandal)
 - Fix unlocked read of ses->chan_max when capturing the rollback
   value. smb3_sync_ses_chan_max() now returns the previous
   chan_max so the read+write happens atomically under chan_lock,
   removing the data race against cifs_try_adding_channels() /
   cifs_chan_skip_or_disable() which also access chan_max under
   chan_lock.
 - Switch smb3_sync_ses_chan_max()'s parameter/return type to
   size_t to match struct cifs_ses::chan_max.
 - On the multichannel rollback path, also restore
   ctx->multichannel_specified and ctx->max_channels_specified
   from cifs_sb->ctx so the user-specified flags do not desync
   from the restored values when smb3_fs_context_dup() copies
   ctx back.

v3: (feedback from Henrique Carvalho)
 - Drop propagation of smb3_update_ses_channels() failure to
   userspace; preserve the pre-patch best-effort semantics on that
   path.
 - Keep the pre-existing CIFS_SES_FLAG_SCALE_CHANNELS loser-path
   -EINVAL via a separate scale_busy flag so that return is not
   silently converted into success.
 - Reword commit message to describe accurately what userspace-
   visible semantics are preserved and what changed internally.

v2: (feedback from Rajasi Mandal)
 - Route loser-path and update-failure exits through a common
   'out:' label so smb3_fs_context_dup() always runs and
   cifs_sb->ctx->UNC is restored after STEAL_STRING.
 - Restore ctx->multichannel/ctx->max_channels from cifs_sb->ctx
   before the dup so dup does not re-desync cifs_sb->ctx from the
   rolled-back ses->chan_max.

 fs/smb/client/fs_context.c | 61 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 52 insertions(+), 9 deletions(-)

diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index b9544eb0381b..252420bd7d5d 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 size_t 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 = {
@@ -1069,14 +1069,20 @@ int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_se
  * @max_channels: new maximum number of channels to allow
  *
  * Updates the session's chan_max field to the new value, protecting the update
- * 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).
+ * with the session's channel lock, and returns the previous chan_max value.
+ * 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 size_t smb3_sync_ses_chan_max(struct cifs_ses *ses, size_t max_channels)
 {
+	size_t old_chan_max;
+
 	spin_lock(&ses->chan_lock);
+	old_chan_max = ses->chan_max;
 	ses->chan_max = max_channels;
 	spin_unlock(&ses->chan_lock);
+
+	return old_chan_max;
 }

 static int smb3_reconfigure(struct fs_context *fc)
@@ -1085,10 +1091,12 @@ static int smb3_reconfigure(struct fs_context *fc)
 	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;
+	size_t old_chan_max;
 	unsigned int rsize = ctx->rsize, wsize = ctx->wsize;
 	char *new_password = NULL, *new_password2 = NULL;
 	bool need_recon = false;
-	int rc;
+	bool scale_busy = false;
+	int rc, mchan_rc = 0;

 	if (ses->expired_pwd)
 		need_recon = true;
@@ -1170,25 +1178,37 @@ static int smb3_reconfigure(struct fs_context *fc)
 	if ((ctx->multichannel != cifs_sb->ctx->multichannel) ||
 	    (ctx->max_channels != cifs_sb->ctx->max_channels)) {

-		/* 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;
+			scale_busy = true;
+			mchan_rc = -EINVAL;
+			goto out;
 		}
 		ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
 		spin_unlock(&ses->ses_lock);

+		/* Synchronize ses->chan_max with the new mount context */
+		old_chan_max = smb3_sync_ses_chan_max(ses, ctx->max_channels);
+
 		mutex_unlock(&ses->session_mutex);

 		rc = smb3_update_ses_channels(ses, ses->server,
 					       false /* from_reconnect */,
 					       false /* disable_mchan */);

+		/*
+		 * On failure, restore chan_max while still holding
+		 * CIFS_SES_FLAG_SCALE_CHANNELS so a concurrent reconfigure
+		 * cannot observe or race with the rollback.
+		 */
+		if (rc < 0) {
+			smb3_sync_ses_chan_max(ses, old_chan_max);
+			mchan_rc = rc;
+		}
+
 		/* Clear scaling flag after operation */
 		spin_lock(&ses->ses_lock);
 		ses->flags &= ~CIFS_SES_FLAG_SCALE_CHANNELS;
@@ -1197,6 +1217,7 @@ static int smb3_reconfigure(struct fs_context *fc)
 		mutex_unlock(&ses->session_mutex);
 	}

+out:
 	STEAL_STRING(cifs_sb, ctx, domainname);
 	STEAL_STRING(cifs_sb, ctx, nodename);
 	STEAL_STRING(cifs_sb, ctx, iocharset);
@@ -1205,6 +1226,18 @@ static int smb3_reconfigure(struct fs_context *fc)
 	ctx->rsize = rsize ? CIFS_ALIGN_RSIZE(fc, rsize) : cifs_sb->ctx->rsize;
 	ctx->wsize = wsize ? CIFS_ALIGN_WSIZE(fc, wsize) : cifs_sb->ctx->wsize;

+	/*
+	 * If the multichannel update failed, restore the old multichannel
+	 * settings in ctx so smb3_fs_context_dup() does not desync
+	 * cifs_sb->ctx from ses->chan_max (which was already rolled back).
+	 */
+	if (mchan_rc) {
+		ctx->multichannel = cifs_sb->ctx->multichannel;
+		ctx->multichannel_specified = cifs_sb->ctx->multichannel_specified;
+		ctx->max_channels = cifs_sb->ctx->max_channels;
+		ctx->max_channels_specified = cifs_sb->ctx->max_channels_specified;
+	}
+
 	smb3_cleanup_fs_context_contents(cifs_sb->ctx);
 	rc = smb3_fs_context_dup(cifs_sb->ctx, ctx);
 	smb3_update_mnt_flags(cifs_sb);
@@ -1213,6 +1246,16 @@ static int smb3_reconfigure(struct fs_context *fc)
 		rc = dfs_cache_remount_fs(cifs_sb);
 #endif

+	/*
+	 * Preserve the pre-existing loser-path semantics: a concurrent
+	 * scaling operation causes the remount to be rejected with
+	 * -EINVAL. smb3_fs_context_dup() / dfs_cache_remount_fs()
+	 * failures take precedence because they reflect real state
+	 * recovery errors. Other multichannel failures remain best-effort.
+	 */
+	if (!rc && scale_busy)
+		rc = -EINVAL;
+
 	return rc;
 }

--
2.43.0

Re: [PATCH v3] smb/client: fix state corruption in smb3_reconfigure multichannel path

[CharSyam]

Hi Rajasi,

Thanks a lot for the careful review — you're right, that read of
ses->chan_max was unprotected and races with chan_lock writers in
cifs_try_adding_channels() / cifs_chan_skip_or_disable().

v4 fixes this by folding the read into smb3_sync_ses_chan_max()
itself: the helper now returns the previous chan_max value, so the
read+write happens atomically under chan_lock. The caller no longer
reads ses->chan_max outside the lock. I also switched the helper's
parameter/return type to size_t to match struct cifs_ses::chan_max.

While I was at it, I also restored ctx->multichannel_specified and
ctx->max_channels_specified together
with multichannel / max_channels on the rollback path,
so the user-specified flags don't end up out of sync with
the restored values once smb3_fs_context_dup() copies the ctx back.
v4 just went out — would appreciate another look when you get a chance.

Thanks,
DaeMyung


2026년 4월 29일 (수) 오후 12:17, RAJASI MANDAL <rajasimandalos@gmail.com>님이 작성:
>
> Hi DaeMyung,
>
> Thanks for the v3. One minor suggestion
>
> > + old_chan_max = ses->chan_max;
> > + /* Synchronize ses->chan_max with the new mount context */
> > + smb3_sync_ses_chan_max(ses, ctx->max_channels);
>
> This reads ses->chan_max without holding chan_lock.
> smb3_sync_ses_chan_max() itself takes chan_lock for the write, and
> cifs_try_adding_channels() / cifs_chan_skip_or_disable() also access
> chan_max under chan_lock. So there is a potential data race between
> this unlocked read and a concurrent reconnect path writing chan_max.
>
> Other than that, this  patch looks good to me.
>
> Thanks,
> Rajasi

Re: [PATCH v4] smb: client: fix state-consistency bugs in smb3_reconfigure() multichannel path

[Henrique Carvalho]

Hi DaeMyung,

Thank you for the update.

This version is better than the previous one, but I think it still needs
some work.

First, `mchan_rc` appears to be vestigial in its current form, so I do
not think it should stay.

Second, I think remount/reconfigure should be all-or-nothing. If any
part of the reconfigure attempt fails, we should not commit the rest of
the changes, otherwise userspace gets an error without a clear view of
what actually took effect. As written, I do not understand why
`scale_busy` is handled specially while other failure paths are not.

Lastly, please shorten the commit message. It would be better to follow
the style of nearby CIFS commit logs.

Thanks,

Henrique