Re: [PATCH 1/2] smb: client: review fixes for remount ctx/retrans sync (patches 1-4)

do you have updated/rebased versions of the patches with the fixes?

Can you resend me the patches?

On Wed, Jul 1, 2026 at 5:35 AM <[email protected]> wrote:
>
> From: Rajasi Mandal <[email protected]>
>
> Fold the review feedback for the first four remount patches into one
> change.  Each fix, in two lines:
>
> - ctx sync (patch 1): sync runtime state into a private reconfigure
>   context, never the shared cifs_sb->ctx, to avoid racing concurrent
>   fspick/remount and /proc/mounts readers.
> - ctx sync (patch 1): compare the new context against a runtime-synced
>   private base_ctx (dup + sync), freed before commit, instead of the raw
>   cifs_sb->ctx.
> - ctx sync (patch 1): drop the fabricated "nopersistent = !use_persistent"
>   derivation; nopersistent is an independent user option and must keep the
>   dup'd value.
> - block options (patch 2): mention "(signloosely)" in the ignore_signature
>   remount rejection so the message names the user-facing option.
> - retrans (patch 3): read/write server->retrans with READ_ONCE/WRITE_ONCE
>   to pair the new remount writer with the lockless retry-path reader.
> - retrans (patch 3): seed the baseline from READ_ONCE(server->retrans) so a
>   bare remount carries the live value forward instead of clobbering it.
> - block cache (patch 4): reword the cache-mode comment to state that
>   cache=ro sets cache_ro and cache=singleclient sets cache_rw.
>
> Signed-off-by: Rajasi Mandal <[email protected]>
> ---
>  fs/smb/client/cifsfs.c     |   6 +-
>  fs/smb/client/connect.c    |   4 +-
>  fs/smb/client/fs_context.c | 122 +++++++++++++++++++++++++++++--------
>  fs/smb/client/smb2ops.c    |   2 +-
>  4 files changed, 104 insertions(+), 30 deletions(-)
>
> diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> index 6b97f7a91235..e99d427fb2c5 100644
> --- a/fs/smb/client/cifsfs.c
> +++ b/fs/smb/client/cifsfs.c
> @@ -612,6 +612,7 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
>         struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
>         struct sockaddr *srcaddr;
>         unsigned int sbflags;
> +       unsigned int retrans;
>
>         srcaddr = (struct sockaddr *)&tcon->ses->server->srcaddr;
>
> @@ -765,8 +766,9 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
>                 seq_printf(s, ",rasize=%u", cifs_sb->ctx->rasize);
>         if (tcon->ses->server->min_offload)
>                 seq_printf(s, ",esize=%u", tcon->ses->server->min_offload);
> -       if (tcon->ses->server->retrans)
> -               seq_printf(s, ",retrans=%u", tcon->ses->server->retrans);
> +       retrans = READ_ONCE(tcon->ses->server->retrans);
> +       if (retrans)
> +               seq_printf(s, ",retrans=%u", retrans);
>         seq_printf(s, ",echo_interval=%lu",
>                         tcon->ses->server->echo_interval / HZ);
>
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index 104658e318b6..14dd472c2b0f 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -1657,7 +1657,7 @@ static int match_server(struct TCP_Server_Info *server,
>         if (server->min_offload != ctx->min_offload)
>                 return 0;
>
> -       if (server->retrans != ctx->retrans)
> +       if (READ_ONCE(server->retrans) != ctx->retrans)
>                 return 0;
>
>         return 1;
> @@ -1872,7 +1872,7 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx,
>          */
>         __module_get(THIS_MODULE);
>         tcp_ses->min_offload = ctx->min_offload;
> -       tcp_ses->retrans = ctx->retrans;
> +       WRITE_ONCE(tcp_ses->retrans, ctx->retrans);
>         /*
>          * at this point we are the only ones with the pointer
>          * to the struct since the kernel thread not created yet
> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> index 53f986d8e9e0..89bf6f4604b3 100644
> --- a/fs/smb/client/fs_context.c
> +++ b/fs/smb/client/fs_context.c
> @@ -928,18 +928,22 @@ static void smb3_fs_context_free(struct fs_context *fc)
>  }
>
>  /*
> - * Sync cifs_sb->ctx with runtime state from tcon/server/ses so the
> - * baseline matches what cifs_show_options() displays.  Wide fields
> - * (dstaddr, ops/vals) are protected by the matching server/tcon lock;
> - * the remaining word-sized scalars rely on the same unsynchronized-read
> - * pattern already used by cifs_show_options().
> + * Sync a private reconfigure context with runtime state from
> + * tcon/server/ses so the baseline matches what cifs_show_options()
> + * displays.  @ctx must be the caller's freshly-duplicated context, not
> + * the live cifs_sb->ctx: this function writes into @ctx, and mutating
> + * the shared cifs_sb->ctx here would race with concurrent fspick/remount
> + * and /proc/mounts readers.  Wide fields (dstaddr, ops/vals) are read
> + * under the matching server/tcon lock; the remaining word-sized scalars
> + * rely on the same unsynchronized-read pattern already used by
> + * cifs_show_options().
>   */
> -static int smb3_sync_ctx_from_runtime(struct cifs_sb_info *cifs_sb)
> +static int smb3_sync_ctx_from_runtime(struct cifs_sb_info *cifs_sb,
> +                                     struct smb3_fs_context *ctx)
>  {
>         struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
>         struct TCP_Server_Info *server = tcon->ses->server;
>         struct cifs_ses *ses = tcon->ses;
> -       struct smb3_fs_context *ctx = cifs_sb->ctx;
>         const char *domain;
>         int unicode;
>
> @@ -961,6 +965,16 @@ static int smb3_sync_ctx_from_runtime(struct cifs_sb_info *cifs_sb)
>         ctx->nosharesock = server->nosharesock;
>         spin_unlock(&server->srv_lock);
>
> +       /*
> +        * retrans lives on the shared TCP_Server_Info and can be updated by
> +        * a remount of any mount sharing the connection.  Sync it from the
> +        * live server so a bare remount carries the current value forward
> +        * instead of writing a stale per-mount copy back in
> +        * smb3_sync_server_opts().  Read with READ_ONCE to pair with the
> +        * lockless WRITE_ONCE writers and the runtime retry-path reader.
> +        */
> +       ctx->retrans = READ_ONCE(server->retrans);
> +
>         /*
>          * tcon->unix_ext can be flipped post-mount by reset_cifs_unix_caps()
>          * on SMB1 reconnect (smb1_reconnect path). Read under tc_lock to pair
> @@ -978,15 +992,27 @@ static int smb3_sync_ctx_from_runtime(struct cifs_sb_info *cifs_sb)
>         }
>         spin_unlock(&tcon->tc_lock);
>         ctx->seal         = tcon->seal;
> +       /*
> +        * persistent reflects the live tcon->use_persistent so the baseline
> +        * matches what cifs_show_options() reports (persistenthandles).  Do
> +        * NOT derive nopersistent from use_persistent: persistent and
> +        * nopersistent are independent user options and use_persistent is a
> +        * derived runtime result.  nopersistent is not reported by
> +        * cifs_show_options(), so it must keep the user's original value
> +        * (carried by the dup); fabricating it here would, after the
> +        * reconfigure commits the ctx, wrongly suppress persistent-handle
> +        * auto-enable on continuous-availability shares at reconnect.
> +        */
>         ctx->persistent   = tcon->use_persistent;
> -       ctx->nopersistent = !tcon->use_persistent;
>         ctx->resilient    = tcon->use_resilient;
>         ctx->witness      = tcon->use_witness;
>
>         /*
>          * Session fields: domainName and unicode are effectively
>          * write-once (set during session setup, never freed/replaced
> -        * while the session exists), so plain reads are safe.
> +        * while the session exists), so plain reads are safe.  @ctx is
> +        * the caller's private copy, so filling in a missing domainname
> +        * here cannot leak or race.
>          */
>         domain = ses->domainName;
>         unicode = ses->unicode;
> @@ -1180,7 +1206,7 @@ static int smb3_verify_reconfigure_ctx(struct fs_context *fc,
>                 return -EINVAL;
>         }
>         if (new_ctx->ignore_signature != old_ctx->ignore_signature) {
> -               cifs_errorf(fc, "can not change ignore_signature during remount\n");
> +               cifs_errorf(fc, "can not change ignore_signature (signloosely) during remount\n");
>                 return -EINVAL;
>         }
>         if (new_ctx->seal != old_ctx->seal) {
> @@ -1212,7 +1238,11 @@ static int smb3_verify_reconfigure_ctx(struct fs_context *fc,
>                 cifs_errorf(fc, "can not change rdma during remount\n");
>                 return -EINVAL;
>         }
> -       /* init default: cache_ro = false, cache_rw = false (i.e. cache=strict) */
> +       /*
> +        * cache=ro sets cache_ro and cache=singleclient sets cache_rw; the
> +        * other cache modes (strict/loose/none) leave both clear and are
> +        * handled separately in smb3_update_mnt_flags().
> +        */
>         if (new_ctx->cache_ro != old_ctx->cache_ro) {
>                 cifs_errorf(fc, "can not change cache=ro during remount\n");
>                 return -EINVAL;
> @@ -1340,7 +1370,7 @@ static void smb3_sync_server_opts(struct cifs_sb_info *cifs_sb)
>         struct TCP_Server_Info *server = cifs_sb_master_tcon(cifs_sb)->ses->server;
>         struct smb3_fs_context *ctx = cifs_sb->ctx;
>
> -       server->retrans = ctx->retrans;
> +       WRITE_ONCE(server->retrans, ctx->retrans);
>  }
>
>  static int smb3_reconfigure(struct fs_context *fc)
> @@ -1348,6 +1378,7 @@ 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 smb3_fs_context *base_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;
> @@ -1360,9 +1391,37 @@ static int smb3_reconfigure(struct fs_context *fc)
>         if (ses->expired_pwd)
>                 need_recon = true;
>
> -       rc = smb3_verify_reconfigure_ctx(fc, ctx, cifs_sb->ctx, need_recon);
> +       /*
> +        * Compare the new context against a runtime-synced baseline, not the
> +        * raw cifs_sb->ctx.  smb3_init_fs_context() syncs the new context
> +        * (fc->fs_private) from runtime state, so the comparison baseline must
> +        * be synced the same way; otherwise a field that drifted at runtime
> +        * (e.g. dstaddr after witness failover, or use_persistent auto-enabled
> +        * on a continuous-availability share) would look like a user-requested
> +        * change and be rejected on an otherwise bare remount.  Sync a private
> +        * copy rather than cifs_sb->ctx: s_umount serializes reconfigures, but
> +        * not the fspick/fsconfig init path, which dup()s cifs_sb->ctx in
> +        * smb3_init_fs_context() before reconfigure_super() takes s_umount.  An
> +        * in-place sync would therefore race that lockless dup (e.g. a torn
> +        * read of the 128-byte dstaddr).
> +        */
> +       base_ctx = kzalloc_obj(*base_ctx);
> +       if (!base_ctx)
> +               return -ENOMEM;
> +       rc = smb3_fs_context_dup(base_ctx, cifs_sb->ctx);
>         if (rc)
> -               return rc;
> +               goto free_base_ctx;
> +       rc = smb3_sync_ctx_from_runtime(cifs_sb, base_ctx);
> +       if (rc)
> +               goto cleanup_base_ctx;
> +
> +       rc = smb3_verify_reconfigure_ctx(fc, ctx, base_ctx, need_recon);
> +       if (rc)
> +               goto cleanup_base_ctx;
> +
> +       smb3_cleanup_fs_context_contents(base_ctx);
> +       kfree(base_ctx);
> +       base_ctx = NULL;
>
>         rc = smb3_handle_conflicting_options(fc);
>         if (rc)
> @@ -1537,6 +1596,13 @@ static int smb3_reconfigure(struct fs_context *fc)
>  free_old_ctx:
>         kfree(old_ctx);
>
> +       return rc;
> +
> +cleanup_base_ctx:
> +       smb3_cleanup_fs_context_contents(base_ctx);
> +free_base_ctx:
> +       kfree(base_ctx);
> +
>         return rc;
>  }
>
> @@ -2258,28 +2324,34 @@ int smb3_init_fs_context(struct fs_context *fc)
>          * instead of building one from scratch with init defaults.
>          *
>          * VFS sets fc->root before calling init_fs_context for reconfigure,
> -        * so we can access the existing superblock's context.  We first sync
> -        * cifs_sb->ctx with runtime state (tcon/server/ses) so that ctx
> -        * matches what cifs_show_options() displays.  Then we dup old_ctx
> -        * into new_ctx.  The parser will overwrite only the options
> -        * explicitly passed on remount, so any difference between new_ctx
> -        * and old_ctx in smb3_verify_reconfigure_ctx() represents a real,
> -        * intentional change by the user.
> +        * so we can access the existing superblock's context.  We dup the
> +        * live cifs_sb->ctx into a private new_ctx, then sync new_ctx with
> +        * runtime state (tcon/server/ses) so it matches what
> +        * cifs_show_options() displays.  Syncing into the private copy (not
> +        * the shared cifs_sb->ctx) avoids racing with concurrent
> +        * fspick/remount and /proc/mounts readers.  The parser will overwrite
> +        * only the options explicitly passed on remount, so any difference
> +        * between new_ctx and old_ctx in smb3_verify_reconfigure_ctx()
> +        * represents a real, intentional change by the user.
>          */
>         if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE) {
>                 struct cifs_sb_info *cifs_sb = CIFS_SB(fc->root->d_sb);
>                 int rc;
>
> -               rc = smb3_sync_ctx_from_runtime(cifs_sb);
> -               if (rc)
> -                       return rc;
> -
>                 ctx = kzalloc_obj(struct smb3_fs_context);
>                 if (!ctx)
>                         return -ENOMEM;
>
>                 rc = smb3_fs_context_dup(ctx, cifs_sb->ctx);
>                 if (rc) {
> +                       smb3_cleanup_fs_context_contents(ctx);
> +                       kfree(ctx);
> +                       return rc;
> +               }
> +
> +               rc = smb3_sync_ctx_from_runtime(cifs_sb, ctx);
> +               if (rc) {
> +                       smb3_cleanup_fs_context_contents(ctx);
>                         kfree(ctx);
>                         return rc;
>                 }
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index a8f8feeeccb5..b30c16b0d157 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -2806,7 +2806,7 @@ bool smb2_should_replay(struct cifs_tcon *tcon,
>         if (!pretries || !pcur_sleep)
>                 return false;
>
> -       if (tcon->retry || (*pretries)++ < tcon->ses->server->retrans) {
> +       if (tcon->retry || (*pretries)++ < READ_ONCE(tcon->ses->server->retrans)) {
>                 /* Update sleep time for exponential backoff */
>                 if (!(*pcur_sleep))
>                         (*pcur_sleep) = 1;
> --
> 2.43.0
>
>


-- 
Thanks,

Steve