On Jun 26, 2013, at 11:24 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Wed, 26 Jun 2013 11:15:08 -0400
> Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
>
>>
>> On Jun 26, 2013, at 10:36 AM, Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxx
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
> Thanks,
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
