From: Meetakshi Setiya <meetakshisetiyaoss@gmail.com>
To: Rajasi Mandal <rajasimandalos@gmail.com>
Cc: stfrench@microsoft.com, linux-cifs@vger.kernel.org,
sfrench@samba.org, Rajasi Mandal <rajasimandal@microsoft.com>
Subject: Re: [PATCH v2 1/9] smb: client: block non-reconfigurable option changes on remount
Date: Fri, 17 Apr 2026 07:05:56 -0700 [thread overview]
Message-ID: <CAFTVevUjoGEzaiUkyC5baXNvXSZTmVrzwE-caRLnOfPZZObjKw@mail.gmail.com> (raw)
In-Reply-To: <20260413073249.2897840-1-rajasimandalos@gmail.com>
Hi Rajasi,
My comments inline
On Mon, Apr 13, 2026 at 12:33 AM Rajasi Mandal <rajasimandalos@gmail.com> wrote:
>
> From: Rajasi Mandal <rajasimandal@microsoft.com>
>
> Options like seal, sign, vers, ip, rdma and others require tearing
> down the connection to take effect, so they cannot be changed on
> remount. Previously they were silently accepted and ignored.
>
> Add checks in smb3_verify_reconfigure_ctx() to reject changes to
> these options on remount. Options shown in /proc/mounts use a bare
> != comparison since libmount feeds them back automatically. Options
> not shown in /proc/mounts use an 'if (new && new != old)' guard so
> their init default does not cause a false mismatch.
>
> Introduce smb3_preserve_non_reconfig_opts() to copy hidden option
> values from the old context into the new one before
> smb3_fs_context_dup() overwrites them with init defaults. Without
> this, a bare remount silently corrupts the stored mount context by
> replacing live values with zero/init defaults for every option not
> shown in /proc/mounts.
>
> Introduce smb3_sync_ctx_from_negotiated() to sync the stored mount
> context with runtime state (persistenthandles, unix, nosharesock,
> resilienthandles, domainname, ops/vals) before the verify step, since
> cifs_show_options() reads these from tcon/server and they can diverge
> from ctx when the server auto-enables a feature or negotiates a
> different dialect than what was parsed (e.g. default -> SMB 3.1.1).
> Without this sync, the fed-back /proc/mounts values mismatch the
> stale ctx and the verify step incorrectly rejects a bare remount.
>
> Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
> ---
> fs/smb/client/fs_context.c | 235 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 235 insertions(+)
>
> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> index 3f0faae99ed5..a0217b5641a2 100644
> --- a/fs/smb/client/fs_context.c
> +++ b/fs/smb/client/fs_context.c
> @@ -1021,10 +1021,241 @@ static int smb3_verify_reconfigure_ctx(struct fs_context *fc,
> cifs_errorf(fc, "can not change nbsessinit during remount\n");
> return -EINVAL;
> }
> + /* init default: compress = false */
> + if (new_ctx->compress &&
> + new_ctx->compress != old_ctx->compress) {
> + cifs_errorf(fc, "can not change compress during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->noblocksnd != old_ctx->noblocksnd) {
> + cifs_errorf(fc, "can not change noblocksend during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->noautotune != old_ctx->noautotune) {
> + cifs_errorf(fc, "can not change noautotune during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->no_sparse != old_ctx->no_sparse) {
> + cifs_errorf(fc, "can not change nosparse during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->nodelete != old_ctx->nodelete) {
> + cifs_errorf(fc, "can not change nodelete during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->cruid_specified &&
> + !uid_eq(new_ctx->cred_uid, old_ctx->cred_uid)) {
> + cifs_errorf(fc, "can not change cruid during remount\n");
> + return -EINVAL;
> + }
> + /* init default: port = 0 */
> + if (new_ctx->port &&
> + new_ctx->port != old_ctx->port) {
> + cifs_errorf(fc, "can not change port during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->min_offload != old_ctx->min_offload) {
> + cifs_errorf(fc, "can not change min_enc_offload during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->snapshot_time != old_ctx->snapshot_time) {
> + cifs_errorf(fc, "can not change snapshot during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->max_credits != old_ctx->max_credits) {
> + cifs_errorf(fc, "can not change max_credits during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->handle_timeout != old_ctx->handle_timeout) {
> + cifs_errorf(fc, "can not change handletimeout during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->got_ip &&
> + !cifs_match_ipaddr((struct sockaddr *)&new_ctx->dstaddr,
> + (struct sockaddr *)&old_ctx->dstaddr)) {
> + cifs_errorf(fc, "can not change ip during remount\n");
> + return -EINVAL;
> + }
> + if (((struct sockaddr *)&new_ctx->srcaddr)->sa_family != AF_UNSPEC &&
> + memcmp(&new_ctx->srcaddr, &old_ctx->srcaddr, sizeof(new_ctx->srcaddr))) {
> + cifs_errorf(fc, "can not change srcaddr during remount\n");
> + return -EINVAL;
> + }
> + /* source_rfc1001_name: both init from same nodename, safe to compare */
> + if (memcmp(new_ctx->source_rfc1001_name, old_ctx->source_rfc1001_name,
> + RFC1001_NAME_LEN)) {
> + cifs_errorf(fc, "can not change netbiosname during remount\n");
> + return -EINVAL;
> + }
> + /* init default: target_rfc1001_name[0] = 0 */
> + if (new_ctx->target_rfc1001_name[0] &&
> + memcmp(new_ctx->target_rfc1001_name, old_ctx->target_rfc1001_name,
> + RFC1001_NAME_LEN)) {
> + cifs_errorf(fc, "can not change servern during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->got_version &&
> + (new_ctx->ops != old_ctx->ops || new_ctx->vals != old_ctx->vals)) {
> + cifs_errorf(fc, "can not change vers during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->witness != old_ctx->witness) {
> + cifs_errorf(fc, "can not change witness during remount\n");
> + return -EINVAL;
> + }
> + /* init default: rootfs = false */
> + if (new_ctx->rootfs &&
> + new_ctx->rootfs != old_ctx->rootfs) {
> + cifs_errorf(fc, "can not change rootfs during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->linux_ext != old_ctx->linux_ext ||
> + new_ctx->no_linux_ext != old_ctx->no_linux_ext) {
> + cifs_errorf(fc, "can not change unix during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->nocase != old_ctx->nocase) {
> + cifs_errorf(fc, "can not change nocase during remount\n");
> + return -EINVAL;
> + }
> + /* init default: intr = false */
> + if (new_ctx->intr &&
> + new_ctx->intr != old_ctx->intr) {
> + cifs_errorf(fc, "can not change intr during remount\n");
> + return -EINVAL;
> + }
> + /* init default: no_psx_acl = 0 */
> + if (new_ctx->no_psx_acl &&
> + new_ctx->no_psx_acl != old_ctx->no_psx_acl) {
> + cifs_errorf(fc, "can not change acl during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->local_lease != old_ctx->local_lease) {
> + cifs_errorf(fc, "can not change locallease during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->sign != old_ctx->sign) {
> + cifs_errorf(fc, "can not change sign during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->ignore_signature != old_ctx->ignore_signature) {
> + cifs_errorf(fc, "can not change ignore_signature during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->seal != old_ctx->seal) {
> + cifs_errorf(fc, "can not change seal during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->nosharesock != old_ctx->nosharesock) {
> + cifs_errorf(fc, "can not change nosharesock during remount\n");
> + return -EINVAL;
> + }
> + /*
> + * persistent is shown in /proc/mounts (bare !=).
> + * nopersistent is NOT shown, so guard with init default check.
> + */
> + if (new_ctx->persistent != old_ctx->persistent ||
> + (new_ctx->nopersistent &&
> + new_ctx->nopersistent != old_ctx->nopersistent)) {
> + cifs_errorf(fc, "can not change persistenthandles during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->resilient != old_ctx->resilient) {
> + cifs_errorf(fc, "can not change resilienthandles during remount\n");
> + return -EINVAL;
> + }
> + if (new_ctx->sockopt_tcp_nodelay != old_ctx->sockopt_tcp_nodelay) {
> + cifs_errorf(fc, "can not change tcpnodelay during remount\n");
> + return -EINVAL;
> + }
> + /* init default: domainauto = false */
> + if (new_ctx->domainauto &&
> + new_ctx->domainauto != old_ctx->domainauto) {
> + cifs_errorf(fc, "can not change domainauto during remount\n");
> + return -EINVAL;
> + }
> + /* init default: rdma = false */
> + if (new_ctx->rdma &&
> + new_ctx->rdma != old_ctx->rdma) {
> + cifs_errorf(fc, "can not change rdma during remount\n");
> + return -EINVAL;
> + }
>
> return 0;
> }
The guard patterns are inconsistent. For example, the bare != options
can be rejected if the auto-negotiated value differs from init
(examples below).
>
> +/*
> + * Sync cifs_sb->ctx with negotiated runtime values for fields that
> + * cifs_show_options() reads from tcon/server rather than from ctx.
> + * The mount.cifs helper reads /proc/mounts and feeds these values back
> + * on remount, so ctx must match the runtime state to avoid false
> + * rejection by smb3_verify_reconfigure_ctx().
> + */
> +static void smb3_sync_ctx_from_negotiated(struct cifs_sb_info *cifs_sb)
> +{
> + struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
> + struct TCP_Server_Info *server = tcon->ses->server;
> + struct smb3_fs_context *ctx = cifs_sb->ctx;
> +
> + /* /proc/mounts shows persistenthandles from tcon->use_persistent */
> + if (tcon->use_persistent && !ctx->persistent) {
> + ctx->persistent = true;
> + ctx->nopersistent = false;
> + }
> + /* /proc/mounts shows resilienthandles from tcon->use_resilient */
> + ctx->resilient = tcon->use_resilient;
> + /* /proc/mounts shows nounix/unix/posix from tcon */
> + if (tcon->posix_extensions) {
> + ctx->linux_ext = 1;
> + ctx->no_linux_ext = 0;
> + } else if (tcon->unix_ext) {
> + ctx->linux_ext = 1;
> + ctx->no_linux_ext = 0;
> + } else {
> + ctx->linux_ext = 0;
> + ctx->no_linux_ext = 1;
> + }
> + /* /proc/mounts shows nosharesock from server->nosharesock */
> + if (server->nosharesock)
> + ctx->nosharesock = true;
> + /*
> + * /proc/mounts shows vers= from server->vals->version_string,
> + * which reflects the negotiated dialect. When mounted without
> + * an explicit vers=, ctx stores smbdefault ops/vals while the
> + * server may have negotiated e.g. SMB 3.1.1. Sync so the
> + * verify check does not reject the fed-back vers= on remount.
> + */
> + ctx->ops = server->ops;
> + ctx->vals = server->vals;
> +}
There is a risk of racing with reconnects here. Negotiate can modify
ops and vals.
When no explicit vers= is passed on remount,
smb3_sync_ctx_from_negotiated syncs old_ctx->ops/vals to whatever the
server has negotiated. The new_ctx, however, still has defaults
(smb30_ops/vals) and duplicating fs_context overwrites cifs_sb->ctx
with these values.
To repro, mount with vers 3.1.1 and remount without providing a version. ()
If you add some logging, you will see that the version string before
remount: 3.1.1 and version string after remount: default (3.0).
Also, I was not able to do a bare remount (-o username,remount)
because I got this error:
CIFS: VFS: can not change unix during remount
I was not using linux extensions. When you sync ctx from negotiated,
no_linux_ext is 1, but in the new context, no_linux_ext is initialised
to 0. So, when you compare the two, there is a spurious rejection.
You might want to check similar failures for other mount options too.
I am getting similar warnings for nosharesock.
There are no guardrails to protect old ctx values that have not been
synced in perserve_non_reconfig_opts.
> +
> +/*
> + * Carry forward non-reconfigurable mount options that are NOT
> + * displayed by cifs_show_options() into the new remount context
> + * before smb3_fs_context_dup() overwrites cifs_sb->ctx.
> + *
> + * Options that cifs_show_options() outputs are automatically
> + * re-fed by libmount (which reads /proc/mounts) on remount,
> + * so they do not need to be preserved here.
> + *
> + * Without this, a bare remount would silently reset these hidden
> + * fields to their init defaults.
> + */
> +static void smb3_preserve_non_reconfig_opts(struct smb3_fs_context *new_ctx,
> + struct smb3_fs_context *old_ctx)
> +{
> + new_ctx->compress = old_ctx->compress;
> + new_ctx->port = old_ctx->port;
> + new_ctx->rootfs = old_ctx->rootfs;
> + new_ctx->intr = old_ctx->intr;
> + new_ctx->no_psx_acl = old_ctx->no_psx_acl;
> + new_ctx->domainauto = old_ctx->domainauto;
> + new_ctx->rdma = old_ctx->rdma;
> + new_ctx->nopersistent = old_ctx->nopersistent;
> + memcpy(new_ctx->target_rfc1001_name, old_ctx->target_rfc1001_name,
> + RFC1001_NAME_LEN_WITH_NULL);
> +}
> +
> #define STEAL_STRING(cifs_sb, ctx, field) \
> do { \
> kfree(ctx->field); \
> @@ -1093,6 +1324,8 @@ static int smb3_reconfigure(struct fs_context *fc)
> if (ses->expired_pwd)
> need_recon = true;
>
> + smb3_sync_ctx_from_negotiated(cifs_sb);
> +
> rc = smb3_verify_reconfigure_ctx(fc, ctx, cifs_sb->ctx, need_recon);
> if (rc)
> return rc;
> @@ -1205,6 +1438,8 @@ 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;
>
> + smb3_preserve_non_reconfig_opts(ctx, cifs_sb->ctx);
> +
> smb3_cleanup_fs_context_contents(cifs_sb->ctx);
> rc = smb3_fs_context_dup(cifs_sb->ctx, ctx);
> smb3_update_mnt_flags(cifs_sb);
> --
> 2.43.0
>
>
Thanks
Meetakshi
prev parent reply other threads:[~2026-04-17 14:06 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-09 9:59 [PATCH 1/9] smb: client: block non-reconfigurable option changes on remount rajasimandalos
2026-04-09 9:59 ` [PATCH 2/9] smb: client: allow both 'lease' and 'nolease' mount options rajasimandalos
2026-04-13 13:30 ` Meetakshi Setiya
2026-04-09 9:59 ` [PATCH 3/9] smb: client: sync tcon-level options on remount rajasimandalos
2026-04-17 14:29 ` Meetakshi Setiya
2026-04-09 9:59 ` [PATCH 4/9] smb: client: sync retrans " rajasimandalos
2026-04-09 9:59 ` [PATCH 5/9] smb: client: sync echo_interval " rajasimandalos
2026-04-22 4:42 ` [PATCH v2] " rajasimandalos
2026-04-09 9:59 ` [PATCH 6/9] smb: client: allow nolease option to be reconfigured " rajasimandalos
2026-04-22 5:06 ` [PATCH v2] " rajasimandalos
2026-04-22 8:36 ` Bharath SM
2026-04-22 15:29 ` Steve French
2026-04-09 9:59 ` [PATCH 7/9] smb: client: block cache=ro and cache=singleclient " rajasimandalos
2026-04-13 13:40 ` Meetakshi Setiya
2026-04-22 4:34 ` RAJASI MANDAL
2026-04-09 9:59 ` [PATCH 8/9] smb: client: fix domainauto remount by syncing domainname from session rajasimandalos
2026-04-09 9:59 ` [PATCH 9/9] smb: client: apply rasize on remount rajasimandalos
2026-04-13 13:29 ` Meetakshi Setiya
2026-04-13 7:32 ` [PATCH v2 1/9] smb: client: block non-reconfigurable option changes " Rajasi Mandal
2026-04-17 14:05 ` Meetakshi Setiya [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFTVevUjoGEzaiUkyC5baXNvXSZTmVrzwE-caRLnOfPZZObjKw@mail.gmail.com \
--to=meetakshisetiyaoss@gmail.com \
--cc=linux-cifs@vger.kernel.org \
--cc=rajasimandal@microsoft.com \
--cc=rajasimandalos@gmail.com \
--cc=sfrench@samba.org \
--cc=stfrench@microsoft.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).