[PATCH] Convert slashes in the UNC parameter to backslashes

[PATCH] Convert slashes in the UNC parameter to backslashes

[Federico Sauter]

This patch ensures that slashes used as separators in the
UNC are properly converted to backslashes. The existing
implementation did not perform that conversion and that
lead to a "invalid argument" error when specifying an UNC
explicitly which contained slashes in it.

---
  fs/cifs/connect.c |   12 ++++++------
  1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 5c670b9..8f4c76f 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1108,6 +1108,7 @@ cifs_parse_mount_options(const char *mountdata, 
const char *devname,
  	char *string = NULL;
  	char *tmp_end, *value;
  	char delim;
+	char *p, *q;

  	separator[0] = ',';
  	separator[1] = 0;
@@ -1573,12 +1574,11 @@ cifs_parse_mount_options(const char *mountdata, 
const char *devname,
  				printk(KERN_WARNING "CIFS: no memory for UNC\n");
  				goto cifs_parse_mount_err;
  			}
-			strcpy(vol->UNC, string);
-
-			if (strncmp(string, "//", 2) == 0) {
-				vol->UNC[0] = '\\';
-				vol->UNC[1] = '\\';
-			} else if (strncmp(string, "\\\\", 2) != 0) {
+			for (p = string, q = vol->UNC; *p; ++p, ++q) {
+				*q = *p == '/'? '\\' : *p;
+			}
+			*q = '\0';
+			if (strncmp(vol->UNC, "\\\\", 2) != 0) {
  				printk(KERN_WARNING "CIFS: UNC Path does not "
  						    "begin with // or \\\\\n");
  				goto cifs_parse_mount_err;
-- 
1.7.2.5

Re: [PATCH] Convert slashes in the UNC parameter to backslashes

[Jeff Layton]

On Tue, 06 Nov 2012 13:29:47 +0100
Federico Sauter <fsauter-LVkJPw3T+odGBRGhe+f61g@public.gmane.org> wrote:

> This patch ensures that slashes used as separators in the
> UNC are properly converted to backslashes. The existing
> implementation did not perform that conversion and that
> lead to a "invalid argument" error when specifying an UNC
> explicitly which contained slashes in it.
> 
> ---
>   fs/cifs/connect.c |   12 ++++++------
>   1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 5c670b9..8f4c76f 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -1108,6 +1108,7 @@ cifs_parse_mount_options(const char *mountdata, 
> const char *devname,
>   	char *string = NULL;
>   	char *tmp_end, *value;
>   	char delim;
> +	char *p, *q;
> 
>   	separator[0] = ',';
>   	separator[1] = 0;
> @@ -1573,12 +1574,11 @@ cifs_parse_mount_options(const char *mountdata, 
> const char *devname,
>   				printk(KERN_WARNING "CIFS: no memory for UNC\n");
>   				goto cifs_parse_mount_err;
>   			}
> -			strcpy(vol->UNC, string);
> -
> -			if (strncmp(string, "//", 2) == 0) {
> -				vol->UNC[0] = '\\';
> -				vol->UNC[1] = '\\';
> -			} else if (strncmp(string, "\\\\", 2) != 0) {
> +			for (p = string, q = vol->UNC; *p; ++p, ++q) {
> +				*q = *p == '/'? '\\' : *p;
> +			}
> +			*q = '\0';
> +			if (strncmp(vol->UNC, "\\\\", 2) != 0) { 
>   				printk(KERN_WARNING "CIFS: UNC Path does not "
>   						    "begin with // or \\\\\n");
>   				goto cifs_parse_mount_err;



Looks harmless enough, but it would be clearer to use the
convert_delimiter() helper function here, rather than open-coding that
functionality.

Long term, it would be good to get rid of the unc= and prefixpath=
options altogether and simply have the kernel extract this information
from the device name at mount time instead.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] Convert slashes in the UNC parameter to backslashes

[Federico Sauter]

Here the corrected (and tested) version.

Jeff: I agree with you regarding removing the UNC parameter altogether. 
Is there any scenario in which an UNC parameter could differ from the 
specified source parameter?

Regards,
Fred



diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 5c670b9..c52faf8 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1574,11 +1574,9 @@ cifs_parse_mount_options(const char *mountdata, 
const char *devname,
                                 goto cifs_parse_mount_err;
                         }
                         strcpy(vol->UNC, string);
+                       convert_delimiter(vol->UNC, '\\');

-                       if (strncmp(string, "//", 2) == 0) {
-                               vol->UNC[0] = '\\';
-                               vol->UNC[1] = '\\';
-                       } else if (strncmp(string, "\\\\", 2) != 0) {
+                       if (strncmp(vol->UNC, "\\\\", 2) != 0) {
                                 printk(KERN_WARNING "CIFS: UNC Path 
does not "
                                                     "begin with // or 
\\\\\n");
                                 goto cifs_parse_mount_err;




On 11/07/2012 01:02 PM, Jeff Layton wrote:
> On Tue, 06 Nov 2012 13:29:47 +0100
> Federico Sauter <fsauter-LVkJPw3T+odGBRGhe+f61g@public.gmane.org> wrote:
>
>> This patch ensures that slashes used as separators in the
>> UNC are properly converted to backslashes. The existing
>> implementation did not perform that conversion and that
>> lead to a "invalid argument" error when specifying an UNC
>> explicitly which contained slashes in it.
>>
>> ---
>>    fs/cifs/connect.c |   12 ++++++------
>>    1 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index 5c670b9..8f4c76f 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -1108,6 +1108,7 @@ cifs_parse_mount_options(const char *mountdata,
>> const char *devname,
>>    	char *string = NULL;
>>    	char *tmp_end, *value;
>>    	char delim;
>> +	char *p, *q;
>>
>>    	separator[0] = ',';
>>    	separator[1] = 0;
>> @@ -1573,12 +1574,11 @@ cifs_parse_mount_options(const char *mountdata,
>> const char *devname,
>>    				printk(KERN_WARNING "CIFS: no memory for UNC\n");
>>    				goto cifs_parse_mount_err;
>>    			}
>> -			strcpy(vol->UNC, string);
>> -
>> -			if (strncmp(string, "//", 2) == 0) {
>> -				vol->UNC[0] = '\\';
>> -				vol->UNC[1] = '\\';
>> -			} else if (strncmp(string, "\\\\", 2) != 0) {
>> +			for (p = string, q = vol->UNC; *p; ++p, ++q) {
>> +				*q = *p == '/'? '\\' : *p;
>> +			}
>> +			*q = '\0';
>> +			if (strncmp(vol->UNC, "\\\\", 2) != 0) {
>>    				printk(KERN_WARNING "CIFS: UNC Path does not "
>>    						    "begin with // or \\\\\n");
>>    				goto cifs_parse_mount_err;
>
>
>
> Looks harmless enough, but it would be clearer to use the
> convert_delimiter() helper function here, rather than open-coding that
> functionality.
>
> Long term, it would be good to get rid of the unc= and prefixpath=
> options altogether and simply have the kernel extract this information
> from the device name at mount time instead.
>

Re: [PATCH] Convert slashes in the UNC parameter to backslashes

[Jeff Layton]

On Wed, 07 Nov 2012 16:20:22 +0100
Federico Sauter <fsauter-LVkJPw3T+odGBRGhe+f61g@public.gmane.org> wrote:

> Here the corrected (and tested) version.
> 
> Jeff: I agree with you regarding removing the UNC parameter altogether. 
> Is there any scenario in which an UNC parameter could differ from the 
> specified source parameter?
> 
> Regards,
> Fred
> 

I suppose someone could provide their own unc= option that differs from
the device name. At that point, the device name in /proc/mounts (and in
programs like "df") might look like you've mounted one location when in
reality you've mounted another.

That doesn't seem like a valid use-case to me though. It's probably
best that we discourage such abuse ;).

The tricky part is how to get there from here. The mount helper
provides this option pretty much unconditionally, though it generally
generates it from the device string. Maybe something like this?

1) fix the kernel to parse a copy of the UNC out of the device string

2) compare that UNC to the one provided by the unc= option

3) if they differ, then printk a warning that mentions that there is a
discrepancy and that the kernel is going to use the value of the unc=
option for now, but that we'll change that default in two releases.

Something like:

"CIFS VFS: the unc= mount option differs from the device passed in. Using
the value of the unc= option. The kernel will begin preferring the value
in the device string in 3.10!"

Then, have a patch ready to go for the 3.10 merge window that will make
the kernel start ignoring the value of the unc= option. You might even
be nice and keep the comparison and warning around for a little while,
but switch it to mention that the kernel is going to ignore the value
of the unc= option when there is a discrepancy.

In another few releases, we can then get rid of the warning and
comparison too.

Sound reasonable?

> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 5c670b9..c52faf8 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -1574,11 +1574,9 @@ cifs_parse_mount_options(const char *mountdata, 
> const char *devname,
>                                  goto cifs_parse_mount_err;
>                          }
>                          strcpy(vol->UNC, string);
> +                       convert_delimiter(vol->UNC, '\\');
> 
> -                       if (strncmp(string, "//", 2) == 0) {
> -                               vol->UNC[0] = '\\';
> -                               vol->UNC[1] = '\\';
> -                       } else if (strncmp(string, "\\\\", 2) != 0) {
> +                       if (strncmp(vol->UNC, "\\\\", 2) != 0) {
>                                  printk(KERN_WARNING "CIFS: UNC Path 
> does not "
>                                                      "begin with // or 
> \\\\\n");
>                                  goto cifs_parse_mount_err;
> 
> 
> 

Looks reasonable. You can add my Reviewed-by. You should probably
resend it with a real "Signed-off-by:" line.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] Convert slashes in the UNC parameter to backslashes

[Scott Lovenberg]

On Wed, Nov 7, 2012 at 10:48 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Wed, 07 Nov 2012 16:20:22 +0100
> Federico Sauter <fsauter-LVkJPw3T+odGBRGhe+f61g@public.gmane.org> wrote:
>
>> Here the corrected (and tested) version.
>>
>> Jeff: I agree with you regarding removing the UNC parameter altogether.
>> Is there any scenario in which an UNC parameter could differ from the
>> specified source parameter?
>>
>> Regards,
>> Fred
>>
>
> I suppose someone could provide their own unc= option that differs from
> the device name. At that point, the device name in /proc/mounts (and in
> programs like "df") might look like you've mounted one location when in
> reality you've mounted another.
>
> That doesn't seem like a valid use-case to me though. It's probably
> best that we discourage such abuse ;).

Would this ever be the case when using a DFS mount?  I'm not sure how
that's handled internally to the kernel (I know Samba doesn't provide
support for multiple servers on a DFS root, but Windows does, IIRC).
So far as I can tell, in the simple case Samba is a proxy, in the
complex case, the share is on another server and the original server
is down.  Does the kernel use the same device in both cases?

>
> The tricky part is how to get there from here. The mount helper
> provides this option pretty much unconditionally, though it generally
> generates it from the device string. Maybe something like this?
>
> 1) fix the kernel to parse a copy of the UNC out of the device string
>
> 2) compare that UNC to the one provided by the unc= option
>
> 3) if they differ, then printk a warning that mentions that there is a
> discrepancy and that the kernel is going to use the value of the unc=
> option for now, but that we'll change that default in two releases.
>
> Something like:
>
> "CIFS VFS: the unc= mount option differs from the device passed in. Using
> the value of the unc= option. The kernel will begin preferring the value
> in the device string in 3.10!"
>
> Then, have a patch ready to go for the 3.10 merge window that will make
> the kernel start ignoring the value of the unc= option. You might even
> be nice and keep the comparison and warning around for a little while,
> but switch it to mention that the kernel is going to ignore the value
> of the unc= option when there is a discrepancy.
>
> In another few releases, we can then get rid of the warning and
> comparison too.
>
> Sound reasonable?
>

Would it be reasonable to deprecate the "UNC" / "path" / "target"
options (they all resolve to OPT_UNC from parse_opt_token()) in
mount.cifs if the kernel is going to do the heavy lifting?  That would
break compatibility between mount.cifs and older kernels, though.  I
guess it doesn't hurt anything to keep passing UNC even if it's not
being used, other than users not understanding the unexplained
behavior of the kernel deciding what the actual value of UNC is and
disregarding their explicit setting of it.  After typing that out, it
sounds kind of bad.
-- 
Peace and Blessings,
-Scott.

Re: [PATCH] Convert slashes in the UNC parameter to backslashes

[Jeff Layton]

On Wed, 7 Nov 2012 11:42:12 -0500
Scott Lovenberg <scott.lovenberg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Wed, Nov 7, 2012 at 10:48 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Wed, 07 Nov 2012 16:20:22 +0100
> > Federico Sauter <fsauter-LVkJPw3T+odGBRGhe+f61g@public.gmane.org> wrote:
> >
> >> Here the corrected (and tested) version.
> >>
> >> Jeff: I agree with you regarding removing the UNC parameter altogether.
> >> Is there any scenario in which an UNC parameter could differ from the
> >> specified source parameter?
> >>
> >> Regards,
> >> Fred
> >>
> >
> > I suppose someone could provide their own unc= option that differs from
> > the device name. At that point, the device name in /proc/mounts (and in
> > programs like "df") might look like you've mounted one location when in
> > reality you've mounted another.
> >
> > That doesn't seem like a valid use-case to me though. It's probably
> > best that we discourage such abuse ;).
> 
> Would this ever be the case when using a DFS mount? 

No.

> I'm not sure how
> that's handled internally to the kernel (I know Samba doesn't provide
> support for multiple servers on a DFS root, but Windows does, IIRC).
> So far as I can tell, in the simple case Samba is a proxy, in the
> complex case, the share is on another server and the original server
> is down.  Does the kernel use the same device in both cases?
> 

I'm not sure what you're asking. Referrals are chased at mount time, or
whenever you wander over them. If there are multiple possible
destinations, I think it just chooses one.

> >
> > The tricky part is how to get there from here. The mount helper
> > provides this option pretty much unconditionally, though it generally
> > generates it from the device string. Maybe something like this?
> >
> > 1) fix the kernel to parse a copy of the UNC out of the device string
> >
> > 2) compare that UNC to the one provided by the unc= option
> >
> > 3) if they differ, then printk a warning that mentions that there is a
> > discrepancy and that the kernel is going to use the value of the unc=
> > option for now, but that we'll change that default in two releases.
> >
> > Something like:
> >
> > "CIFS VFS: the unc= mount option differs from the device passed in. Using
> > the value of the unc= option. The kernel will begin preferring the value
> > in the device string in 3.10!"
> >
> > Then, have a patch ready to go for the 3.10 merge window that will make
> > the kernel start ignoring the value of the unc= option. You might even
> > be nice and keep the comparison and warning around for a little while,
> > but switch it to mention that the kernel is going to ignore the value
> > of the unc= option when there is a discrepancy.
> >
> > In another few releases, we can then get rid of the warning and
> > comparison too.
> >
> > Sound reasonable?
> >
> 
> Would it be reasonable to deprecate the "UNC" / "path" / "target"
> options (they all resolve to OPT_UNC from parse_opt_token()) in
> mount.cifs if the kernel is going to do the heavy lifting?  That would
> break compatibility between mount.cifs and older kernels, though.  I
> guess it doesn't hurt anything to keep passing UNC even if it's not
> being used, other than users not understanding the unexplained
> behavior of the kernel deciding what the actual value of UNC is and
> disregarding their explicit setting of it.  After typing that out, it
> sounds kind of bad.

