[PATCH] mount.cifs: don't send a mandatory ver= option to the kernel

[PATCH] mount.cifs: don't send a mandatory ver= option to the kernel

[Jeff Layton]

Traditionally, this ver= option was used to specify the "options
version" that we're passing in. It has always been set to '1' though
and we have never changed that.

Eventually we want to have a ver= (or vers=) option that allows users
to specify the SMB version that they want to use to talk to the server.

At that point, this option will just get in the way. Let's go ahead
and remove it now in preparation for that day.

Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 mount.cifs.c |   20 +++++++-------------
 1 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index 0408158..3041987 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -100,12 +100,6 @@
 #define MAX_DOMAIN_SIZE 64
 
 /*
- * value of the ver= option that gets passed to the kernel. Used to indicate
- * behavioral changes introduced in the mount helper.
- */
-#define OPTIONS_VERSION "1"
-
-/*
  * mount.cifs has been the subject of many "security" bugs that have arisen
  * because of users and distributions installing it as a setuid root program
  * before it had been audited for security holes. The default behavior is
@@ -1833,21 +1827,21 @@ assemble_mountinfo(struct parsed_mount_info *parsed_info,
 			goto assemble_exit;
 	}
 
-	/* copy in ver= string. It's not really needed, but what the hell */
-	if (*parsed_info->options)
-		strlcat(parsed_info->options, ",", sizeof(parsed_info->options));
-	strlcat(parsed_info->options, "ver=", sizeof(parsed_info->options));
-	strlcat(parsed_info->options, OPTIONS_VERSION, sizeof(parsed_info->options));
-
 	/* copy in user= string */
 	if (parsed_info->got_user) {
-		strlcat(parsed_info->options, ",user=",
+		if (*parsed_info->options)
+			strlcat(parsed_info->options, ",",
+				sizeof(parsed_info->options));
+		strlcat(parsed_info->options, "user=",
 			sizeof(parsed_info->options));
 		strlcat(parsed_info->options, parsed_info->username,
 			sizeof(parsed_info->options));
 	}
 
 	if (*parsed_info->domain) {
+		if (*parsed_info->options)
+			strlcat(parsed_info->options, ",",
+				sizeof(parsed_info->options));
 		strlcat(parsed_info->options, ",domain=",
 			sizeof(parsed_info->options));
 		strlcat(parsed_info->options, parsed_info->domain,
-- 
1.7.7.6

Re: [PATCH] mount.cifs: don't send a mandatory ver= option to the kernel

[Jeff Layton]

On Fri, 11 May 2012 15:03:21 -0400
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:

> Traditionally, this ver= option was used to specify the "options
> version" that we're passing in. It has always been set to '1' though
> and we have never changed that.
> 
> Eventually we want to have a ver= (or vers=) option that allows users
> to specify the SMB version that they want to use to talk to the server.
> 
> At that point, this option will just get in the way. Let's go ahead
> and remove it now in preparation for that day.
> 
> Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> ---
>  mount.cifs.c |   20 +++++++-------------
>  1 files changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/mount.cifs.c b/mount.cifs.c
> index 0408158..3041987 100644
> --- a/mount.cifs.c
> +++ b/mount.cifs.c
> @@ -100,12 +100,6 @@
>  #define MAX_DOMAIN_SIZE 64
>  
>  /*
> - * value of the ver= option that gets passed to the kernel. Used to indicate
> - * behavioral changes introduced in the mount helper.
> - */
> -#define OPTIONS_VERSION "1"
> -
> -/*
>   * mount.cifs has been the subject of many "security" bugs that have arisen
>   * because of users and distributions installing it as a setuid root program
>   * before it had been audited for security holes. The default behavior is
> @@ -1833,21 +1827,21 @@ assemble_mountinfo(struct parsed_mount_info *parsed_info,
>  			goto assemble_exit;
>  	}
>  
> -	/* copy in ver= string. It's not really needed, but what the hell */
> -	if (*parsed_info->options)
> -		strlcat(parsed_info->options, ",", sizeof(parsed_info->options));
> -	strlcat(parsed_info->options, "ver=", sizeof(parsed_info->options));
> -	strlcat(parsed_info->options, OPTIONS_VERSION, sizeof(parsed_info->options));
> -
>  	/* copy in user= string */
>  	if (parsed_info->got_user) {
> -		strlcat(parsed_info->options, ",user=",
> +		if (*parsed_info->options)
> +			strlcat(parsed_info->options, ",",
> +				sizeof(parsed_info->options));
> +		strlcat(parsed_info->options, "user=",
>  			sizeof(parsed_info->options));
>  		strlcat(parsed_info->options, parsed_info->username,
>  			sizeof(parsed_info->options));
>  	}
>  
>  	if (*parsed_info->domain) {
> +		if (*parsed_info->options)
> +			strlcat(parsed_info->options, ",",
> +				sizeof(parsed_info->options));
>  		strlcat(parsed_info->options, ",domain=",
>  			sizeof(parsed_info->options));
>  		strlcat(parsed_info->options, parsed_info->domain,

Committed...
-- 
Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>

Re: [PATCH] mount.cifs: don't send a mandatory ver= option to the kernel

[Sachin Prabhu]

On Fri, 2012-05-11 at 15:03 -0400, Jeff Layton wrote:
> Traditionally, this ver= option was used to specify the "options
> version" that we're passing in. It has always been set to '1' though
> and we have never changed that.
> 
> Eventually we want to have a ver= (or vers=) option that allows users
> to specify the SMB version that they want to use to talk to the server.
> 
> At that point, this option will just get in the way. Let's go ahead
> and remove it now in preparation for that day.
> 

Do we need 'ver=' mount option to specify the SMB version number? Isn't
'vers=' sufficient for this?

Sachin Prabhu

> Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> ---
>  mount.cifs.c |   20 +++++++-------------
>  1 files changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/mount.cifs.c b/mount.cifs.c
> index 0408158..3041987 100644
> --- a/mount.cifs.c
> +++ b/mount.cifs.c
> @@ -100,12 +100,6 @@
>  #define MAX_DOMAIN_SIZE 64
>  
>  /*
> - * value of the ver= option that gets passed to the kernel. Used to indicate
> - * behavioral changes introduced in the mount helper.
> - */
> -#define OPTIONS_VERSION "1"
> -
> -/*
>   * mount.cifs has been the subject of many "security" bugs that have arisen
>   * because of users and distributions installing it as a setuid root program
>   * before it had been audited for security holes. The default behavior is
> @@ -1833,21 +1827,21 @@ assemble_mountinfo(struct parsed_mount_info *parsed_info,
>  			goto assemble_exit;
>  	}
>  
> -	/* copy in ver= string. It's not really needed, but what the hell */
> -	if (*parsed_info->options)
> -		strlcat(parsed_info->options, ",", sizeof(parsed_info->options));
> -	strlcat(parsed_info->options, "ver=", sizeof(parsed_info->options));
> -	strlcat(parsed_info->options, OPTIONS_VERSION, sizeof(parsed_info->options));
> -
>  	/* copy in user= string */
>  	if (parsed_info->got_user) {
> -		strlcat(parsed_info->options, ",user=",
> +		if (*parsed_info->options)
> +			strlcat(parsed_info->options, ",",
> +				sizeof(parsed_info->options));
> +		strlcat(parsed_info->options, "user=",
>  			sizeof(parsed_info->options));
>  		strlcat(parsed_info->options, parsed_info->username,
>  			sizeof(parsed_info->options));
>  	}
>  
>  	if (*parsed_info->domain) {
> +		if (*parsed_info->options)
> +			strlcat(parsed_info->options, ",",
> +				sizeof(parsed_info->options));
>  		strlcat(parsed_info->options, ",domain=",
>  			sizeof(parsed_info->options));
>  		strlcat(parsed_info->options, parsed_info->domain,

Re: [PATCH] mount.cifs: don't send a mandatory ver= option to the kernel

[Steve French]

On Thu, May 17, 2012 at 11:44 AM, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, 2012-05-11 at 15:03 -0400, Jeff Layton wrote:
>> Traditionally, this ver= option was used to specify the "options
>> version" that we're passing in. It has always been set to '1' though
>> and we have never changed that.
>>
>> Eventually we want to have a ver= (or vers=) option that allows users
>> to specify the SMB version that they want to use to talk to the server.
>>
>> At that point, this option will just get in the way. Let's go ahead
>> and remove it now in preparation for that day.
>>
>
> Do we need 'ver=' mount option to specify the SMB version number? Isn't
> 'vers=' sufficient for this?

Yes - "vers" is sufficient (although I am fine with adding synonyms to
make it easier for nfs users - e.g. "smbvers=" or if we want to have
smb2 specific utility programs (e.g. for a phone or tablet) that call
do_mount automatically set vers=2.1.

I think that there will be times when we want (the kernel cifs.ko) to
know the version number of the user space utility (bugs? security
problems? debugging mount errors, at least when debugging set to
maximum level to log to dmesg?) so I think we should pass the real
mount.cifs version number in "ver=" as was the original intent (if for
nothing else it would help debugging mount bugs so we could log it to
dmesg in a few cases) - but since we have been sending "1" instead of
the mount.cifs version (and it is not currently used by the kernel
code) I can understand Jeff's point.  In any case, we can't use "ver="
to mean "vers=" (ie the requested dialect the user wants to use) as if
so current mount.cifs will force cifs by sending "ver=1" and when we
want to move the default in the future it would get in the way
(cifs.ko would require an update of mount.cifs but we would have no
way of telling in kernel whether it was the old mount.cifs - kind of a
catch-22 due to the lack of version number)


>> Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>> ---
>>  mount.cifs.c |   20 +++++++-------------
>>  1 files changed, 7 insertions(+), 13 deletions(-)
>>
>> diff --git a/mount.cifs.c b/mount.cifs.c
>> index 0408158..3041987 100644
>> --- a/mount.cifs.c
>> +++ b/mount.cifs.c
>> @@ -100,12 +100,6 @@
>>  #define MAX_DOMAIN_SIZE 64
>>
>>  /*
>> - * value of the ver= option that gets passed to the kernel. Used to indicate
>> - * behavioral changes introduced in the mount helper.
>> - */
>> -#define OPTIONS_VERSION "1"
>> -
>> -/*
>>   * mount.cifs has been the subject of many "security" bugs that have arisen
>>   * because of users and distributions installing it as a setuid root program
>>   * before it had been audited for security holes. The default behavior is
>> @@ -1833,21 +1827,21 @@ assemble_mountinfo(struct parsed_mount_info *parsed_info,
>>                       goto assemble_exit;
>>       }
>>
>> -     /* copy in ver= string. It's not really needed, but what the hell */
>> -     if (*parsed_info->options)
>> -             strlcat(parsed_info->options, ",", sizeof(parsed_info->options));
>> -     strlcat(parsed_info->options, "ver=", sizeof(parsed_info->options));
>> -     strlcat(parsed_info->options, OPTIONS_VERSION, sizeof(parsed_info->options));
>> -
>>       /* copy in user= string */
>>       if (parsed_info->got_user) {
>> -             strlcat(parsed_info->options, ",user=",
>> +             if (*parsed_info->options)
>> +                     strlcat(parsed_info->options, ",",
>> +                             sizeof(parsed_info->options));
>> +             strlcat(parsed_info->options, "user=",
>>                       sizeof(parsed_info->options));
>>               strlcat(parsed_info->options, parsed_info->username,
>>                       sizeof(parsed_info->options));
>>       }
>>
>>       if (*parsed_info->domain) {
>> +             if (*parsed_info->options)
>> +                     strlcat(parsed_info->options, ",",
>> +                             sizeof(parsed_info->options));
>>               strlcat(parsed_info->options, ",domain=",
>>                       sizeof(parsed_info->options));
>>               strlcat(parsed_info->options, parsed_info->domain,



-- 
Thanks,

Steve

Re: [PATCH] mount.cifs: don't send a mandatory ver= option to the kernel

[Jeff Layton]

On Thu, 17 May 2012 12:03:26 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Thu, May 17, 2012 at 11:44 AM, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Fri, 2012-05-11 at 15:03 -0400, Jeff Layton wrote:
> >> Traditionally, this ver= option was used to specify the "options
> >> version" that we're passing in. It has always been set to '1' though
> >> and we have never changed that.
> >>
> >> Eventually we want to have a ver= (or vers=) option that allows users
> >> to specify the SMB version that they want to use to talk to the server.
> >>
> >> At that point, this option will just get in the way. Let's go ahead
> >> and remove it now in preparation for that day.
> >>
> >
> > Do we need 'ver=' mount option to specify the SMB version number? Isn't
> > 'vers=' sufficient for this?
> 
> Yes - "vers" is sufficient (although I am fine with adding synonyms to
> make it easier for nfs users - e.g. "smbvers=" or if we want to have
> smb2 specific utility programs (e.g. for a phone or tablet) that call
> do_mount automatically set vers=2.1.
> 
> I think that there will be times when we want (the kernel cifs.ko) to
> know the version number of the user space utility (bugs? security
> problems? debugging mount errors, at least when debugging set to
> maximum level to log to dmesg?) so I think we should pass the real
> mount.cifs version number in "ver=" as was the original intent (if for
> nothing else it would help debugging mount bugs so we could log it to
> dmesg in a few cases) - but since we have been sending "1" instead of
> the mount.cifs version (and it is not currently used by the kernel
> code) I can understand Jeff's point.  In any case, we can't use "ver="
> to mean "vers=" (ie the requested dialect the user wants to use) as if
> so current mount.cifs will force cifs by sending "ver=1" and when we
> want to move the default in the future it would get in the way
> (cifs.ko would require an update of mount.cifs but we would have no
> way of telling in kernel whether it was the old mount.cifs - kind of a
> catch-22 due to the lack of version number)
> 

Yes, I don't think we're going to use ver= as a synonym for vers=.

That said, we haven't needed a way for the kernel to recognize the
userspace "options version" and no other userspace mount helper that
I'm aware of has such a thing. After all, a userspace mount helper
isn't even required...someone can mount cifs just fine w/o one as long
as they pass in ip=.

Handling an options version is more of a problem with binary mount
options, where you need to know if you're breaking the ABI. With text
based options, it's just not an issue.

So I think going ahead and getting rid of this in the kernel is the
right thing to do. It's just useless cruft that may get in the way in
the future.

If the kernel ever needs to determine the mount helper "version" for
some reason, then it can treat a lack of ver= at all identically to how
it handles a helper that passes in ver=1.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] mount.cifs: don't send a mandatory ver= option to the kernel

[Steve French]

On Thu, May 17, 2012 at 12:54 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Thu, 17 May 2012 12:03:26 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Thu, May 17, 2012 at 11:44 AM, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > On Fri, 2012-05-11 at 15:03 -0400, Jeff Layton wrote:
>> >> Traditionally, this ver= option was used to specify the "options
>> >> version" that we're passing in. It has always been set to '1' though
>> >> and we have never changed that.
>> >>
>> >> Eventually we want to have a ver= (or vers=) option that allows users
>> >> to specify the SMB version that they want to use to talk to the server.
>> >>
>> >> At that point, this option will just get in the way. Let's go ahead
>> >> and remove it now in preparation for that day.
>> >>
>> >
>> > Do we need 'ver=' mount option to specify the SMB version number? Isn't
>> > 'vers=' sufficient for this?
>>
>> Yes - "vers" is sufficient (although I am fine with adding synonyms to
>> make it easier for nfs users - e.g. "smbvers=" or if we want to have
>> smb2 specific utility programs (e.g. for a phone or tablet) that call
>> do_mount automatically set vers=2.1.
...
> Yes, I don't think we're going to use ver= as a synonym for vers=.
>
> That said, we haven't needed a way for the kernel to recognize the
> userspace "options version" and no other userspace mount helper that
> I'm aware of has such a thing. After all, a userspace mount helper
> isn't even required...someone can mount cifs just fine w/o one as long
> as they pass in ip=.
>
> Handling an options version is more of a problem with binary mount
> options, where you need to know if you're breaking the ABI. With text
> based options, it's just not an issue.
>
> So I think going ahead and getting rid of this in the kernel is the
> right thing to do. It's just useless cruft that may get in the way in
> the future.
>
> If the kernel ever needs to determine the mount helper "version" for
> some reason, then it can treat a lack of ver= at all identically to how
> it handles a helper that passes in ver=1.

The main case I can think of is debugging - there were a few times
that I have wanted to know the mount.cifs version in looking at dmesg
output of mount failures.   On a normal linux distro you can go to the
package manager or simply go to bash and type mount.cifs -V that isn't
as practical on a phone or on some Linux tablets.

-- 
Thanks,

Steve

Re: [PATCH] mount.cifs: don't send a mandatory ver= option to the kernel

[Jeff Layton]

On Thu, 17 May 2012 12:58:45 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Thu, May 17, 2012 at 12:54 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > On Thu, 17 May 2012 12:03:26 -0500
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> On Thu, May 17, 2012 at 11:44 AM, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >> > On Fri, 2012-05-11 at 15:03 -0400, Jeff Layton wrote:
> >> >> Traditionally, this ver= option was used to specify the "options
> >> >> version" that we're passing in. It has always been set to '1' though
> >> >> and we have never changed that.
> >> >>
> >> >> Eventually we want to have a ver= (or vers=) option that allows users
> >> >> to specify the SMB version that they want to use to talk to the server.
> >> >>
> >> >> At that point, this option will just get in the way. Let's go ahead
> >> >> and remove it now in preparation for that day.
> >> >>
> >> >
> >> > Do we need 'ver=' mount option to specify the SMB version number? Isn't
> >> > 'vers=' sufficient for this?
> >>
> >> Yes - "vers" is sufficient (although I am fine with adding synonyms to
> >> make it easier for nfs users - e.g. "smbvers=" or if we want to have
> >> smb2 specific utility programs (e.g. for a phone or tablet) that call
> >> do_mount automatically set vers=2.1.
> ...
> > Yes, I don't think we're going to use ver= as a synonym for vers=.
> >
> > That said, we haven't needed a way for the kernel to recognize the
> > userspace "options version" and no other userspace mount helper that
> > I'm aware of has such a thing. After all, a userspace mount helper
> > isn't even required...someone can mount cifs just fine w/o one as long
> > as they pass in ip=.
> >
> > Handling an options version is more of a problem with binary mount
> > options, where you need to know if you're breaking the ABI. With text
> > based options, it's just not an issue.
> >
> > So I think going ahead and getting rid of this in the kernel is the
> > right thing to do. It's just useless cruft that may get in the way in
> > the future.
> >
> > If the kernel ever needs to determine the mount helper "version" for
> > some reason, then it can treat a lack of ver= at all identically to how
> > it handles a helper that passes in ver=1.
> 
> The main case I can think of is debugging - there were a few times
> that I have wanted to know the mount.cifs version in looking at dmesg
> output of mount failures.   On a normal linux distro you can go to the
> package manager or simply go to bash and type mount.cifs -V that isn't
> as practical on a phone or on some Linux tablets.
> 


If that's the case, then what we have now is completely inadequate to
that task, since the mount helper has always passed in "ver=1". That
doesn't tell you anything about the actual version in use.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] mount.cifs: don't send a mandatory ver= option to the kernel

[Scott Lovenberg]

On 5/17/2012 2:25 PM, Jeff Layton wrote:
> On Thu, 17 May 2012 12:58:45 -0500
> Steve French<smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>  wrote:
>
>> On Thu, May 17, 2012 at 12:54 PM, Jeff Layton<jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>  wrote:
>>> On Thu, 17 May 2012 12:03:26 -0500
>>> Steve French<smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>  wrote:
>>>
>>>> On Thu, May 17, 2012 at 11:44 AM, Sachin Prabhu<sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>  wrote:
>>>>> On Fri, 2012-05-11 at 15:03 -0400, Jeff Layton wrote:
>>>>>> Traditionally, this ver= option was used to specify the "options
>>>>>> version" that we're passing in. It has always been set to '1' though
>>>>>> and we have never changed that.
>>>>>>
>>>>>> Eventually we want to have a ver= (or vers=) option that allows users
>>>>>> to specify the SMB version that they want to use to talk to the server.
>>>>>>
>>>>>> At that point, this option will just get in the way. Let's go ahead
>>>>>> and remove it now in preparation for that day.
>>>>>>
>>>>> Do we need 'ver=' mount option to specify the SMB version number? Isn't
>>>>> 'vers=' sufficient for this?
>>>> Yes - "vers" is sufficient (although I am fine with adding synonyms to
>>>> make it easier for nfs users - e.g. "smbvers=" or if we want to have
>>>> smb2 specific utility programs (e.g. for a phone or tablet) that call
>>>> do_mount automatically set vers=2.1.
>> ...
>>> Yes, I don't think we're going to use ver= as a synonym for vers=.
>>>
>>> That said, we haven't needed a way for the kernel to recognize the
>>> userspace "options version" and no other userspace mount helper that
>>> I'm aware of has such a thing. After all, a userspace mount helper
>>> isn't even required...someone can mount cifs just fine w/o one as long
>>> as they pass in ip=.
>>>
>>> Handling an options version is more of a problem with binary mount
>>> options, where you need to know if you're breaking the ABI. With text
>>> based options, it's just not an issue.
>>>
>>> So I think going ahead and getting rid of this in the kernel is the
>>> right thing to do. It's just useless cruft that may get in the way in
>>> the future.
>>>
>>> If the kernel ever needs to determine the mount helper "version" for
>>> some reason, then it can treat a lack of ver= at all identically to how
>>> it handles a helper that passes in ver=1.
>> The main case I can think of is debugging - there were a few times
>> that I have wanted to know the mount.cifs version in looking at dmesg
>> output of mount failures.   On a normal linux distro you can go to the
>> package manager or simply go to bash and type mount.cifs -V that isn't
>> as practical on a phone or on some Linux tablets.
>>
>
> If that's the case, then what we have now is completely inadequate to
> that task, since the mount helper has always passed in "ver=1". That
> doesn't tell you anything about the actual version in use.
>
Stupid question, if you just want an entry in dmesg (or whatever 
logging), why does it need to be thrown over the wall to the kernel?  
Can't it be logged from the mount helper at launch time?

At one point I know that we found a parsing error in the kernel mounting 
code that wasn't in the mount helper code (I can dig it up if anyone 
cares).  Doesn't going down this road mean matching up the mount helper 
with a specific range of kernels for full compatibility.  Right now it's 
fairly loosely coupled and that's a good thing IMHO.

Re: [PATCH] mount.cifs: don't send a mandatory ver= option to the kernel

[Steve French]

On Thu, May 17, 2012 at 1:30 PM, Scott Lovenberg
<scott.lovenberg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 5/17/2012 2:25 PM, Jeff Layton wrote:
>>
>> On Thu, 17 May 2012 12:58:45 -0500
>> Steve French<smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>  wrote:
>>
>>> On Thu, May 17, 2012 at 12:54 PM, Jeff Layton<jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>  wrote:
>>>>
>>>> On Thu, 17 May 2012 12:03:26 -0500
>>>> Steve French<smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>  wrote:
>>>>
>>>>> On Thu, May 17, 2012 at 11:44 AM, Sachin Prabhu<sprabhu-H+wXaHxf7aJhl2p70BpVqQ@public.gmane.orgm>
>>>>>  wrote:
>>>>>>
>>>>>> On Fri, 2012-05-11 at 15:03 -0400, Jeff Layton wrote:
>>>>>>>
>>>>>>> Traditionally, this ver= option was used to specify the "options
>>>>>>> version" that we're passing in. It has always been set to '1' though
>>>>>>> and we have never changed that.
>>>>>>>
>>>>>>> Eventually we want to have a ver= (or vers=) option that allows users
>>>>>>> to specify the SMB version that they want to use to talk to the
>>>>>>> server.
>>>>>>>
>>>>>>> At that point, this option will just get in the way. Let's go ahead
>>>>>>> and remove it now in preparation for that day.
>>>>>>>
>>>>>> Do we need 'ver=' mount option to specify the SMB version number?
>>>>>> Isn't
>>>>>> 'vers=' sufficient for this?
>>>>>
>>>>> Yes - "vers" is sufficient (although I am fine with adding synonyms to
>>>>> make it easier for nfs users - e.g. "smbvers=" or if we want to have
>>>>> smb2 specific utility programs (e.g. for a phone or tablet) that call
>>>>> do_mount automatically set vers=2.1.
>>>
>>> ...
>>>>
>>>> Yes, I don't think we're going to use ver= as a synonym for vers=.
>>>>
>>>> That said, we haven't needed a way for the kernel to recognize the
>>>> userspace "options version" and no other userspace mount helper that
>>>> I'm aware of has such a thing. After all, a userspace mount helper
>>>> isn't even required...someone can mount cifs just fine w/o one as long
>>>> as they pass in ip=.
>>>>
>>>> Handling an options version is more of a problem with binary mount
>>>> options, where you need to know if you're breaking the ABI. With text
>>>> based options, it's just not an issue.
>>>>
>>>> So I think going ahead and getting rid of this in the kernel is the
>>>> right thing to do. It's just useless cruft that may get in the way in
>>>> the future.
>>>>
>>>> If the kernel ever needs to determine the mount helper "version" for
>>>> some reason, then it can treat a lack of ver= at all identically to how
>>>> it handles a helper that passes in ver=1.
>>>
>>> The main case I can think of is debugging - there were a few times
>>> that I have wanted to know the mount.cifs version in looking at dmesg
>>> output of mount failures.   On a normal linux distro you can go to the
>>> package manager or simply go to bash and type mount.cifs -V that isn't
>>> as practical on a phone or on some Linux tablets.
>>>
>>
>> If that's the case, then what we have now is completely inadequate to
>> that task, since the mount helper has always passed in "ver=1". That
>> doesn't tell you anything about the actual version in use.
>>
> Stupid question, if you just want an entry in dmesg (or whatever logging),
> why does it need to be thrown over the wall to the kernel?  Can't it be
> logged from the mount helper at launch time?
>
> At one point I know that we found a parsing error in the kernel mounting
> code that wasn't in the mount helper code (I can dig it up if anyone cares).
>  Doesn't going down this road mean matching up the mount helper with a
> specific range of kernels for full compatibility.  Right now it's fairly
> loosely coupled and that's a good thing IMHO.

Yes - we could probably do this in mount.cifs - perhaps with
a new debugging or a maximal verbose option but since
we already have debug messages in kernel
for mount that get turned on when debugging is enabled.

Since mount.cifs is optional - I agree that we don't want tight coupling.
The general problem I see is that it is hard to change calls to
mount.cifs for debugging (perhaps on a phone)
but somewhat easier for the kernel.

-- 
Thanks,

Steve