A patch to implement the already documented "sep" option for the CIFS file system.

A patch to implement the already documented "sep" option for the CIFS file system.

[Dmitry Telegin]

DESCRIPTION OF THE PROBLEM

Some users are accustomed to using shared folders in Windows with a
comma in the name, for example: "//server3/Block 3,4".
When they try to migrate to Linux, they cannot mount such paths.

An example of the line generated by "mount.cifs" for the kernel when
mounting "//server3/Block 3,4":
"ip=10.0.2.212,unc=\\server3\Block 3,4,iocharset=utf8,user=user1,domain=AD"
Accordingly, due to the extra comma, we have an error:
"Sep 7 21:57:18 S1 kernel: [ 795.604821] CIFS: Unknown mount option "4""


DOCUMENTATION

https://www.kernel.org/doc/html/latest/admin-guide/cifs/usage.html
The "sep" parameter is described here to specify a new delimiter
instead of a comma.
I quote:
   "sep
   if first mount option (after the -o), overrides the comma as the
separator between the mount parms. e.g.:
   -o user=myname,password=mypassword,domain=mydom
   could be passed instead with period as the separator by:
   -o sep=.user=myname.password=mypassword.domain=mydom
   this might be useful when comma is contained within username or
password or domain."


RESEARCH WORK

I looked at the "mount.cifs" code. There is no provision for the use
of a comma by the user, since the comma is used to form the parameter
string to the kernel (man 2 mount). This line can be seen by adding
the "--verbose" flag to the mount.
"mount.cifs --help" lists "sep" as a possible option, but does not
implement it in the code and does not describe it in "man 8
mount.cifs".

I looked at the "pam-mount" code - the mount options are assembled
with a wildcard comma. The result is a text line: "mount -t cifs ...".

The handling of options in the "mount" utility is based on the
"libmount" library, which is hardcoded to use only a comma as a
delimiter.

I tried to mount "//server3/Block 3,4" with my own program (man 2
mount) by specifying "sep=!" - successfully.


SOLUTION

It would be nice if we add in "mount.cifs", in "mount" utility and in
"pam-mount" the ability to set a custom mount option separator. In
other words, we need to implement the already documented "sep" option.

1. For "mount.cifs" I did it in:
https://github.com/dmitry-telegin/cifs-utils in branch
"custom_option_separator". Commit:
https://github.com/dmitry-telegin/cifs-utils/commit/04325b842a82edf655a14174e763bc0b2a6870e1

2. For "mount" utility I did it in:
https://github.com/dmitry-telegin/util-linux in branch
"custom_option_separator". Commit:
https://github.com/dmitry-telegin/util-linux/commit/5e0ecd2498edae0bf0bcab4ba6a68a9803b34ccf

3. For "pam-mount" I did it in:
https://sourceforge.net/u/dmitry-t/pam-mount/ci/master/tree/ in branch
"custom_option_separator". Commit:
https://sourceforge.net/u/dmitry-t/pam-mount/ci/9860f9234977f1110230482b5d87bdcb8bc6ce03/

I checked the work on the Linux 5.10 kernel.

--
Dmitry Telegin

Re: A patch to implement the already documented "sep" option for the CIFS file system.

[Steve French]

makes sense.

Did anyone else review this yet?  (the mount.cifs version of the patch)

On Sat, Oct 8, 2022 at 2:41 PM Dmitry Telegin
<dmitry.s.telegin@gmail.com> wrote:
>
> DESCRIPTION OF THE PROBLEM
>
> Some users are accustomed to using shared folders in Windows with a
> comma in the name, for example: "//server3/Block 3,4".
> When they try to migrate to Linux, they cannot mount such paths.
>
> An example of the line generated by "mount.cifs" for the kernel when
> mounting "//server3/Block 3,4":
> "ip=10.0.2.212,unc=\\server3\Block 3,4,iocharset=utf8,user=user1,domain=AD"
> Accordingly, due to the extra comma, we have an error:
> "Sep 7 21:57:18 S1 kernel: [ 795.604821] CIFS: Unknown mount option "4""
>
>
> DOCUMENTATION
>
> https://www.kernel.org/doc/html/latest/admin-guide/cifs/usage.html
> The "sep" parameter is described here to specify a new delimiter
> instead of a comma.
> I quote:
>    "sep
>    if first mount option (after the -o), overrides the comma as the
> separator between the mount parms. e.g.:
>    -o user=myname,password=mypassword,domain=mydom
>    could be passed instead with period as the separator by:
>    -o sep=.user=myname.password=mypassword.domain=mydom
>    this might be useful when comma is contained within username or
> password or domain."
>
>
> RESEARCH WORK
>
> I looked at the "mount.cifs" code. There is no provision for the use
> of a comma by the user, since the comma is used to form the parameter
> string to the kernel (man 2 mount). This line can be seen by adding
> the "--verbose" flag to the mount.
> "mount.cifs --help" lists "sep" as a possible option, but does not
> implement it in the code and does not describe it in "man 8
> mount.cifs".
>
> I looked at the "pam-mount" code - the mount options are assembled
> with a wildcard comma. The result is a text line: "mount -t cifs ...".
>
> The handling of options in the "mount" utility is based on the
> "libmount" library, which is hardcoded to use only a comma as a
> delimiter.
>
> I tried to mount "//server3/Block 3,4" with my own program (man 2
> mount) by specifying "sep=!" - successfully.
>
>
> SOLUTION
>
> It would be nice if we add in "mount.cifs", in "mount" utility and in
> "pam-mount" the ability to set a custom mount option separator. In
> other words, we need to implement the already documented "sep" option.
>
> 1. For "mount.cifs" I did it in:
> https://github.com/dmitry-telegin/cifs-utils in branch
> "custom_option_separator". Commit:
> https://github.com/dmitry-telegin/cifs-utils/commit/04325b842a82edf655a14174e763bc0b2a6870e1
>
> 2. For "mount" utility I did it in:
> https://github.com/dmitry-telegin/util-linux in branch
> "custom_option_separator". Commit:
> https://github.com/dmitry-telegin/util-linux/commit/5e0ecd2498edae0bf0bcab4ba6a68a9803b34ccf
>
> 3. For "pam-mount" I did it in:
> https://sourceforge.net/u/dmitry-t/pam-mount/ci/master/tree/ in branch
> "custom_option_separator". Commit:
> https://sourceforge.net/u/dmitry-t/pam-mount/ci/9860f9234977f1110230482b5d87bdcb8bc6ce03/
>
> I checked the work on the Linux 5.10 kernel.
>
> --
> Dmitry Telegin