Eventually, yes...we'd stop the mount helper from providing a unc=
option as well. That said, we'll have to contend with the old kernel +
new userspace problem for quite a while, so that can't happen at the
same time.

The first step is to get the kernel to where it doesn't actually need
the option. Once that's done we can formulate a plan to stop providing
it.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] Convert slashes in the UNC parameter to backslashes

[Federico Sauter]

On 11/07/2012 07:21 PM, Jeff Layton wrote:
>>>
>>> The tricky part is how to get there from here. The mount helper
>>> provides this option pretty much unconditionally, though it generally
>>> generates it from the device string. Maybe something like this?
>>>
>>> 1) fix the kernel to parse a copy of the UNC out of the device string
>>>
>>> 2) compare that UNC to the one provided by the unc= option
>>>
>>> 3) if they differ, then printk a warning that mentions that there is a
>>> discrepancy and that the kernel is going to use the value of the unc=
>>> option for now, but that we'll change that default in two releases.
>>>
>>> Something like:
>>>
>>> "CIFS VFS: the unc= mount option differs from the device passed in. Using
>>> the value of the unc= option. The kernel will begin preferring the value
>>> in the device string in 3.10!"
>>>
>>> Then, have a patch ready to go for the 3.10 merge window that will make
>>> the kernel start ignoring the value of the unc= option. You might even
>>> be nice and keep the comparison and warning around for a little while,
>>> but switch it to mention that the kernel is going to ignore the value
>>> of the unc= option when there is a discrepancy.
>>>
>>> In another few releases, we can then get rid of the warning and
>>> comparison too.
>>>
>>> Sound reasonable?
>>>
>>
>> Would it be reasonable to deprecate the "UNC" / "path" / "target"
>> options (they all resolve to OPT_UNC from parse_opt_token()) in
>> mount.cifs if the kernel is going to do the heavy lifting?  That would
>> break compatibility between mount.cifs and older kernels, though.  I
>> guess it doesn't hurt anything to keep passing UNC even if it's not
>> being used, other than users not understanding the unexplained
>> behavior of the kernel deciding what the actual value of UNC is and
>> disregarding their explicit setting of it.  After typing that out, it
>> sounds kind of bad.
>
> Eventually, yes...we'd stop the mount helper from providing a unc=
> option as well. That said, we'll have to contend with the old kernel +
> new userspace problem for quite a while, so that can't happen at the
> same time.
>
> The first step is to get the kernel to where it doesn't actually need
> the option. Once that's done we can formulate a plan to stop providing
> it.
>

