[PATCH 1/2] mount.nfs: v4 mounts should fail when -o flag is used.

[PATCH 1/2] mount.nfs: v4 mounts should fail when -o flag is used.

[Steve Dickson]

When the pseudo root is set with fsid=0, explicit
v4 mounts (via the -o flag) should fail when
the incorrect export is tried instead of rolling
back to v3.

Signed-off-by: Steve Dickson <steved@redhat.com>
---
 utils/mount/network.c | 3 ++-
 utils/mount/network.h | 8 ++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index 281e935..e39263e 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -1299,7 +1299,8 @@ nfs_nfs_version(struct mount_options *options, struct nfs_version *version)
 		if (!(version->minor = strtol(version_val, &cptr, 10)) && cptr == version_val)
 			goto ret_error;
 		version->v_mode = V_SPECIFIC;
-	} else if (version->major > 3 && *cptr == '\0')
+	} else if (version->major > 3 && *cptr == '\0' &&
+			version->v_mode == V_DEFAULT) /* v_mode has not been set */
 		version->v_mode = V_GENERAL;
 
 	if (*cptr != '\0')
diff --git a/utils/mount/network.h b/utils/mount/network.h
index 9cc5dec..45e2b24 100644
--- a/utils/mount/network.h
+++ b/utils/mount/network.h
@@ -58,10 +58,10 @@ int clnt_ping(struct sockaddr_in *, const unsigned long,
 struct mount_options;
 
 enum {
-	V_DEFAULT = 0,
-	V_GENERAL,
-	V_SPECIFIC,
-	V_PARSE_ERR,
+	V_DEFAULT = 0, /* not set */
+	V_GENERAL,     /* single digit => 4 */
+	V_SPECIFIC,    /* single digit < 4 or decimal included */
+	V_PARSE_ERR,   /* miss all others */
 };
 
 struct nfs_version {
-- 
2.9.4

[PATCH 2/2] mount.nfs: v4 mounts should fail when nfs4 is specified with -t flag

[Steve Dickson]

When the pseudo root is set with fsid=0, explicit v4
mounts (set with the -t fstype mount flag) should
fail when the incorrect export is tried instead
of rolling back to v3.

Signed-off-by: Steve Dickson <steved@redhat.com>
---
 utils/mount/stropts.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 0fbb375..a56e965 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -315,9 +315,10 @@ static int nfs_set_version(struct nfsmount_info *mi)
 	if (!nfs_nfs_version(mi->options, &mi->version))
 		return 0;
 
-	if (strncmp(mi->type, "nfs4", 4) == 0)
+	if (strncmp(mi->type, "nfs4", 4) == 0) {
 		mi->version.major = 4;
-
+		mi->version.v_mode = V_SPECIFIC;
+	}
 	/*
 	 * Before 2.6.32, the kernel NFS client didn't
 	 * support "-t nfs vers=4" mounts, so NFS version
-- 
2.9.4

Re: [PATCH 1/2] mount.nfs: v4 mounts should fail when -o flag is used.

[NeilBrown]

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

On Fri, May 19 2017, Steve Dickson wrote:

> When the pseudo root is set with fsid=0, explicit
> v4 mounts (via the -o flag) should fail when
> the incorrect export is tried instead of rolling
> back to v3.

Hi Steve,
 I think this patch makes sense, but the above description doesn't.
 Where does fsid=0 fit in anywhere here?

I think you want to say

  When the protocol is set with "-t nfs4", we should behave just like
  with do with "-o vers=4" and not fall back to v3.

Is that what you were really trying to say?

Thanks,
NeilBrown


>
> Signed-off-by: Steve Dickson <steved@redhat.com>
> ---
>  utils/mount/network.c | 3 ++-
>  utils/mount/network.h | 8 ++++----
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/utils/mount/network.c b/utils/mount/network.c
> index 281e935..e39263e 100644
> --- a/utils/mount/network.c
> +++ b/utils/mount/network.c
> @@ -1299,7 +1299,8 @@ nfs_nfs_version(struct mount_options *options, struct nfs_version *version)
>  		if (!(version->minor = strtol(version_val, &cptr, 10)) && cptr == version_val)
>  			goto ret_error;
>  		version->v_mode = V_SPECIFIC;
> -	} else if (version->major > 3 && *cptr == '\0')
> +	} else if (version->major > 3 && *cptr == '\0' &&
> +			version->v_mode == V_DEFAULT) /* v_mode has not been set */
>  		version->v_mode = V_GENERAL;
>  
>  	if (*cptr != '\0')
> diff --git a/utils/mount/network.h b/utils/mount/network.h
> index 9cc5dec..45e2b24 100644
> --- a/utils/mount/network.h
> +++ b/utils/mount/network.h
> @@ -58,10 +58,10 @@ int clnt_ping(struct sockaddr_in *, const unsigned long,
>  struct mount_options;
>  
>  enum {
> -	V_DEFAULT = 0,
> -	V_GENERAL,
> -	V_SPECIFIC,
> -	V_PARSE_ERR,
> +	V_DEFAULT = 0, /* not set */
> +	V_GENERAL,     /* single digit => 4 */
> +	V_SPECIFIC,    /* single digit < 4 or decimal included */
> +	V_PARSE_ERR,   /* miss all others */
>  };
>  
>  struct nfs_version {
> -- 
> 2.9.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH 1/2] mount.nfs: v4 mounts should fail when -o flag is used.

[Steve Dickson]

On 05/21/2017 11:03 PM, NeilBrown wrote:
> On Fri, May 19 2017, Steve Dickson wrote:
> 
>> When the pseudo root is set with fsid=0, explicit
>> v4 mounts (via the -o flag) should fail when
>> the incorrect export is tried instead of rolling
>> back to v3.
> 
> Hi Steve,
>  I think this patch makes sense, but the above description doesn't.
>  Where does fsid=0 fit in anywhere here?
It sets the export to be the pseudo root
    /home *(rw,fsid=0,sec=sys:krb5:krb5i:krb5p)

so when then that export using either -t nfs4 or -o v4
    mount -o v4.0 127.0.0.1:/home /mnt

the mount should fail instead of rolling back to v3
Basically its be used to cause the error. 

> 
> I think you want to say
> 
>   When the protocol is set with "-t nfs4", we should behave just like
>   with do with "-o vers=4" and not fall back to v3.
Actually the first patch fixes the -o vers=4 case since
that too was rolling back to v3 in the above scenario
 
> 
> Is that what you were really trying to say?
How about

When the protocol is set the "-o v4" flag,
and the mount fails due to a pseudo root
issue, the mount should fail not, roll 
back to v3.

steved.

> 
> Thanks,
> NeilBrown
> 
> 
>>
>> Signed-off-by: Steve Dickson <steved@redhat.com>
>> ---
>>  utils/mount/network.c | 3 ++-
>>  utils/mount/network.h | 8 ++++----
>>  2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/utils/mount/network.c b/utils/mount/network.c
>> index 281e935..e39263e 100644
>> --- a/utils/mount/network.c
>> +++ b/utils/mount/network.c
>> @@ -1299,7 +1299,8 @@ nfs_nfs_version(struct mount_options *options, struct nfs_version *version)
>>  		if (!(version->minor = strtol(version_val, &cptr, 10)) && cptr == version_val)
>>  			goto ret_error;
>>  		version->v_mode = V_SPECIFIC;
>> -	} else if (version->major > 3 && *cptr == '\0')
>> +	} else if (version->major > 3 && *cptr == '\0' &&
>> +			version->v_mode == V_DEFAULT) /* v_mode has not been set */
>>  		version->v_mode = V_GENERAL;
>>  
>>  	if (*cptr != '\0')
>> diff --git a/utils/mount/network.h b/utils/mount/network.h
>> index 9cc5dec..45e2b24 100644
>> --- a/utils/mount/network.h
>> +++ b/utils/mount/network.h
>> @@ -58,10 +58,10 @@ int clnt_ping(struct sockaddr_in *, const unsigned long,
>>  struct mount_options;
>>  
>>  enum {
>> -	V_DEFAULT = 0,
>> -	V_GENERAL,
>> -	V_SPECIFIC,
>> -	V_PARSE_ERR,
>> +	V_DEFAULT = 0, /* not set */
>> +	V_GENERAL,     /* single digit => 4 */
>> +	V_SPECIFIC,    /* single digit < 4 or decimal included */
>> +	V_PARSE_ERR,   /* miss all others */
>>  };
>>  
>>  struct nfs_version {
>> -- 
>> 2.9.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] mount.nfs: v4 mounts should fail when -o flag is used.

[NeilBrown]

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

On Mon, May 22 2017, Steve Dickson wrote:

> On 05/21/2017 11:03 PM, NeilBrown wrote:
>> On Fri, May 19 2017, Steve Dickson wrote:
>> 
>>> When the pseudo root is set with fsid=0, explicit
>>> v4 mounts (via the -o flag) should fail when
>>> the incorrect export is tried instead of rolling
>>> back to v3.
>> 
>> Hi Steve,
>>  I think this patch makes sense, but the above description doesn't.
>>  Where does fsid=0 fit in anywhere here?
> It sets the export to be the pseudo root
>     /home *(rw,fsid=0,sec=sys:krb5:krb5i:krb5p)
>
> so when then that export using either -t nfs4 or -o v4
>     mount -o v4.0 127.0.0.1:/home /mnt
>
> the mount should fail instead of rolling back to v3
> Basically its be used to cause the error. 
>
>> 
>> I think you want to say
>> 
>>   When the protocol is set with "-t nfs4", we should behave just like
>>   with do with "-o vers=4" and not fall back to v3.
> Actually the first patch fixes the -o vers=4 case since
> that too was rolling back to v3 in the above scenario
>  
>> 
>> Is that what you were really trying to say?
> How about
>
> When the protocol is set the "-o v4" flag,
> and the mount fails due to a pseudo root
> issue, the mount should fail not, roll 
> back to v3.

Better, but I don't think the pseudo root has any relevance.
If you ask for v4, you should get v4, not v3.  How the server may or may
not behave differently between v3 and v4 is irrelevant.  You should get
what you asked for.

But now that I look at the code again... I don't understand.

You say this is for the "-o v4" case.

In that case, the current code in nfs_nfs_version() will find the "v4"
entry in nfs_version_opttbl[] and set
  version_val = "4";
  version->v_mode = V_SPECIFIC;
then
  version_major = 4;
then as *cptr == '\0' and ->major > 4,
  version->v_mode = V_GENERAL;

Your change skips that last step so it finished with
   v_mod == V_SPECIFIC.

According to the extra comments you have added for the modes:

>>> +	V_GENERAL,     /* single digit => 4 */
>>> +	V_SPECIFIC,    /* single digit < 4 or decimal included */

And it seems to me that "v4" should be V_GENERAL, not V_SPECIFIC.
So I think the current code is correct.

Except... nfs_try_mount() will then call nfs_autonegotiate(),
and nfs_autonegotiate() isn't very consistent about how it
interprets V_GENERAL and V_SPECIFIC.
For EINVAL, it gets the difference right, for other errors it doesn't.

So I think that this is the fix you want:

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 0fbb37569ef9..98cf813fe439 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -838,9 +838,6 @@ check_result:
 	case EINVAL:
 		/* A less clear indication that our client
 		 * does not support NFSv4 minor version. */
-		if (mi->version.v_mode == V_GENERAL &&
-			mi->version.minor == 0)
-				return result;
 		if (mi->version.v_mode != V_SPECIFIC) {
 			if (mi->version.minor > 0) {
 				mi->version.minor--;
@@ -862,6 +859,9 @@ check_result:
 		/* UDP-Only servers won't support v4, but maybe it
 		 * just isn't ready yet.  So try v3, but double-check
 		 * with rpcbind for v4. */
+		if (mi->version.v_mode == V_GENERAL)
+			/* Mustn't try v2,v3 */
+			return result;
 		result = nfs_try_mount_v3v2(mi, TRUE);
 		if (result == 0 && errno == EAGAIN) {
 			/* v4 server seems to be registered now. */
@@ -878,6 +878,9 @@ check_result:
 	}
 
 fall_back:
+	if (mi->version.v_mode == V_GENERAL)
+		/* v2,3 fallback not allowed */
+		return result;
 	return nfs_try_mount_v3v2(mi, FALSE);
 }
 


I haven't even compile-tested of course :-)

Thanks,
NeilBrown


>
> steved.
>
>> 
>> Thanks,
>> NeilBrown
>> 
>> 
>>>
>>> Signed-off-by: Steve Dickson <steved@redhat.com>
>>> ---
>>>  utils/mount/network.c | 3 ++-
>>>  utils/mount/network.h | 8 ++++----
>>>  2 files changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/utils/mount/network.c b/utils/mount/network.c
>>> index 281e935..e39263e 100644
>>> --- a/utils/mount/network.c
>>> +++ b/utils/mount/network.c
>>> @@ -1299,7 +1299,8 @@ nfs_nfs_version(struct mount_options *options, struct nfs_version *version)
>>>  		if (!(version->minor = strtol(version_val, &cptr, 10)) && cptr == version_val)
>>>  			goto ret_error;
>>>  		version->v_mode = V_SPECIFIC;
>>> -	} else if (version->major > 3 && *cptr == '\0')
>>> +	} else if (version->major > 3 && *cptr == '\0' &&
>>> +			version->v_mode == V_DEFAULT) /* v_mode has not been set */
>>>  		version->v_mode = V_GENERAL;
>>>  
>>>  	if (*cptr != '\0')
>>> diff --git a/utils/mount/network.h b/utils/mount/network.h
>>> index 9cc5dec..45e2b24 100644
>>> --- a/utils/mount/network.h
>>> +++ b/utils/mount/network.h
>>> @@ -58,10 +58,10 @@ int clnt_ping(struct sockaddr_in *, const unsigned long,
>>>  struct mount_options;
>>>  
>>>  enum {
>>> -	V_DEFAULT = 0,
>>> -	V_GENERAL,
>>> -	V_SPECIFIC,
>>> -	V_PARSE_ERR,
>>> +	V_DEFAULT = 0, /* not set */
>>> +	V_GENERAL,     /* single digit => 4 */
>>> +	V_SPECIFIC,    /* single digit < 4 or decimal included */
>>> +	V_PARSE_ERR,   /* miss all others */
>>>  };
>>>  
>>>  struct nfs_version {
>>> -- 
>>> 2.9.4
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH 1/2] mount.nfs: v4 mounts should fail when -o flag is used.

[Steve Dickson]

Sorry for the delayed response... that damn 
flux capacitor broke... again! ;-) 

On 05/22/2017 08:52 PM, NeilBrown wrote:
> On Mon, May 22 2017, Steve Dickson wrote:
> 
>> On 05/21/2017 11:03 PM, NeilBrown wrote:
>>> On Fri, May 19 2017, Steve Dickson wrote:
>>>
>>>> When the pseudo root is set with fsid=0, explicit
>>>> v4 mounts (via the -o flag) should fail when
>>>> the incorrect export is tried instead of rolling
>>>> back to v3.
>>>
>>> Hi Steve,
>>>  I think this patch makes sense, but the above description doesn't.
>>>  Where does fsid=0 fit in anywhere here?
>> It sets the export to be the pseudo root
>>     /home *(rw,fsid=0,sec=sys:krb5:krb5i:krb5p)
>>
>> so when then that export using either -t nfs4 or -o v4
>>     mount -o v4.0 127.0.0.1:/home /mnt
>>
>> the mount should fail instead of rolling back to v3
>> Basically its be used to cause the error. 
>>
>>>
>>> I think you want to say
>>>
>>>   When the protocol is set with "-t nfs4", we should behave just like
>>>   with do with "-o vers=4" and not fall back to v3.
>> Actually the first patch fixes the -o vers=4 case since
>> that too was rolling back to v3 in the above scenario
>>  
>>>
>>> Is that what you were really trying to say?
>> How about
>>
>> When the protocol is set the "-o v4" flag,
>> and the mount fails due to a pseudo root
>> issue, the mount should fail not, roll 
>> back to v3.
> 
> Better, but I don't think the pseudo root has any relevance.
> If you ask for v4, you should get v4, not v3.  How the server may or may
> not behave differently between v3 and v4 is irrelevant.  You should get
> what you asked for.
Fine.

> 
> But now that I look at the code again... I don't understand.
> 
> You say this is for the "-o v4" case.
> 
> In that case, the current code in nfs_nfs_version() will find the "v4"
> entry in nfs_version_opttbl[] and set
>   version_val = "4";
>   version->v_mode = V_SPECIFIC;
> then
>   version_major = 4;
> then as *cptr == '\0' and ->major > 4,
>   version->v_mode = V_GENERAL;
> 
> Your change skips that last step so it finished with
>    v_mod == V_SPECIFIC.
Right.. If v_mode has already set don't reset it.

> 
> According to the extra comments you have added for the modes:
> 
>>>> +	V_GENERAL,     /* single digit => 4 */
>>>> +	V_SPECIFIC,    /* single digit < 4 or decimal included */
> 
> And it seems to me that "v4" should be V_GENERAL, not V_SPECIFIC.
> So I think the current code is correct.
Personally I don't see a needed for V_GENERAL v_mode type
I guess it has to do with the specifying minor version or not
but if any thing is specified on the command line or 
nfsmount.conf then it is V_SPECIFIC... IMHO.

> 
> Except... nfs_try_mount() will then call nfs_autonegotiate(),
> and nfs_autonegotiate() isn't very consistent about how it
> interprets V_GENERAL and V_SPECIFIC.
> For EINVAL, it gets the difference right, for other errors it doesn't.
> 
> So I think that this is the fix you want:
> 
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index 0fbb37569ef9..98cf813fe439 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -838,9 +838,6 @@ check_result:
>  	case EINVAL:
>  		/* A less clear indication that our client
>  		 * does not support NFSv4 minor version. */
> -		if (mi->version.v_mode == V_GENERAL &&
> -			mi->version.minor == 0)
> -				return result;
>  		if (mi->version.v_mode != V_SPECIFIC) {
>  			if (mi->version.minor > 0) {
>  				mi->version.minor--;
> @@ -862,6 +859,9 @@ check_result:
>  		/* UDP-Only servers won't support v4, but maybe it
>  		 * just isn't ready yet.  So try v3, but double-check
>  		 * with rpcbind for v4. */
> +		if (mi->version.v_mode == V_GENERAL)
> +			/* Mustn't try v2,v3 */
> +			return result;
>  		result = nfs_try_mount_v3v2(mi, TRUE);
>  		if (result == 0 && errno == EAGAIN) {
>  			/* v4 server seems to be registered now. */
> @@ -878,6 +878,9 @@ check_result:
>  	}
>  
>  fall_back:
> +	if (mi->version.v_mode == V_GENERAL)
> +		/* v2,3 fallback not allowed */
> +		return result;
>  	return nfs_try_mount_v3v2(mi, FALSE);
>  }
>  
> 
> 
> I haven't even compile-tested of course :-)
I have and it does compile and work... Would you
mind reposting the patch in the proper format?
You can added Tested-by: Steve Dickson <steved@redhat.com>

Note: The second patch should probably use V_GENERAL as well.

tia,

steved.
> 
> Thanks,
> NeilBrown
> 
> 
>>
>> steved.
>>
>>>
>>> Thanks,
>>> NeilBrown
>>>
>>>
>>>>
>>>> Signed-off-by: Steve Dickson <steved@redhat.com>
>>>> ---
>>>>  utils/mount/network.c | 3 ++-
>>>>  utils/mount/network.h | 8 ++++----
>>>>  2 files changed, 6 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/utils/mount/network.c b/utils/mount/network.c
>>>> index 281e935..e39263e 100644
>>>> --- a/utils/mount/network.c
>>>> +++ b/utils/mount/network.c
>>>> @@ -1299,7 +1299,8 @@ nfs_nfs_version(struct mount_options *options, struct nfs_version *version)
>>>>  		if (!(version->minor = strtol(version_val, &cptr, 10)) && cptr == version_val)
>>>>  			goto ret_error;
>>>>  		version->v_mode = V_SPECIFIC;
>>>> -	} else if (version->major > 3 && *cptr == '\0')
>>>> +	} else if (version->major > 3 && *cptr == '\0' &&
>>>> +			version->v_mode == V_DEFAULT) /* v_mode has not been set */
>>>>  		version->v_mode = V_GENERAL;
>>>>  
>>>>  	if (*cptr != '\0')
>>>> diff --git a/utils/mount/network.h b/utils/mount/network.h
>>>> index 9cc5dec..45e2b24 100644
>>>> --- a/utils/mount/network.h
>>>> +++ b/utils/mount/network.h
>>>> @@ -58,10 +58,10 @@ int clnt_ping(struct sockaddr_in *, const unsigned long,
>>>>  struct mount_options;
>>>>  
>>>>  enum {
>>>> -	V_DEFAULT = 0,
>>>> -	V_GENERAL,
>>>> -	V_SPECIFIC,
>>>> -	V_PARSE_ERR,
>>>> +	V_DEFAULT = 0, /* not set */
>>>> +	V_GENERAL,     /* single digit => 4 */
>>>> +	V_SPECIFIC,    /* single digit < 4 or decimal included */
>>>> +	V_PARSE_ERR,   /* miss all others */
>>>>  };
>>>>  
>>>>  struct nfs_version {
>>>> -- 
>>>> 2.9.4
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mount.nfs: improve version negotiation when vers=4 is specified.

[NeilBrown]

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


If NFSv4, in general, is requested (possibly by -t nfs4 or -o v4 or -o
vers=4 etc) then we need to negotiate the best minor version, but must
not fallback to v3 or v2.  Internally, this state is reflected in v_mode
== V_GENERAL.  This means that a major version was given, but the minor
version still needs to be negotiated.

This is handled by nfs_autonegotiate(). It currently does the right
thing for EPROTONOSUPPORT and EINVAL, but not for other errors.
In particular, ENOENT can cause problems as  NFSv4 might export
a different namespace than NFSv3 (e.g. by using fsid=0 in the Linux
NFS server).  Currently a mount request for NFSv4 and a particular path
can result if an NFSv3 mount if the path is available with v3 but
not v4.

So move the special handling of V_GENERAL into the common fall_back:
code, and add extra checking in the ENCONNREFUSED case, which does
not use fall_back:.

Tested-by: Steve Dickson <steved@redhat.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---
 utils/mount/stropts.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 0fbb37569ef9..b20ad1c8bf55 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -838,9 +838,6 @@ check_result:
 	case EINVAL:
 		/* A less clear indication that our client
 		 * does not support NFSv4 minor version. */
-		if (mi->version.v_mode == V_GENERAL &&
-			mi->version.minor == 0)
-				return result;
 		if (mi->version.v_mode != V_SPECIFIC) {
 			if (mi->version.minor > 0) {
 				mi->version.minor--;
@@ -862,6 +859,9 @@ check_result:
 		/* UDP-Only servers won't support v4, but maybe it
 		 * just isn't ready yet.  So try v3, but double-check
 		 * with rpcbind for v4. */
+		if (mi->version.v_mode == V_GENERAL)
+			/* Mustn't try v2,v3 */
+			return result;
 		result = nfs_try_mount_v3v2(mi, TRUE);
 		if (result == 0 && errno == EAGAIN) {
 			/* v4 server seems to be registered now. */
@@ -878,6 +878,9 @@ check_result:
 	}
 
 fall_back:
+	if (mi->version.v_mode == V_GENERAL)
+		/* v2,3 fallback not allowed */
+		return result;
 	return nfs_try_mount_v3v2(mi, FALSE);
 }
 
-- 
2.12.2


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

Re: [PATCH] mount.nfs: improve version negotiation when vers=4 is specified.

[Steve Dickson]

On 05/31/2017 08:22 PM, NeilBrown wrote:
> 
> If NFSv4, in general, is requested (possibly by -t nfs4 or -o v4 or -o
> vers=4 etc) then we need to negotiate the best minor version, but must
> not fallback to v3 or v2.  Internally, this state is reflected in v_mode
> == V_GENERAL.  This means that a major version was given, but the minor
> version still needs to be negotiated.
> 
> This is handled by nfs_autonegotiate(). It currently does the right
> thing for EPROTONOSUPPORT and EINVAL, but not for other errors.
> In particular, ENOENT can cause problems as  NFSv4 might export
> a different namespace than NFSv3 (e.g. by using fsid=0 in the Linux
> NFS server).  Currently a mount request for NFSv4 and a particular path
> can result if an NFSv3 mount if the path is available with v3 but
> not v4.
> 
> So move the special handling of V_GENERAL into the common fall_back:
> code, and add extra checking in the ENCONNREFUSED case, which does
> not use fall_back:.
> 
> Tested-by: Steve Dickson <steved@redhat.com>
> Signed-off-by: NeilBrown <neilb@suse.com>
Committed... with the addition change to nfs_set_version() 
which stops -t nfs4 from rolling back to v3.

Thanks! 

steved.
 
> ---
>  utils/mount/stropts.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index 0fbb37569ef9..b20ad1c8bf55 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -838,9 +838,6 @@ check_result:
>  	case EINVAL:
>  		/* A less clear indication that our client
>  		 * does not support NFSv4 minor version. */
> -		if (mi->version.v_mode == V_GENERAL &&
> -			mi->version.minor == 0)
> -				return result;
>  		if (mi->version.v_mode != V_SPECIFIC) {
>  			if (mi->version.minor > 0) {
>  				mi->version.minor--;
> @@ -862,6 +859,9 @@ check_result:
>  		/* UDP-Only servers won't support v4, but maybe it
>  		 * just isn't ready yet.  So try v3, but double-check
>  		 * with rpcbind for v4. */
> +		if (mi->version.v_mode == V_GENERAL)
> +			/* Mustn't try v2,v3 */
> +			return result;
>  		result = nfs_try_mount_v3v2(mi, TRUE);
>  		if (result == 0 && errno == EAGAIN) {
>  			/* v4 server seems to be registered now. */
> @@ -878,6 +878,9 @@ check_result:
>  	}
>  
>  fall_back:
> +	if (mi->version.v_mode == V_GENERAL)
> +		/* v2,3 fallback not allowed */
> +		return result;
>  	return nfs_try_mount_v3v2(mi, FALSE);
>  }
>  
> 

Re: [PATCH 1/2] mount.nfs: v4 mounts should fail when -o flag is used.

[NeilBrown]

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

On Wed, May 31 2017, Steve Dickson wrote:

> Sorry for the delayed response... that damn 
> flux capacitor broke... again! ;-) 

That's what you get for buying it on e-bay??


>> 
>> According to the extra comments you have added for the modes:
>> 
>>>>> +	V_GENERAL,     /* single digit => 4 */
>>>>> +	V_SPECIFIC,    /* single digit < 4 or decimal included */
>> 
>> And it seems to me that "v4" should be V_GENERAL, not V_SPECIFIC.
>> So I think the current code is correct.
> Personally I don't see a needed for V_GENERAL v_mode type
> I guess it has to do with the specifying minor version or not
> but if any thing is specified on the command line or 
> nfsmount.conf then it is V_SPECIFIC... IMHO.

Maybe V_GENERAL should be V_MAJOR or V_SPECIFIC_MAJOR.
The v4 code assumes that if V_SPECIFIC is set, then
the version option provided to the mount command
can be passed unchanged to the kernel.
So it sometimes means V_NO_NEGOTIATE.

>> I haven't even compile-tested of course :-)
> I have and it does compile and work... Would you
> mind reposting the patch in the proper format?
> You can added Tested-by: Steve Dickson <steved@redhat.com>

Done.

>
> Note: The second patch should probably use V_GENERAL as well.

Yes, definitely.

Thanks,
NeilBrown

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