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

Message ID 877f18jsx5.fsf@notabene.neil.brown.name
State New
Headers show

Commit Message

NeilBrown May 23, 2017, 12:52 a.m. UTC
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:



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

Comments

Steve Dickson May 31, 2017, 3:33 p.m. UTC | #1
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
--
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
NeilBrown June 1, 2017, 12:27 a.m. UTC | #2
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

Patch
diff mbox

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);
 }