-- 
Thanks,

Steve

Re: A patch to implement the already documented "sep" option for the CIFS file system.

[Enzo Matsumiya]

On 10/11, Steve French wrote:
>makes sense.
>
>Did anyone else review this yet?  (the mount.cifs version of the patch)

At a glance, the patch seems ok and solves a real problem.

However, I think a better approach would be to parse the string in user
space, i.e. it's much easier for mount.cifs to fetch what's the
UNC/password string if they're passed quoted (shell handles it), and
then use 0x1E (ASCII Record Separator) instead of a comma.  Then, in
the kernel, we'd only need to strsep() by 0x1E.  Since 0x1E is a
non-printable ASCII character, I have a hard assumption it's not allowed
as UNC/password in most systems.  Also since it would be only used
internally between mount.cifs and cifs.ko, users would not need to know
nor care about it.

Thoughts?


Enzo

>On Sat, Oct 8, 2022 at 2:41 PM Dmitry Telegin
><dmitry.s.telegin@gmail.com> wrote:
>>
>> DESCRIPTION OF THE PROBLEM
>>
>> Some users are accustomed to using shared folders in Windows with a
>> comma in the name, for example: "//server3/Block 3,4".
>> When they try to migrate to Linux, they cannot mount such paths.
>>
>> An example of the line generated by "mount.cifs" for the kernel when
>> mounting "//server3/Block 3,4":
>> "ip=10.0.2.212,unc=\\server3\Block 3,4,iocharset=utf8,user=user1,domain=AD"
>> Accordingly, due to the extra comma, we have an error:
>> "Sep 7 21:57:18 S1 kernel: [ 795.604821] CIFS: Unknown mount option "4""
>>
>>
>> DOCUMENTATION
>>
>> https://www.kernel.org/doc/html/latest/admin-guide/cifs/usage.html
>> The "sep" parameter is described here to specify a new delimiter
>> instead of a comma.
>> I quote:
>>    "sep
>>    if first mount option (after the -o), overrides the comma as the
>> separator between the mount parms. e.g.:
>>    -o user=myname,password=mypassword,domain=mydom
>>    could be passed instead with period as the separator by:
>>    -o sep=.user=myname.password=mypassword.domain=mydom
>>    this might be useful when comma is contained within username or
>> password or domain."
>>
>>
>> RESEARCH WORK
>>
>> I looked at the "mount.cifs" code. There is no provision for the use
>> of a comma by the user, since the comma is used to form the parameter
>> string to the kernel (man 2 mount). This line can be seen by adding
>> the "--verbose" flag to the mount.
>> "mount.cifs --help" lists "sep" as a possible option, but does not
>> implement it in the code and does not describe it in "man 8
>> mount.cifs".
>>
>> I looked at the "pam-mount" code - the mount options are assembled
>> with a wildcard comma. The result is a text line: "mount -t cifs ...".
>>
>> The handling of options in the "mount" utility is based on the
>> "libmount" library, which is hardcoded to use only a comma as a
>> delimiter.
>>
>> I tried to mount "//server3/Block 3,4" with my own program (man 2
>> mount) by specifying "sep=!" - successfully.
>>
>>
>> SOLUTION
>>
>> It would be nice if we add in "mount.cifs", in "mount" utility and in
>> "pam-mount" the ability to set a custom mount option separator. In
>> other words, we need to implement the already documented "sep" option.
>>
>> 1. For "mount.cifs" I did it in:
>> https://github.com/dmitry-telegin/cifs-utils in branch
>> "custom_option_separator". Commit:
>> https://github.com/dmitry-telegin/cifs-utils/commit/04325b842a82edf655a14174e763bc0b2a6870e1
>>
>> 2. For "mount" utility I did it in:
>> https://github.com/dmitry-telegin/util-linux in branch
>> "custom_option_separator". Commit:
>> https://github.com/dmitry-telegin/util-linux/commit/5e0ecd2498edae0bf0bcab4ba6a68a9803b34ccf
>>
>> 3. For "pam-mount" I did it in:
>> https://sourceforge.net/u/dmitry-t/pam-mount/ci/master/tree/ in branch
>> "custom_option_separator". Commit:
>> https://sourceforge.net/u/dmitry-t/pam-mount/ci/9860f9234977f1110230482b5d87bdcb8bc6ce03/
>>
>> I checked the work on the Linux 5.10 kernel.
>>
>> --
>> Dmitry Telegin
>
>
>
>-- 
>Thanks,
>
>Steve

Re: A patch to implement the already documented "sep" option for the CIFS file system.

[Steve French]

All three approaches seem to parse it in user space, which makes
sense.  Easier to do it in mount.cifs than in kernel fs_context
parsing

