mount.nfs: v4 mounts should fail when -o flag is used.
Subsystems (2)
PATCH 1/2 mount.nfs: v4 mounts should fail when -o flag is used.
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.
--- utils/mount/network.c | 3 ++- utils/mount/network.h | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-)
utils/mount/network.c +2 -1
@@ -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' )
utils/mount/network.h +4 -4
@@ -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 {
Signature · 1 line
2.9.4
PATCH 2/2 mount.nfs: v4 mounts should fail when nfs4 is specified with -t flag
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.
--- utils/mount/stropts.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
utils/mount/stropts.c +3 -2
@@ -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
Signature · 1 line
2.9.4
Quoted 4 lines · 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.
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
Quoted 47 lines
Signed-off-by Steve Dickson <[email protected]>--- utils/mount/network.c | 3 ++- utils/mount/network.h | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-)utils/mount/network.c +2
-1@@ -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' )utils/mount/network.h +4
-4@@ -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 {Signature · 6 lines
2.9.4 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 10 lines · NeilBrown
Quoted 4 lines · 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.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.
Quoted 5 lines
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
Quoted 2 lines
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.
Quoted 52 lines
Thanks,
NeilBrownQuoted 47 lines
Signed-off-by Steve Dickson <[email protected]>--- utils/mount/network.c | 3 ++- utils/mount/network.h | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-)utils/mount/network.c +2
-1@@ -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' )utils/mount/network.h +4
-4@@ -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 {Signature · 6 lines
2.9.4 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 29 lines · Steve Dickson
Quoted 10 lines · NeilBrown
Quoted 4 lines · 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.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 /mntthe mount should fail instead of rolling back to v3 Basically its be used to cause the error.
Quoted 5 lines
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
Quoted 1 line
Quoted 7 lines
Quoted 1 line
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:
Quoted 2 lines
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ 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:
utils/mount/stropts.c +6 -3
@@ -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
Quoted 55 lines
steved.
Quoted 52 lines
Thanks,
NeilBrownQuoted 47 lines
Signed-off-by Steve Dickson <[email protected]>--- utils/mount/network.c | 3 ++- utils/mount/network.h | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-)utils/mount/network.c +2
-1@@ -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' )utils/mount/network.h +4
-4@@ -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 {Signature · 6 lines
2.9.4 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Sorry for the delayed response... that damn
flux capacitor broke... again! ;-)
Quoted 39 lines · NeilBrown
Quoted 29 lines · Steve Dickson
Quoted 10 lines · NeilBrown
Quoted 4 lines · 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.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 /mntthe mount should fail instead of rolling back to v3 Basically its be used to cause the error.
Quoted 5 lines
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
Quoted 1 line
Quoted 7 lines
Quoted 1 line
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.
Quoted 4 lines
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.
Quoted 14 lines
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;
Quoted 2 lines
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.
Quoted 8 lines
According to the extra comments you have added for the modes:
Quoted 2 lines
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ V_GENERAL, /* single digit => 4 */ + V_SPECIFIC, /* single digit < 4 or decimal included */And it seems to me that "v4" should be
V_GENERAL, notV_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.
Quoted 44 lines
Except...
nfs_try_mount()will then callnfs_autonegotiate(), andnfs_autonegotiate()isn't very consistent about how it interpretsV_GENERALandV_SPECIFIC.
For EINVAL, it gets the difference right, for other errors it doesn't.So I think that this is the fix you want:
utils/mount/stropts.c +6
-3@@ -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); }
Quoted 1 line
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 <[email protected]>
Note: The second patch should probably use V_GENERAL as well.
tia,
steved.
Quoted 60 lines
Thanks,
NeilBrownQuoted 55 lines
steved.
Quoted 52 lines
Thanks,
NeilBrownQuoted 47 lines
Signed-off-by Steve Dickson <[email protected]>--- utils/mount/network.c | 3 ++- utils/mount/network.h | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-)utils/mount/network.c +2
-1@@ -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' )utils/mount/network.h +4
-4@@ -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 {Signature · 6 lines
2.9.4 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 1/1 mount.nfs: improve version negotiation when vers=4 is specified.
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:.
--- utils/mount/stropts.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
utils/mount/stropts.c +6 -3
@@ -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); }
Signature · 1 line
2.12.2
Re: [PATCH] mount.nfs: improve version negotiation when vers=4 is specified.
Quoted 15 lines · NeilBrown
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.
Quoted 6 lines
So move the special handling of
V_GENERALinto the commonfall_back:
code, and add extra checking in the ENCONNREFUSED case, which does not usefall_back:.Tested-by Steve Dickson <[email protected]>Signed-off-by NeilBrown <[email protected]>
Committed... with the addition change to nfs_set_version() which stops -t nfs4 from rolling back to v3.
Thanks!
steved.
Quoted 39 lines
--- utils/mount/stropts.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)utils/mount/stropts.c +6
-3@@ -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); }
Quoted 2 lines · Steve Dickson
Sorry for the delayed response... that damn
flux capacitor broke... again! ;-)
That's what you get for buying it on e-bay??
Quoted 8 lines
Quoted 8 lines
According to the extra comments you have added for the modes:
Quoted 2 lines
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ V_GENERAL, /* single digit => 4 */ + V_SPECIFIC, /* single digit < 4 or decimal included */And it seems to me that "v4" should be
V_GENERAL, notV_SPECIFIC. So I think the current code is correct.
Quoted 4 lines
Personally I don't see a needed for
V_GENERALv_modetype 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 isV_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.
Quoted 4 lines
Quoted 1 line
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 <[email protected]>
Done.
Quoted 2 lines
Note: The second patch should probably use
V_GENERALas well.
Yes, definitely.
Thanks,
NeilBrown