[PATCH 0/2] nfs: allow NFSv3 to fall back to AUTH_UNIX if initial auth selection fails

[PATCH 0/2] nfs: allow NFSv3 to fall back to AUTH_UNIX if initial auth selection fails

[Jeff Layton]

I got a report of a regression in recent kernels. Windows 2012 servers
support v3 and v4.1. They also return a list of authflavors that starts
with AUTH_GSS flavors and ends with AUTH_SYS.

Since commit 4580a92 (NFS: Use server-recommended security flavor by
default (NFSv3)) mounting this server with nfsv3 fails unless you
specify sec=sys.

NFSv4.0 has code that allows it to fall back to using AUTH_SYS if the
initial attempt to use AUTH_GSS fails (in
nfs4_discover_server_trunking()). This is an attempt to make the v3
mounting code just as resilient in the same situation.

Jeff Layton (2):
  nfs: make nfs_select_flavor take a list of authflavors and a length
  nfs: allow NFSv3 to fall back to using AUTH_UNIX automatically if
    available

 fs/nfs/super.c | 51 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 14 deletions(-)

-- 
1.8.1.4

[PATCH 1/2] nfs: make nfs_select_flavor take a list of authflavors and a length

[Jeff Layton]

We don't actually need a nfs_mount_request structure here, and by
doing this we can call this function outside of nfs_request_mount().

Cc: Chuck Lever <chuck.lever@oracle.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/super.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 2d7525f..5538bcc 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1614,9 +1614,9 @@ out_security_failure:
  * Returns 0 on success, -EACCES on failure.
  */
 static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
