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. I can replicate the problem with a Linux NFS server
by exporing a filesystem with "sec=krb5:sys".
This patchset overhauls the NFSv3 auth code to try each authflavor in
the list provided by the server in the order that it specified them.
With this, I'm again able to mount the server without needing any
special mount options.
Thanks to Chuck Lever for suggestions thus far...
Jeff Layton (3):
nfs: refactor "need_mount" code out of nfs_try_mount
nfs: move server_authlist into nfs_try_mount_request
nfs: have NFSv3 try server-specified auth flavors in turn
fs/nfs/super.c | 182 ++++++++++++++++++++++++++++++++-------------------------
1 file changed, 103 insertions(+), 79 deletions(-)
--
1.8.1.4
On Jun 27, 2013, at 11:18 AM, Jeff Layton <[email protected]> wrote:
> On Thu, 27 Jun 2013 15:09:42 +0000
> "Adamson, Dros" <[email protected]> wrote:
>
>> I think this looks reasonable. I agree with Jeff's use of bools - we don't want to try AUTH_UNIX twice.
>>
>> I am currently cleaning up a patchset that adds support for multiple security flavors passed through mount with the same syntax as /etc/exports, i.e. sec=krb5:krb5i. The initial motivation of the patchset is to handle crossing SECINFO boundaries in a sane way. Currently you could be mounted with krb5p, then follow a SECINFO to using AUTH_UNIX for a submount which is pretty confusing to users. This patchset will limit the client to mount-specified flavors when following a SECINFO (or any flavor if no sec= was specified).
>>
>> Part of this patchset modifies how we track mount specified auth flavors - including keeping a list of parsed auth flavors that doesn't change (no more overwriting auth_flavor[0]). I'll work this patch into my branch and hopefully will be able to clean up some of this logic.
>>
>> -dros
>>
>
> That sounds like good work, but wait until I repost?
No problem - I keep getting new tasks that preempt this work anyway ;)
>
> I've got at least one more respin on the way to incorporate Chuck's
> suggestions. Also, I think it'll be clearer to change the logic a bit
> when the authlist we get from the server is empty for whatever reason.
>
> Rather than picking an authflavor at that point, we can simply edit
> the list to just have a single RPC_AUTH_NULL entry in it. At that
> point, the rest of the logic in that function can do the right thing.
> That also addresses Chuck's concern about checking for an empty authlist
> twice.
>
Ok, that sounds good. My patchset will probably change that, but I definitely think that it's the right way to go to keep everything self contained and clean.
-dros
>
>>
>> On Jun 27, 2013, at 10:14 AM, Jeff Layton <[email protected]> wrote:
>>
>>> On Thu, 27 Jun 2013 09:00:59 -0400
>>> Jeff Layton <[email protected]> wrote:
>>>
>>>> On Wed, 26 Jun 2013 16:42:39 -0400
>>>> Chuck Lever <[email protected]> wrote:
>>>>
>>>>>
>>>>> On Jun 26, 2013, at 3:45 PM, Jeff Layton <[email protected]> wrote:
>>>>>
>>>>>> The current scheme is to try and pick the auth flavor that the server
>>>>>> prefers. In some cases though, we may find that we're not actually
>>>>>> able to use that auth flavor later. For instance, the server may
>>>>>> prefer an AUTH_GSS flavor, but we may not be able to get GSSAPI creds.
>>>>>>
>>>>>> The current code just gives up at that point. Change it instead to
>>>>>> try the ->create_server call using each of the different authflavors
>>>>>> in the server's list if one was not specified at mount time. Once
>>>>>> we have a successful ->create_server call, return the result. Only
>>>>>> give up and return error if all attempts fail.
>>>>>
>>>>> Overall, looks about right. Thanks for retaining the dprintk's! Should Cc: Dros on these as well, since he was the last person to touch this code.
>>>>>
>>>>> More below.
>>>>>
>>>>
>>>> Thanks for the review so far. Replies inline below...
>>>>
>>>>>>
>>>>>> Cc: Chuck Lever <[email protected]>
>>>>>> Signed-off-by: Jeff Layton <[email protected]>
>>>>>> ---
>>>>>> fs/nfs/super.c | 148 +++++++++++++++++++++++++++++++--------------------------
>>>>>> 1 file changed, 80 insertions(+), 68 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>>>>>> index a0949f5..53ea73d 100644
>>>>>> --- a/fs/nfs/super.c
>>>>>> +++ b/fs/nfs/super.c
>>>>>> @@ -1608,29 +1608,20 @@ out_security_failure:
>>>>>> }
>>>>>>
>>>>>> /*
>>>>>> - * Select a security flavor for this mount. The selected flavor
>>>>>> - * is planted in args->auth_flavors[0].
>>>>>> - *
>>>>>> - * Returns 0 on success, -EACCES on failure.
>>>>>> + * Ensure that the specified authtype in args->auth_flavors[0] is supported by
>>>>>> + * the server. Returns 0 if it's ok, and -EACCES if not.
>>>>>> */
>>>>>> -static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
>>>>>> - struct nfs_mount_request *request)
>>>>>> +static int nfs_verify_authflavor(struct nfs_parsed_mount_data *args,
>>>>>> + rpc_authflavor_t *server_authlist, unsigned int count)
>>>>>> {
>>>>>> - unsigned int i, count = *(request->auth_flav_len);
>>>>>> - rpc_authflavor_t flavor;
>>>>>> -
>>>>>> - /*
>>>>>> - * The NFSv2 MNT operation does not return a flavor list.
>>>>>> - */
>>>>>> - if (args->mount_server.version != NFS_MNT3_VERSION)
>>>>>> - goto out_default;
>>>>>> + int i;
>>>>>
>>>>> count is unsigned, therefore i must also be. Otherwise you get mixed sign comparisons in the for() loops.
>>>>>
>>>>
>>>> Thanks, fixed...
>>>>
>>>>>>
>>>>>> /*
>>>>>> - * Certain releases of Linux's mountd return an empty
>>>>>> - * flavor list in some cases.
>>>>>> + * The NFSv2 MNT operation does not return a flavor list and certain
>>>>>> + * releases of Linux's mountd return an empty flavor list in some cases.
>>>>>> */
>>>>>> - if (count == 0)
>>>>>> - goto out_default;
>>>>>> + if (args->mount_server.version != NFS_MNT3_VERSION || count == 0)
>>>>>> + goto out;
>>>>>
>>>>> The above chunk (except the function name change) seems like unnecessary churn.
>>>>>
>>>>> We leave these checks separate from each other for clarity and because they are not logically related to each other, even if the outcome is the same. Keep each check a separate, independent step with its own documentation.
>>>>>
>>>>
>>>> Ok, fair enough. I've reverted that consolidation and kept it more or less as-is.
>>>>
>>>>>>
>>>>>> /*
>>>>>> * If the sec= mount option is used, the specified flavor or AUTH_NULL
>>>>>> @@ -1640,60 +1631,19 @@ static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
>>>>>> * means that the server will ignore the rpc creds, so any flavor
>>>>>> * can be used.
>>>>>> */
>>>>>> - 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)
>>>>>> - goto out;
>>>>>> - }
>>>>>> - dfprintk(MOUNT, "NFS: auth flavor %d not supported by server\n",
>>>>>> - args->auth_flavors[0]);
>>>>>> - goto out_err;
>>>>>> - }
>>>>>> -
>>>>>> - /*
>>>>>> - * RFC 2623, section 2.7 suggests we SHOULD prefer the
>>>>>> - * flavor listed first. However, some servers list
>>>>>> - * AUTH_NULL first. Avoid ever choosing AUTH_NULL.
>>>>>> - */
>>>>>> for (i = 0; i < count; i++) {
>>>>>> - struct rpcsec_gss_info info;
>>>>>> -
>>>>>> - flavor = request->auth_flavs[i];
>>>>>> - switch (flavor) {
>>>>>> - case RPC_AUTH_UNIX:
>>>>>> - goto out_set;
>>>>>> - case RPC_AUTH_NULL:
>>>>>> - continue;
>>>>>> - default:
>>>>>> - if (rpcauth_get_gssinfo(flavor, &info) == 0)
>>>>>> - goto out_set;
>>>>>> - }
>>>>>> - }
>>>>>> -
>>>>>> - /*
>>>>>> - * As a last chance, see if the server list contains AUTH_NULL -
>>>>>> - * if it does, use the default flavor.
>>>>>> - */
>>>>>> - for (i = 0; i < count; i++) {
>>>>>> - if (request->auth_flavs[i] == RPC_AUTH_NULL)
>>>>>> - goto out_default;
>>>>>> + if (args->auth_flavors[0] == server_authlist[i] ||
>>>>>> + server_authlist[i] == RPC_AUTH_NULL)
>>>>>> + goto out;
>>>>>> }
>>>>>>
>>>>>> - dfprintk(MOUNT, "NFS: no auth flavors in common with server\n");
>>>>>> - goto out_err;
>>>>>> + dfprintk(MOUNT, "NFS: auth flavor %u not supported by server\n",
>>>>>> + args->auth_flavors[0]);
>>>>>> + return -EACCES;
>>>>>>
>>>>>> -out_default:
>>>>>> - /* use default if flavor not already set */
>>>>>> - flavor = (args->auth_flavors[0] == RPC_AUTH_MAXFLAVOR) ?
>>>>>> - RPC_AUTH_UNIX : args->auth_flavors[0];
>>>>>> -out_set:
>>>>>> - args->auth_flavors[0] = flavor;
>>>>>> out:
>>>>>> - dfprintk(MOUNT, "NFS: using auth flavor %d\n", args->auth_flavors[0]);
>>>>>> + dfprintk(MOUNT, "NFS: using auth flavor %u\n", args->auth_flavors[0]);
>>>>>> return 0;
>>>>>> -out_err:
>>>>>> - return -EACCES;
>>>>>> }
>>>>>>
>>>>>> /*
>>>>>> @@ -1756,13 +1706,16 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
>>>>>> return status;
>>>>>> }
>>>>>>
>>>>>> - return nfs_select_flavor(args, &request);
>>>>>> + return 0;
>>>>>> }
>>>>>>
>>>>>> static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_info,
>>>>>> struct nfs_subversion *nfs_mod)
>>>>>> {
>>>>>> - int status;
>>>>>> + int status, i;
>>>>>
>>>>> As above, if authlist_len is unsigned, then i should be too.
>>>>>
>>>>
>>>> Fixed...
>>>>
>>>>>> + bool tried_auth_unix = false;
>>>>>> + bool auth_null_in_list = false;
>>>>>> + struct nfs_server *server = ERR_PTR(-EACCES);
>>>>>> struct nfs_parsed_mount_data *args = mount_info->parsed;
>>>>>> rpc_authflavor_t authlist[NFS_MAX_SECFLAVORS];
>>>>>> unsigned int authlist_len = ARRAY_SIZE(authlist);
>>>>>> @@ -1772,6 +1725,65 @@ static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_inf
>>>>>> if (status)
>>>>>> return ERR_PTR(status);
>>>>>>
>>>>>> + /*
>>>>>> + * If the server's authlist is empty, and no sec= option was specified
>>>>>> + * then just default to AUTH_UNIX.
>>>>>> + */
>>>>>> + if (args->auth_flavors[0] == RPC_AUTH_MAXFLAVOR &&
>>>>>> + (args->mount_server.version != NFS_MNT3_VERSION || authlist_len == 0)) {
>>>>>> + dfprintk(MOUNT, "NFS: No authflavor specified, and no server list.");
>>>>>> + args->auth_flavors[0] = RPC_AUTH_UNIX;
>>>>>> + }
>>>>>
>>>>> Why duplicate these checks?
>>>>>
>>>>
>>>> They're not duplicated (exactly).
>>>>
>>>> When we have no authlist from the server, we need to set auth_flavor[0]
>>>> to something valid that we can try to use since iterating over the list
>>>> won't work at that point. Once we have auth_flavor[0] set to something,
>>>> we then call nfs_verify_authflavor to validate that it's usable.
>>>>
>>>> So, yes we are doing those checks twice, but they serve different
>>>> purposes.
>>>>
>>>>>> +
>>>>>> + /*
>>>>>> + * Was a sec= authflavor specified in the options? First, verify
>>>>>> + * whether the server supports it, and then just try to use it if so.
>>>>>> + */
>>>>>> + if (args->auth_flavors[0] != RPC_AUTH_MAXFLAVOR) {
>>>>>> + status = nfs_verify_authflavor(args, authlist, authlist_len);
>>>>>> + dfprintk(MOUNT, "NFS: using auth flavor %u\n", args->auth_flavors[0]);
>>>>>> + if (status)
>>>>>> + return ERR_PTR(status);
>>>>>> + return nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
>>>>>> + }
>>>>>> +
>>>>>> + /*
>>>>>> + * No sec= option was provided. RFC 2623, section 2.7 suggests we
>>>>>> + * SHOULD prefer the flavor listed first. However, some servers list
>>>>>> + * AUTH_NULL first. Avoid ever choosing AUTH_NULL.
>>>>>> + */
>>>>>> + for (i = 0; i < authlist_len; ++i) {
>>>>>> + rpc_authflavor_t flavor;
>>>>>> + struct rpcsec_gss_info info;
>>>>>> +
>>>>>> + flavor = authlist[i];
>>>>>> + switch (flavor) {
>>>>>> + case RPC_AUTH_UNIX:
>>>>>> + tried_auth_unix = true;
>>>>>> + break;
>>>>>> + case RPC_AUTH_NULL:
>>>>>> + auth_null_in_list = true;
>>>>>
>>>>> I think I complained about this when Dros proposed a similar optimization last month. I'd rather not have this extra boolean here. It makes the logic more fragile to have the next check below depend on this loop. If someone decides to move or alter this loop, then it can silently break the check below.
>>>>>
>>>>> The boolean feels like premature optimization especially because the flavor list is always short, and the check below is executed very infrequently.
>>>>>
>>>>> The NFSv3 mount code will always be one giant heuristic. IMO, the best approach is making the source code as plain as possible, rather than defaulting to our normal kernel style of terse, compact code.
>>>>>
>>>>
>>>> I don't know...the booleans seem a lot clearer to me. I can have the
>>>> code rewalk the list, but I'm not sure I buy the argument that it helps
>>>> future-proof this code. Anyone modifying this had _better_ understand
>>>> the logic first. ;)
>>>>
>>>
>>> Actually, now that I look, I'm not sure we can reasonably get away
>>> without using some sort of bool here. We only want to try using
>>> RPC_AUTH_UNIX if we haven't already tried it -- there's no point in
>>> trying it twice. If we rewalk the list and don't track what was done in
>>> the earlier loop, then we have to assume that just because it was in
>>> the list that we already tried it.
>>>
>>> So, I think that keeping track of what was done in the earlier loop
>>> will be *more* resilient in the face of future changes, since that
>>> assumption is rather subtle. Besides, I like my bikesheds blue. ;)
>>>
>>>
>>>>
>>>>>> + continue;
>>>>>> + default:
>>>>>> + if (rpcauth_get_gssinfo(flavor, &info) != 0)
>>>>>> + continue;
>>>>>> + }
>>>>>> + dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", flavor);
>>>>>> + args->auth_flavors[0] = flavor;
>>>>>> + server = nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
>>>>>> + if (!IS_ERR(server))
>>>>>> + return server;
>>>>>> + }
>>>>>> +
>>>>>> + /*
>>>>>> + * Nothing we tried so far worked. As a last ditch effort, try to use
>>>>>> + * AUTH_UNIX if we haven't already, and AUTH_NULL was in server's list.
>>>>>> + */
>>>>>> + if (tried_auth_unix || !auth_null_in_list)
>>>>>> + return server;
>>>>>
>>>>> It would be more clear and less leaky if you "return ERR_PTR(-EACCES);" here, maybe?
>>>>>
>>>>
>>>> Possibly, though that means that you're then assuming that the last
>>>> call to ->create_server returned -EACCES. I'm not sure it's valid to
>>>> assume that.
>>>>
>>>> I think it would be best to return whatever it ended up returning, and
>>>> only return an explicit -EACCES if we never called create_server at all
>>>> in the loop. The fact that we initialize server to ERR_PTR(-EACCES)
>>>> makes that work in this patch.
>>>>
>>>> After all, in the !need_mount case, we end up returning whatever
>>>> ->create_server returned. If it's not valid to let that error bubble
>>>> up, then the place to fix that is in nfs_try_mount.
>>>>
>>>>>> +
>>>>>> + dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", RPC_AUTH_UNIX);
>>>>>> + args->auth_flavors[0] = RPC_AUTH_UNIX;
>>>>>> return nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
>>>>>> }
>>>>>
>>>>
>>>
>>>
>>> --
>>> Jeff Layton <[email protected]>
>>
>
>
> --
> Jeff Layton <[email protected]>
I think this looks reasonable. I agree with Jeff's use of bools - we don't want to try AUTH_UNIX twice.
I am currently cleaning up a patchset that adds support for multiple security flavors passed through mount with the same syntax as /etc/exports, i.e. sec=krb5:krb5i. The initial motivation of the patchset is to handle crossing SECINFO boundaries in a sane way. Currently you could be mounted with krb5p, then follow a SECINFO to using AUTH_UNIX for a submount which is pretty confusing to users. This patchset will limit the client to mount-specified flavors when following a SECINFO (or any flavor if no sec= was specified).
Part of this patchset modifies how we track mount specified auth flavors - including keeping a list of parsed auth flavors that doesn't change (no more overwriting auth_flavor[0]). I'll work this patch into my branch and hopefully will be able to clean up some of this logic.
-dros
On Jun 27, 2013, at 10:14 AM, Jeff Layton <[email protected]> wrote:
> On Thu, 27 Jun 2013 09:00:59 -0400
> Jeff Layton <[email protected]> wrote:
>
>> On Wed, 26 Jun 2013 16:42:39 -0400
>> Chuck Lever <[email protected]> wrote:
>>
>>>
>>> On Jun 26, 2013, at 3:45 PM, Jeff Layton <[email protected]> wrote:
>>>
>>>> The current scheme is to try and pick the auth flavor that the server
>>>> prefers. In some cases though, we may find that we're not actually
>>>> able to use that auth flavor later. For instance, the server may
>>>> prefer an AUTH_GSS flavor, but we may not be able to get GSSAPI creds.
>>>>
>>>> The current code just gives up at that point. Change it instead to
>>>> try the ->create_server call using each of the different authflavors
>>>> in the server's list if one was not specified at mount time. Once
>>>> we have a successful ->create_server call, return the result. Only
>>>> give up and return error if all attempts fail.
>>>
>>> Overall, looks about right. Thanks for retaining the dprintk's! Should Cc: Dros on these as well, since he was the last person to touch this code.
>>>
>>> More below.
>>>
>>
>> Thanks for the review so far. Replies inline below...
>>
>>>>
>>>> Cc: Chuck Lever <[email protected]>
>>>> Signed-off-by: Jeff Layton <[email protected]>
>>>> ---
>>>> fs/nfs/super.c | 148 +++++++++++++++++++++++++++++++--------------------------
>>>> 1 file changed, 80 insertions(+), 68 deletions(-)
>>>>
>>>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>>>> index a0949f5..53ea73d 100644
>>>> --- a/fs/nfs/super.c
>>>> +++ b/fs/nfs/super.c
>>>> @@ -1608,29 +1608,20 @@ out_security_failure:
>>>> }
>>>>
>>>> /*
>>>> - * Select a security flavor for this mount. The selected flavor
>>>> - * is planted in args->auth_flavors[0].
>>>> - *
>>>> - * Returns 0 on success, -EACCES on failure.
>>>> + * Ensure that the specified authtype in args->auth_flavors[0] is supported by
>>>> + * the server. Returns 0 if it's ok, and -EACCES if not.
>>>> */
>>>> -static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
>>>> - struct nfs_mount_request *request)
>>>> +static int nfs_verify_authflavor(struct nfs_parsed_mount_data *args,
>>>> + rpc_authflavor_t *server_authlist, unsigned int count)
>>>> {
>>>> - unsigned int i, count = *(request->auth_flav_len);
>>>> - rpc_authflavor_t flavor;
>>>> -
>>>> - /*
>>>> - * The NFSv2 MNT operation does not return a flavor list.
>>>> - */
>>>> - if (args->mount_server.version != NFS_MNT3_VERSION)
>>>> - goto out_default;
>>>> + int i;
>>>
>>> count is unsigned, therefore i must also be. Otherwise you get mixed sign comparisons in the for() loops.
>>>
>>
>> Thanks, fixed...
>>
>>>>
>>>> /*
>>>> - * Certain releases of Linux's mountd return an empty
>>>> - * flavor list in some cases.
>>>> + * The NFSv2 MNT operation does not return a flavor list and certain
>>>> + * releases of Linux's mountd return an empty flavor list in some cases.
>>>> */
>>>> - if (count == 0)
>>>> - goto out_default;
>>>> + if (args->mount_server.version != NFS_MNT3_VERSION || count == 0)
>>>> + goto out;
>>>
>>> The above chunk (except the function name change) seems like unnecessary churn.
>>>
>>> We leave these checks separate from each other for clarity and because they are not logically related to each other, even if the outcome is the same. Keep each check a separate, independent step with its own documentation.
>>>
>>
>> Ok, fair enough. I've reverted that consolidation and kept it more or less as-is.
>>
>>>>
>>>> /*
>>>> * If the sec= mount option is used, the specified flavor or AUTH_NULL
>>>> @@ -1640,60 +1631,19 @@ static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
>>>> * means that the server will ignore the rpc creds, so any flavor
>>>> * can be used.
>>>> */
>>>> - 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)
>>>> - goto out;
>>>> - }
>>>> - dfprintk(MOUNT, "NFS: auth flavor %d not supported by server\n",
>>>> - args->auth_flavors[0]);
>>>> - goto out_err;
>>>> - }
>>>> -
>>>> - /*
>>>> - * RFC 2623, section 2.7 suggests we SHOULD prefer the
>>>> - * flavor listed first. However, some servers list
>>>> - * AUTH_NULL first. Avoid ever choosing AUTH_NULL.
>>>> - */
>>>> for (i = 0; i < count; i++) {
>>>> - struct rpcsec_gss_info info;
>>>> -
>>>> - flavor = request->auth_flavs[i];
>>>> - switch (flavor) {
>>>> - case RPC_AUTH_UNIX:
>>>> - goto out_set;
>>>> - case RPC_AUTH_NULL:
>>>> - continue;
>>>> - default:
>>>> - if (rpcauth_get_gssinfo(flavor, &info) == 0)
>>>> - goto out_set;
>>>> - }
>>>> - }
>>>> -
>>>> - /*
>>>> - * As a last chance, see if the server list contains AUTH_NULL -
>>>> - * if it does, use the default flavor.
>>>> - */
>>>> - for (i = 0; i < count; i++) {
>>>> - if (request->auth_flavs[i] == RPC_AUTH_NULL)
>>>> - goto out_default;
>>>> + if (args->auth_flavors[0] == server_authlist[i] ||
>>>> + server_authlist[i] == RPC_AUTH_NULL)
>>>> + goto out;
>>>> }
>>>>
>>>> - dfprintk(MOUNT, "NFS: no auth flavors in common with server\n");
>>>> - goto out_err;
>>>> + dfprintk(MOUNT, "NFS: auth flavor %u not supported by server\n",
>>>> + args->auth_flavors[0]);
>>>> + return -EACCES;
>>>>
>>>> -out_default:
>>>> - /* use default if flavor not already set */
>>>> - flavor = (args->auth_flavors[0] == RPC_AUTH_MAXFLAVOR) ?
>>>> - RPC_AUTH_UNIX : args->auth_flavors[0];
>>>> -out_set:
>>>> - args->auth_flavors[0] = flavor;
>>>> out:
>>>> - dfprintk(MOUNT, "NFS: using auth flavor %d\n", args->auth_flavors[0]);
>>>> + dfprintk(MOUNT, "NFS: using auth flavor %u\n", args->auth_flavors[0]);
>>>> return 0;
>>>> -out_err:
>>>> - return -EACCES;
>>>> }
>>>>
>>>> /*
>>>> @@ -1756,13 +1706,16 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
>>>> return status;
>>>> }
>>>>
>>>> - return nfs_select_flavor(args, &request);
>>>> + return 0;
>>>> }
>>>>
>>>> static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_info,
>>>> struct nfs_subversion *nfs_mod)
>>>> {
>>>> - int status;
>>>> + int status, i;
>>>
>>> As above, if authlist_len is unsigned, then i should be too.
>>>
>>
>> Fixed...
>>
>>>> + bool tried_auth_unix = false;
>>>> + bool auth_null_in_list = false;
>>>> + struct nfs_server *server = ERR_PTR(-EACCES);
>>>> struct nfs_parsed_mount_data *args = mount_info->parsed;
>>>> rpc_authflavor_t authlist[NFS_MAX_SECFLAVORS];
>>>> unsigned int authlist_len = ARRAY_SIZE(authlist);
>>>> @@ -1772,6 +1725,65 @@ static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_inf
>>>> if (status)
>>>> return ERR_PTR(status);
>>>>
>>>> + /*
>>>> + * If the server's authlist is empty, and no sec= option was specified
>>>> + * then just default to AUTH_UNIX.
>>>> + */
>>>> + if (args->auth_flavors[0] == RPC_AUTH_MAXFLAVOR &&
>>>> + (args->mount_server.version != NFS_MNT3_VERSION || authlist_len == 0)) {
>>>> + dfprintk(MOUNT, "NFS: No authflavor specified, and no server list.");
>>>> + args->auth_flavors[0] = RPC_AUTH_UNIX;
>>>> + }
>>>
>>> Why duplicate these checks?
>>>
>>
>> They're not duplicated (exactly).
>>
>> When we have no authlist from the server, we need to set auth_flavor[0]
>> to something valid that we can try to use since iterating over the list
>> won't work at that point. Once we have auth_flavor[0] set to something,
>> we then call nfs_verify_authflavor to validate that it's usable.
>>
>> So, yes we are doing those checks twice, but they serve different
>> purposes.
>>
>>>> +
>>>> + /*
>>>> + * Was a sec= authflavor specified in the options? First, verify
>>>> + * whether the server supports it, and then just try to use it if so.
>>>> + */
>>>> + if (args->auth_flavors[0] != RPC_AUTH_MAXFLAVOR) {
>>>> + status = nfs_verify_authflavor(args, authlist, authlist_len);
>>>> + dfprintk(MOUNT, "NFS: using auth flavor %u\n", args->auth_flavors[0]);
>>>> + if (status)
>>>> + return ERR_PTR(status);
>>>> + return nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
>>>> + }
>>>> +
>>>> + /*
>>>> + * No sec= option was provided. RFC 2623, section 2.7 suggests we
>>>> + * SHOULD prefer the flavor listed first. However, some servers list
>>>> + * AUTH_NULL first. Avoid ever choosing AUTH_NULL.
>>>> + */
>>>> + for (i = 0; i < authlist_len; ++i) {
>>>> + rpc_authflavor_t flavor;
>>>> + struct rpcsec_gss_info info;
>>>> +
>>>> + flavor = authlist[i];
>>>> + switch (flavor) {
>>>> + case RPC_AUTH_UNIX:
>>>> + tried_auth_unix = true;
>>>> + break;
>>>> + case RPC_AUTH_NULL:
>>>> + auth_null_in_list = true;
>>>
>>> I think I complained about this when Dros proposed a similar optimization last month. I'd rather not have this extra boolean here. It makes the logic more fragile to have the next check below depend on this loop. If someone decides to move or alter this loop, then it can silently break the check below.
>>>
>>> The boolean feels like premature optimization especially because the flavor list is always short, and the check below is executed very infrequently.
>>>
>>> The NFSv3 mount code will always be one giant heuristic. IMO, the best approach is making the source code as plain as possible, rather than defaulting to our normal kernel style of terse, compact code.
>>>
>>
>> I don't know...the booleans seem a lot clearer to me. I can have the
>> code rewalk the list, but I'm not sure I buy the argument that it helps
>> future-proof this code. Anyone modifying this had _better_ understand
>> the logic first. ;)
>>
>
> Actually, now that I look, I'm not sure we can reasonably get away
> without using some sort of bool here. We only want to try using
> RPC_AUTH_UNIX if we haven't already tried it -- there's no point in
> trying it twice. If we rewalk the list and don't track what was done in
> the earlier loop, then we have to assume that just because it was in
> the list that we already tried it.
>
> So, I think that keeping track of what was done in the earlier loop
> will be *more* resilient in the face of future changes, since that
> assumption is rather subtle. Besides, I like my bikesheds blue. ;)
>
>
>>
>>>> + continue;
>>>> + default:
>>>> + if (rpcauth_get_gssinfo(flavor, &info) != 0)
>>>> + continue;
>>>> + }
>>>> + dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", flavor);
>>>> + args->auth_flavors[0] = flavor;
>>>> + server = nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
>>>> + if (!IS_ERR(server))
>>>> + return server;
>>>> + }
>>>> +
>>>> + /*
>>>> + * Nothing we tried so far worked. As a last ditch effort, try to use
>>>> + * AUTH_UNIX if we haven't already, and AUTH_NULL was in server's list.
>>>> + */
>>>> + if (tried_auth_unix || !auth_null_in_list)
>>>> + return server;
>>>
>>> It would be more clear and less leaky if you "return ERR_PTR(-EACCES);" here, maybe?
>>>
>>
>> Possibly, though that means that you're then assuming that the last
>> call to ->create_server returned -EACCES. I'm not sure it's valid to
>> assume that.
>>
>> I think it would be best to return whatever it ended up returning, and
>> only return an explicit -EACCES if we never called create_server at all
>> in the loop. The fact that we initialize server to ERR_PTR(-EACCES)
>> makes that work in this patch.
>>
>> After all, in the !need_mount case, we end up returning whatever
>> ->create_server returned. If it's not valid to let that error bubble
>> up, then the place to fix that is in nfs_try_mount.
>>
>>>> +
>>>> + dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", RPC_AUTH_UNIX);
>>>> + args->auth_flavors[0] = RPC_AUTH_UNIX;
>>>> return nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
>>>> }
>>>
>>
>
>
> --
> Jeff Layton <[email protected]>
The current scheme is to try and pick the auth flavor that the server
prefers. In some cases though, we may find that we're not actually
able to use that auth flavor later. For instance, the server may
prefer an AUTH_GSS flavor, but we may not be able to get GSSAPI creds.
The current code just gives up at that point. Change it instead to
try the ->create_server call using each of the different authflavors
in the server's list if one was not specified at mount time. Once
we have a successful ->create_server call, return the result. Only
give up and return error if all attempts fail.
Cc: Chuck Lever <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfs/super.c | 148 +++++++++++++++++++++++++++++++--------------------------
1 file changed, 80 insertions(+), 68 deletions(-)
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index a0949f5..53ea73d 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1608,29 +1608,20 @@ out_security_failure:
}
/*
- * Select a security flavor for this mount. The selected flavor
- * is planted in args->auth_flavors[0].
- *
- * Returns 0 on success, -EACCES on failure.
+ * Ensure that the specified authtype in args->auth_flavors[0] is supported by
+ * the server. Returns 0 if it's ok, and -EACCES if not.
*/
-static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
- struct nfs_mount_request *request)
+static int nfs_verify_authflavor(struct nfs_parsed_mount_data *args,
+ rpc_authflavor_t *server_authlist, unsigned int count)
{
- unsigned int i, count = *(request->auth_flav_len);
- rpc_authflavor_t flavor;
-
- /*
- * The NFSv2 MNT operation does not return a flavor list.
- */
- if (args->mount_server.version != NFS_MNT3_VERSION)
- goto out_default;
+ int i;
/*
- * Certain releases of Linux's mountd return an empty
- * flavor list in some cases.
+ * The NFSv2 MNT operation does not return a flavor list and certain
+ * releases of Linux's mountd return an empty flavor list in some cases.
*/
- if (count == 0)
- goto out_default;
+ if (args->mount_server.version != NFS_MNT3_VERSION || count == 0)
+ goto out;
/*
* If the sec= mount option is used, the specified flavor or AUTH_NULL
@@ -1640,60 +1631,19 @@ static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
* means that the server will ignore the rpc creds, so any flavor
* can be used.
*/
- 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)
- goto out;
- }
- dfprintk(MOUNT, "NFS: auth flavor %d not supported by server\n",
- args->auth_flavors[0]);
- goto out_err;
- }
-
- /*
- * RFC 2623, section 2.7 suggests we SHOULD prefer the
- * flavor listed first. However, some servers list
- * AUTH_NULL first. Avoid ever choosing AUTH_NULL.
- */
for (i = 0; i < count; i++) {
- struct rpcsec_gss_info info;
-
- flavor = request->auth_flavs[i];
- switch (flavor) {
- case RPC_AUTH_UNIX:
- goto out_set;
- case RPC_AUTH_NULL:
- continue;
- default:
- if (rpcauth_get_gssinfo(flavor, &info) == 0)
- goto out_set;
- }
- }
-
- /*
- * As a last chance, see if the server list contains AUTH_NULL -
- * if it does, use the default flavor.
- */
- for (i = 0; i < count; i++) {
- if (request->auth_flavs[i] == RPC_AUTH_NULL)
- goto out_default;
+ if (args->auth_flavors[0] == server_authlist[i] ||
+ server_authlist[i] == RPC_AUTH_NULL)
+ goto out;
}
- dfprintk(MOUNT, "NFS: no auth flavors in common with server\n");
- goto out_err;
+ dfprintk(MOUNT, "NFS: auth flavor %u not supported by server\n",
+ args->auth_flavors[0]);
+ return -EACCES;
-out_default:
- /* use default if flavor not already set */
- flavor = (args->auth_flavors[0] == RPC_AUTH_MAXFLAVOR) ?
- RPC_AUTH_UNIX : args->auth_flavors[0];
-out_set:
- args->auth_flavors[0] = flavor;
out:
- dfprintk(MOUNT, "NFS: using auth flavor %d\n", args->auth_flavors[0]);
+ dfprintk(MOUNT, "NFS: using auth flavor %u\n", args->auth_flavors[0]);
return 0;
-out_err:
- return -EACCES;
}
/*
@@ -1756,13 +1706,16 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
return status;
}
- return nfs_select_flavor(args, &request);
+ return 0;
}
static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_info,
struct nfs_subversion *nfs_mod)
{
- int status;
+ int status, i;
+ bool tried_auth_unix = false;
+ bool auth_null_in_list = false;
+ struct nfs_server *server = ERR_PTR(-EACCES);
struct nfs_parsed_mount_data *args = mount_info->parsed;
rpc_authflavor_t authlist[NFS_MAX_SECFLAVORS];
unsigned int authlist_len = ARRAY_SIZE(authlist);
@@ -1772,6 +1725,65 @@ static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_inf
if (status)
return ERR_PTR(status);
+ /*
+ * If the server's authlist is empty, and no sec= option was specified
+ * then just default to AUTH_UNIX.
+ */
+ if (args->auth_flavors[0] == RPC_AUTH_MAXFLAVOR &&
+ (args->mount_server.version != NFS_MNT3_VERSION || authlist_len == 0)) {
+ dfprintk(MOUNT, "NFS: No authflavor specified, and no server list.");
+ args->auth_flavors[0] = RPC_AUTH_UNIX;
+ }
+
+ /*
+ * Was a sec= authflavor specified in the options? First, verify
+ * whether the server supports it, and then just try to use it if so.
+ */
+ if (args->auth_flavors[0] != RPC_AUTH_MAXFLAVOR) {
+ status = nfs_verify_authflavor(args, authlist, authlist_len);
+ dfprintk(MOUNT, "NFS: using auth flavor %u\n", args->auth_flavors[0]);
+ if (status)
+ return ERR_PTR(status);
+ return nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
+ }
+
+ /*
+ * No sec= option was provided. RFC 2623, section 2.7 suggests we
+ * SHOULD prefer the flavor listed first. However, some servers list
+ * AUTH_NULL first. Avoid ever choosing AUTH_NULL.
+ */
+ for (i = 0; i < authlist_len; ++i) {
+ rpc_authflavor_t flavor;
+ struct rpcsec_gss_info info;
+
+ flavor = authlist[i];
+ switch (flavor) {
+ case RPC_AUTH_UNIX:
+ tried_auth_unix = true;
+ break;
+ case RPC_AUTH_NULL:
+ auth_null_in_list = true;
+ continue;
+ default:
+ if (rpcauth_get_gssinfo(flavor, &info) != 0)
+ continue;
+ }
+ dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", flavor);
+ args->auth_flavors[0] = flavor;
+ server = nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
+ if (!IS_ERR(server))
+ return server;
+ }
+
+ /*
+ * Nothing we tried so far worked. As a last ditch effort, try to use
+ * AUTH_UNIX if we haven't already, and AUTH_NULL was in server's list.
+ */
+ if (tried_auth_unix || !auth_null_in_list)
+ return server;
+
+ dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", RPC_AUTH_UNIX);
+ args->auth_flavors[0] = RPC_AUTH_UNIX;
return nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
}
--
1.8.1.4
On Wed, 26 Jun 2013 16:42:39 -0400
Chuck Lever <[email protected]> wrote:
>
> On Jun 26, 2013, at 3:45 PM, Jeff Layton <[email protected]> wrote:
>
> > The current scheme is to try and pick the auth flavor that the server
> > prefers. In some cases though, we may find that we're not actually
> > able to use that auth flavor later. For instance, the server may
> > prefer an AUTH_GSS flavor, but we may not be able to get GSSAPI creds.
> >
> > The current code just gives up at that point. Change it instead to
> > try the ->create_server call using each of the different authflavors
> > in the server's list if one was not specified at mount time. Once
> > we have a successful ->create_server call, return the result. Only
> > give up and return error if all attempts fail.
>
> Overall, looks about right. Thanks for retaining the dprintk's! Should Cc: Dros on these as well, since he was the last person to touch this code.
>
> More below.
>
Thanks for the review so far. Replies inline below...
> >
> > Cc: Chuck Lever <[email protected]>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > fs/nfs/super.c | 148 +++++++++++++++++++++++++++++++--------------------------
> > 1 file changed, 80 insertions(+), 68 deletions(-)
> >
> > diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> > index a0949f5..53ea73d 100644
> > --- a/fs/nfs/super.c
> > +++ b/fs/nfs/super.c
> > @@ -1608,29 +1608,20 @@ out_security_failure:
> > }
> >
> > /*
> > - * Select a security flavor for this mount. The selected flavor
> > - * is planted in args->auth_flavors[0].
> > - *
> > - * Returns 0 on success, -EACCES on failure.
> > + * Ensure that the specified authtype in args->auth_flavors[0] is supported by
> > + * the server. Returns 0 if it's ok, and -EACCES if not.
> > */
> > -static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
> > - struct nfs_mount_request *request)
> > +static int nfs_verify_authflavor(struct nfs_parsed_mount_data *args,
> > + rpc_authflavor_t *server_authlist, unsigned int count)
> > {
> > - unsigned int i, count = *(request->auth_flav_len);
> > - rpc_authflavor_t flavor;
> > -
> > - /*
> > - * The NFSv2 MNT operation does not return a flavor list.
> > - */
> > - if (args->mount_server.version != NFS_MNT3_VERSION)
> > - goto out_default;
> > + int i;
>
> count is unsigned, therefore i must also be. Otherwise you get mixed sign comparisons in the for() loops.
>
Thanks, fixed...
> >
> > /*
> > - * Certain releases of Linux's mountd return an empty
> > - * flavor list in some cases.
> > + * The NFSv2 MNT operation does not return a flavor list and certain
> > + * releases of Linux's mountd return an empty flavor list in some cases.
> > */
> > - if (count == 0)
> > - goto out_default;
> > + if (args->mount_server.version != NFS_MNT3_VERSION || count == 0)
> > + goto out;
>
> The above chunk (except the function name change) seems like unnecessary churn.
>
> We leave these checks separate from each other for clarity and because they are not logically related to each other, even if the outcome is the same. Keep each check a separate, independent step with its own documentation.
>
Ok, fair enough. I've reverted that consolidation and kept it more or less as-is.
> >
> > /*
> > * If the sec= mount option is used, the specified flavor or AUTH_NULL
> > @@ -1640,60 +1631,19 @@ static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
> > * means that the server will ignore the rpc creds, so any flavor
> > * can be used.
> > */
> > - 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)
> > - goto out;
> > - }
> > - dfprintk(MOUNT, "NFS: auth flavor %d not supported by server\n",
> > - args->auth_flavors[0]);
> > - goto out_err;
> > - }
> > -
> > - /*
> > - * RFC 2623, section 2.7 suggests we SHOULD prefer the
> > - * flavor listed first. However, some servers list
> > - * AUTH_NULL first. Avoid ever choosing AUTH_NULL.
> > - */
> > for (i = 0; i < count; i++) {
> > - struct rpcsec_gss_info info;
> > -
> > - flavor = request->auth_flavs[i];
> > - switch (flavor) {
> > - case RPC_AUTH_UNIX:
> > - goto out_set;
> > - case RPC_AUTH_NULL:
> > - continue;
> > - default:
> > - if (rpcauth_get_gssinfo(flavor, &info) == 0)
> > - goto out_set;
> > - }
> > - }
> > -
> > - /*
> > - * As a last chance, see if the server list contains AUTH_NULL -
> > - * if it does, use the default flavor.
> > - */
> > - for (i = 0; i < count; i++) {
> > - if (request->auth_flavs[i] == RPC_AUTH_NULL)
> > - goto out_default;
> > + if (args->auth_flavors[0] == server_authlist[i] ||
> > + server_authlist[i] == RPC_AUTH_NULL)
> > + goto out;
> > }
> >
> > - dfprintk(MOUNT, "NFS: no auth flavors in common with server\n");
> > - goto out_err;
> > + dfprintk(MOUNT, "NFS: auth flavor %u not supported by server\n",
> > + args->auth_flavors[0]);
> > + return -EACCES;
> >
> > -out_default:
> > - /* use default if flavor not already set */
> > - flavor = (args->auth_flavors[0] == RPC_AUTH_MAXFLAVOR) ?
> > - RPC_AUTH_UNIX : args->auth_flavors[0];
> > -out_set:
> > - args->auth_flavors[0] = flavor;
> > out:
> > - dfprintk(MOUNT, "NFS: using auth flavor %d\n", args->auth_flavors[0]);
> > + dfprintk(MOUNT, "NFS: using auth flavor %u\n", args->auth_flavors[0]);
> > return 0;
> > -out_err:
> > - return -EACCES;
> > }
> >
> > /*
> > @@ -1756,13 +1706,16 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
> > return status;
> > }
> >
> > - return nfs_select_flavor(args, &request);
> > + return 0;
> > }
> >
> > static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_info,
> > struct nfs_subversion *nfs_mod)
> > {
> > - int status;
> > + int status, i;
>
> As above, if authlist_len is unsigned, then i should be too.
>
Fixed...
> > + bool tried_auth_unix = false;
> > + bool auth_null_in_list = false;
> > + struct nfs_server *server = ERR_PTR(-EACCES);
> > struct nfs_parsed_mount_data *args = mount_info->parsed;
> > rpc_authflavor_t authlist[NFS_MAX_SECFLAVORS];
> > unsigned int authlist_len = ARRAY_SIZE(authlist);
> > @@ -1772,6 +1725,65 @@ static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_inf
> > if (status)
> > return ERR_PTR(status);
> >
> > + /*
> > + * If the server's authlist is empty, and no sec= option was specified
> > + * then just default to AUTH_UNIX.
> > + */
> > + if (args->auth_flavors[0] == RPC_AUTH_MAXFLAVOR &&
> > + (args->mount_server.version != NFS_MNT3_VERSION || authlist_len == 0)) {
> > + dfprintk(MOUNT, "NFS: No authflavor specified, and no server list.");
> > + args->auth_flavors[0] = RPC_AUTH_UNIX;
> > + }
>
> Why duplicate these checks?
>
They're not duplicated (exactly).
When we have no authlist from the server, we need to set auth_flavor[0]
to something valid that we can try to use since iterating over the list
won't work at that point. Once we have auth_flavor[0] set to something,
we then call nfs_verify_authflavor to validate that it's usable.
So, yes we are doing those checks twice, but they serve different
purposes.
> > +
> > + /*
> > + * Was a sec= authflavor specified in the options? First, verify
> > + * whether the server supports it, and then just try to use it if so.
> > + */
> > + if (args->auth_flavors[0] != RPC_AUTH_MAXFLAVOR) {
> > + status = nfs_verify_authflavor(args, authlist, authlist_len);
> > + dfprintk(MOUNT, "NFS: using auth flavor %u\n", args->auth_flavors[0]);
> > + if (status)
> > + return ERR_PTR(status);
> > + return nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
> > + }
> > +
> > + /*
> > + * No sec= option was provided. RFC 2623, section 2.7 suggests we
> > + * SHOULD prefer the flavor listed first. However, some servers list
> > + * AUTH_NULL first. Avoid ever choosing AUTH_NULL.
> > + */
> > + for (i = 0; i < authlist_len; ++i) {
> > + rpc_authflavor_t flavor;
> > + struct rpcsec_gss_info info;
> > +
> > + flavor = authlist[i];
> > + switch (flavor) {
> > + case RPC_AUTH_UNIX:
> > + tried_auth_unix = true;
> > + break;
> > + case RPC_AUTH_NULL:
> > + auth_null_in_list = true;
>
> I think I complained about this when Dros proposed a similar optimization last month. I'd rather not have this extra boolean here. It makes the logic more fragile to have the next check below depend on this loop. If someone decides to move or alter this loop, then it can silently break the check below.
>
> The boolean feels like premature optimization especially because the flavor list is always short, and the check below is executed very infrequently.
>
> The NFSv3 mount code will always be one giant heuristic. IMO, the best approach is making the source code as plain as possible, rather than defaulting to our normal kernel style of terse, compact code.
>
I don't know...the booleans seem a lot clearer to me. I can have the
code rewalk the list, but I'm not sure I buy the argument that it helps
future-proof this code. Anyone modifying this had _better_ understand
the logic first. ;)
> > + continue;
> > + default:
> > + if (rpcauth_get_gssinfo(flavor, &info) != 0)
> > + continue;
> > + }
> > + dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", flavor);
> > + args->auth_flavors[0] = flavor;
> > + server = nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
> > + if (!IS_ERR(server))
> > + return server;
> > + }
> > +
> > + /*
> > + * Nothing we tried so far worked. As a last ditch effort, try to use
> > + * AUTH_UNIX if we haven't already, and AUTH_NULL was in server's list.
> > + */
> > + if (tried_auth_unix || !auth_null_in_list)
> > + return server;
>
> It would be more clear and less leaky if you "return ERR_PTR(-EACCES);" here, maybe?
>
Possibly, though that means that you're then assuming that the last
call to ->create_server returned -EACCES. I'm not sure it's valid to
assume that.
I think it would be best to return whatever it ended up returning, and
only return an explicit -EACCES if we never called create_server at all
in the loop. The fact that we initialize server to ERR_PTR(-EACCES)
makes that work in this patch.
After all, in the !need_mount case, we end up returning whatever
->create_server returned. If it's not valid to let that error bubble
up, then the place to fix that is in nfs_try_mount.
> > +
> > + dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", RPC_AUTH_UNIX);
> > + args->auth_flavors[0] = RPC_AUTH_UNIX;
> > return nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
> > }
>
--
Jeff Layton <[email protected]>
On Jun 26, 2013, at 3:45 PM, Jeff Layton <[email protected]> wrote:
> The current scheme is to try and pick the auth flavor that the server
> prefers. In some cases though, we may find that we're not actually
> able to use that auth flavor later. For instance, the server may
> prefer an AUTH_GSS flavor, but we may not be able to get GSSAPI creds.
>
> The current code just gives up at that point. Change it instead to
> try the ->create_server call using each of the different authflavors
> in the server's list if one was not specified at mount time. Once
> we have a successful ->create_server call, return the result. Only
> give up and return error if all attempts fail.
Overall, looks about right. Thanks for retaining the dprintk's! Should Cc: Dros on these as well, since he was the last person to touch this code.
More below.
>
> Cc: Chuck Lever <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfs/super.c | 148 +++++++++++++++++++++++++++++++--------------------------
> 1 file changed, 80 insertions(+), 68 deletions(-)
>
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index a0949f5..53ea73d 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -1608,29 +1608,20 @@ out_security_failure:
> }
>
> /*
> - * Select a security flavor for this mount. The selected flavor
> - * is planted in args->auth_flavors[0].
> - *
> - * Returns 0 on success, -EACCES on failure.
> + * Ensure that the specified authtype in args->auth_flavors[0] is supported by
> + * the server. Returns 0 if it's ok, and -EACCES if not.
> */
> -static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
> - struct nfs_mount_request *request)
> +static int nfs_verify_authflavor(struct nfs_parsed_mount_data *args,
> + rpc_authflavor_t *server_authlist, unsigned int count)
> {
> - unsigned int i, count = *(request->auth_flav_len);
> - rpc_authflavor_t flavor;
> -
> - /*
> - * The NFSv2 MNT operation does not return a flavor list.
> - */
> - if (args->mount_server.version != NFS_MNT3_VERSION)
> - goto out_default;
> + int i;
count is unsigned, therefore i must also be. Otherwise you get mixed sign comparisons in the for() loops.
>
> /*
> - * Certain releases of Linux's mountd return an empty
> - * flavor list in some cases.
> + * The NFSv2 MNT operation does not return a flavor list and certain
> + * releases of Linux's mountd return an empty flavor list in some cases.
> */
> - if (count == 0)
> - goto out_default;
> + if (args->mount_server.version != NFS_MNT3_VERSION || count == 0)
> + goto out;
The above chunk (except the function name change) seems like unnecessary churn.
We leave these checks separate from each other for clarity and because they are not logically related to each other, even if the outcome is the same. Keep each check a separate, independent step with its own documentation.
>
> /*
> * If the sec= mount option is used, the specified flavor or AUTH_NULL
> @@ -1640,60 +1631,19 @@ static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
> * means that the server will ignore the rpc creds, so any flavor
> * can be used.
> */
> - 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)
> - goto out;
> - }
> - dfprintk(MOUNT, "NFS: auth flavor %d not supported by server\n",
> - args->auth_flavors[0]);
> - goto out_err;
> - }
> -
> - /*
> - * RFC 2623, section 2.7 suggests we SHOULD prefer the
> - * flavor listed first. However, some servers list
> - * AUTH_NULL first. Avoid ever choosing AUTH_NULL.
> - */
> for (i = 0; i < count; i++) {
> - struct rpcsec_gss_info info;
> -
> - flavor = request->auth_flavs[i];
> - switch (flavor) {
> - case RPC_AUTH_UNIX:
> - goto out_set;
> - case RPC_AUTH_NULL:
> - continue;
> - default:
> - if (rpcauth_get_gssinfo(flavor, &info) == 0)
> - goto out_set;
> - }
> - }
> -
> - /*
> - * As a last chance, see if the server list contains AUTH_NULL -
> - * if it does, use the default flavor.
> - */
> - for (i = 0; i < count; i++) {
> - if (request->auth_flavs[i] == RPC_AUTH_NULL)
> - goto out_default;
> + if (args->auth_flavors[0] == server_authlist[i] ||
> + server_authlist[i] == RPC_AUTH_NULL)
> + goto out;
> }
>
> - dfprintk(MOUNT, "NFS: no auth flavors in common with server\n");
> - goto out_err;
> + dfprintk(MOUNT, "NFS: auth flavor %u not supported by server\n",
> + args->auth_flavors[0]);
> + return -EACCES;
>
> -out_default:
> - /* use default if flavor not already set */
> - flavor = (args->auth_flavors[0] == RPC_AUTH_MAXFLAVOR) ?
> - RPC_AUTH_UNIX : args->auth_flavors[0];
> -out_set:
> - args->auth_flavors[0] = flavor;
> out:
> - dfprintk(MOUNT, "NFS: using auth flavor %d\n", args->auth_flavors[0]);
> + dfprintk(MOUNT, "NFS: using auth flavor %u\n", args->auth_flavors[0]);
> return 0;
> -out_err:
> - return -EACCES;
> }
>
> /*
> @@ -1756,13 +1706,16 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
> return status;
> }
>
> - return nfs_select_flavor(args, &request);
> + return 0;
> }
>
> static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_info,
> struct nfs_subversion *nfs_mod)
> {
> - int status;
> + int status, i;
As above, if authlist_len is unsigned, then i should be too.
> + bool tried_auth_unix = false;
> + bool auth_null_in_list = false;
> + struct nfs_server *server = ERR_PTR(-EACCES);
> struct nfs_parsed_mount_data *args = mount_info->parsed;
> rpc_authflavor_t authlist[NFS_MAX_SECFLAVORS];
> unsigned int authlist_len = ARRAY_SIZE(authlist);
> @@ -1772,6 +1725,65 @@ static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_inf
> if (status)
> return ERR_PTR(status);
>
> + /*
> + * If the server's authlist is empty, and no sec= option was specified
> + * then just default to AUTH_UNIX.
> + */
> + if (args->auth_flavors[0] == RPC_AUTH_MAXFLAVOR &&
> + (args->mount_server.version != NFS_MNT3_VERSION || authlist_len == 0)) {
> + dfprintk(MOUNT, "NFS: No authflavor specified, and no server list.");
> + args->auth_flavors[0] = RPC_AUTH_UNIX;
> + }
Why duplicate these checks?
> +
> + /*
> + * Was a sec= authflavor specified in the options? First, verify
> + * whether the server supports it, and then just try to use it if so.
> + */
> + if (args->auth_flavors[0] != RPC_AUTH_MAXFLAVOR) {
> + status = nfs_verify_authflavor(args, authlist, authlist_len);
> + dfprintk(MOUNT, "NFS: using auth flavor %u\n", args->auth_flavors[0]);
> + if (status)
> + return ERR_PTR(status);
> + return nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
> + }
> +
> + /*
> + * No sec= option was provided. RFC 2623, section 2.7 suggests we
> + * SHOULD prefer the flavor listed first. However, some servers list
> + * AUTH_NULL first. Avoid ever choosing AUTH_NULL.
> + */
> + for (i = 0; i < authlist_len; ++i) {
> + rpc_authflavor_t flavor;
> + struct rpcsec_gss_info info;
> +
> + flavor = authlist[i];
> + switch (flavor) {
> + case RPC_AUTH_UNIX:
> + tried_auth_unix = true;
> + break;
> + case RPC_AUTH_NULL:
> + auth_null_in_list = true;
I think I complained about this when Dros proposed a similar optimization last month. I'd rather not have this extra boolean here. It makes the logic more fragile to have the next check below depend on this loop. If someone decides to move or alter this loop, then it can silently break the check below.
The boolean feels like premature optimization especially because the flavor list is always short, and the check below is executed very infrequently.
The NFSv3 mount code will always be one giant heuristic. IMO, the best approach is making the source code as plain as possible, rather than defaulting to our normal kernel style of terse, compact code.
> + continue;
> + default:
> + if (rpcauth_get_gssinfo(flavor, &info) != 0)
> + continue;
> + }
> + dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", flavor);
> + args->auth_flavors[0] = flavor;
> + server = nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
> + if (!IS_ERR(server))
> + return server;
> + }
> +
> + /*
> + * Nothing we tried so far worked. As a last ditch effort, try to use
> + * AUTH_UNIX if we haven't already, and AUTH_NULL was in server's list.
> + */
> + if (tried_auth_unix || !auth_null_in_list)
> + return server;
It would be more clear and less leaky if you "return ERR_PTR(-EACCES);" here, maybe?
> +
> + dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", RPC_AUTH_UNIX);
> + args->auth_flavors[0] = RPC_AUTH_UNIX;
> return nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
> }
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Thu, 27 Jun 2013 09:00:59 -0400
Jeff Layton <[email protected]> wrote:
> On Wed, 26 Jun 2013 16:42:39 -0400
> Chuck Lever <[email protected]> wrote:
>
> >
> > On Jun 26, 2013, at 3:45 PM, Jeff Layton <[email protected]> wrote:
> >
> > > The current scheme is to try and pick the auth flavor that the server
> > > prefers. In some cases though, we may find that we're not actually
> > > able to use that auth flavor later. For instance, the server may
> > > prefer an AUTH_GSS flavor, but we may not be able to get GSSAPI creds.
> > >
> > > The current code just gives up at that point. Change it instead to
> > > try the ->create_server call using each of the different authflavors
> > > in the server's list if one was not specified at mount time. Once
> > > we have a successful ->create_server call, return the result. Only
> > > give up and return error if all attempts fail.
> >
> > Overall, looks about right. Thanks for retaining the dprintk's! Should Cc: Dros on these as well, since he was the last person to touch this code.
> >
> > More below.
> >
>
> Thanks for the review so far. Replies inline below...
>
> > >
> > > Cc: Chuck Lever <[email protected]>
> > > Signed-off-by: Jeff Layton <[email protected]>
> > > ---
> > > fs/nfs/super.c | 148 +++++++++++++++++++++++++++++++--------------------------
> > > 1 file changed, 80 insertions(+), 68 deletions(-)
> > >
> > > diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> > > index a0949f5..53ea73d 100644
> > > --- a/fs/nfs/super.c
> > > +++ b/fs/nfs/super.c
> > > @@ -1608,29 +1608,20 @@ out_security_failure:
> > > }
> > >
> > > /*
> > > - * Select a security flavor for this mount. The selected flavor
> > > - * is planted in args->auth_flavors[0].
> > > - *
> > > - * Returns 0 on success, -EACCES on failure.
> > > + * Ensure that the specified authtype in args->auth_flavors[0] is supported by
> > > + * the server. Returns 0 if it's ok, and -EACCES if not.
> > > */
> > > -static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
> > > - struct nfs_mount_request *request)
> > > +static int nfs_verify_authflavor(struct nfs_parsed_mount_data *args,
> > > + rpc_authflavor_t *server_authlist, unsigned int count)
> > > {
> > > - unsigned int i, count = *(request->auth_flav_len);
> > > - rpc_authflavor_t flavor;
> > > -
> > > - /*
> > > - * The NFSv2 MNT operation does not return a flavor list.
> > > - */
> > > - if (args->mount_server.version != NFS_MNT3_VERSION)
> > > - goto out_default;
> > > + int i;
> >
> > count is unsigned, therefore i must also be. Otherwise you get mixed sign comparisons in the for() loops.
> >
>
> Thanks, fixed...
>
> > >
> > > /*
> > > - * Certain releases of Linux's mountd return an empty
> > > - * flavor list in some cases.
> > > + * The NFSv2 MNT operation does not return a flavor list and certain
> > > + * releases of Linux's mountd return an empty flavor list in some cases.
> > > */
> > > - if (count == 0)
> > > - goto out_default;
> > > + if (args->mount_server.version != NFS_MNT3_VERSION || count == 0)
> > > + goto out;
> >
> > The above chunk (except the function name change) seems like unnecessary churn.
> >
> > We leave these checks separate from each other for clarity and because they are not logically related to each other, even if the outcome is the same. Keep each check a separate, independent step with its own documentation.
> >
>
> Ok, fair enough. I've reverted that consolidation and kept it more or less as-is.
>
> > >
> > > /*
> > > * If the sec= mount option is used, the specified flavor or AUTH_NULL
> > > @@ -1640,60 +1631,19 @@ static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
> > > * means that the server will ignore the rpc creds, so any flavor
> > > * can be used.
> > > */
> > > - 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)
> > > - goto out;
> > > - }
> > > - dfprintk(MOUNT, "NFS: auth flavor %d not supported by server\n",
> > > - args->auth_flavors[0]);
> > > - goto out_err;
> > > - }
> > > -
> > > - /*
> > > - * RFC 2623, section 2.7 suggests we SHOULD prefer the
> > > - * flavor listed first. However, some servers list
> > > - * AUTH_NULL first. Avoid ever choosing AUTH_NULL.
> > > - */
> > > for (i = 0; i < count; i++) {
> > > - struct rpcsec_gss_info info;
> > > -
> > > - flavor = request->auth_flavs[i];
> > > - switch (flavor) {
> > > - case RPC_AUTH_UNIX:
> > > - goto out_set;
> > > - case RPC_AUTH_NULL:
> > > - continue;
> > > - default:
> > > - if (rpcauth_get_gssinfo(flavor, &info) == 0)
> > > - goto out_set;
> > > - }
> > > - }
> > > -
> > > - /*
> > > - * As a last chance, see if the server list contains AUTH_NULL -
> > > - * if it does, use the default flavor.
> > > - */
> > > - for (i = 0; i < count; i++) {
> > > - if (request->auth_flavs[i] == RPC_AUTH_NULL)
> > > - goto out_default;
> > > + if (args->auth_flavors[0] == server_authlist[i] ||
> > > + server_authlist[i] == RPC_AUTH_NULL)
> > > + goto out;
> > > }
> > >
> > > - dfprintk(MOUNT, "NFS: no auth flavors in common with server\n");
> > > - goto out_err;
> > > + dfprintk(MOUNT, "NFS: auth flavor %u not supported by server\n",
> > > + args->auth_flavors[0]);
> > > + return -EACCES;
> > >
> > > -out_default:
> > > - /* use default if flavor not already set */
> > > - flavor = (args->auth_flavors[0] == RPC_AUTH_MAXFLAVOR) ?
> > > - RPC_AUTH_UNIX : args->auth_flavors[0];
> > > -out_set:
> > > - args->auth_flavors[0] = flavor;
> > > out:
> > > - dfprintk(MOUNT, "NFS: using auth flavor %d\n", args->auth_flavors[0]);
> > > + dfprintk(MOUNT, "NFS: using auth flavor %u\n", args->auth_flavors[0]);
> > > return 0;
> > > -out_err:
> > > - return -EACCES;
> > > }
> > >
> > > /*
> > > @@ -1756,13 +1706,16 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
> > > return status;
> > > }
> > >
> > > - return nfs_select_flavor(args, &request);
> > > + return 0;
> > > }
> > >
> > > static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_info,
> > > struct nfs_subversion *nfs_mod)
> > > {
> > > - int status;
> > > + int status, i;
> >
> > As above, if authlist_len is unsigned, then i should be too.
> >
>
> Fixed...
>
> > > + bool tried_auth_unix = false;
> > > + bool auth_null_in_list = false;
> > > + struct nfs_server *server = ERR_PTR(-EACCES);
> > > struct nfs_parsed_mount_data *args = mount_info->parsed;
> > > rpc_authflavor_t authlist[NFS_MAX_SECFLAVORS];
> > > unsigned int authlist_len = ARRAY_SIZE(authlist);
> > > @@ -1772,6 +1725,65 @@ static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_inf
> > > if (status)
> > > return ERR_PTR(status);
> > >
> > > + /*
> > > + * If the server's authlist is empty, and no sec= option was specified
> > > + * then just default to AUTH_UNIX.
> > > + */
> > > + if (args->auth_flavors[0] == RPC_AUTH_MAXFLAVOR &&
> > > + (args->mount_server.version != NFS_MNT3_VERSION || authlist_len == 0)) {
> > > + dfprintk(MOUNT, "NFS: No authflavor specified, and no server list.");
> > > + args->auth_flavors[0] = RPC_AUTH_UNIX;
> > > + }
> >
> > Why duplicate these checks?
> >
>
> They're not duplicated (exactly).
>
> When we have no authlist from the server, we need to set auth_flavor[0]
> to something valid that we can try to use since iterating over the list
> won't work at that point. Once we have auth_flavor[0] set to something,
> we then call nfs_verify_authflavor to validate that it's usable.
>
> So, yes we are doing those checks twice, but they serve different
> purposes.
>
> > > +
> > > + /*
> > > + * Was a sec= authflavor specified in the options? First, verify
> > > + * whether the server supports it, and then just try to use it if so.
> > > + */
> > > + if (args->auth_flavors[0] != RPC_AUTH_MAXFLAVOR) {
> > > + status = nfs_verify_authflavor(args, authlist, authlist_len);
> > > + dfprintk(MOUNT, "NFS: using auth flavor %u\n", args->auth_flavors[0]);
> > > + if (status)
> > > + return ERR_PTR(status);
> > > + return nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
> > > + }
> > > +
> > > + /*
> > > + * No sec= option was provided. RFC 2623, section 2.7 suggests we
> > > + * SHOULD prefer the flavor listed first. However, some servers list
> > > + * AUTH_NULL first. Avoid ever choosing AUTH_NULL.
> > > + */
> > > + for (i = 0; i < authlist_len; ++i) {
> > > + rpc_authflavor_t flavor;
> > > + struct rpcsec_gss_info info;
> > > +
> > > + flavor = authlist[i];
> > > + switch (flavor) {
> > > + case RPC_AUTH_UNIX:
> > > + tried_auth_unix = true;
> > > + break;
> > > + case RPC_AUTH_NULL:
> > > + auth_null_in_list = true;
> >
> > I think I complained about this when Dros proposed a similar optimization last month. I'd rather not have this extra boolean here. It makes the logic more fragile to have the next check below depend on this loop. If someone decides to move or alter this loop, then it can silently break the check below.
> >
> > The boolean feels like premature optimization especially because the flavor list is always short, and the check below is executed very infrequently.
> >
> > The NFSv3 mount code will always be one giant heuristic. IMO, the best approach is making the source code as plain as possible, rather than defaulting to our normal kernel style of terse, compact code.
> >
>
> I don't know...the booleans seem a lot clearer to me. I can have the
> code rewalk the list, but I'm not sure I buy the argument that it helps
> future-proof this code. Anyone modifying this had _better_ understand
> the logic first. ;)
>
Actually, now that I look, I'm not sure we can reasonably get away
without using some sort of bool here. We only want to try using
RPC_AUTH_UNIX if we haven't already tried it -- there's no point in
trying it twice. If we rewalk the list and don't track what was done in
the earlier loop, then we have to assume that just because it was in
the list that we already tried it.
So, I think that keeping track of what was done in the earlier loop
will be *more* resilient in the face of future changes, since that
assumption is rather subtle. Besides, I like my bikesheds blue. ;)
>
> > > + continue;
> > > + default:
> > > + if (rpcauth_get_gssinfo(flavor, &info) != 0)
> > > + continue;
> > > + }
> > > + dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", flavor);
> > > + args->auth_flavors[0] = flavor;
> > > + server = nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
> > > + if (!IS_ERR(server))
> > > + return server;
> > > + }
> > > +
> > > + /*
> > > + * Nothing we tried so far worked. As a last ditch effort, try to use
> > > + * AUTH_UNIX if we haven't already, and AUTH_NULL was in server's list.
> > > + */
> > > + if (tried_auth_unix || !auth_null_in_list)
> > > + return server;
> >
> > It would be more clear and less leaky if you "return ERR_PTR(-EACCES);" here, maybe?
> >
>
> Possibly, though that means that you're then assuming that the last
> call to ->create_server returned -EACCES. I'm not sure it's valid to
> assume that.
>
> I think it would be best to return whatever it ended up returning, and
> only return an explicit -EACCES if we never called create_server at all
> in the loop. The fact that we initialize server to ERR_PTR(-EACCES)
> makes that work in this patch.
>
> After all, in the !need_mount case, we end up returning whatever
> ->create_server returned. If it's not valid to let that error bubble
> up, then the place to fix that is in nfs_try_mount.
>
> > > +
> > > + dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", RPC_AUTH_UNIX);
> > > + args->auth_flavors[0] = RPC_AUTH_UNIX;
> > > return nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
> > > }
> >
>
--
Jeff Layton <[email protected]>
In a later patch we're going to want to cycle over this list and attempt
to call ->create_server for each different flavor until one succeeds.
Move the list allocation to the stack of nfs_try_mount_request() and
pass a pointer to it and its length to nfs_request_mount().
Cc: Chuck Lever <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfs/super.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index afeee81..a0949f5 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,
+ rpc_authflavor_t *server_authlist,
+ unsigned int *server_authlist_len)
{
- 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,
};
@@ -1763,8 +1763,12 @@ static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_inf
struct nfs_subversion *nfs_mod)
{
int status;
+ struct nfs_parsed_mount_data *args = mount_info->parsed;
+ rpc_authflavor_t authlist[NFS_MAX_SECFLAVORS];
+ unsigned int authlist_len = ARRAY_SIZE(authlist);
- status = nfs_request_mount(mount_info->parsed, mount_info->mntfh);
+ status = nfs_request_mount(args, mount_info->mntfh, authlist,
+ &authlist_len);
if (status)
return ERR_PTR(status);
--
1.8.1.4
This looks like pointless refactoring for now, but we'll flesh out
the need_mount case a little more in a later patch.
Cc: Chuck Lever <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfs/super.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 2d7525f..afeee81 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1759,21 +1759,29 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
return nfs_select_flavor(args, &request);
}
+static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_info,
+ struct nfs_subversion *nfs_mod)
+{
+ int status;
+
+ status = nfs_request_mount(mount_info->parsed, mount_info->mntfh);
+ if (status)
+ return ERR_PTR(status);
+
+ return nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
+}
+
struct dentry *nfs_try_mount(int flags, const char *dev_name,
struct nfs_mount_info *mount_info,
struct nfs_subversion *nfs_mod)
{
- int status;
struct nfs_server *server;
- if (mount_info->parsed->need_mount) {
- status = nfs_request_mount(mount_info->parsed, mount_info->mntfh);
- if (status)
- return ERR_PTR(status);
- }
+ if (mount_info->parsed->need_mount)
+ server = nfs_try_mount_request(mount_info, nfs_mod);
+ else
+ server = nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
- /* Get a volume representation */
- server = nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
if (IS_ERR(server))
return ERR_CAST(server);
--
1.8.1.4
On Thu, 27 Jun 2013 15:09:42 +0000
"Adamson, Dros" <[email protected]> wrote:
> I think this looks reasonable. I agree with Jeff's use of bools - we don't want to try AUTH_UNIX twice.
>
> I am currently cleaning up a patchset that adds support for multiple security flavors passed through mount with the same syntax as /etc/exports, i.e. sec=krb5:krb5i. The initial motivation of the patchset is to handle crossing SECINFO boundaries in a sane way. Currently you could be mounted with krb5p, then follow a SECINFO to using AUTH_UNIX for a submount which is pretty confusing to users. This patchset will limit the client to mount-specified flavors when following a SECINFO (or any flavor if no sec= was specified).
>
> Part of this patchset modifies how we track mount specified auth flavors - including keeping a list of parsed auth flavors that doesn't change (no more overwriting auth_flavor[0]). I'll work this patch into my branch and hopefully will be able to clean up some of this logic.
>
> -dros
>
That sounds like good work, but wait until I repost...
I've got at least one more respin on the way to incorporate Chuck's
suggestions. Also, I think it'll be clearer to change the logic a bit
when the authlist we get from the server is empty for whatever reason.
Rather than picking an authflavor at that point, we can simply edit
the list to just have a single RPC_AUTH_NULL entry in it. At that
point, the rest of the logic in that function can do the right thing.
That also addresses Chuck's concern about checking for an empty authlist
twice.
>
> On Jun 27, 2013, at 10:14 AM, Jeff Layton <[email protected]> wrote:
>
> > On Thu, 27 Jun 2013 09:00:59 -0400
> > Jeff Layton <[email protected]> wrote:
> >
> >> On Wed, 26 Jun 2013 16:42:39 -0400
> >> Chuck Lever <[email protected]> wrote:
> >>
> >>>
> >>> On Jun 26, 2013, at 3:45 PM, Jeff Layton <[email protected]> wrote:
> >>>
> >>>> The current scheme is to try and pick the auth flavor that the server
> >>>> prefers. In some cases though, we may find that we're not actually
> >>>> able to use that auth flavor later. For instance, the server may
> >>>> prefer an AUTH_GSS flavor, but we may not be able to get GSSAPI creds.
> >>>>
> >>>> The current code just gives up at that point. Change it instead to
> >>>> try the ->create_server call using each of the different authflavors
> >>>> in the server's list if one was not specified at mount time. Once
> >>>> we have a successful ->create_server call, return the result. Only
> >>>> give up and return error if all attempts fail.
> >>>
> >>> Overall, looks about right. Thanks for retaining the dprintk's! Should Cc: Dros on these as well, since he was the last person to touch this code.
> >>>
> >>> More below.
> >>>
> >>
> >> Thanks for the review so far. Replies inline below...
> >>
> >>>>
> >>>> Cc: Chuck Lever <[email protected]>
> >>>> Signed-off-by: Jeff Layton <[email protected]>
> >>>> ---
> >>>> fs/nfs/super.c | 148 +++++++++++++++++++++++++++++++--------------------------
> >>>> 1 file changed, 80 insertions(+), 68 deletions(-)
> >>>>
> >>>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> >>>> index a0949f5..53ea73d 100644
> >>>> --- a/fs/nfs/super.c
> >>>> +++ b/fs/nfs/super.c
> >>>> @@ -1608,29 +1608,20 @@ out_security_failure:
> >>>> }
> >>>>
> >>>> /*
> >>>> - * Select a security flavor for this mount. The selected flavor
> >>>> - * is planted in args->auth_flavors[0].
> >>>> - *
> >>>> - * Returns 0 on success, -EACCES on failure.
> >>>> + * Ensure that the specified authtype in args->auth_flavors[0] is supported by
> >>>> + * the server. Returns 0 if it's ok, and -EACCES if not.
> >>>> */
> >>>> -static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
> >>>> - struct nfs_mount_request *request)
> >>>> +static int nfs_verify_authflavor(struct nfs_parsed_mount_data *args,
> >>>> + rpc_authflavor_t *server_authlist, unsigned int count)
> >>>> {
> >>>> - unsigned int i, count = *(request->auth_flav_len);
> >>>> - rpc_authflavor_t flavor;
> >>>> -
> >>>> - /*
> >>>> - * The NFSv2 MNT operation does not return a flavor list.
> >>>> - */
> >>>> - if (args->mount_server.version != NFS_MNT3_VERSION)
> >>>> - goto out_default;
> >>>> + int i;
> >>>
> >>> count is unsigned, therefore i must also be. Otherwise you get mixed sign comparisons in the for() loops.
> >>>
> >>
> >> Thanks, fixed...
> >>
> >>>>
> >>>> /*
> >>>> - * Certain releases of Linux's mountd return an empty
> >>>> - * flavor list in some cases.
> >>>> + * The NFSv2 MNT operation does not return a flavor list and certain
> >>>> + * releases of Linux's mountd return an empty flavor list in some cases.
> >>>> */
> >>>> - if (count == 0)
> >>>> - goto out_default;
> >>>> + if (args->mount_server.version != NFS_MNT3_VERSION || count == 0)
> >>>> + goto out;
> >>>
> >>> The above chunk (except the function name change) seems like unnecessary churn.
> >>>
> >>> We leave these checks separate from each other for clarity and because they are not logically related to each other, even if the outcome is the same. Keep each check a separate, independent step with its own documentation.
> >>>
> >>
> >> Ok, fair enough. I've reverted that consolidation and kept it more or less as-is.
> >>
> >>>>
> >>>> /*
> >>>> * If the sec= mount option is used, the specified flavor or AUTH_NULL
> >>>> @@ -1640,60 +1631,19 @@ static int nfs_select_flavor(struct nfs_parsed_mount_data *args,
> >>>> * means that the server will ignore the rpc creds, so any flavor
> >>>> * can be used.
> >>>> */
> >>>> - 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)
> >>>> - goto out;
> >>>> - }
> >>>> - dfprintk(MOUNT, "NFS: auth flavor %d not supported by server\n",
> >>>> - args->auth_flavors[0]);
> >>>> - goto out_err;
> >>>> - }
> >>>> -
> >>>> - /*
> >>>> - * RFC 2623, section 2.7 suggests we SHOULD prefer the
> >>>> - * flavor listed first. However, some servers list
> >>>> - * AUTH_NULL first. Avoid ever choosing AUTH_NULL.
> >>>> - */
> >>>> for (i = 0; i < count; i++) {
> >>>> - struct rpcsec_gss_info info;
> >>>> -
> >>>> - flavor = request->auth_flavs[i];
> >>>> - switch (flavor) {
> >>>> - case RPC_AUTH_UNIX:
> >>>> - goto out_set;
> >>>> - case RPC_AUTH_NULL:
> >>>> - continue;
> >>>> - default:
> >>>> - if (rpcauth_get_gssinfo(flavor, &info) == 0)
> >>>> - goto out_set;
> >>>> - }
> >>>> - }
> >>>> -
> >>>> - /*
> >>>> - * As a last chance, see if the server list contains AUTH_NULL -
> >>>> - * if it does, use the default flavor.
> >>>> - */
> >>>> - for (i = 0; i < count; i++) {
> >>>> - if (request->auth_flavs[i] == RPC_AUTH_NULL)
> >>>> - goto out_default;
> >>>> + if (args->auth_flavors[0] == server_authlist[i] ||
> >>>> + server_authlist[i] == RPC_AUTH_NULL)
> >>>> + goto out;
> >>>> }
> >>>>
> >>>> - dfprintk(MOUNT, "NFS: no auth flavors in common with server\n");
> >>>> - goto out_err;
> >>>> + dfprintk(MOUNT, "NFS: auth flavor %u not supported by server\n",
> >>>> + args->auth_flavors[0]);
> >>>> + return -EACCES;
> >>>>
> >>>> -out_default:
> >>>> - /* use default if flavor not already set */
> >>>> - flavor = (args->auth_flavors[0] == RPC_AUTH_MAXFLAVOR) ?
> >>>> - RPC_AUTH_UNIX : args->auth_flavors[0];
> >>>> -out_set:
> >>>> - args->auth_flavors[0] = flavor;
> >>>> out:
> >>>> - dfprintk(MOUNT, "NFS: using auth flavor %d\n", args->auth_flavors[0]);
> >>>> + dfprintk(MOUNT, "NFS: using auth flavor %u\n", args->auth_flavors[0]);
> >>>> return 0;
> >>>> -out_err:
> >>>> - return -EACCES;
> >>>> }
> >>>>
> >>>> /*
> >>>> @@ -1756,13 +1706,16 @@ static int nfs_request_mount(struct nfs_parsed_mount_data *args,
> >>>> return status;
> >>>> }
> >>>>
> >>>> - return nfs_select_flavor(args, &request);
> >>>> + return 0;
> >>>> }
> >>>>
> >>>> static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_info,
> >>>> struct nfs_subversion *nfs_mod)
> >>>> {
> >>>> - int status;
> >>>> + int status, i;
> >>>
> >>> As above, if authlist_len is unsigned, then i should be too.
> >>>
> >>
> >> Fixed...
> >>
> >>>> + bool tried_auth_unix = false;
> >>>> + bool auth_null_in_list = false;
> >>>> + struct nfs_server *server = ERR_PTR(-EACCES);
> >>>> struct nfs_parsed_mount_data *args = mount_info->parsed;
> >>>> rpc_authflavor_t authlist[NFS_MAX_SECFLAVORS];
> >>>> unsigned int authlist_len = ARRAY_SIZE(authlist);
> >>>> @@ -1772,6 +1725,65 @@ static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_inf
> >>>> if (status)
> >>>> return ERR_PTR(status);
> >>>>
> >>>> + /*
> >>>> + * If the server's authlist is empty, and no sec= option was specified
> >>>> + * then just default to AUTH_UNIX.
> >>>> + */
> >>>> + if (args->auth_flavors[0] == RPC_AUTH_MAXFLAVOR &&
> >>>> + (args->mount_server.version != NFS_MNT3_VERSION || authlist_len == 0)) {
> >>>> + dfprintk(MOUNT, "NFS: No authflavor specified, and no server list.");
> >>>> + args->auth_flavors[0] = RPC_AUTH_UNIX;
> >>>> + }
> >>>
> >>> Why duplicate these checks?
> >>>
> >>
> >> They're not duplicated (exactly).
> >>
> >> When we have no authlist from the server, we need to set auth_flavor[0]
> >> to something valid that we can try to use since iterating over the list
> >> won't work at that point. Once we have auth_flavor[0] set to something,
> >> we then call nfs_verify_authflavor to validate that it's usable.
> >>
> >> So, yes we are doing those checks twice, but they serve different
> >> purposes.
> >>
> >>>> +
> >>>> + /*
> >>>> + * Was a sec= authflavor specified in the options? First, verify
> >>>> + * whether the server supports it, and then just try to use it if so.
> >>>> + */
> >>>> + if (args->auth_flavors[0] != RPC_AUTH_MAXFLAVOR) {
> >>>> + status = nfs_verify_authflavor(args, authlist, authlist_len);
> >>>> + dfprintk(MOUNT, "NFS: using auth flavor %u\n", args->auth_flavors[0]);
> >>>> + if (status)
> >>>> + return ERR_PTR(status);
> >>>> + return nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
> >>>> + }
> >>>> +
> >>>> + /*
> >>>> + * No sec= option was provided. RFC 2623, section 2.7 suggests we
> >>>> + * SHOULD prefer the flavor listed first. However, some servers list
> >>>> + * AUTH_NULL first. Avoid ever choosing AUTH_NULL.
> >>>> + */
> >>>> + for (i = 0; i < authlist_len; ++i) {
> >>>> + rpc_authflavor_t flavor;
> >>>> + struct rpcsec_gss_info info;
> >>>> +
> >>>> + flavor = authlist[i];
> >>>> + switch (flavor) {
> >>>> + case RPC_AUTH_UNIX:
> >>>> + tried_auth_unix = true;
> >>>> + break;
> >>>> + case RPC_AUTH_NULL:
> >>>> + auth_null_in_list = true;
> >>>
> >>> I think I complained about this when Dros proposed a similar optimization last month. I'd rather not have this extra boolean here. It makes the logic more fragile to have the next check below depend on this loop. If someone decides to move or alter this loop, then it can silently break the check below.
> >>>
> >>> The boolean feels like premature optimization especially because the flavor list is always short, and the check below is executed very infrequently.
> >>>
> >>> The NFSv3 mount code will always be one giant heuristic. IMO, the best approach is making the source code as plain as possible, rather than defaulting to our normal kernel style of terse, compact code.
> >>>
> >>
> >> I don't know...the booleans seem a lot clearer to me. I can have the
> >> code rewalk the list, but I'm not sure I buy the argument that it helps
> >> future-proof this code. Anyone modifying this had _better_ understand
> >> the logic first. ;)
> >>
> >
> > Actually, now that I look, I'm not sure we can reasonably get away
> > without using some sort of bool here. We only want to try using
> > RPC_AUTH_UNIX if we haven't already tried it -- there's no point in
> > trying it twice. If we rewalk the list and don't track what was done in
> > the earlier loop, then we have to assume that just because it was in
> > the list that we already tried it.
> >
> > So, I think that keeping track of what was done in the earlier loop
> > will be *more* resilient in the face of future changes, since that
> > assumption is rather subtle. Besides, I like my bikesheds blue. ;)
> >
> >
> >>
> >>>> + continue;
> >>>> + default:
> >>>> + if (rpcauth_get_gssinfo(flavor, &info) != 0)
> >>>> + continue;
> >>>> + }
> >>>> + dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", flavor);
> >>>> + args->auth_flavors[0] = flavor;
> >>>> + server = nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
> >>>> + if (!IS_ERR(server))
> >>>> + return server;
> >>>> + }
> >>>> +
> >>>> + /*
> >>>> + * Nothing we tried so far worked. As a last ditch effort, try to use
> >>>> + * AUTH_UNIX if we haven't already, and AUTH_NULL was in server's list.
> >>>> + */
> >>>> + if (tried_auth_unix || !auth_null_in_list)
> >>>> + return server;
> >>>
> >>> It would be more clear and less leaky if you "return ERR_PTR(-EACCES);" here, maybe?
> >>>
> >>
> >> Possibly, though that means that you're then assuming that the last
> >> call to ->create_server returned -EACCES. I'm not sure it's valid to
> >> assume that.
> >>
> >> I think it would be best to return whatever it ended up returning, and
> >> only return an explicit -EACCES if we never called create_server at all
> >> in the loop. The fact that we initialize server to ERR_PTR(-EACCES)
> >> makes that work in this patch.
> >>
> >> After all, in the !need_mount case, we end up returning whatever
> >> ->create_server returned. If it's not valid to let that error bubble
> >> up, then the place to fix that is in nfs_try_mount.
> >>
> >>>> +
> >>>> + dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", RPC_AUTH_UNIX);
> >>>> + args->auth_flavors[0] = RPC_AUTH_UNIX;
> >>>> return nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
> >>>> }
> >>>
> >>
> >
> >
> > --
> > Jeff Layton <[email protected]>
>
--
Jeff Layton <[email protected]>