[PATCH 0/3] NFSD: Various fixes for the 'versions' file.

[PATCH 0/3] NFSD: Various fixes for the 'versions' file.

[NeilBrown]

Following is the patch to provide non-regressed (though slightly
clunky) behaviour to the 'versions' file, and to fix a couple of
minor related bugs I found while working on the code.

NeilBrown


---

NeilBrown (3):
      NFSD: further refinement of content of /proc/fs/nfsd/versions
      NFSD: fix nfsd_minorversion(.., NFSD_AVAIL)
      NFSD: fix nfsd_reset_versions for NFSv4.


 fs/nfsd/nfsctl.c |   43 +++++++++++++++++++++++++++++++++----------
 fs/nfsd/nfssvc.c |   28 +++++++++++++---------------
 2 files changed, 46 insertions(+), 25 deletions(-)

--
Signature

[PATCH 1/3] NFSD: further refinement of content of /proc/fs/nfsd/versions

[NeilBrown]

Prior to
  e35659f1b03c ("NFSD: correctly range-check v4.x minor version when setting versions.")

v4.0 could not be disabled without disabling all NFSv4 protocols.
So the 'versions' file contained ±4 ±4.1 ±4.2.
Writing "-4" would disable all v4 completely.  Writing +4 would enabled those
minor versions that are currently enabled, either by default or otherwise.

After that commit, it was possible to disable v4.0 independently.  To
maximize backward compatibility with use cases which never disabled
v4.0, the "versions" file would never contain "+4.0" - that was implied
by "+4", unless explicitly negated by "-4.0".

This introduced an inconsistency in that it was possible to disable all
minor versions, but still have the major version advertised.
e.g. "-4.0 -4.1 -4.2 +4" would result in NFSv4 support being advertised,
but all attempts to use it rejected.

Commit
  d3635ff07e8c ("nfsd: fix configuration of supported minor versions")

and following removed this inconsistency. If all minor version were disabled,
the major would be disabled too.  If any minor was enabled, the major would be
disabled.
This patch also treated "+4" as equivalent to "+4.0" and "-4" as "-4.0".
A consequence of this is that writing "-4" would only disable 4.0.
This is a regression against the earlier behaviour, in a use case that rpc.nfsd
actually uses.
The command "rpc.nfsd -N 4" will write "+2 +3 -4" to the versions files.
Previously, that would disable v4 completely.  Now it will only disable v4.0.

Also "4.0" never appears in the "versions" file when read.
So if only v4.1 is available, the previous kernel would have reported
"+4 -4.0 +4.1 -4.2"  the current kernel reports "-4 +4.1 -4.2" which
could easily confuse.

This patch restores the implication that "+4" and "-4" apply more
globals and do not imply "4.0".
Specifically:
 writing "-4" will disable all 4.x minor versions.
 writing "+4" will enable all 4.1 minor version if none are currently enabled.
    rpc.nfsd will list minor versions before major versions, so
      rpc.nfsd -V 4.2 -N 4.1
    will write "-4.1 +4.2 +2 +3 +4"
    so it would be a regression for "+4" to enable always all versions.
 reading "-4" implies that no v4.x are enabled
 reading "+4" implies that some v4.x are enabled, and that v4.0 is enabled unless
 "-4.0" is also present.  All other minor versions will explicitly be listed.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/nfsd/nfsctl.c |   43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 73e75ac90525..8bf8f667a8cf 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -538,13 +538,21 @@ static ssize_t write_pool_threads(struct file *file, char *buf, size_t size)
 
 static ssize_t
 nfsd_print_version_support(char *buf, int remaining, const char *sep,
-		unsigned vers, unsigned minor)
+		unsigned vers, int minor)
 {
-	const char *format = (minor == 0) ? "%s%c%u" : "%s%c%u.%u";
+	const char *format = minor < 0 ? "%s%c%u" : "%s%c%u.%u";
 	bool supported = !!nfsd_vers(vers, NFSD_TEST);
 
-	if (vers == 4 && !nfsd_minorversion(minor, NFSD_TEST))
+	if (vers == 4 && minor >= 0 &&
+	    !nfsd_minorversion(minor, NFSD_TEST))
 		supported = false;
+	if (minor == 0 && supported)
+		/*
+		 * special case for backward compatability.
+		 * +4.0 is never reported, it is implied by
+		 * +4, unless -4.0 is present.
+		 */
+		return 0;
 	return snprintf(buf, remaining, format, sep,
 			supported ? '+' : '-', vers, minor);
 }
@@ -554,7 +562,6 @@ static ssize_t __write_versions(struct file *file, char *buf, size_t size)
 	char *mesg = buf;
 	char *vers, *minorp, sign;
 	int len, num, remaining;