On Tue, Oct 11, 2022 at 1:57 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> On 10/11, Steve French wrote:
> >makes sense.
> >
> >Did anyone else review this yet?  (the mount.cifs version of the patch)
>
> At a glance, the patch seems ok and solves a real problem.
>
> However, I think a better approach would be to parse the string in user
> space, i.e. it's much easier for mount.cifs to fetch what's the
> UNC/password string if they're passed quoted (shell handles it), and
> then use 0x1E (ASCII Record Separator) instead of a comma.  Then, in
> the kernel, we'd only need to strsep() by 0x1E.  Since 0x1E is a
> non-printable ASCII character, I have a hard assumption it's not allowed
> as UNC/password in most systems.  Also since it would be only used
> internally between mount.cifs and cifs.ko, users would not need to know
> nor care about it.
>
> Thoughts?
>
>
> Enzo
>
> >On Sat, Oct 8, 2022 at 2:41 PM Dmitry Telegin
> ><dmitry.s.telegin@gmail.com> wrote:
> >>
> >> DESCRIPTION OF THE PROBLEM
> >>
> >> Some users are accustomed to using shared folders in Windows with a
> >> comma in the name, for example: "//server3/Block 3,4".
> >> When they try to migrate to Linux, they cannot mount such paths.
> >>
> >> An example of the line generated by "mount.cifs" for the kernel when
> >> mounting "//server3/Block 3,4":
> >> "ip=10.0.2.212,unc=\\server3\Block 3,4,iocharset=utf8,user=user1,domain=AD"
> >> Accordingly, due to the extra comma, we have an error:
> >> "Sep 7 21:57:18 S1 kernel: [ 795.604821] CIFS: Unknown mount option "4""
> >>
> >>
> >> DOCUMENTATION
> >>
> >> https://www.kernel.org/doc/html/latest/admin-guide/cifs/usage.html
> >> The "sep" parameter is described here to specify a new delimiter
> >> instead of a comma.
> >> I quote:
> >>    "sep
> >>    if first mount option (after the -o), overrides the comma as the
> >> separator between the mount parms. e.g.:
> >>    -o user=myname,password=mypassword,domain=mydom
> >>    could be passed instead with period as the separator by:
> >>    -o sep=.user=myname.password=mypassword.domain=mydom
> >>    this might be useful when comma is contained within username or
> >> password or domain."
> >>
> >>
> >> RESEARCH WORK
> >>
> >> I looked at the "mount.cifs" code. There is no provision for the use
> >> of a comma by the user, since the comma is used to form the parameter
> >> string to the kernel (man 2 mount). This line can be seen by adding
> >> the "--verbose" flag to the mount.
> >> "mount.cifs --help" lists "sep" as a possible option, but does not
> >> implement it in the code and does not describe it in "man 8
> >> mount.cifs".
> >>
> >> I looked at the "pam-mount" code - the mount options are assembled
> >> with a wildcard comma. The result is a text line: "mount -t cifs ...".
> >>
> >> The handling of options in the "mount" utility is based on the
> >> "libmount" library, which is hardcoded to use only a comma as a
> >> delimiter.
> >>
> >> I tried to mount "//server3/Block 3,4" with my own program (man 2
> >> mount) by specifying "sep=!" - successfully.
> >>
> >>
> >> SOLUTION
> >>
> >> It would be nice if we add in "mount.cifs", in "mount" utility and in
> >> "pam-mount" the ability to set a custom mount option separator. In
> >> other words, we need to implement the already documented "sep" option.
> >>
> >> 1. For "mount.cifs" I did it in:
> >> https://github.com/dmitry-telegin/cifs-utils in branch
> >> "custom_option_separator". Commit:
> >> https://github.com/dmitry-telegin/cifs-utils/commit/04325b842a82edf655a14174e763bc0b2a6870e1
> >>
> >> 2. For "mount" utility I did it in:
> >> https://github.com/dmitry-telegin/util-linux in branch
> >> "custom_option_separator". Commit:
> >> https://github.com/dmitry-telegin/util-linux/commit/5e0ecd2498edae0bf0bcab4ba6a68a9803b34ccf
> >>
> >> 3. For "pam-mount" I did it in:
> >> https://sourceforge.net/u/dmitry-t/pam-mount/ci/master/tree/ in branch
> >> "custom_option_separator". Commit:
> >> https://sourceforge.net/u/dmitry-t/pam-mount/ci/9860f9234977f1110230482b5d87bdcb8bc6ce03/
> >>
> >> I checked the work on the Linux 5.10 kernel.
> >>
> >> --
> >> Dmitry Telegin
> >
> >
> >
> >--
> >Thanks,
> >
> >Steve



-- 
Thanks,

Steve

Re: A patch to implement the already documented "sep" option for the CIFS file system.

[Enzo Matsumiya]

On 10/11, Steve French wrote:
>All three approaches seem to parse it in user space, which makes
>sense.  Easier to do it in mount.cifs than in kernel fs_context
>parsing

Yes, I'm just saying that the original approach (having a 'sep=' option)
would require implementation in the kernel as well (it currently doesn't
exist).  If we just replace the comma by 0x1E in both mount.cifs and the
kernel, we don't need all this fiddling with custom separators.


Enzo

