| 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: <byjdlepkzmhm6j4ap5eyzdcusl7dgq3iuhkduf3s5h4mrayj32@lzwe2rksc4ei>
Date: Mon, 22 Sep 2025 15:52:23 -0300
From: Enzo Matsumiya <ematsumiya@...e.de>
To: Henrique Carvalho <henrique.carvalho@...e.com>
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 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
Cheers,
Enzo
---------------------
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];
Powered by blists - more mailing lists