-	unsigned minor;
 	ssize_t tlen = 0;
 	char *sep;
 	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
@@ -575,6 +582,7 @@ static ssize_t __write_versions(struct file *file, char *buf, size_t size)
 		if (len <= 0) return -EINVAL;
 		do {
 			enum vers_op cmd;
+			unsigned minor;
 			sign = *vers;
 			if (sign == '+' || sign == '-')
 				num = simple_strtol((vers+1), &minorp, 0);
@@ -585,8 +593,8 @@ static ssize_t __write_versions(struct file *file, char *buf, size_t size)
 					return -EINVAL;
 				if (kstrtouint(minorp+1, 0, &minor) < 0)
 					return -EINVAL;
-			} else
-				minor = 0;
+			}
+
 			cmd = sign == '-' ? NFSD_CLEAR : NFSD_SET;
 			switch(num) {
 			case 2:
@@ -594,8 +602,20 @@ static ssize_t __write_versions(struct file *file, char *buf, size_t size)
 				nfsd_vers(num, cmd);
 				break;
 			case 4:
-				if (nfsd_minorversion(minor, cmd) >= 0)
-					break;
+				if (*minorp == '.') {
+					if (nfsd_minorversion(minor, cmd) < 0)
+						return -EINVAL;
+				} else if ((cmd == NFSD_SET) != nfsd_vers(num, NFSD_TEST)) {
+					/*
+					 * Either we have +4 and no minors are enabled,
+					 * or we have -4 and at least one minor is enabled.
+					 * In either case, propagate 'cmd' to all minors.
+					 */
+					minor = 0;
+					while (nfsd_minorversion(minor, cmd) >= 0)
+						minor++;
+				}
+				break;
 			default:
 				return -EINVAL;
 			}
@@ -612,9 +632,11 @@ static ssize_t __write_versions(struct file *file, char *buf, size_t size)
 	sep = "";
 	remaining = SIMPLE_TRANSACTION_LIMIT;
 	for (num=2 ; num <= 4 ; num++) {
+		int minor;
 		if (!nfsd_vers(num, NFSD_AVAIL))
 			continue;
-		minor = 0;
+
+		minor = -1;
 		do {
 			len = nfsd_print_version_support(buf, remaining,
 					sep, num, minor);
@@ -624,7 +646,8 @@ static ssize_t __write_versions(struct file *file, char *buf, size_t size)
 			buf += len;
 			tlen += len;
 			minor++;
-			sep = " ";
+			if (len)
+				sep = " ";
 		} while (num == 4 && minor <= NFSD_SUPPORTED_MINOR_VERSION);
 	}
 out:

[PATCH 2/3] NFSD: fix nfsd_minorversion(.., NFSD_AVAIL)

[NeilBrown]