>On Tue, Oct 11, 2022 at 1:57 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>>
>> On 10/11, Steve French wrote:
>> >makes sense.
>> >
>> >Did anyone else review this yet?  (the mount.cifs version of the patch)
>>
>> At a glance, the patch seems ok and solves a real problem.
>>
>> However, I think a better approach would be to parse the string in user
>> space, i.e. it's much easier for mount.cifs to fetch what's the
>> UNC/password string if they're passed quoted (shell handles it), and
>> then use 0x1E (ASCII Record Separator) instead of a comma.  Then, in
>> the kernel, we'd only need to strsep() by 0x1E.  Since 0x1E is a
>> non-printable ASCII character, I have a hard assumption it's not allowed
>> as UNC/password in most systems.  Also since it would be only used
>> internally between mount.cifs and cifs.ko, users would not need to know
>> nor care about it.
>>
>> Thoughts?
>>
>>
>> Enzo
>>
>> >On Sat, Oct 8, 2022 at 2:41 PM Dmitry Telegin
>> ><dmitry.s.telegin@gmail.com> wrote:
>> >>
>> >> DESCRIPTION OF THE PROBLEM
>> >>
>> >> Some users are accustomed to using shared folders in Windows with a
>> >> comma in the name, for example: "//server3/Block 3,4".
>> >> When they try to migrate to Linux, they cannot mount such paths.
>> >>
>> >> An example of the line generated by "mount.cifs" for the kernel when
>> >> mounting "//server3/Block 3,4":
>> >> "ip=10.0.2.212,unc=\\server3\Block 3,4,iocharset=utf8,user=user1,domain=AD"
>> >> Accordingly, due to the extra comma, we have an error:
>> >> "Sep 7 21:57:18 S1 kernel: [ 795.604821] CIFS: Unknown mount option "4""
>> >>
>> >>
>> >> DOCUMENTATION
>> >>
>> >> https://www.kernel.org/doc/html/latest/admin-guide/cifs/usage.html
>> >> The "sep" parameter is described here to specify a new delimiter
>> >> instead of a comma.
>> >> I quote:
>> >>    "sep
>> >>    if first mount option (after the -o), overrides the comma as the
>> >> separator between the mount parms. e.g.:
>> >>    -o user=myname,password=mypassword,domain=mydom
>> >>    could be passed instead with period as the separator by:
>> >>    -o sep=.user=myname.password=mypassword.domain=mydom
>> >>    this might be useful when comma is contained within username or
>> >> password or domain."
>> >>
>> >>
>> >> RESEARCH WORK
>> >>
>> >> I looked at the "mount.cifs" code. There is no provision for the use
>> >> of a comma by the user, since the comma is used to form the parameter
>> >> string to the kernel (man 2 mount). This line can be seen by adding
>> >> the "--verbose" flag to the mount.
>> >> "mount.cifs --help" lists "sep" as a possible option, but does not
>> >> implement it in the code and does not describe it in "man 8
>> >> mount.cifs".
>> >>
>> >> I looked at the "pam-mount" code - the mount options are assembled
>> >> with a wildcard comma. The result is a text line: "mount -t cifs ...".
>> >>
>> >> The handling of options in the "mount" utility is based on the
>> >> "libmount" library, which is hardcoded to use only a comma as a
>> >> delimiter.
>> >>
>> >> I tried to mount "//server3/Block 3,4" with my own program (man 2
>> >> mount) by specifying "sep=!" - successfully.
>> >>
>> >>
>> >> SOLUTION
>> >>
>> >> It would be nice if we add in "mount.cifs", in "mount" utility and in
>> >> "pam-mount" the ability to set a custom mount option separator. In
>> >> other words, we need to implement the already documented "sep" option.
>> >>
>> >> 1. For "mount.cifs" I did it in:
>> >> https://github.com/dmitry-telegin/cifs-utils in branch
>> >> "custom_option_separator". Commit:
>> >> https://github.com/dmitry-telegin/cifs-utils/commit/04325b842a82edf655a14174e763bc0b2a6870e1
>> >>
>> >> 2. For "mount" utility I did it in:
>> >> https://github.com/dmitry-telegin/util-linux in branch
>> >> "custom_option_separator". Commit:
>> >> https://github.com/dmitry-telegin/util-linux/commit/5e0ecd2498edae0bf0bcab4ba6a68a9803b34ccf
>> >>
>> >> 3. For "pam-mount" I did it in:
>> >> https://sourceforge.net/u/dmitry-t/pam-mount/ci/master/tree/ in branch
>> >> "custom_option_separator". Commit:
>> >> https://sourceforge.net/u/dmitry-t/pam-mount/ci/9860f9234977f1110230482b5d87bdcb8bc6ce03/
>> >>
>> >> I checked the work on the Linux 5.10 kernel.
>> >>
>> >> --
>> >> Dmitry Telegin
>> >
>> >
>> >
>> >--
>> >Thanks,
>> >
>> >Steve
>
>
>
>-- 
>Thanks,
>
>Steve

Re: A patch to implement the already documented "sep" option for the CIFS file system.

[Enzo Matsumiya]

On 10/11, Enzo Matsumiya wrote:
>On 10/11, Steve French wrote:
>>All three approaches seem to parse it in user space, which makes
>>sense.  Easier to do it in mount.cifs than in kernel fs_context
>>parsing
>
>Yes, I'm just saying that the original approach (having a 'sep=' option)
>would require implementation in the kernel as well (it currently doesn't
>exist).  If we just replace the comma by 0x1E in both mount.cifs and the
>kernel, we don't need all this fiddling with custom separators.

Hastily hacked PoC patches follow.  Worked with my simple test:

   mount.cifs -o vers=3.1.1,sign,credentials=/root/sambacreds "//192.168.0.15/my, share" /mnt/samba


Enzo

Kernel patch:

--- a/fs/cifs/fs_context.c
+++ b/fs/cifs/fs_context.c
@@ -414,7 +414,7 @@ int smb3_parse_opt(const char *options, const char *key, char **val)
  	if (!opts)
  		return -ENOMEM;
  
