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