-			      struct nfs_mount_request *request)
+			unsigned int count, rpc_authflavor_t *server_authlist)
 {
-	unsigned int i, count = *(request->auth_flav_len);
+	unsigned int i;
 	rpc_authflavor_t flavor;
 
 	/*
@@ -1642,8 +1642,8 @@ static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
 	 */
 	if (args->auth_flavors[0] != RPC_AUTH_MAXFLAVOR) {
 		for (i = 0; i < count; i++) {
-			if (args->auth_flavors[0] == request->auth_flavs[i] ||
-			    request->auth_flavs[i] == RPC_AUTH_NULL)
+			if (args->auth_flavors[0] == server_authlist[i] ||
+			    server_authlist[i] == RPC_AUTH_NULL)
 				goto out;
 		}
 		dfprintk(MOUNT, "NFS: auth flavor %d not supported by server\n",
@@ -1659,7 +1659,7 @@ static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
 	for (i = 0; i < count; i++) {
 		struct rpcsec_gss_info info;
 
-		flavor = request->auth_flavs[i];
+		flavor = server_authlist[i];
 		switch (flavor) {
 		case RPC_AUTH_UNIX:
 			goto out_set;
@@ -1676,7 +1676,7 @@ static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
 	 * if it does, use the default flavor.
 	 */
 	for (i = 0; i < count; i++) {
-		if (request->auth_flavs[i] == RPC_AUTH_NULL)
+		if (server_authlist[i] == RPC_AUTH_NULL)
 			goto out_default;
 	}
 
@@ -1756,7 +1756,7 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
 		return status;
 	}
 
-	return nfs_select_flavor(args, &request);
+	return nfs_select_flavor(args, server_authlist_len, server_authlist);
 }
 
 struct dentry *nfs_try_mount(int flags, const char *dev_name,
-- 
1.8.1.4

[PATCH 2/2] nfs: allow NFSv3 to fall back to using AUTH_UNIX automatically if available

[Jeff Layton]

Currently, when using NFSv3 the mount will fail if the server happens to
have AUTH_GSS flavors in the returned authlist before AUTH_UNIX. This
seems to have been a deliberate change in commit 4580a92 (NFS: Use
server-recommended security flavor by default (NFSv3)).

While the workarounds are fine, I think we can do better here and allow
this to keep "just working". Allow the client to fall back to
automatically trying AUTH_UNIX under if the following are all true:

    - the server return -EACCES from ->create_server call
    - the client had to do a MNT request (i.e. no binary options)
    - we didn't just try to use AUTH_UNIX
    - the admin did not explcitly specify a sec= option

At that point, try to use AUTH_UNIX, if the server listed it.

Cc: Chuck Lever <chuck.lever@oracle.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/super.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 5538bcc..6df327e 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1701,10 +1701,10 @@ out_err:
  * corresponding to the provided path.
  */
 static int nfs_request_mount(struct nfs_parsed_mount_data *args,
-			     struct nfs_fh *root_fh)
+			     struct nfs_fh *root_fh,
+			     unsigned int *server_authlist_len,
+			     rpc_authflavor_t *server_authlist)
 {
-	rpc_authflavor_t server_authlist[NFS_MAX_SECFLAVORS];
-	unsigned int server_authlist_len = ARRAY_SIZE(server_authlist);
 	struct nfs_mount_request request = {
 		.sap		= (struct sockaddr *)
 						&args->mount_server.address,
@@ -1712,7 +1712,7 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
 		.protocol	= args->mount_server.protocol,
 		.fh		= root_fh,
 		.noresvport	= args->flags & NFS_MOUNT_NORESVPORT,
-		.auth_flav_len	= &server_authlist_len,
+		.auth_flav_len	= server_authlist_len,
 		.auth_flavs	= server_authlist,
 		.net		= args->net,
 	};
@@ -1756,7 +1756,7 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
 		return status;
 	}
 
-	return nfs_select_flavor(args, server_authlist_len, server_authlist);
+	return 0;
 }
 
 struct dentry *nfs_try_mount(int flags, const char *dev_name,
@@ -1765,17 +1765,40 @@ struct dentry *nfs_try_mount(int flags, const char *dev_name,
 {
 	int status;
 	struct nfs_server *server;
+	struct nfs_parsed_mount_data *args = mount_info->parsed;
+	rpc_authflavor_t requested_authflavor = args->auth_flavors[0];
+	rpc_authflavor_t server_authlist[NFS_MAX_SECFLAVORS];
+	unsigned int server_authlist_len = ARRAY_SIZE(server_authlist);
 
-	if (mount_info->parsed->need_mount) {
-		status = nfs_request_mount(mount_info->parsed, mount_info->mntfh);
+	if (args->need_mount) {
+		status = nfs_request_mount(args, mount_info->mntfh,
+					&server_authlist_len, server_authlist);
+		if (status)
+			return ERR_PTR(status);
+retry:
+		status = nfs_select_flavor(args, server_authlist_len,
+						server_authlist);
 		if (status)
 			return ERR_PTR(status);
 	}
 
 	/* Get a volume representation */
 	server = nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
-	if (IS_ERR(server))
+	if (IS_ERR(server)) {
+		/*
+		 * If the initial attempt fails with -EACCES, then that likely
+		 * means that we couldn't get proper credentials to talk to the
+		 * server. If an authflavor wasn't specified in the options,
+		 * and we didn't try to use it already, then try AUTH_UNIX.
+		 */
+		if (PTR_ERR(server) == -EACCES && args->need_mount &&
+		    args->auth_flavors[0] != RPC_AUTH_UNIX &&
+		    requested_authflavor == RPC_AUTH_MAXFLAVOR) {
+			args->auth_flavors[0] = RPC_AUTH_UNIX;
+			goto retry;
+		}
 		return ERR_CAST(server);
+	}
 
 	return nfs_fs_mount_common(server, flags, dev_name, mount_info, nfs_mod);
 }
-- 
1.8.1.4

Re: [PATCH 2/2] nfs: allow NFSv3 to fall back to using AUTH_UNIX automatically if available

[Chuck Lever]

On Jun 26, 2013, at 10:36 AM, Jeff Layton <jlayton@redhat.com> wrote:

> Currently, when using NFSv3 the mount will fail if the server happens to
> have AUTH_GSS flavors in the returned authlist before AUTH_UNIX. This
> seems to have been a deliberate change in commit 4580a92 (NFS: Use
> server-recommended security flavor by default (NFSv3)).

As an aside, this (from the patch description for 4580a92):

>     If a server lists Kerberos pseudoflavors before "sys" in its export
>     options, our client now chooses Kerberos over AUTH_UNIX for mount
>     points, when no security flavor is specified by the mount command.
>     This could be surprising to some administrators or users, who would
>     then need to have Kerberos credentials to access the export.


is a description of side-effects of the changes in 4580a92.  This text is intended as a warning that behavior could change after 4580a92, not as a statement of purpose.  It describes a known limitation of the approach introduced in 4580a92.

> While the workarounds are fine, I think we can do better here and allow
> this to keep "just working". Allow the client to fall back to
> automatically trying AUTH_UNIX under if the following are all true:
> 
>    - the server return -EACCES from ->create_server call
>    - the client had to do a MNT request (i.e. no binary options)
>    - we didn't just try to use AUTH_UNIX
>    - the admin did not explcitly specify a sec= option
> 
> At that point, try to use AUTH_UNIX, if the server listed it.

During these checks, how do you know the server specified AUTH_SYS in its list?  It seems to me you want to retry with the next flavor in server_authlist until you've exhausted the list.

Thus, IMO, the code you want to emulate is not nfs4_discover_server_trunking(), but rather nfs4_find_root_sec(), substituting the flavor list returned by the server for the static array of flavors.

> Cc: Chuck Lever <chuck.lever@oracle.com>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> fs/nfs/super.c | 39 +++++++++++++++++++++++++++++++--------
> 1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 5538bcc..6df327e 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -1701,10 +1701,10 @@ out_err:
>  * corresponding to the provided path.
>  */
> static int nfs_request_mount(struct nfs_parsed_mount_data *args,
> -			     struct nfs_fh *root_fh)
> +			     struct nfs_fh *root_fh,
> +			     unsigned int *server_authlist_len,
> +			     rpc_authflavor_t *server_authlist)
> {
> -	rpc_authflavor_t server_authlist[NFS_MAX_SECFLAVORS];
> -	unsigned int server_authlist_len = ARRAY_SIZE(server_authlist);
> 	struct nfs_mount_request request = {
> 		.sap		= (struct sockaddr *)
> 						&args->mount_server.address,
> @@ -1712,7 +1712,7 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
> 		.protocol	= args->mount_server.protocol,
> 		.fh		= root_fh,
> 		.noresvport	= args->flags & NFS_MOUNT_NORESVPORT,
> -		.auth_flav_len	= &server_authlist_len,
> +		.auth_flav_len	= server_authlist_len,
> 		.auth_flavs	= server_authlist,
> 		.net		= args->net,
> 	};
> @@ -1756,7 +1756,7 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
> 		return status;
> 	}
> 
> -	return nfs_select_flavor(args, server_authlist_len, server_authlist);
> +	return 0;
> }
> 
> struct dentry *nfs_try_mount(int flags, const char *dev_name,
> @@ -1765,17 +1765,40 @@ struct dentry *nfs_try_mount(int flags, const char *dev_name,
> {
> 	int status;
> 	struct nfs_server *server;
> +	struct nfs_parsed_mount_data *args = mount_info->parsed;
> +	rpc_authflavor_t requested_authflavor = args->auth_flavors[0];
> +	rpc_authflavor_t server_authlist[NFS_MAX_SECFLAVORS];
> +	unsigned int server_authlist_len = ARRAY_SIZE(server_authlist);
> 
> -	if (mount_info->parsed->need_mount) {
> -		status = nfs_request_mount(mount_info->parsed, mount_info->mntfh);
> +	if (args->need_mount) {
> +		status = nfs_request_mount(args, mount_info->mntfh,
> +					&server_authlist_len, server_authlist);
> +		if (status)
> +			return ERR_PTR(status);
> +retry:
> +		status = nfs_select_flavor(args, server_authlist_len,
> +						server_authlist);
> 		if (status)
> 			return ERR_PTR(status);
> 	}
> 
> 	/* Get a volume representation */
> 	server = nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
> -	if (IS_ERR(server))
> +	if (IS_ERR(server)) {
> +		/*
> +		 * If the initial attempt fails with -EACCES, then that likely
> +		 * means that we couldn't get proper credentials to talk to the
> +		 * server. If an authflavor wasn't specified in the options,
> +		 * and we didn't try to use it already, then try AUTH_UNIX.
> +		 */
> +		if (PTR_ERR(server) == -EACCES && args->need_mount &&
> +		    args->auth_flavors[0] != RPC_AUTH_UNIX &&
> +		    requested_authflavor == RPC_AUTH_MAXFLAVOR) {
> +			args->auth_flavors[0] = RPC_AUTH_UNIX;

This might be a lot easier to read if you refactored nfs_try_mount() into two subfunctions: one which handles the args->need_mount case, and the other which handles the opposite case.

> +			goto retry;
> +		}
> 		return ERR_CAST(server);
> +	}
> 
> 	return nfs_fs_mount_common(server, flags, dev_name, mount_info, nfs_mod);
> }
> -- 
> 1.8.1.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

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 2/2] nfs: allow NFSv3 to fall back to using AUTH_UNIX automatically if available

[Jeff Layton]

On Wed, 26 Jun 2013 11:15:08 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Jun 26, 2013, at 10:36 AM, Jeff Layton <jlayton@redhat.com> wrote:
> 
> > Currently, when using NFSv3 the mount will fail if the server happens to
> > have AUTH_GSS flavors in the returned authlist before AUTH_UNIX. This
> > seems to have been a deliberate change in commit 4580a92 (NFS: Use
> > server-recommended security flavor by default (NFSv3)).
> 
> As an aside, this (from the patch description for 4580a92):
> 
> >     If a server lists Kerberos pseudoflavors before "sys" in its export
> >     options, our client now chooses Kerberos over AUTH_UNIX for mount
> >     points, when no security flavor is specified by the mount command.
> >     This could be surprising to some administrators or users, who would
> >     then need to have Kerberos credentials to access the export.
> 
> 
> is a description of side-effects of the changes in 4580a92.  This text is intended as a warning that behavior could change after 4580a92, not as a statement of purpose.  It describes a known limitation of the approach introduced in 4580a92.
> 

Ok, good to know...

> > While the workarounds are fine, I think we can do better here and allow
> > this to keep "just working". Allow the client to fall back to
> > automatically trying AUTH_UNIX under if the following are all true:
> > 
> >    - the server return -EACCES from ->create_server call
> >    - the client had to do a MNT request (i.e. no binary options)
> >    - we didn't just try to use AUTH_UNIX
> >    - the admin did not explcitly specify a sec= option
> > 
> > At that point, try to use AUTH_UNIX, if the server listed it.
> 
> During these checks, how do you know the server specified AUTH_SYS in its list?  It seems to me you want to retry with the next flavor in server_authlist until you've exhausted the list.
> 
> Thus, IMO, the code you want to emulate is not nfs4_discover_server_trunking(), but rather nfs4_find_root_sec(), substituting the flavor list returned by the server for the static array of flavors.
> 

Possibly. Often, we get a list that looks something like this:

[  379.237691] NFS: received 4 auth flavors
[  379.257005] NFS:   auth flavor[0]: 390005
[  379.278296] NFS:   auth flavor[1]: 390004
[  379.298526] NFS:   auth flavor[2]: 390003
[  379.318313] NFS:   auth flavor[3]: 1

So we could instead walk down the list and try each in turn, but how
likely is it that one of the other AUTH_GSS flavors will work once the
first one failed? I'm not sure it's worth adding the extra complexity
to do that when it's likely that the only one that will eventually work
is the AUTH_SYS one anyway.

> > Cc: Chuck Lever <chuck.lever@oracle.com>
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> > fs/nfs/super.c | 39 +++++++++++++++++++++++++++++++--------
> > 1 file changed, 31 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> > index 5538bcc..6df327e 100644
> > --- a/fs/nfs/super.c
> > +++ b/fs/nfs/super.c
> > @@ -1701,10 +1701,10 @@ out_err:
> >  * corresponding to the provided path.
> >  */
> > static int nfs_request_mount(struct nfs_parsed_mount_data *args,
> > -			     struct nfs_fh *root_fh)
> > +			     struct nfs_fh *root_fh,
> > +			     unsigned int *server_authlist_len,
> > +			     rpc_authflavor_t *server_authlist)
> > {
> > -	rpc_authflavor_t server_authlist[NFS_MAX_SECFLAVORS];
> > -	unsigned int server_authlist_len = ARRAY_SIZE(server_authlist);
> > 	struct nfs_mount_request request = {
> > 		.sap		= (struct sockaddr *)
> > 						&args->mount_server.address,
> > @@ -1712,7 +1712,7 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
> > 		.protocol	= args->mount_server.protocol,
> > 		.fh		= root_fh,
> > 		.noresvport	= args->flags & NFS_MOUNT_NORESVPORT,
> > -		.auth_flav_len	= &server_authlist_len,
> > +		.auth_flav_len	= server_authlist_len,
> > 		.auth_flavs	= server_authlist,
> > 		.net		= args->net,
> > 	};
> > @@ -1756,7 +1756,7 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
> > 		return status;
> > 	}
> > 
> > -	return nfs_select_flavor(args, server_authlist_len, server_authlist);
> > +	return 0;
> > }
> > 
> > struct dentry *nfs_try_mount(int flags, const char *dev_name,
> > @@ -1765,17 +1765,40 @@ struct dentry *nfs_try_mount(int flags, const char *dev_name,
> > {
> > 	int status;
> > 	struct nfs_server *server;
> > +	struct nfs_parsed_mount_data *args = mount_info->parsed;
> > +	rpc_authflavor_t requested_authflavor = args->auth_flavors[0];
> > +	rpc_authflavor_t server_authlist[NFS_MAX_SECFLAVORS];
> > +	unsigned int server_authlist_len = ARRAY_SIZE(server_authlist);
> > 
> > -	if (mount_info->parsed->need_mount) {
> > -		status = nfs_request_mount(mount_info->parsed, mount_info->mntfh);
> > +	if (args->need_mount) {
> > +		status = nfs_request_mount(args, mount_info->mntfh,
> > +					&server_authlist_len, server_authlist);
> > +		if (status)
> > +			return ERR_PTR(status);
> > +retry:
> > +		status = nfs_select_flavor(args, server_authlist_len,
> > +						server_authlist);
> > 		if (status)
> > 			return ERR_PTR(status);
> > 	}
> > 
> > 	/* Get a volume representation */
> > 	server = nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
> > -	if (IS_ERR(server))
> > +	if (IS_ERR(server)) {
> > +		/*
> > +		 * If the initial attempt fails with -EACCES, then that likely
> > +		 * means that we couldn't get proper credentials to talk to the
> > +		 * server. If an authflavor wasn't specified in the options,
> > +		 * and we didn't try to use it already, then try AUTH_UNIX.
> > +		 */
> > +		if (PTR_ERR(server) == -EACCES && args->need_mount &&
> > +		    args->auth_flavors[0] != RPC_AUTH_UNIX &&
> > +		    requested_authflavor == RPC_AUTH_MAXFLAVOR) {
> > +			args->auth_flavors[0] = RPC_AUTH_UNIX;
> 
> This might be a lot easier to read if you refactored nfs_try_mount() into two subfunctions: one which handles the args->need_mount case, and the other which handles the opposite case.
> 

Good point. I'll look into doing that.

> > +			goto retry;
> > +		}
> > 		return ERR_CAST(server);
> > +	}
> > 
> > 	return nfs_fs_mount_common(server, flags, dev_name, mount_info, nfs_mod);
> > }
> > -- 
> > 1.8.1.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
> 

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/2] nfs: allow NFSv3 to fall back to using AUTH_UNIX automatically if available

[Jeff Layton]

On Wed, 26 Jun 2013 11:15:08 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Jun 26, 2013, at 10:36 AM, Jeff Layton <jlayton@redhat.com> wrote:
> 
> > Currently, when using NFSv3 the mount will fail if the server happens to
> > have AUTH_GSS flavors in the returned authlist before AUTH_UNIX. This
> > seems to have been a deliberate change in commit 4580a92 (NFS: Use
> > server-recommended security flavor by default (NFSv3)).
> 
> As an aside, this (from the patch description for 4580a92):
> 
> >     If a server lists Kerberos pseudoflavors before "sys" in its export
> >     options, our client now chooses Kerberos over AUTH_UNIX for mount
> >     points, when no security flavor is specified by the mount command.
> >     This could be surprising to some administrators or users, who would
> >     then need to have Kerberos credentials to access the export.
> 
> 
> is a description of side-effects of the changes in 4580a92.  This text is intended as a warning that behavior could change after 4580a92, not as a statement of purpose.  It describes a known limitation of the approach introduced in 4580a92.
> 
> > While the workarounds are fine, I think we can do better here and allow
> > this to keep "just working". Allow the client to fall back to
> > automatically trying AUTH_UNIX under if the following are all true:
> > 
> >    - the server return -EACCES from ->create_server call
> >    - the client had to do a MNT request (i.e. no binary options)
> >    - we didn't just try to use AUTH_UNIX
> >    - the admin did not explcitly specify a sec= option
> > 
> > At that point, try to use AUTH_UNIX, if the server listed it.
> 
> During these checks, how do you know the server specified AUTH_SYS in its list?  It seems to me you want to retry with the next flavor in server_authlist until you've exhausted the list.
> 

Oh and to answer your question, we don't know that at this point, but
it won't matter.

This patch sets args->auth_flavors[0] = RPC_AUTH_UNIX and then has it
call nfs_select_flavor() again. If the server didn't have AUTH_UNIX in
its list, then that function will fail at that point and we can just
return the error.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/2] nfs: allow NFSv3 to fall back to using AUTH_UNIX automatically if available

[Chuck Lever]

On Jun 26, 2013, at 11:24 AM, Jeff Layton <jlayton@redhat.com> wrote:

> On Wed, 26 Jun 2013 11:15:08 -0400
> Chuck Lever <chuck.lever@oracle.com> wrote:
> 
>> 
>> On Jun 26, 2013, at 10:36 AM, Jeff Layton <jlayton@redhat.com> wrote:
>> 
>>> Currently, when using NFSv3 the mount will fail if the server happens to
>>> have AUTH_GSS flavors in the returned authlist before AUTH_UNIX. This
>>> seems to have been a deliberate change in commit 4580a92 (NFS: Use
>>> server-recommended security flavor by default (NFSv3)).
>> 
>> As an aside, this (from the patch description for 4580a92):
>> 
>>>    If a server lists Kerberos pseudoflavors before "sys" in its export
>>>    options, our client now chooses Kerberos over AUTH_UNIX for mount
>>>    points, when no security flavor is specified by the mount command.
>>>    This could be surprising to some administrators or users, who would
>>>    then need to have Kerberos credentials to access the export.
>> 
>> 
>> is a description of side-effects of the changes in 4580a92.  This text is intended as a warning that behavior could change after 4580a92, not as a statement of purpose.  It describes a known limitation of the approach introduced in 4580a92.
>> 
> 
> Ok, good to know...
> 
>>> While the workarounds are fine, I think we can do better here and allow
>>> this to keep "just working". Allow the client to fall back to
>>> automatically trying AUTH_UNIX under if the following are all true:
>>> 
>>>   - the server return -EACCES from ->create_server call
>>>   - the client had to do a MNT request (i.e. no binary options)
>>>   - we didn't just try to use AUTH_UNIX
>>>   - the admin did not explcitly specify a sec= option
>>> 
>>> At that point, try to use AUTH_UNIX, if the server listed it.
>> 
>> During these checks, how do you know the server specified AUTH_SYS in its list?  It seems to me you want to retry with the next flavor in server_authlist until you've exhausted the list.
>> 
>> Thus, IMO, the code you want to emulate is not nfs4_discover_server_trunking(), but rather nfs4_find_root_sec(), substituting the flavor list returned by the server for the static array of flavors.
>> 
> 
> Possibly. Often, we get a list that looks something like this:
> 
> [  379.237691] NFS: received 4 auth flavors
> [  379.257005] NFS:   auth flavor[0]: 390005
> [  379.278296] NFS:   auth flavor[1]: 390004
> [  379.298526] NFS:   auth flavor[2]: 390003
> [  379.318313] NFS:   auth flavor[3]: 1
> 
> So we could instead walk down the list and try each in turn, but how
> likely is it that one of the other AUTH_GSS flavors will work once the
> first one failed?

I don't think the client can tell with generality.  -EACCES from ->create_server() may be a result of IP-based access control on the server, for example.

> I'm not sure it's worth adding the extra complexity
> to do that when it's likely that the only one that will eventually work
> is the AUTH_SYS one anyway.

Based on the size and complexity of nfs4_find_root_sec(), I don't think trying flavors in a loop will be complex, and the flavor list is already very short.

> 
>>> Cc: Chuck Lever <chuck.lever@oracle.com>
>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>> ---
>>> fs/nfs/super.c | 39 +++++++++++++++++++++++++++++++--------
>>> 1 file changed, 31 insertions(+), 8 deletions(-)
>>> 
>>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>>> index 5538bcc..6df327e 100644
>>> --- a/fs/nfs/super.c
>>> +++ b/fs/nfs/super.c
>>> @@ -1701,10 +1701,10 @@ out_err:
>>> * corresponding to the provided path.
>>> */
>>> static int nfs_request_mount(struct nfs_parsed_mount_data *args,
>>> -			     struct nfs_fh *root_fh)
>>> +			     struct nfs_fh *root_fh,
>>> +			     unsigned int *server_authlist_len,
>>> +			     rpc_authflavor_t *server_authlist)
>>> {
>>> -	rpc_authflavor_t server_authlist[NFS_MAX_SECFLAVORS];
>>> -	unsigned int server_authlist_len = ARRAY_SIZE(server_authlist);
>>> 	struct nfs_mount_request request = {
>>> 		.sap		= (struct sockaddr *)
>>> 						&args->mount_server.address,
>>> @@ -1712,7 +1712,7 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
>>> 		.protocol	= args->mount_server.protocol,
>>> 		.fh		= root_fh,
>>> 		.noresvport	= args->flags & NFS_MOUNT_NORESVPORT,
>>> -		.auth_flav_len	= &server_authlist_len,
>>> +		.auth_flav_len	= server_authlist_len,
>>> 		.auth_flavs	= server_authlist,
>>> 		.net		= args->net,
>>> 	};
>>> @@ -1756,7 +1756,7 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
>>> 		return status;
>>> 	}
>>> 
>>> -	return nfs_select_flavor(args, server_authlist_len, server_authlist);
>>> +	return 0;
>>> }
>>> 
>>> struct dentry *nfs_try_mount(int flags, const char *dev_name,
>>> @@ -1765,17 +1765,40 @@ struct dentry *nfs_try_mount(int flags, const char *dev_name,
>>> {
>>> 	int status;
>>> 	struct nfs_server *server;
>>> +	struct nfs_parsed_mount_data *args = mount_info->parsed;
>>> +	rpc_authflavor_t requested_authflavor = args->auth_flavors[0];
>>> +	rpc_authflavor_t server_authlist[NFS_MAX_SECFLAVORS];
>>> +	unsigned int server_authlist_len = ARRAY_SIZE(server_authlist);
>>> 
>>> -	if (mount_info->parsed->need_mount) {
>>> -		status = nfs_request_mount(mount_info->parsed, mount_info->mntfh);
>>> +	if (args->need_mount) {
>>> +		status = nfs_request_mount(args, mount_info->mntfh,
>>> +					&server_authlist_len, server_authlist);
>>> +		if (status)
>>> +			return ERR_PTR(status);
>>> +retry:
>>> +		status = nfs_select_flavor(args, server_authlist_len,
>>> +						server_authlist);
>>> 		if (status)
>>> 			return ERR_PTR(status);
>>> 	}
>>> 
>>> 	/* Get a volume representation */
>>> 	server = nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
>>> -	if (IS_ERR(server))
>>> +	if (IS_ERR(server)) {
>>> +		/*
>>> +		 * If the initial attempt fails with -EACCES, then that likely
>>> +		 * means that we couldn't get proper credentials to talk to the
>>> +		 * server. If an authflavor wasn't specified in the options,
>>> +		 * and we didn't try to use it already, then try AUTH_UNIX.
>>> +		 */
>>> +		if (PTR_ERR(server) == -EACCES && args->need_mount &&
>>> +		    args->auth_flavors[0] != RPC_AUTH_UNIX &&
>>> +		    requested_authflavor == RPC_AUTH_MAXFLAVOR) {
>>> +			args->auth_flavors[0] = RPC_AUTH_UNIX;
>> 
>> This might be a lot easier to read if you refactored nfs_try_mount() into two subfunctions: one which handles the args->need_mount case, and the other which handles the opposite case.
>> 
> 
> Good point. I'll look into doing that.
> 
>>> +			goto retry;
>>> +		}
>>> 		return ERR_CAST(server);
>>> +	}
>>> 
>>> 	return nfs_fs_mount_common(server, flags, dev_name, mount_info, nfs_mod);
>>> }
>>> -- 
>>> 1.8.1.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
>> 
> 
> Thanks,
> -- 
> Jeff Layton <jlayton@redhat.com>
> --
> 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

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 2/2] nfs: allow NFSv3 to fall back to using AUTH_UNIX automatically if available

[Jeff Layton]

On Wed, 26 Jun 2013 11:37:19 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Jun 26, 2013, at 11:24 AM, Jeff Layton <jlayton@redhat.com> wrote:
> 
> > On Wed, 26 Jun 2013 11:15:08 -0400
> > Chuck Lever <chuck.lever@oracle.com> wrote:
> > 
> >> 
> >> On Jun 26, 2013, at 10:36 AM, Jeff Layton <jlayton@redhat.com> wrote:
> >> 
> >>> Currently, when using NFSv3 the mount will fail if the server happens to
> >>> have AUTH_GSS flavors in the returned authlist before AUTH_UNIX. This
> >>> seems to have been a deliberate change in commit 4580a92 (NFS: Use
> >>> server-recommended security flavor by default (NFSv3)).
> >> 
> >> As an aside, this (from the patch description for 4580a92):
> >> 
> >>>    If a server lists Kerberos pseudoflavors before "sys" in its export
> >>>    options, our client now chooses Kerberos over AUTH_UNIX for mount
> >>>    points, when no security flavor is specified by the mount command.
> >>>    This could be surprising to some administrators or users, who would
> >>>    then need to have Kerberos credentials to access the export.
> >> 
> >> 
> >> is a description of side-effects of the changes in 4580a92.  This text is intended as a warning that behavior could change after 4580a92, not as a statement of purpose.  It describes a known limitation of the approach introduced in 4580a92.
> >> 
> > 
> > Ok, good to know...
> > 
> >>> While the workarounds are fine, I think we can do better here and allow
> >>> this to keep "just working". Allow the client to fall back to
> >>> automatically trying AUTH_UNIX under if the following are all true:
> >>> 
> >>>   - the server return -EACCES from ->create_server call
> >>>   - the client had to do a MNT request (i.e. no binary options)
> >>>   - we didn't just try to use AUTH_UNIX
> >>>   - the admin did not explcitly specify a sec= option
> >>> 
> >>> At that point, try to use AUTH_UNIX, if the server listed it.
> >> 
> >> During these checks, how do you know the server specified AUTH_SYS in its list?  It seems to me you want to retry with the next flavor in server_authlist until you've exhausted the list.
> >> 
> >> Thus, IMO, the code you want to emulate is not nfs4_discover_server_trunking(), but rather nfs4_find_root_sec(), substituting the flavor list returned by the server for the static array of flavors.
> >> 
> > 
> > Possibly. Often, we get a list that looks something like this:
> > 
> > [  379.237691] NFS: received 4 auth flavors
> > [  379.257005] NFS:   auth flavor[0]: 390005
> > [  379.278296] NFS:   auth flavor[1]: 390004
> > [  379.298526] NFS:   auth flavor[2]: 390003
> > [  379.318313] NFS:   auth flavor[3]: 1
> > 
> > So we could instead walk down the list and try each in turn, but how
> > likely is it that one of the other AUTH_GSS flavors will work once the
> > first one failed?
> 
> I don't think the client can tell with generality.  -EACCES from ->create_server() may be a result of IP-based access control on the server, for example.
> 

Sure, but if the alternative is failure, then there's little harm in retrying.

> > I'm not sure it's worth adding the extra complexity
> > to do that when it's likely that the only one that will eventually work
> > is the AUTH_SYS one anyway.
> 
> Based on the size and complexity of nfs4_find_root_sec(), I don't think trying flavors in a loop will be complex, and the flavor list is already very short.
> 

Ok, I'll look at doing that. It'll mean more of an overhaul of this
code, but the refactoring you mention below should make it cleaner.

> > 
> >>> Cc: Chuck Lever <chuck.lever@oracle.com>
> >>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> >>> ---
> >>> fs/nfs/super.c | 39 +++++++++++++++++++++++++++++++--------
> >>> 1 file changed, 31 insertions(+), 8 deletions(-)
> >>> 
> >>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> >>> index 5538bcc..6df327e 100644
> >>> --- a/fs/nfs/super.c
> >>> +++ b/fs/nfs/super.c
> >>> @@ -1701,10 +1701,10 @@ out_err:
> >>> * corresponding to the provided path.
> >>> */
> >>> static int nfs_request_mount(struct nfs_parsed_mount_data *args,
> >>> -			     struct nfs_fh *root_fh)
> >>> +			     struct nfs_fh *root_fh,
> >>> +			     unsigned int *server_authlist_len,
> >>> +			     rpc_authflavor_t *server_authlist)
> >>> {
> >>> -	rpc_authflavor_t server_authlist[NFS_MAX_SECFLAVORS];
> >>> -	unsigned int server_authlist_len = ARRAY_SIZE(server_authlist);
> >>> 	struct nfs_mount_request request = {
> >>> 		.sap		= (struct sockaddr *)
> >>> 						&args->mount_server.address,
> >>> @@ -1712,7 +1712,7 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
> >>> 		.protocol	= args->mount_server.protocol,
> >>> 		.fh		= root_fh,
> >>> 		.noresvport	= args->flags & NFS_MOUNT_NORESVPORT,
> >>> -		.auth_flav_len	= &server_authlist_len,
> >>> +		.auth_flav_len	= server_authlist_len,
> >>> 		.auth_flavs	= server_authlist,
> >>> 		.net		= args->net,
> >>> 	};
> >>> @@ -1756,7 +1756,7 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
> >>> 		return status;
> >>> 	}
> >>> 
> >>> -	return nfs_select_flavor(args, server_authlist_len, server_authlist);
> >>> +	return 0;
> >>> }
> >>> 
> >>> struct dentry *nfs_try_mount(int flags, const char *dev_name,
> >>> @@ -1765,17 +1765,40 @@ struct dentry *nfs_try_mount(int flags, const char *dev_name,
> >>> {
> >>> 	int status;
> >>> 	struct nfs_server *server;
> >>> +	struct nfs_parsed_mount_data *args = mount_info->parsed;
> >>> +	rpc_authflavor_t requested_authflavor = args->auth_flavors[0];
> >>> +	rpc_authflavor_t server_authlist[NFS_MAX_SECFLAVORS];
> >>> +	unsigned int server_authlist_len = ARRAY_SIZE(server_authlist);
> >>> 
> >>> -	if (mount_info->parsed->need_mount) {
> >>> -		status = nfs_request_mount(mount_info->parsed, mount_info->mntfh);
> >>> +	if (args->need_mount) {
> >>> +		status = nfs_request_mount(args, mount_info->mntfh,
> >>> +					&server_authlist_len, server_authlist);
> >>> +		if (status)
> >>> +			return ERR_PTR(status);
> >>> +retry:
> >>> +		status = nfs_select_flavor(args, server_authlist_len,
> >>> +						server_authlist);
> >>> 		if (status)
> >>> 			return ERR_PTR(status);
> >>> 	}
> >>> 
> >>> 	/* Get a volume representation */
> >>> 	server = nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
> >>> -	if (IS_ERR(server))
> >>> +	if (IS_ERR(server)) {
> >>> +		/*
> >>> +		 * If the initial attempt fails with -EACCES, then that likely
> >>> +		 * means that we couldn't get proper credentials to talk to the
> >>> +		 * server. If an authflavor wasn't specified in the options,
> >>> +		 * and we didn't try to use it already, then try AUTH_UNIX.
> >>> +		 */
> >>> +		if (PTR_ERR(server) == -EACCES && args->need_mount &&
> >>> +		    args->auth_flavors[0] != RPC_AUTH_UNIX &&
> >>> +		    requested_authflavor == RPC_AUTH_MAXFLAVOR) {
> >>> +			args->auth_flavors[0] = RPC_AUTH_UNIX;
> >> 
> >> This might be a lot easier to read if you refactored nfs_try_mount() into two subfunctions: one which handles the args->need_mount case, and the other which handles the opposite case.
> >> 
> > 
> > Good point. I'll look into doing that.
> > 
> >>> +			goto retry;
> >>> +		}
> >>> 		return ERR_CAST(server);
> >>> +	}
> >>> 
> >>> 	return nfs_fs_mount_common(server, flags, dev_name, mount_info, nfs_mod);
> >>> }
> >>> -- 
> >>> 1.8.1.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
> >> 
> > 
> > Thanks,
> > -- 
> > Jeff Layton <jlayton@redhat.com>
> > --
> > 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
> 


-- 
Jeff Layton <jlayton@redhat.com>