Current code will return 1 if the version is supported,
and -1 if it isn't.
This is confusing and inconsistent with the one place where this
is used.
So change to return 1 if it is supported, and zero if not.
i.e. an error is never returned.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/nfsd/nfssvc.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 786a4a2cb2d7..892137b1e330 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -167,7 +167,8 @@ nfsd_adjust_nfsd_versions4(void)
 
 int nfsd_minorversion(u32 minorversion, enum vers_op change)
 {
-	if (minorversion > NFSD_SUPPORTED_MINOR_VERSION)
+	if (minorversion > NFSD_SUPPORTED_MINOR_VERSION &&
+	    change != NFSD_AVAIL)
 		return -1;
 	switch(change) {
 	case NFSD_SET:

Re: [PATCH 2/3] NFSD: fix nfsd_minorversion(.., NFSD_AVAIL)

[J. Bruce Fields]

On Fri, Mar 10, 2017 at 11:36:39AM +1100, NeilBrown wrote:
> Current code will return 1 if the version is supported,
> and -1 if it isn't.
> This is confusing and inconsistent with the one place where this
> is used.

It's used?  I don't see it.--b.

> So change to return 1 if it is supported, and zero if not.
> i.e. an error is never returned.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  fs/nfsd/nfssvc.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 786a4a2cb2d7..892137b1e330 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -167,7 +167,8 @@ nfsd_adjust_nfsd_versions4(void)
>  
>  int nfsd_minorversion(u32 minorversion, enum vers_op change)
>  {
> -	if (minorversion > NFSD_SUPPORTED_MINOR_VERSION)
> +	if (minorversion > NFSD_SUPPORTED_MINOR_VERSION &&
> +	    change != NFSD_AVAIL)
>  		return -1;
>  	switch(change) {
>  	case NFSD_SET:
> 

Re: [PATCH 2/3] NFSD: fix nfsd_minorversion(.., NFSD_AVAIL)

[J. Bruce Fields]

On Fri, Mar 10, 2017 at 05:16:48PM -0500, J. Bruce Fields wrote:
> On Fri, Mar 10, 2017 at 11:36:39AM +1100, NeilBrown wrote:
> > Current code will return 1 if the version is supported,
> > and -1 if it isn't.
> > This is confusing and inconsistent with the one place where this
> > is used.
> 
> It's used?  I don't see it.--b.

But, whatever, I'm OK with it.

I'll queue all three up for 4.11 absent any objection.

--b.

> 
> > So change to return 1 if it is supported, and zero if not.
> > i.e. an error is never returned.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.com>
> > ---
> >  fs/nfsd/nfssvc.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > index 786a4a2cb2d7..892137b1e330 100644
> > --- a/fs/nfsd/nfssvc.c
> > +++ b/fs/nfsd/nfssvc.c
> > @@ -167,7 +167,8 @@ nfsd_adjust_nfsd_versions4(void)
> >  
> >  int nfsd_minorversion(u32 minorversion, enum vers_op change)
> >  {
> > -	if (minorversion > NFSD_SUPPORTED_MINOR_VERSION)
> > +	if (minorversion > NFSD_SUPPORTED_MINOR_VERSION &&
> > +	    change != NFSD_AVAIL)
> >  		return -1;
> >  	switch(change) {
> >  	case NFSD_SET:
> > 

Re: [PATCH 2/3] NFSD: fix nfsd_minorversion(.., NFSD_AVAIL)

[NeilBrown]

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

On Fri, Mar 10 2017, J. Bruce Fields wrote:

> On Fri, Mar 10, 2017 at 11:36:39AM +1100, NeilBrown wrote:
>> Current code will return 1 if the version is supported,
>> and -1 if it isn't.
>> This is confusing and inconsistent with the one place where this
>> is used.
>
> It's used?  I don't see it.--b.

You are correct of course.  NFSD_AVAIL is used once, but not with
nfsd_minorversion.
Would it ever make sense to selectively include different minor
versions at compile time?  Probably not, but it probably doesn't matter
anyway.

NeilBrown


>
>> So change to return 1 if it is supported, and zero if not.
>> i.e. an error is never returned.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>  fs/nfsd/nfssvc.c |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
>> index 786a4a2cb2d7..892137b1e330 100644
>> --- a/fs/nfsd/nfssvc.c
>> +++ b/fs/nfsd/nfssvc.c
>> @@ -167,7 +167,8 @@ nfsd_adjust_nfsd_versions4(void)
>>  
>>  int nfsd_minorversion(u32 minorversion, enum vers_op change)
>>  {
>> -	if (minorversion > NFSD_SUPPORTED_MINOR_VERSION)
>> +	if (minorversion > NFSD_SUPPORTED_MINOR_VERSION &&
>> +	    change != NFSD_AVAIL)
>>  		return -1;
>>  	switch(change) {
>>  	case NFSD_SET:
>> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[PATCH 3/3] NFSD: fix nfsd_reset_versions for NFSv4.

[NeilBrown]

If you write "-2 -3 -4" to the "versions" file, it will
notice that no versions are enabled, and nfsd_reset_versions()
is called.
This enables all major versions, not no minor versions.
So we lose the invariant that NFSv4 is only advertised when
at least one minor is enabled.

Fix the code to explicitly enable minor versions for v4,
change it to use nfsd_vers() to test and set, and simplify
the code.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/nfsd/nfssvc.c |   25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 892137b1e330..31e1f9593457 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -416,23 +416,20 @@ static void nfsd_last_thread(struct svc_serv *serv, struct net *net)
 
 void nfsd_reset_versions(void)
 {
-	int found_one = 0;
 	int i;
 
-	for (i = NFSD_MINVERS; i < NFSD_NRVERS; i++) {
-		if (nfsd_program.pg_vers[i])
-			found_one = 1;
-	}
+	for (i = 0; i < NFSD_NRVERS; i++)
+		if (nfsd_vers(i, NFSD_TEST))
+			return;
 
-	if (!found_one) {
-		for (i = NFSD_MINVERS; i < NFSD_NRVERS; i++)
-			nfsd_program.pg_vers[i] = nfsd_version[i];
-#if defined(CONFIG_NFSD_V2_ACL) || defined(CONFIG_NFSD_V3_ACL)
-		for (i = NFSD_ACL_MINVERS; i < NFSD_ACL_NRVERS; i++)
-			nfsd_acl_program.pg_vers[i] =
-				nfsd_acl_version[i];
-#endif
-	}
+	for (i = 0; i < NFSD_NRVERS; i++)
+		if (i != 4)
+			nfsd_vers(i, NFSD_SET);
+		else {
+			int minor = 0;
+			while (nfsd_minorversion(minor, NFSD_SET) >= 0)
+				minor++;
+		}
 }
 
 /*