-	while ((p = strsep(&opts, ","))) {
+	while ((p = strsep(&opts, "\x1e"))) {
  		char *nval;
  
  		if (!*p)
@@ -585,7 +585,7 @@ static int smb3_fs_context_parse_monolithic(struct fs_context *fc,
  		return ret;
  
  	/* BB Need to add support for sep= here TBD */
-	while ((key = strsep(&options, ",")) != NULL) {
+	while ((key = strsep(&options, "\x1e")) != NULL) {
  		size_t len;
  		char *value;


cifs-utils patch:
  
diff --git a/mount.cifs.c b/mount.cifs.c
index 2278995c9653..1d48e257a794 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -1195,7 +1195,7 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
  
  		/* go ahead and copy */
  		if (out_len)
-			strlcat(out, ",", MAX_OPTIONS_LEN);
+			strlcat(out, "\x1e", MAX_OPTIONS_LEN);
  
  		strlcat(out, data, MAX_OPTIONS_LEN);
  		out_len = strlen(out);
@@ -1215,7 +1215,7 @@ nocopy:
  		}
  
  		if (out_len) {
-			strlcat(out, ",", MAX_OPTIONS_LEN);
+			strlcat(out, "\x1e", MAX_OPTIONS_LEN);
  			out_len++;
  		}
  		snprintf(out + out_len, word_len + 5, "uid=%s", txtbuf);
@@ -1235,7 +1235,7 @@ nocopy:
  		}
  
  		if (out_len) {
-			strlcat(out, ",", MAX_OPTIONS_LEN);
+			strlcat(out, "\x1e", MAX_OPTIONS_LEN);
  			out_len++;
  		}
  		snprintf(out + out_len, word_len + 7, "cruid=%s", txtbuf);
@@ -1251,7 +1251,7 @@ nocopy:
  		}
  
  		if (out_len) {
-			strlcat(out, ",", MAX_OPTIONS_LEN);
+			strlcat(out, "\x1e", MAX_OPTIONS_LEN);
  			out_len++;
  		}
  		snprintf(out + out_len, word_len + 5, "gid=%s", txtbuf);
@@ -1267,7 +1267,7 @@ nocopy:
  		}
  
  		if (out_len) {
-			strlcat(out, ",", MAX_OPTIONS_LEN);
+			strlcat(out, "\x1e", MAX_OPTIONS_LEN);
  			out_len++;
  		}
  		snprintf(out + out_len, word_len + 11, "backupuid=%s", txtbuf);
@@ -1283,7 +1283,7 @@ nocopy:
  		}
  
  		if (out_len) {
-			strlcat(out, ",", MAX_OPTIONS_LEN);
+			strlcat(out, "\x1e", MAX_OPTIONS_LEN);
  			out_len++;
  		}
  		snprintf(out + out_len, word_len + 11, "backupgid=%s", txtbuf);
@@ -1299,7 +1299,7 @@ nocopy:
  		}
  
  		if (out_len) {
-			strlcat(out, ",", MAX_OPTIONS_LEN);
+			strlcat(out, "\x1e", MAX_OPTIONS_LEN);
  			out_len++;
  		}
  		snprintf(out + out_len, word_len + 10, "snapshot=%s", txtbuf);
