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

[Steve French]

On Tue, May 12, 2026 at 5:37 PM Henrique Carvalho
<henrique.carvalho@xxxxxxxx> 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@xxxxxxxxx>
> > Closes: https://lore.kernel.org/lkml/CAEY6_V1+dzW3OD5zqXhsWyXwrDTrg5tAMGZ1AJ7_GAuRE+aevA@xxxxxxxxxxxxxx/
> > Link: https://lore.kernel.org/lkml/xkr2dlvgibq5j6gkcxd3yhhnj4atgxw2uy4eug2pxm7wy7nbms@iq6cf5taa65v/
> > Signed-off-by: DaeMyung Kang <charsyam@xxxxxxxxx>
>
> 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@xxxxxxxx>
>
> > ---
> > 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