| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAH2r5mtHDDe9pkbeR3iTCzoQJBShSGiNB7aj5H9fNX50zPmP2Q@mail.gmail.com>
Date: Mon, 1 Dec 2025 12:35:40 -0600
From: Steve French <smfrench@...il.com>
To: RAJASI MANDAL <rajasimandalos@...il.com>
Cc: Shyam Prasad N <nspmangalore@...il.com>, Meetakshi Setiya <meetakshisetiyaoss@...il.com>,
sfrench@...ba.org, linux-cifs@...r.kernel.org, pc@...guebit.org,
ronniesahlberg@...il.com, sprasad@...rosoft.com, tom@...pey.com,
bharathsm@...rosoft.com, samba-technical@...ts.samba.org,
linux-kernel@...r.kernel.org, Rajasi Mandal <rajasimandal@...rosoft.com>
Subject: Re: [PATCH] cifs: client: allow changing multichannel mount options
on remount
merged the two patches into ksmbd-for-next (will move to cifs-2.6.git
for-next once the next P/R is complete, but want to avoid merge
conflicts for now) pending additional testing and review
On Mon, Dec 1, 2025 at 3:21 AM RAJASI MANDAL <rajasimandalos@...il.com> wrote:
>
> Sending a follow-up patch with corrected comment as pointed out by Meetakshi. Attaching the new patch here for easy reference.
>
> Regards,
> Rajasi
>
> On Mon, Nov 24, 2025 at 5:20 PM Shyam Prasad N <nspmangalore@...il.com> wrote:
>>
>> On Thu, Nov 20, 2025 at 11:40 AM Meetakshi Setiya
>> <meetakshisetiyaoss@...il.com> wrote:
>> >
>> > Hi Rajasi,
>> >
>> > On Tue, Nov 18, 2025 at 7:59 AM Rajasi Mandal <rajasimandalos@...il.com> wrote:
>> > >
>> > > From: Rajasi Mandal <rajasimandal@...rosoft.com>
>> > >
>> > > Previously, the client did not update a session's channel state when
>> > > multichannel or max_channels mount options were changed via remount.
>> > > This led to inconsistent behavior and prevented enabling or disabling
>> > > multichannel support without a full unmount/remount cycle.
>> > >
>> > > Enable dynamic reconfiguration of multichannel and max_channels during
>> > > remount by:
>> > > - Introducing smb3_sync_ses_chan_max(), a centralized function for
>> > > channel updates which synchronizes the session's channels with the
>> > > updated configuration.
>> > > - Replacing cifs_disable_secondary_channels() with
>> > > cifs_decrease_secondary_channels(), which accepts a from_reconfigure
>> > > flag to support targeted cleanup during reconfiguration.
>> > > - Updating remount logic to detect changes in multichannel or
>> > > max_channels and trigger appropriate session/channel updates.
>> > >
>> > > Current limitation:
>> > > - The query_interfaces worker runs even when max_channels=1 so that
>> > > multichannel can be enabled later via remount without requiring an
>> > > unmount. This is a temporary approach and may be refined in the
>> > > future.
>> > >
>> > > Users can safely modify multichannel and max_channels on an existing
>> > > mount. The client will correctly adjust the session's channel state to
>> > > match the new configuration, preserving durability where possible and
>> > > avoiding unnecessary disconnects.
>> > >
>> > > Reviewed-by: Shyam Prasad N <sprasad@...rosoft.com>
>> > > Signed-off-by: Rajasi Mandal <rajasimandal@...rosoft.com>
>> > > ---
>> > > fs/smb/client/cifsproto.h | 4 ++-
>> > > fs/smb/client/connect.c | 4 ++-
>> > > fs/smb/client/fs_context.c | 50 +++++++++++++++++++++++++++++++++-
>> > > fs/smb/client/sess.c | 32 +++++++++++++++-------
>> > > fs/smb/client/smb2pdu.c | 55 ++++++++++++++++++++++++++++----------
>> > > 5 files changed, 119 insertions(+), 26 deletions(-)
>> > >
>> > > diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
>> > > index 3528c365a452..a1fc9e1918bc 100644
>> > > --- a/fs/smb/client/cifsproto.h
>> > > +++ b/fs/smb/client/cifsproto.h
>> > > @@ -649,6 +649,8 @@ int cifs_alloc_hash(const char *name, struct shash_desc **sdesc);
>> > > void cifs_free_hash(struct shash_desc **sdesc);
>> > >
>> > > int cifs_try_adding_channels(struct cifs_ses *ses);
>> > > +int smb3_update_ses_channels(struct cifs_ses *ses, struct TCP_Server_Info *server,
>> > > + bool from_reconnect, bool from_reconfigure);
>> > > bool is_ses_using_iface(struct cifs_ses *ses, struct cifs_server_iface *iface);
>> > > void cifs_ses_mark_for_reconnect(struct cifs_ses *ses);
>> > >
>> > > @@ -674,7 +676,7 @@ bool
>> > > cifs_chan_is_iface_active(struct cifs_ses *ses,
>> > > struct TCP_Server_Info *server);
>> > > void
>> > > -cifs_disable_secondary_channels(struct cifs_ses *ses);
>> > > +cifs_decrease_secondary_channels(struct cifs_ses *ses, bool from_reconfigure);
>> > > void
>> > > cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server);
>> > > int
>> > > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
>> > > index 55cb4b0cbd48..cebe4a5f54f2 100644
>> > > --- a/fs/smb/client/connect.c
>> > > +++ b/fs/smb/client/connect.c
>> > > @@ -3927,7 +3927,9 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, struct smb3_fs_context *ctx)
>> > > ctx->prepath = NULL;
>> > >
>> > > out:
>> > > - cifs_try_adding_channels(mnt_ctx.ses);
>> > > + smb3_update_ses_channels(mnt_ctx.ses, mnt_ctx.server,
>> > > + false /* from_reconnect */,
>> > > + false /* from_reconfigure */);
>> >
>> > Shouldn't this be added to the non-dfs cifs_mount() too? I see that
>> > even cifs_try_adding_channels() is not present in this function,
>> > @Shyam Prasad is this expected?
>>
>> That's a good catch.
>> I think this is a day-0 bug.
>> I can see that the original change by Aurelien has the change only in
>> DFS cifs_mount:
>> commit d70e9fa55884760b6d6c293dbf20d8c52ce11fb7
>> Author: Aurelien Aptel <aaptel@...e.com>
>> Date: Fri Sep 20 06:31:10 2019 +0200
>>
>> cifs: try opening channels after mounting
>>
>> We should have a follow-up patch to fix this.
>>
>> >
>> > > rc = mount_setup_tlink(cifs_sb, mnt_ctx.ses, mnt_ctx.tcon);
>> > > if (rc)
>> > > goto error;
>> > > diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
>> > > index 0f894d09157b..751ed6ebd458 100644
>> > > --- a/fs/smb/client/fs_context.c
>> > > +++ b/fs/smb/client/fs_context.c
>> > > @@ -717,6 +717,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 int smb3_reconfigure(struct fs_context *fc);
>> > >
>> > > static const struct fs_context_operations smb3_fs_context_ops = {
>> > > @@ -1013,6 +1014,22 @@ int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_se
>> > > return 0;
>> > > }
>> > >
>> > > +/*
>> > > + * smb3_sync_ses_chan_max - Synchronize the session's maximum channel count
>> > > + * @ses: pointer to the old CIFS session structure
>> > > + * @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).
>> > > + */
>> > > +static void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int max_channels)
>> > > +{
>> > > + spin_lock(&ses->chan_lock);
>> > > + ses->chan_max = max_channels;
>> > > + spin_unlock(&ses->chan_lock);
>> > > +}
>> > > +
>> > > static int smb3_reconfigure(struct fs_context *fc)
>> > > {
>> > > struct smb3_fs_context *ctx = smb3_fc2context(fc);
>> > > @@ -1095,7 +1112,38 @@ static int smb3_reconfigure(struct fs_context *fc)
>> > > ses->password2 = new_password2;
>> > > }
>> > >
>> > > - mutex_unlock(&ses->session_mutex);
>> > > + /*
>> > > + * 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)) {
>> > > +
>> > > + /* 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);
>> > > + 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 */,
>> > > + true /* from_reconfigure */);
>> > > +
>> > > + /* Clear scaling flag after operation */
>> > > + spin_lock(&ses->ses_lock);
>> > > + ses->flags &= ~CIFS_SES_FLAG_SCALE_CHANNELS;
>> > > + spin_unlock(&ses->ses_lock);
>> > > + } else {
>> > > + mutex_unlock(&ses->session_mutex);
>> > > + }
>> > >
>> > > STEAL_STRING(cifs_sb, ctx, domainname);
>> > > STEAL_STRING(cifs_sb, ctx, nodename);
>> > > diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
>> > > index ef3b498b0a02..cfd83986a84a 100644
>> > > --- a/fs/smb/client/sess.c
>> > > +++ b/fs/smb/client/sess.c
>> > > @@ -265,12 +265,16 @@ int cifs_try_adding_channels(struct cifs_ses *ses)
>> > > }
>> > >
>> > > /*
>> > > - * called when multichannel is disabled by the server.
>> > > - * this always gets called from smb2_reconnect
>> > > - * and cannot get called in parallel threads.
>> > > + * cifs_decrease_secondary_channels - Reduce the number of active secondary channels
>> > > + * @ses: pointer to the CIFS session structure
>> > > + * @from_reconfigure: if true, only reduce to chan_max; if false, reduce to a single channel
>> > > + *
>> > > + * This function disables and cleans up extra secondary channels for a CIFS session.
>> > > + * If called during reconfiguration, it reduces the channel count to the new maximum (chan_max).
>> > > + * Otherwise, it disables all but the primary channel.
>> > > */
>> > > void
>> > > -cifs_disable_secondary_channels(struct cifs_ses *ses)
>> > > +cifs_decrease_secondary_channels(struct cifs_ses *ses, bool from_reconfigure)
>> > > {
>> > > int i, chan_count;
>> > > struct TCP_Server_Info *server;
>> > > @@ -281,12 +285,13 @@ cifs_disable_secondary_channels(struct cifs_ses *ses)
>> > > if (chan_count == 1)
>> > > goto done;
>> > >
>> > > - ses->chan_count = 1;
>> > > -
>> > > - /* for all secondary channels reset the need reconnect bit */
>> > > - ses->chans_need_reconnect &= 1;
>> > > + /* Update the chan_count to the new maximum */
>> > > + if (from_reconfigure)
>> > > + ses->chan_count = ses->chan_max;
>> > > + else
>> > > + ses->chan_count = 1;
>> > >
>> > > - for (i = 1; i < chan_count; i++) {
>> > > + for (i = ses->chan_max ; i < chan_count; i++) {
>> > > iface = ses->chans[i].iface;
>> > > server = ses->chans[i].server;
>> > >
>> > > @@ -318,6 +323,15 @@ cifs_disable_secondary_channels(struct cifs_ses *ses)
>> > > spin_lock(&ses->chan_lock);
>> > > }
>> > >
>> > > + /* For extra secondary channels, reset the need reconnect bit */
>> > > + if (ses->chan_count == 1) {
>> > > + cifs_dbg(VFS, "server does not support multichannel anymore. Disable all other channels\n");
>> > > + ses->chans_need_reconnect &= 1;
>> > > + } else {
>> > > + cifs_dbg(VFS, "Disable extra secondary channels\n");
>> > > + ses->chans_need_reconnect &= ((1UL << ses->chan_max) - 1);
>> > > + }
>> > > +
>> > > done:
>> > > spin_unlock(&ses->chan_lock);
>> > > }
>> > > diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
>> > > index 8b4a4573e9c3..d051da46eaab 100644
>> > > --- a/fs/smb/client/smb2pdu.c
>> > > +++ b/fs/smb/client/smb2pdu.c
>> > > @@ -168,7 +168,7 @@ smb2_hdr_assemble(struct smb2_hdr *shdr, __le16 smb2_cmd,
>> > > static int
>> > > cifs_chan_skip_or_disable(struct cifs_ses *ses,
>> > > struct TCP_Server_Info *server,
>> > > - bool from_reconnect)
>> > > + bool from_reconnect, bool from_reconfigure)
>> > > {
>> > > struct TCP_Server_Info *pserver;
>> > > unsigned int chan_index;
>> > > @@ -206,14 +206,41 @@ cifs_chan_skip_or_disable(struct cifs_ses *ses,
>> > > return -EHOSTDOWN;
>> > > }
>> > >
>> > > - cifs_server_dbg(VFS,
>> > > - "server does not support multichannel anymore. Disable all other channels\n");
>> > > - cifs_disable_secondary_channels(ses);
>> > > -
>> > > + cifs_decrease_secondary_channels(ses, from_reconfigure);
>> > >
>> > > return 0;
>> > > }
>> > >
>> > > +/*
>> > > + * smb3_update_ses_channels - Synchronize session channels with new configuration
>> > > + * @ses: pointer to the CIFS session structure
>> > > + * @server: pointer to the TCP server info structure
>> > > + * @from_reconnect: indicates if called from reconnect context
>> > > + * @from_reconfigure: indicates if called from reconfigure context
>> > > + *
>> > > + * Returns 0 on success or error code on failure.
>> > > + *
>> > > + * Note: Outside of reconfigure, this function is only called from reconnect scenarios
>> > > + * when the server stops advertising multichannel (MC) capability.
>> > > + */
>> >
>> > I see that function is being called during mount too. Did you
>> > mean cifs_decrease_secondary_channels()?
>> >
>> > > +int smb3_update_ses_channels(struct cifs_ses *ses, struct TCP_Server_Info *server,
>> > > + bool from_reconnect, bool from_reconfigure)
>> > > +{
>> > > + int rc = 0;
>> > > + /*
>> > > + * If the current channel count is less than the new chan_max,
>> > > + * try to add channels to reach the new maximum.
>> > > + * Otherwise, disable or skip extra channels to match the new configuration.
>> > > + */
>> > > + if (ses->chan_count < ses->chan_max)
>> > > + rc = cifs_try_adding_channels(ses);
>> > > + else
>> > > + rc = cifs_chan_skip_or_disable(ses, server, from_reconnect,
>> > > + from_reconfigure);
>> > > +
>> > > + return rc;
>> > > +}
>> > > +
>> > > static int
>> > > smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
>> > > struct TCP_Server_Info *server, bool from_reconnect)
>> > > @@ -355,8 +382,8 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
>> > > */
>> > > if (ses->chan_count > 1 &&
>> > > !(server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
>> > > - rc = cifs_chan_skip_or_disable(ses, server,
>> > > - from_reconnect);
>> > > + rc = smb3_update_ses_channels(ses, server,
>> > > + from_reconnect, false /* from_reconfigure */);
>> > > if (rc) {
>> > > mutex_unlock(&ses->session_mutex);
>> > > goto out;
>> > > @@ -438,8 +465,9 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
>> > > * treat this as server not supporting multichannel
>> > > */
>> > >
>> > > - rc = cifs_chan_skip_or_disable(ses, server,
>> > > - from_reconnect);
>> > > + rc = smb3_update_ses_channels(ses, server,
>> > > + from_reconnect,
>> > > + false /* from_reconfigure */);
>> > > goto skip_add_channels;
>> > > } else if (rc)
>> > > cifs_tcon_dbg(FYI, "%s: failed to query server interfaces: %d\n",
>> > > @@ -451,7 +479,8 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
>> > > if (ses->chan_count == 1)
>> > > cifs_server_dbg(VFS, "supports multichannel now\n");
>> > >
>> > > - cifs_try_adding_channels(ses);
>> > > + smb3_update_ses_channels(ses, server, from_reconnect,
>> > > + false /* from_reconfigure */);
>> > > }
>> > > } else {
>> > > mutex_unlock(&ses->session_mutex);
>> > > @@ -1105,8 +1134,7 @@ SMB2_negotiate(const unsigned int xid,
>> > > req->SecurityMode = 0;
>> > >
>> > > req->Capabilities = cpu_to_le32(server->vals->req_capabilities);
>> > > - if (ses->chan_max > 1)
>> > > - req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> > > + req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> > >
>> > > /* ClientGUID must be zero for SMB2.02 dialect */
>> > > if (server->vals->protocol_id == SMB20_PROT_ID)
>> > > @@ -1312,8 +1340,7 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
>> > >
>> > > pneg_inbuf->Capabilities =
>> > > cpu_to_le32(server->vals->req_capabilities);
>> > > - if (tcon->ses->chan_max > 1)
>> > > - pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> > > + pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> > >
>> > > memcpy(pneg_inbuf->Guid, server->client_guid,
>> > > SMB2_CLIENT_GUID_SIZE);
>> > > --
>> > > 2.43.0
>> > >
>> > >
>> >
>> > Thanks
>> > Meetakshi
>> >
>>
>>
>> --
>> Regards,
>> Shyam
--
Thanks,
Steve
Powered by blists - more mailing lists