I came across this issue precisely because I was using a mount helper 
different than samba (busybox, to be precise.) Thus, the unc parameter 
was being passed in a slightly different way and thus an error message 
resulted. This is to say that, if we radically changed the 
unc/path/target options interface, it would most likely break existing 
mount implementations.

I suggest that we just copy the device name as UNC in the parse_options 
function and ignore whatever the user had provided explicitly without 
issuing any warnings. I personally like warnings, but the current 
situation is that this option is implicitly passed by the mount helper, 
and thus it would most likely confuse users more than it helps them.

Please let me know what you think. I can quickly provide a tested patch, 
if you agree to the course of action.


Best regards,


-- 
Federico Sauter / Senior firmware programmer
Innominate Security Technologies AG / protecting industrial networks
tel: +49.30.921028-210 / fax: +49.30.921028-020
Rudower Chaussee 13 / D-12489 Berlin / http://www.innominate.com/

Register Court: AG Charlottenburg, HR B 81603
Management Board: Dirk Seewald
Chairman of the Supervisory Board: Christoph Leifer

Re: [PATCH] Convert slashes in the UNC parameter to backslashes

[Jeff Layton]

On Thu, 08 Nov 2012 13:16:25 +0100
Federico Sauter <fsauter-LVkJPw3T+odGBRGhe+f61g@public.gmane.org> wrote:

