Don't allow invalid 'vers' and 'minorversion' combinations in mount options,
such as "vers=3,minorversion=1".
Signed-off-by: Weston Andros Adamson <[email protected]>
---
%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
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
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
>
>
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 <[email protected]>
>>> ---
>>> %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;
>>
>
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 <[email protected]>
> ---
> %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;
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
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 <[email protected]>
>> ---
>> %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;
>