[PATCH 1/2] NFS: dont allow minorversion= opt when vers != 4

[PATCH 1/2] NFS: dont allow minorversion= opt when vers != 4

[Weston Andros Adamson]

Don't allow invalid 'vers' and 'minorversion' combinations in mount options,
such as "vers=3,minorversion=1".

Signed-off-by: Weston Andros Adamson <dros@netapp.com>
---
 fs/nfs/super.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 8e210b2..50baca1 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1519,6 +1519,9 @@ static int nfs_parse_mount_options(char *raw,
 	if (!sloppy && invalid_option)
 		return 0;
 
+	if (mnt->minorversion && mnt->version != 4)
+		goto out_minorversion_mismatch;
+
 	/*
 	 * verify that any proto=/mountproto= options match the address
 	 * familiies in the addr=/mountaddr= options.
@@ -1552,6 +1555,10 @@ out_invalid_address:
 out_invalid_value:
 	printk(KERN_INFO "NFS: bad mount option value specified: %s\n", p);
 	return 0;
+out_minorversion_mismatch:
+	printk(KERN_INFO "NFS: mount option vers=%d does not support "
+			 "minorversion=%u\n", mnt->version, mnt->minorversion);
+	return 0;
 out_nomem:
 	printk(KERN_INFO "NFS: not enough memory to parse option\n");
 	return 0;
-- 
1.7.4.4

[PATCH 1/2] NFS: dont allow minorversion= opt when vers != 4

[Weston Andros Adamson]

Don't allow invalid 'vers' and 'minorversion' combinations in mount options,
such as "vers=3,minorversion=1".

Signed-off-by: Weston Andros Adamson <dros@netapp.com>
---
%d -> %u for printing mnt->version.

 fs/nfs/super.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 8e210b2..b88e023 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1519,6 +1519,9 @@ static int nfs_parse_mount_options(char *raw,
 	if (!sloppy && invalid_option)
 		return 0;
 
+	if (mnt->minorversion && mnt->version != 4)
+		goto out_minorversion_mismatch;
+
 	/*
 	 * verify that any proto=/mountproto= options match the address
 	 * familiies in the addr=/mountaddr= options.
@@ -1552,6 +1555,10 @@ out_invalid_address:
 out_invalid_value:
 	printk(KERN_INFO "NFS: bad mount option value specified: %s\n", p);
 	return 0;
+out_minorversion_mismatch:
+	printk(KERN_INFO "NFS: mount option vers=%u does not support "
+			 "minorversion=%u\n", mnt->version, mnt->minorversion);
+	return 0;
 out_nomem:
 	printk(KERN_INFO "NFS: not enough memory to parse option\n");
 	return 0;
-- 
1.7.4.4

Re: [PATCH 1/2] NFS: dont allow minorversion= opt when vers != 4

[Boaz Harrosh]

On 02/01/2012 09:06 PM, Weston Andros Adamson wrote:
> Don't allow invalid 'vers' and 'minorversion' combinations in mount options,
> such as "vers=3,minorversion=1".
> 

Just my $0.017 I don't see the point in this. 

If vers==3 then minorversion is ignored, just like today.
What kind  of extra protection does it buy us?

But maybe it's just me

Thanks
Boaz

> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
> ---
> %d -> %u for printing mnt->version.
> 
>  fs/nfs/super.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 8e210b2..b88e023 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -1519,6 +1519,9 @@ static int nfs_parse_mount_options(char *raw,
>  	if (!sloppy && invalid_option)
>  		return 0;
>  
> +	if (mnt->minorversion && mnt->version != 4)
> +		goto out_minorversion_mismatch;
> +
>  	/*
>  	 * verify that any proto=/mountproto= options match the address
>  	 * familiies in the addr=/mountaddr= options.
> @@ -1552,6 +1555,10 @@ out_invalid_address:
>  out_invalid_value:
>  	printk(KERN_INFO "NFS: bad mount option value specified: %s\n", p);
>  	return 0;
> +out_minorversion_mismatch:
> +	printk(KERN_INFO "NFS: mount option vers=%u does not support "
> +			 "minorversion=%u\n", mnt->version, mnt->minorversion);
> +	return 0;
>  out_nomem:
>  	printk(KERN_INFO "NFS: not enough memory to parse option\n");
>  	return 0;

Re: [PATCH 1/2] NFS: dont allow minorversion= opt when vers != 4

[Adamson, Dros]

[-- Attachment #1: Type: text/plain, Size: 2194 bytes --]

On Feb 1, 2012, at 5:44 PM, Boaz Harrosh wrote:

> On 02/01/2012 09:06 PM, Weston Andros Adamson wrote:
>> Don't allow invalid 'vers' and 'minorversion' combinations in mount options,
>> such as "vers=3,minorversion=1".
>> 
> 
> Just my $0.017 I don't see the point in this. 
> 
> If vers==3 then minorversion is ignored, just like today.
> What kind  of extra protection does it buy us?

No, minorversion is not ignored when vers=3.  

Try an invalid (v4) minorversion:

$ sudo mount -t nfs -o vers=3,minorversion=2 server:/export /mnt 
mount.nfs: an incorrect mount option was specified
$ dmesg | tail -1
[ 1734.758101] NFS: bad mount option value specified: minorversion=2

I can understand why this was never a priority, but I find it quite confusing when version=3,minorversion=1 succeeds -- I've fat-fingered that more than a few times, started running tests and only later realized my mistake.

> 
> But maybe it's just me
> 

I know it's not just me who's been confused by this in the past :)

-dros

> Thanks
> Boaz
> 
>> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
>> ---
>> %d -> %u for printing mnt->version.
>> 
>> fs/nfs/super.c |    7 +++++++
>> 1 files changed, 7 insertions(+), 0 deletions(-)
>> 
>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>> index 8e210b2..b88e023 100644
>> --- a/fs/nfs/super.c
>> +++ b/fs/nfs/super.c
>> @@ -1519,6 +1519,9 @@ static int nfs_parse_mount_options(char *raw,
>> 	if (!sloppy && invalid_option)
>> 		return 0;
>> 
>> +	if (mnt->minorversion && mnt->version != 4)
>> +		goto out_minorversion_mismatch;
>> +
>> 	/*
>> 	 * verify that any proto=/mountproto= options match the address
>> 	 * familiies in the addr=/mountaddr= options.
>> @@ -1552,6 +1555,10 @@ out_invalid_address:
>> out_invalid_value:
>> 	printk(KERN_INFO "NFS: bad mount option value specified: %s\n", p);
>> 	return 0;
>> +out_minorversion_mismatch:
>> +	printk(KERN_INFO "NFS: mount option vers=%u does not support "
>> +			 "minorversion=%u\n", mnt->version, mnt->minorversion);
>> +	return 0;
>> out_nomem:
>> 	printk(KERN_INFO "NFS: not enough memory to parse option\n");
>> 	return 0;
> 


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 1374 bytes --]

Re: [PATCH 1/2] NFS: dont allow minorversion= opt when vers != 4

[Bryan Schumaker]

On 02/01/12 18:07, Adamson, Dros wrote:

> On Feb 1, 2012, at 5:44 PM, Boaz Harrosh wrote:
> 
>> On 02/01/2012 09:06 PM, Weston Andros Adamson wrote:
>>> Don't allow invalid 'vers' and 'minorversion' combinations in mount options,
>>> such as "vers=3,minorversion=1".
>>>
>>
>> Just my $0.017 I don't see the point in this. 
>>
>> If vers==3 then minorversion is ignored, just like today.
>> What kind  of extra protection does it buy us?
> 
> No, minorversion is not ignored when vers=3.  


But after mounting, does setting vers=3, minorversion=1 cause any change in NFS v3 behavior?

- Bryan

> 
> Try an invalid (v4) minorversion:
> 
> $ sudo mount -t nfs -o vers=3,minorversion=2 server:/export /mnt 
> mount.nfs: an incorrect mount option was specified
> $ dmesg | tail -1
> [ 1734.758101] NFS: bad mount option value specified: minorversion=2
> 
> I can understand why this was never a priority, but I find it quite confusing when version=3,minorversion=1 succeeds -- I've fat-fingered that more than a few times, started running tests and only later realized my mistake.
> 
>>
>> But maybe it's just me
>>
> 
> I know it's not just me who's been confused by this in the past :)
> 
> -dros
> 
>> Thanks
>> Boaz
>>
>>> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
>>> ---
>>> %d -> %u for printing mnt->version.
>>>
>>> fs/nfs/super.c |    7 +++++++
>>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>>> index 8e210b2..b88e023 100644
>>> --- a/fs/nfs/super.c
>>> +++ b/fs/nfs/super.c
>>> @@ -1519,6 +1519,9 @@ static int nfs_parse_mount_options(char *raw,
>>> 	if (!sloppy && invalid_option)
>>> 		return 0;
>>>
>>> +	if (mnt->minorversion && mnt->version != 4)
>>> +		goto out_minorversion_mismatch;
>>> +
>>> 	/*
>>> 	 * verify that any proto=/mountproto= options match the address
>>> 	 * familiies in the addr=/mountaddr= options.
>>> @@ -1552,6 +1555,10 @@ out_invalid_address:
>>> out_invalid_value:
>>> 	printk(KERN_INFO "NFS: bad mount option value specified: %s\n", p);
>>> 	return 0;
>>> +out_minorversion_mismatch:
>>> +	printk(KERN_INFO "NFS: mount option vers=%u does not support "
>>> +			 "minorversion=%u\n", mnt->version, mnt->minorversion);
>>> +	return 0;
>>> out_nomem:
>>> 	printk(KERN_INFO "NFS: not enough memory to parse option\n");
>>> 	return 0;
>>
> 

Re: [PATCH 1/2] NFS: dont allow minorversion= opt when vers != 4

[Adamson, Dros]

[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]


On Feb 2, 2012, at 8:51 AM, Bryan Schumaker wrote:

> On 02/01/12 18:07, Adamson, Dros wrote:
> 
>> On Feb 1, 2012, at 5:44 PM, Boaz Harrosh wrote:
>> 
>>> On 02/01/2012 09:06 PM, Weston Andros Adamson wrote:
>>>> Don't allow invalid 'vers' and 'minorversion' combinations in mount options,
>>>> such as "vers=3,minorversion=1".
>>>> 
>>> 
>>> Just my $0.017 I don't see the point in this. 
>>> 
>>> If vers==3 then minorversion is ignored, just like today.
>>> What kind  of extra protection does it buy us?
>> 
>> No, minorversion is not ignored when vers=3.  
> 
> 
> But after mounting, does setting vers=3, minorversion=1 cause any change in NFS v3 behavior?
> 

No it doesn't.  Past the parsing of options, minorversion is ignored for versions other than 4.

I just don't understand how anyone can have problem with this patch.  Why would we want to validate minorversion in some cases, but not all cases?  How would this patch be a bad thing?

It's about usability -- if this can confuse NFS developers, how are end users going to handle it?

-dros

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 1374 bytes --]

Re: [PATCH 1/2] NFS: dont allow minorversion= opt when vers != 4

[Bryan Schumaker]

On 02/02/12 12:03, Adamson, Dros wrote:

> 
> On Feb 2, 2012, at 8:51 AM, Bryan Schumaker wrote:
> 
>> On 02/01/12 18:07, Adamson, Dros wrote:
>>
>>> On Feb 1, 2012, at 5:44 PM, Boaz Harrosh wrote:
>>>
>>>> On 02/01/2012 09:06 PM, Weston Andros Adamson wrote:
>>>>> Don't allow invalid 'vers' and 'minorversion' combinations in mount options,
>>>>> such as "vers=3,minorversion=1".
>>>>>
>>>>
>>>> Just my $0.017 I don't see the point in this. 
>>>>
>>>> If vers==3 then minorversion is ignored, just like today.
>>>> What kind  of extra protection does it buy us?
>>>
>>> No, minorversion is not ignored when vers=3.  
>>
>>
>> But after mounting, does setting vers=3, minorversion=1 cause any change in NFS v3 behavior?
>>
> 
> No it doesn't.  Past the parsing of options, minorversion is ignored for versions other than 4.
> 
> I just don't understand how anyone can have problem with this patch.  Why would we want to validate minorversion in some cases, but not all cases?  How would this patch be a bad thing?
> 

I don't have a problem with the patch, it makes sense that we shouldn't confuse developers or users.  I was just curious if there was a spot where we had "if minor_version == 1: do_something()" without checking for major_version == 4.

- Bryan

> It's about usability -- if this can confuse NFS developers, how are end users going to handle it?
> 
> -dros

Re: [PATCH 1/2] NFS: dont allow minorversion= opt when vers != 4

[Adamson, Dros]

[-- Attachment #1: Type: text/plain, Size: 2304 bytes --]


On Feb 2, 2012, at 2:00 PM, Bryan Schumaker wrote:

> On 02/02/12 12:03, Adamson, Dros wrote:
> 
>> 
>> On Feb 2, 2012, at 8:51 AM, Bryan Schumaker wrote:
>> 
>>> On 02/01/12 18:07, Adamson, Dros wrote:
>>> 
>>>> On Feb 1, 2012, at 5:44 PM, Boaz Harrosh wrote:
>>>> 
>>>>> On 02/01/2012 09:06 PM, Weston Andros Adamson wrote:
>>>>>> Don't allow invalid 'vers' and 'minorversion' combinations in mount options,
>>>>>> such as "vers=3,minorversion=1".
>>>>>> 
>>>>> 
>>>>> Just my $0.017 I don't see the point in this. 
>>>>> 
>>>>> If vers==3 then minorversion is ignored, just like today.
>>>>> What kind  of extra protection does it buy us?
>>>> 
>>>> No, minorversion is not ignored when vers=3.  
>>> 
>>> 
>>> But after mounting, does setting vers=3, minorversion=1 cause any change in NFS v3 behavior?
>>> 
>> 
>> No it doesn't.  Past the parsing of options, minorversion is ignored for versions other than 4.
>> 
>> I just don't understand how anyone can have problem with this patch.  Why would we want to validate minorversion in some cases, but not all cases?  How would this patch be a bad thing?
>> 
> 
> I don't have a problem with the patch, it makes sense that we shouldn't confuse developers or users.  I was just curious if there was a spot where we had "if minor_version == 1: do_something()" without checking for major_version == 4.
> 

Ah, I misunderstood… 

Versions != 4 pass the nfs_parsed_mount_data struct to nfs_create_server(), which completely ignores the minorversion member.
Version == 4 passes the nfs_parsed_mount_data struct to  nfs4_create_server(), which (through nfs4_init_server()) uses the minorversion member.

So, having a set minorversion when mounting vers != 4 has no effect on how the NFS module operates. This is Boaz's argument for why the patch isn't needed. I understand that reasoning, but this is a user experience enhancement and I think they are important too.

This patch only addresses an inconsistency in mount option validation. This doesn't change anything at the protocol level. I should have done a better job explaining this in the original post!

-dros

> - Bryan
> 
>> It's about usability -- if this can confuse NFS developers, how are end users going to handle it?
>> 
>> -dros
> 
> 


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 1374 bytes --]