> 
> 
> On 11/07/2012 07:21 PM, Jeff Layton wrote:
> >>>
> >>> The tricky part is how to get there from here. The mount helper
> >>> provides this option pretty much unconditionally, though it generally
> >>> generates it from the device string. Maybe something like this?
> >>>
> >>> 1) fix the kernel to parse a copy of the UNC out of the device string
> >>>
> >>> 2) compare that UNC to the one provided by the unc= option
> >>>
> >>> 3) if they differ, then printk a warning that mentions that there is a
> >>> discrepancy and that the kernel is going to use the value of the unc=
> >>> option for now, but that we'll change that default in two releases.
> >>>
> >>> Something like:
> >>>
> >>> "CIFS VFS: the unc= mount option differs from the device passed in. Using
> >>> the value of the unc= option. The kernel will begin preferring the value
> >>> in the device string in 3.10!"
> >>>
> >>> Then, have a patch ready to go for the 3.10 merge window that will make
> >>> the kernel start ignoring the value of the unc= option. You might even
> >>> be nice and keep the comparison and warning around for a little while,
> >>> but switch it to mention that the kernel is going to ignore the value
> >>> of the unc= option when there is a discrepancy.
> >>>
> >>> In another few releases, we can then get rid of the warning and
> >>> comparison too.
> >>>
> >>> Sound reasonable?
> >>>
> >>
> >> Would it be reasonable to deprecate the "UNC" / "path" / "target"
> >> options (they all resolve to OPT_UNC from parse_opt_token()) in
> >> mount.cifs if the kernel is going to do the heavy lifting?  That would
> >> break compatibility between mount.cifs and older kernels, though.  I
> >> guess it doesn't hurt anything to keep passing UNC even if it's not
> >> being used, other than users not understanding the unexplained
> >> behavior of the kernel deciding what the actual value of UNC is and
> >> disregarding their explicit setting of it.  After typing that out, it
> >> sounds kind of bad.
> >
> > Eventually, yes...we'd stop the mount helper from providing a unc=
> > option as well. That said, we'll have to contend with the old kernel +
> > new userspace problem for quite a while, so that can't happen at the
> > same time.
> >
> > The first step is to get the kernel to where it doesn't actually need
> > the option. Once that's done we can formulate a plan to stop providing
> > it.
> >
> 
> I came across this issue precisely because I was using a mount helper 
> different than samba (busybox, to be precise.) Thus, the unc parameter 
> was being passed in a slightly different way and thus an error message 
> resulted. This is to say that, if we radically changed the 
> unc/path/target options interface, it would most likely break existing 
> mount implementations.
> 
> I suggest that we just copy the device name as UNC in the parse_options 
> function and ignore whatever the user had provided explicitly without 
> issuing any warnings. I personally like warnings, but the current 
> situation is that this option is implicitly passed by the mount helper, 
> and thus it would most likely confuse users more than it helps them.
> 
> Please let me know what you think. I can quickly provide a tested patch, 
> if you agree to the course of action.
> 

I think we need to be extra cautious with any change in behavior of a
userland interface.

In practice the mount helper assembles the unc= option from the content
of the device string. If someone is using the standard mount.cifs
helper, then it's not likely they'll ever see this warning (assuming we
code up the logic correctly of course).

In the unlikely case that someone is passing in a unc= option that does
not match the device string, then I think the responsible thing is to
warn them that they can expect the behavior to change in the future.

The custom in kernel changes is to warn for two releases before
changing the behavior of a userland interface. If you have the patches
to give the warning ready to go for 3.8, then we can change that
behavior in 3.10

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>