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

[DaeMyung Kang <charsyam@gmail.com>]

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