@@ -1558,17 +1558,17 @@ add_mtab(char *devname, char *mountpoint, unsigned long flags, const char *fstyp
  			strlcat(mountent.mnt_opts, "rw", MTAB_OPTIONS_LEN);
  
  		if (flags & MS_MANDLOCK)
-			strlcat(mountent.mnt_opts, ",mand", MTAB_OPTIONS_LEN);
+			strlcat(mountent.mnt_opts, "\x1emand", MTAB_OPTIONS_LEN);
  		if (flags & MS_NOEXEC)
-			strlcat(mountent.mnt_opts, ",noexec", MTAB_OPTIONS_LEN);
+			strlcat(mountent.mnt_opts, "\x1enoexec", MTAB_OPTIONS_LEN);
  		if (flags & MS_NOSUID)
-			strlcat(mountent.mnt_opts, ",nosuid", MTAB_OPTIONS_LEN);
+			strlcat(mountent.mnt_opts, "\x1enosuid", MTAB_OPTIONS_LEN);
  		if (flags & MS_NODEV)
-			strlcat(mountent.mnt_opts, ",nodev", MTAB_OPTIONS_LEN);
+			strlcat(mountent.mnt_opts, "\x1enodev", MTAB_OPTIONS_LEN);
  		if (flags & MS_SYNCHRONOUS)
-			strlcat(mountent.mnt_opts, ",sync", MTAB_OPTIONS_LEN);
+			strlcat(mountent.mnt_opts, "\x1esync", MTAB_OPTIONS_LEN);
  		if (mount_user) {
-			strlcat(mountent.mnt_opts, ",user=", MTAB_OPTIONS_LEN);
+			strlcat(mountent.mnt_opts, "\x1euser=", MTAB_OPTIONS_LEN);
  			strlcat(mountent.mnt_opts, mount_user,
  				MTAB_OPTIONS_LEN);
  		}
@@ -1942,7 +1942,7 @@ assemble_mountinfo(struct parsed_mount_info *parsed_info,
  	/* copy in user= string */
  	if (parsed_info->got_user) {
  		if (*parsed_info->options)
-			strlcat(parsed_info->options, ",",
+			strlcat(parsed_info->options, "\x1e",
  				sizeof(parsed_info->options));
  		strlcat(parsed_info->options, "user=",
  			sizeof(parsed_info->options));
@@ -1952,14 +1952,14 @@ assemble_mountinfo(struct parsed_mount_info *parsed_info,
  
  	if (*parsed_info->domain) {
  		if (*parsed_info->options)
-			strlcat(parsed_info->options, ",",
+			strlcat(parsed_info->options, "\x1e",
  				sizeof(parsed_info->options));
  		strlcat(parsed_info->options, "domain=",
  			sizeof(parsed_info->options));
  		strlcat(parsed_info->options, parsed_info->domain,
  			sizeof(parsed_info->options));
  	} else if (parsed_info->got_domain) {
-		strlcat(parsed_info->options, ",domain=",
+		strlcat(parsed_info->options, "\x1e" "domain=",
  			sizeof(parsed_info->options));
  	}
  
@@ -2216,23 +2216,23 @@ mount_retry:
  	strlcpy(options, "ip=", options_size);
  	strlcat(options, currentaddress, options_size);
  
-	strlcat(options, ",unc=\\\\", options_size);
+	strlcat(options, "\x1eunc=\\\\", options_size);
  	strlcat(options, parsed_info->host, options_size);
  	strlcat(options, "\\", options_size);
  	strlcat(options, parsed_info->share, options_size);
  
  	if (*parsed_info->options) {
-		strlcat(options, ",", options_size);
+		strlcat(options, "\x1e", options_size);
  		strlcat(options, parsed_info->options, options_size);
  	}
  
  	if (*parsed_info->prefix) {
-		strlcat(options, ",prefixpath=", options_size);
+		strlcat(options, "\x1eprefixpath=", options_size);
  		strlcat(options, parsed_info->prefix, options_size);
  	}
  
  	if (sloppy)
-		strlcat(options, ",sloppy", options_size);
+		strlcat(options, "\x1esloppy", options_size);
  
  	if (parsed_info->verboseflag)
  		fprintf(stderr, "%s kernel mount options: %s",
@@ -2243,10 +2243,10 @@ mount_retry:
  		 * Commas have to be doubled, or else they will
  		 * look like the parameter separator
  		 */
-		strlcat(options, ",pass=", options_size);
+		strlcat(options, "\x1epass=", options_size);
  		strlcat(options, parsed_info->password, options_size);
  		if (parsed_info->verboseflag)
-			fprintf(stderr, ",pass=********");
+			fprintf(stderr, "\x1epass=********");
  	}
  
  	if (parsed_info->verboseflag)

Re: A patch to implement the already documented "sep" option for the CIFS file system.

[Leif Sahlberg]

On Wed, Oct 12, 2022 at 4:58 AM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> On 10/11, Steve French wrote:
> >makes sense.
> >
> >Did anyone else review this yet?  (the mount.cifs version of the patch)
>
> At a glance, the patch seems ok and solves a real problem.
>
> However, I think a better approach would be to parse the string in user
> space, i.e. it's much easier for mount.cifs to fetch what's the
> UNC/password string if they're passed quoted (shell handles it), and
> then use 0x1E (ASCII Record Separator) instead of a comma.  Then, in
> the kernel, we'd only need to strsep() by 0x1E.  Since 0x1E is a
> non-printable ASCII character, I have a hard assumption it's not allowed
> as UNC/password in most systems.  Also since it would be only used
> internally between mount.cifs and cifs.ko, users would not need to know
> nor care about it.
>
> Thoughts?

The trouble with changing to 0x1e is that that would create a
dependency between mount.cifs and the kernel.
And that we would need to upgrade, or downgrade, them in lockstep
which will cause a lot of pain.
Old version of mount.cifs can not talk to new version of kernel and v.v.

I think a better way to handle this would be to use an escape-character for ','.
(and remove all traces of "sep=")

>
>
> Enzo
>
> >On Sat, Oct 8, 2022 at 2:41 PM Dmitry Telegin
> ><dmitry.s.telegin@gmail.com> wrote:
> >>
> >> DESCRIPTION OF THE PROBLEM
> >>
> >> Some users are accustomed to using shared folders in Windows with a
> >> comma in the name, for example: "//server3/Block 3,4".
> >> When they try to migrate to Linux, they cannot mount such paths.
> >>
> >> An example of the line generated by "mount.cifs" for the kernel when
> >> mounting "//server3/Block 3,4":
> >> "ip=10.0.2.212,unc=\\server3\Block 3,4,iocharset=utf8,user=user1,domain=AD"
> >> Accordingly, due to the extra comma, we have an error:
> >> "Sep 7 21:57:18 S1 kernel: [ 795.604821] CIFS: Unknown mount option "4""
> >>
> >>
> >> DOCUMENTATION
> >>
> >> https://www.kernel.org/doc/html/latest/admin-guide/cifs/usage.html
> >> The "sep" parameter is described here to specify a new delimiter
> >> instead of a comma.
> >> I quote:
> >>    "sep
> >>    if first mount option (after the -o), overrides the comma as the
> >> separator between the mount parms. e.g.:
> >>    -o user=myname,password=mypassword,domain=mydom
> >>    could be passed instead with period as the separator by:
> >>    -o sep=.user=myname.password=mypassword.domain=mydom
> >>    this might be useful when comma is contained within username or
> >> password or domain."
> >>
> >>
> >> RESEARCH WORK
> >>
> >> I looked at the "mount.cifs" code. There is no provision for the use
> >> of a comma by the user, since the comma is used to form the parameter
> >> string to the kernel (man 2 mount). This line can be seen by adding
> >> the "--verbose" flag to the mount.
> >> "mount.cifs --help" lists "sep" as a possible option, but does not
> >> implement it in the code and does not describe it in "man 8
> >> mount.cifs".
> >>
> >> I looked at the "pam-mount" code - the mount options are assembled
> >> with a wildcard comma. The result is a text line: "mount -t cifs ...".
> >>
> >> The handling of options in the "mount" utility is based on the
> >> "libmount" library, which is hardcoded to use only a comma as a
> >> delimiter.
> >>
> >> I tried to mount "//server3/Block 3,4" with my own program (man 2
> >> mount) by specifying "sep=!" - successfully.
> >>
> >>
> >> SOLUTION
> >>
> >> It would be nice if we add in "mount.cifs", in "mount" utility and in
> >> "pam-mount" the ability to set a custom mount option separator. In
> >> other words, we need to implement the already documented "sep" option.
> >>
> >> 1. For "mount.cifs" I did it in:
> >> https://github.com/dmitry-telegin/cifs-utils in branch
> >> "custom_option_separator". Commit:
> >> https://github.com/dmitry-telegin/cifs-utils/commit/04325b842a82edf655a14174e763bc0b2a6870e1
> >>
> >> 2. For "mount" utility I did it in:
> >> https://github.com/dmitry-telegin/util-linux in branch
> >> "custom_option_separator". Commit:
> >> https://github.com/dmitry-telegin/util-linux/commit/5e0ecd2498edae0bf0bcab4ba6a68a9803b34ccf
> >>
> >> 3. For "pam-mount" I did it in:
> >> https://sourceforge.net/u/dmitry-t/pam-mount/ci/master/tree/ in branch
> >> "custom_option_separator". Commit:
> >> https://sourceforge.net/u/dmitry-t/pam-mount/ci/9860f9234977f1110230482b5d87bdcb8bc6ce03/
> >>
> >> I checked the work on the Linux 5.10 kernel.
> >>
> >> --
> >> Dmitry Telegin
> >
> >
> >
> >--
> >Thanks,
> >
> >Steve
>

Re: A patch to implement the already documented "sep" option for the CIFS file system.

[Leif Sahlberg]

On Wed, Oct 12, 2022 at 6:41 AM Leif Sahlberg <lsahlber@redhat.com> wrote:
>
> On Wed, Oct 12, 2022 at 4:58 AM Enzo Matsumiya <ematsumiya@suse.de> wrote:
> >
> > On 10/11, Steve French wrote:
> > >makes sense.
> > >
> > >Did anyone else review this yet?  (the mount.cifs version of the patch)
> >
> > At a glance, the patch seems ok and solves a real problem.
> >
> > However, I think a better approach would be to parse the string in user
> > space, i.e. it's much easier for mount.cifs to fetch what's the
> > UNC/password string if they're passed quoted (shell handles it), and
> > then use 0x1E (ASCII Record Separator) instead of a comma.  Then, in
> > the kernel, we'd only need to strsep() by 0x1E.  Since 0x1E is a
> > non-printable ASCII character, I have a hard assumption it's not allowed
> > as UNC/password in most systems.  Also since it would be only used
> > internally between mount.cifs and cifs.ko, users would not need to know
> > nor care about it.
> >
> > Thoughts?
>
> The trouble with changing to 0x1e is that that would create a
> dependency between mount.cifs and the kernel.
> And that we would need to upgrade, or downgrade, them in lockstep
> which will cause a lot of pain.
> Old version of mount.cifs can not talk to new version of kernel and v.v.
>
> I think a better way to handle this would be to use an escape-character for ','.
> (and remove all traces of "sep=")

A good escape character would possibly be something i the range 0xf8
to 0xff as these
byte values can not exist in utf8 encoded streams.


>
> >
> >
> > Enzo
> >
> > >On Sat, Oct 8, 2022 at 2:41 PM Dmitry Telegin
> > ><dmitry.s.telegin@gmail.com> wrote:
> > >>
> > >> DESCRIPTION OF THE PROBLEM
> > >>
> > >> Some users are accustomed to using shared folders in Windows with a
> > >> comma in the name, for example: "//server3/Block 3,4".
> > >> When they try to migrate to Linux, they cannot mount such paths.
> > >>
> > >> An example of the line generated by "mount.cifs" for the kernel when
> > >> mounting "//server3/Block 3,4":
> > >> "ip=10.0.2.212,unc=\\server3\Block 3,4,iocharset=utf8,user=user1,domain=AD"
> > >> Accordingly, due to the extra comma, we have an error:
> > >> "Sep 7 21:57:18 S1 kernel: [ 795.604821] CIFS: Unknown mount option "4""
> > >>
> > >>
> > >> DOCUMENTATION
> > >>
> > >> https://www.kernel.org/doc/html/latest/admin-guide/cifs/usage.html
> > >> The "sep" parameter is described here to specify a new delimiter
> > >> instead of a comma.
> > >> I quote:
> > >>    "sep
> > >>    if first mount option (after the -o), overrides the comma as the
> > >> separator between the mount parms. e.g.:
> > >>    -o user=myname,password=mypassword,domain=mydom
> > >>    could be passed instead with period as the separator by:
> > >>    -o sep=.user=myname.password=mypassword.domain=mydom
> > >>    this might be useful when comma is contained within username or
> > >> password or domain."
> > >>
> > >>
> > >> RESEARCH WORK
> > >>
> > >> I looked at the "mount.cifs" code. There is no provision for the use
> > >> of a comma by the user, since the comma is used to form the parameter
> > >> string to the kernel (man 2 mount). This line can be seen by adding
> > >> the "--verbose" flag to the mount.
> > >> "mount.cifs --help" lists "sep" as a possible option, but does not
> > >> implement it in the code and does not describe it in "man 8
> > >> mount.cifs".
> > >>
> > >> I looked at the "pam-mount" code - the mount options are assembled
> > >> with a wildcard comma. The result is a text line: "mount -t cifs ...".
> > >>
> > >> The handling of options in the "mount" utility is based on the
> > >> "libmount" library, which is hardcoded to use only a comma as a
> > >> delimiter.
> > >>
> > >> I tried to mount "//server3/Block 3,4" with my own program (man 2
> > >> mount) by specifying "sep=!" - successfully.
> > >>
> > >>
> > >> SOLUTION
> > >>
> > >> It would be nice if we add in "mount.cifs", in "mount" utility and in
> > >> "pam-mount" the ability to set a custom mount option separator. In
> > >> other words, we need to implement the already documented "sep" option.
> > >>
> > >> 1. For "mount.cifs" I did it in:
> > >> https://github.com/dmitry-telegin/cifs-utils in branch
> > >> "custom_option_separator". Commit:
> > >> https://github.com/dmitry-telegin/cifs-utils/commit/04325b842a82edf655a14174e763bc0b2a6870e1
> > >>
> > >> 2. For "mount" utility I did it in:
> > >> https://github.com/dmitry-telegin/util-linux in branch
> > >> "custom_option_separator". Commit:
> > >> https://github.com/dmitry-telegin/util-linux/commit/5e0ecd2498edae0bf0bcab4ba6a68a9803b34ccf
> > >>
> > >> 3. For "pam-mount" I did it in:
> > >> https://sourceforge.net/u/dmitry-t/pam-mount/ci/master/tree/ in branch
> > >> "custom_option_separator". Commit:
> > >> https://sourceforge.net/u/dmitry-t/pam-mount/ci/9860f9234977f1110230482b5d87bdcb8bc6ce03/
> > >>
> > >> I checked the work on the Linux 5.10 kernel.
> > >>
> > >> --
> > >> Dmitry Telegin
> > >
> > >
> > >
> > >--
> > >Thanks,
> > >
> > >Steve
> >

Re: A patch to implement the already documented "sep" option for the CIFS file system.

[Dmitry Telegin]

> Yes, I'm just saying that the original approach (having a 'sep=' option)
> would require implementation in the kernel as well (it currently doesn't
> exist).

I tested the solution I proposed on the kernels 4.19 and 5.10.
"sep=" is successfully recognized on these Linux kernels.
Did the new kernel remove support for this option?

About escape sequences.
Please pay attention to the thoughts of the authors of "pam-mount" and
"util-linux" in order to get a complete solution:
https://sourceforge.net/p/pam-mount/pam-mount/merge-requests/7/
https://github.com/util-linux/util-linux/pull/1836

вт, 11 окт. 2022 г. в 23:21, Enzo Matsumiya <ematsumiya@suse.de>:
>
> On 10/11, Steve French wrote:
> >All three approaches seem to parse it in user space, which makes
> >sense.  Easier to do it in mount.cifs than in kernel fs_context
> >parsing
>
> Yes, I'm just saying that the original approach (having a 'sep=' option)
> would require implementation in the kernel as well (it currently doesn't
> exist).  If we just replace the comma by 0x1E in both mount.cifs and the
> kernel, we don't need all this fiddling with custom separators.
>
>
> Enzo
>
> >On Tue, Oct 11, 2022 at 1:57 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
> >>
> >> On 10/11, Steve French wrote:
> >> >makes sense.
> >> >
> >> >Did anyone else review this yet?  (the mount.cifs version of the patch)
> >>
> >> At a glance, the patch seems ok and solves a real problem.
> >>
> >> However, I think a better approach would be to parse the string in user
> >> space, i.e. it's much easier for mount.cifs to fetch what's the
> >> UNC/password string if they're passed quoted (shell handles it), and
> >> then use 0x1E (ASCII Record Separator) instead of a comma.  Then, in
> >> the kernel, we'd only need to strsep() by 0x1E.  Since 0x1E is a
> >> non-printable ASCII character, I have a hard assumption it's not allowed
> >> as UNC/password in most systems.  Also since it would be only used
> >> internally between mount.cifs and cifs.ko, users would not need to know
> >> nor care about it.
> >>
> >> Thoughts?
> >>
> >>
> >> Enzo
> >>
> >> >On Sat, Oct 8, 2022 at 2:41 PM Dmitry Telegin
> >> ><dmitry.s.telegin@gmail.com> wrote:
> >> >>
> >> >> DESCRIPTION OF THE PROBLEM
> >> >>
> >> >> Some users are accustomed to using shared folders in Windows with a
> >> >> comma in the name, for example: "//server3/Block 3,4".
> >> >> When they try to migrate to Linux, they cannot mount such paths.
> >> >>
> >> >> An example of the line generated by "mount.cifs" for the kernel when
> >> >> mounting "//server3/Block 3,4":
> >> >> "ip=10.0.2.212,unc=\\server3\Block 3,4,iocharset=utf8,user=user1,domain=AD"
> >> >> Accordingly, due to the extra comma, we have an error:
> >> >> "Sep 7 21:57:18 S1 kernel: [ 795.604821] CIFS: Unknown mount option "4""
> >> >>
> >> >>
> >> >> DOCUMENTATION
> >> >>
> >> >> https://www.kernel.org/doc/html/latest/admin-guide/cifs/usage.html
> >> >> The "sep" parameter is described here to specify a new delimiter
> >> >> instead of a comma.
> >> >> I quote:
> >> >>    "sep
> >> >>    if first mount option (after the -o), overrides the comma as the
> >> >> separator between the mount parms. e.g.:
> >> >>    -o user=myname,password=mypassword,domain=mydom
> >> >>    could be passed instead with period as the separator by:
> >> >>    -o sep=.user=myname.password=mypassword.domain=mydom
> >> >>    this might be useful when comma is contained within username or
> >> >> password or domain."
> >> >>
> >> >>
> >> >> RESEARCH WORK
> >> >>
> >> >> I looked at the "mount.cifs" code. There is no provision for the use
> >> >> of a comma by the user, since the comma is used to form the parameter
> >> >> string to the kernel (man 2 mount). This line can be seen by adding
> >> >> the "--verbose" flag to the mount.
> >> >> "mount.cifs --help" lists "sep" as a possible option, but does not
> >> >> implement it in the code and does not describe it in "man 8
> >> >> mount.cifs".
> >> >>
> >> >> I looked at the "pam-mount" code - the mount options are assembled
> >> >> with a wildcard comma. The result is a text line: "mount -t cifs ...".
> >> >>
> >> >> The handling of options in the "mount" utility is based on the
> >> >> "libmount" library, which is hardcoded to use only a comma as a
> >> >> delimiter.
> >> >>
> >> >> I tried to mount "//server3/Block 3,4" with my own program (man 2
> >> >> mount) by specifying "sep=!" - successfully.
> >> >>
> >> >>
> >> >> SOLUTION
> >> >>
> >> >> It would be nice if we add in "mount.cifs", in "mount" utility and in
> >> >> "pam-mount" the ability to set a custom mount option separator. In
> >> >> other words, we need to implement the already documented "sep" option.
> >> >>
> >> >> 1. For "mount.cifs" I did it in:
> >> >> https://github.com/dmitry-telegin/cifs-utils in branch
> >> >> "custom_option_separator". Commit:
> >> >> https://github.com/dmitry-telegin/cifs-utils/commit/04325b842a82edf655a14174e763bc0b2a6870e1
> >> >>
> >> >> 2. For "mount" utility I did it in:
> >> >> https://github.com/dmitry-telegin/util-linux in branch
> >> >> "custom_option_separator". Commit:
> >> >> https://github.com/dmitry-telegin/util-linux/commit/5e0ecd2498edae0bf0bcab4ba6a68a9803b34ccf
> >> >>
> >> >> 3. For "pam-mount" I did it in:
> >> >> https://sourceforge.net/u/dmitry-t/pam-mount/ci/master/tree/ in branch
> >> >> "custom_option_separator". Commit:
> >> >> https://sourceforge.net/u/dmitry-t/pam-mount/ci/9860f9234977f1110230482b5d87bdcb8bc6ce03/
> >> >>
> >> >> I checked the work on the Linux 5.10 kernel.
> >> >>
> >> >> --
> >> >> Dmitry Telegin
> >> >
> >> >
> >> >
> >> >--
> >> >Thanks,
> >> >
> >> >Steve
> >
> >
> >
> >--
> >Thanks,
> >
> >Steve

--
Dmitry Telegin