Skip to contentSkip to conversation

NFS: dont allow minorversion= opt when vers != 4

PATCH In review Weston Andros Adamson 4 participants 7 messages Feb 1, 2012 – Feb 2, 2012

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

Weston Andros Adamson

author
#0 lore raw

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

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(-)
fs/nfs/super.c +7 -0
index 8e210b2..b88e023 100644
@@ -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;
Signature · 1 line
1.7.4.4
#1 lore raw
Quoted 3 lines · Weston Andros Adamson

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

Quoted 32 lines
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(-)
fs/nfs/super.c +7 -0
index 8e210b2..b88e023 100644
@@ -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;
#2 lore raw
Quoted 9 lines · Boaz Harrosh
Quoted 3 lines · Weston Andros Adamson

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.

Quoted 3 lines

But maybe it's just me

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

-dros

Quoted 36 lines

Thanks
Boaz

Quoted 32 lines
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(-)
fs/nfs/super.c +3 -0
index 8e210b2..b88e023 100644
@@ -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;
fs/nfs/super.c +4 -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;
#3 lore raw
Quoted 7 lines · Dros
Quoted 5 lines · Boaz Harrosh
Quoted 3 lines · Weston Andros Adamson

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

Quoted 6 lines
Quoted 4 lines

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

Quoted 55 lines

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.

Quoted 3 lines

But maybe it's just me

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

-dros

Quoted 36 lines

Thanks
Boaz

Quoted 32 lines
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(-)
fs/nfs/super.c +3 -0
index 8e210b2..b88e023 100644
@@ -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;
fs/nfs/super.c +4 -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;
#4 lore raw
Quoted 11 lines · Bryan Schumaker
Quoted 9 lines · Dros
Quoted 7 lines · Boaz Harrosh
Quoted 3 lines · Weston Andros Adamson

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.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

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

#5 lore raw
Quoted 20 lines · Dros
Quoted 17 lines · Bryan Schumaker
Quoted 13 lines · Dros
Quoted 9 lines · Boaz Harrosh
Quoted 3 lines · Weston Andros Adamson

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.

Quoted 7 lines
Quoted 2 lines

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

Quoted 3 lines

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

-dros

#6 lore raw
Quoted 25 lines · Bryan Schumaker
Quoted 23 lines · Dros
Quoted 19 lines · Bryan Schumaker
Quoted 7 lines · Dros
Quoted 5 lines · Boaz Harrosh
Quoted 3 lines · Weston Andros Adamson

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

Quoted 6 lines
Quoted 4 lines

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?

Quoted 7 lines
Quoted 4 lines

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

Quoted 7 lines

- Bryan

Quoted 3 lines

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

-dros