| 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: <64ded651-0319-4c1b-aeb9-f2229a45bd8a@suse.com>
Date: Mon, 22 Sep 2025 16:21:25 -0300
From: Henrique Carvalho <henrique.carvalho@...e.com>
To: Enzo Matsumiya <ematsumiya@...e.de>
Cc: Steve French <smfrench@...il.com>, rajasimandalos@...il.com,
linux-cifs@...r.kernel.org, sfrench@...ba.org, pc@...guebit.org,
ronniesahlberg@...il.com, sprasad@...rosoft.com, tom@...pey.com,
bharathsm@...rosoft.com, linux-kernel@...r.kernel.org,
Rajasi Mandal <rajasimandal@...rosoft.com>
Subject: Re: [PATCH 1/2] cifs: client: force multichannel=off when
max_channels=1
On 9/22/25 3:52 PM, Enzo Matsumiya wrote:
> On 09/22, Henrique Carvalho wrote:
>>
>>
>> On 9/22/25 1:14 PM, Steve French wrote:
>>> . >Do we even need ->multichannel flag at all?
>>>
>>> Yes - especially in the future. The goal is for the user to have
>>> three options:
>>> 1) (preferred) "multichannel" (max channels will be dynamically
>>> selected and can change) the client gets to choose how many channels
>>> to connect to based on what it sees in the output of the most recent
>>> query interfaces call (it can change on the fly as server dynamically
>>> adds and removes channels or networks become temporarily unreachable)
>>
>> I'm guessing this would be required while we are transitioning from
>> setting channels dynamically to having multichannel on by default, as
>> you commented below. Because once we have it on by default, I don't
>> think there is a point in having the flag.
>
> Exactly.
>
>>> 2) "max_channels=" This is for the case where user wants to choose
>>> the number of channels rather than have the client automatically
>>> (hopefully soon in the future) choose it for you
>>> 3) if server has mchan bugs, allow client to mount with no
>>> multichannel (or equivalent max_channels=1)
>>>
>>> But ... "remount" also has to work for the three above (and currently
>>> doesn't) and this is what I am hoping the recent patches can fix (?)
>>> but haven't tested them enough yet
>
> I was talking more in the context of code, that it could use some
> refactoring/improvements -- I also think such functionality (hence the
> patches) are necessary.
>
> There's no sense for me, as a user, to specify e.g.:
> # mount.cifs -o ...,multichannel,max_channels=2 ...
>
> Was there only a single option for this, it would be less confusing and
> wouldn't require this patch here.
>
>
> The below patch (PoC) is an idea I had that would make things much
> clearer for users -- have 'multipath' mount option be either a flag or a
> value option, e.g.:
>
> # mount.cifs -o ...,multichannel //srv/share /mnt/test
> # findmnt -t cifs -u | grep -Eo 'max_channels=[0-9]+'
> max_channels=2
> # umount /mnt/test
> # mount.cifs -o ...,multichannel=4 //srv/share /mnt/test
> # findmnt -t cifs -u | grep -Eo 'max_channels=[0-9]+'
> max_channels=4
>
>
I agree. This is clearer and we can drop one flag.
>
> ---------------------
>
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index dd12f3eb61dc..ad9a588c7103 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -2487,7 +2487,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server,
> struct smb3_fs_context *ctx)
> spin_lock(&ses->chan_lock);
> ses->chans[0].server = server;
> ses->chan_count = 1;
> - ses->chan_max = ctx->multichannel ? ctx->max_channels:1;
> + ses->chan_max = ctx->max_channels;
> ses->chans_need_reconnect = 1;
> spin_unlock(&ses->chan_lock);
>
> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> index 072383899e81..ceb19a58b743 100644
> --- a/fs/smb/client/fs_context.c
> +++ b/fs/smb/client/fs_context.c
> @@ -165,7 +165,7 @@ const struct fs_parameter_spec smb3_fs_parameters[] = {
> fsparam_u32("max_cached_dirs", Opt_max_cached_dirs),
> fsparam_u32("handletimeout", Opt_handletimeout),
> fsparam_u64("snapshot", Opt_snapshot),
> - fsparam_u32("max_channels", Opt_max_channels),
> + fsparam_u32("multichannel", Opt_multichannel),
>
> /* Mount options which take string value */
> fsparam_string("source", Opt_source),
> @@ -1252,14 +1252,16 @@ static int smb3_fs_context_parse_param(struct
> fs_context *fc,
> ctx->nodelete = 1;
> break;
> case Opt_multichannel:
> - if (result.negated) {
> - ctx->multichannel = false;
> - ctx->max_channels = 1;
> - } else {
> - ctx->multichannel = true;
> - /* if number of channels not specified, default to 2 */
> - if (ctx->max_channels < 2)
> + if (param->type == fs_value_is_flag) {
> + if (!result.negated && ctx->max_channels < 2)
> ctx->max_channels = 2;
> + } else {
> + if (result.uint_32 < 1 || result.uint_32 >
> CIFS_MAX_CHANNELS) {
> + cifs_errorf(fc, "%s: Invalid max_channels value, needs
> to be 1-%d\n",
> + __func__, CIFS_MAX_CHANNELS);
> + goto cifs_parse_mount_err;
> + }
> + ctx->max_channels = result.uint_32;
> }
> break;
> case Opt_uid:
> @@ -1395,17 +1397,6 @@ static int smb3_fs_context_parse_param(struct
> fs_context *fc,
> }
> ctx->max_credits = result.uint_32;
> break;
> - case Opt_max_channels:
> - if (result.uint_32 < 1 || result.uint_32 > CIFS_MAX_CHANNELS) {
> - cifs_errorf(fc, "%s: Invalid max_channels value, needs to
> be 1-%d\n",
> - __func__, CIFS_MAX_CHANNELS);
> - goto cifs_parse_mount_err;
> - }
> - ctx->max_channels = result.uint_32;
> - /* If more than one channel requested ... they want multichan */
> - if (result.uint_32 > 1)
> - ctx->multichannel = true;
> - break;
> case Opt_max_cached_dirs:
> if (result.uint_32 < 1) {
> cifs_errorf(fc, "%s: Invalid max_cached_dirs, needs to be 1
> or more\n",
> @@ -1901,7 +1892,6 @@ int smb3_init_fs_context(struct fs_context *fc)
> ctx->echo_interval = SMB_ECHO_INTERVAL_DEFAULT;
>
> /* default to no multichannel (single server connection) */
> - ctx->multichannel = false;
> ctx->max_channels = 1;
>
> ctx->backupuid_specified = false; /* no backup intent for a user */
> diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
> index b0fec6b9a23b..ff75a7cc11dd 100644
> --- a/fs/smb/client/fs_context.h
> +++ b/fs/smb/client/fs_context.h
> @@ -175,7 +175,6 @@ enum cifs_param {
> Opt_max_credits,
> Opt_max_cached_dirs,
> Opt_snapshot,
> - Opt_max_channels,
> Opt_handletimeout,
>
> /* Mount options which take string value */
> @@ -293,7 +292,6 @@ struct smb3_fs_context {
> bool resilient:1; /* noresilient not required since not fored for
> CA */
> bool domainauto:1;
> bool rdma:1;
> - bool multichannel:1;
> bool use_client_guid:1;
> /* reuse existing guid for multichannel */
> u8 client_guid[SMB2_CLIENT_GUID_SIZE];
>
--
Henrique
SUSE Labs
Powered by blists - more mailing lists