Return-Path: linux-nfs-owner@vger.kernel.org Received: from mx11.netapp.com ([216.240.18.76]:38052 "EHLO mx11.netapp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932195AbaFPSZp convert rfc822-to-8bit (ORCPT ); Mon, 16 Jun 2014 14:25:45 -0400 From: "Adamson, Andy" To: Trond Myklebust CC: "linux-nfs@vger.kernel.org" Subject: Re: [PATCH Version 2 1/1] NFS NFS test SECINFO RPC_AUTH_GSS pseudoflavors for support Date: Mon, 16 Jun 2014 18:25:28 +0000 Message-ID: References: <1402599752-3168-1-git-send-email-andros@netapp.com> <1402599752-3168-2-git-send-email-andros@netapp.com> <1402941396.4801.7.camel@leira.trondhjem.org> In-Reply-To: <1402941396.4801.7.camel@leira.trondhjem.org> Content-Type: text/plain; charset="Windows-1252" MIME-Version: 1.0 Sender: linux-nfs-owner@vger.kernel.org List-ID: On Jun 16, 2014, at 1:56 PM, Trond Myklebust wrote: > On Thu, 2014-06-12 at 15:02 -0400, andros@netapp.com wrote: >> From: Andy Adamson >> >> Fix nfs4_negotiate_security to create an rpc_clnt used to test each SECINFO >> returned pseudoflavor. Check credential creation (and gss_context creation) >> which is important for RPC_AUTH_GSS pseudoflavors which can fail for multiple >> reasons including mis-configuration. >> >> Don't call nfs4_negotiate in nfs4_submount as it was just called by >> nfs4_proc_lookup_mountpoint (nfs4_proc_lookup_common) >> >> Signed-off-by: Andy Adamson >> >> Conflicts: >> fs/nfs/nfs4namespace.c > > Queued for 3.17, but I had to fix a couple of issues with > nfs_find_best_sec() first. See below: Thanks for the fixes which look good. > > >> --- >> fs/nfs/nfs4_fs.h | 2 +- >> fs/nfs/nfs4namespace.c | 103 ++++++++++++++++++++++++++++--------------------- >> fs/nfs/nfs4proc.c | 2 +- >> net/sunrpc/auth.c | 1 + >> 4 files changed, 61 insertions(+), 47 deletions(-) >> >> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h >> index f63cb87..ba2affa 100644 >> --- a/fs/nfs/nfs4_fs.h >> +++ b/fs/nfs/nfs4_fs.h >> @@ -230,7 +230,7 @@ int nfs_atomic_open(struct inode *, struct dentry *, struct file *, >> extern struct file_system_type nfs4_fs_type; >> >> /* nfs4namespace.c */ >> -struct rpc_clnt *nfs4_create_sec_client(struct rpc_clnt *, struct inode *, struct qstr *); >> +struct rpc_clnt *nfs4_negotiate_security(struct rpc_clnt *, struct inode *, struct qstr *); >> struct vfsmount *nfs4_submount(struct nfs_server *, struct dentry *, >> struct nfs_fh *, struct nfs_fattr *); >> int nfs4_replace_transport(struct nfs_server *server, >> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c >> index 3d5dbf8..5517f02 100644 >> --- a/fs/nfs/nfs4namespace.c >> +++ b/fs/nfs/nfs4namespace.c >> @@ -139,19 +139,28 @@ static size_t nfs_parse_server_name(char *string, size_t len, >> * @server: NFS server struct >> * @flavors: List of security tuples returned by SECINFO procedure >> * >> - * Return the pseudoflavor of the first security mechanism in >> - * "flavors" that is locally supported. Return RPC_AUTH_UNIX if >> - * no matching flavor is found in the array. The "flavors" array >> + * Return an rpc client that uses the first security mechanism in >> + * "flavors" that is locally supported. The "flavors" array >> * is searched in the order returned from the server, per RFC 3530 >> - * recommendation. >> + * recommendation and each flavor is checked for membership in the >> + * sec= mount option list if it exists. >> + * >> + * Return -EPERM if no matching flavor is found in the array. >> + * >> + * Please call rpc_shutdown_client() when you are done with this rpc client. >> + * >> */ >> -static rpc_authflavor_t nfs_find_best_sec(struct nfs_server *server, >> +static struct rpc_clnt *nfs_find_best_sec(struct rpc_clnt *clnt, >> + struct nfs_server *server, >> struct nfs4_secinfo_flavors *flavors) >> { >> - rpc_authflavor_t pseudoflavor; >> + rpc_authflavor_t pflavor; >> struct nfs4_secinfo4 *secinfo; >> + struct rpc_clnt *new; >> unsigned int i; >> + struct rpc_cred *cred; >> >> + new = ERR_PTR(-EPERM); >> for (i = 0; i < flavors->num_flavors; i++) { >> secinfo = &flavors->flavors[i]; >> >> @@ -159,62 +168,71 @@ static rpc_authflavor_t nfs_find_best_sec(struct nfs_server *server, >> case RPC_AUTH_NULL: >> case RPC_AUTH_UNIX: >> case RPC_AUTH_GSS: >> - pseudoflavor = rpcauth_get_pseudoflavor(secinfo->flavor, >> + pflavor = rpcauth_get_pseudoflavor(secinfo->flavor, >> &secinfo->flavor_info); >> - /* make sure pseudoflavor matches sec= mount opt */ >> - if (pseudoflavor != RPC_AUTH_MAXFLAVOR && >> - nfs_auth_info_match(&server->auth_info, >> - pseudoflavor)) >> - return pseudoflavor; >> - break; >> + /* does the pseudoflavor match a sec= mount opt? */ >> + if (pflavor != RPC_AUTH_MAXFLAVOR && >> + nfs_auth_info_match(&server->auth_info, pflavor)) { >> + >> + /* Cloning creates an rpc_auth for the flavor */ >> + new = rpc_clone_client_set_auth(clnt, pflavor); >> + if (IS_ERR(new)) >> + continue; > > If IS_ERR(new), we can end up returning an error which is not EPERM. > >> + /** >> + * Check that the user actually can use the >> + * flavor. This is mostly for RPC_AUTH_GSS >> + * where cr_init obtains a gss context >> + */ >> + cred = rpcauth_lookupcred(new->cl_auth, 0); >> + if (IS_ERR(cred)) { >> + rpc_shutdown_client(new); >> + continue; >> + } >> + put_rpccred(cred); >> + break; > > If IS_ERR(cred), we can end up freeing 'new', and then returning the > pointer to the freed client. ugg. Should have caught this one? Thanks ?>Andy > >> + } >> } >> } >> - >> - /* if there were any sec= options then nothing matched */ >> - if (server->auth_info.flavor_len > 0) >> - return -EPERM; >> - >> - return RPC_AUTH_UNIX; >> + return new; >> } >> >> -static rpc_authflavor_t nfs4_negotiate_security(struct inode *inode, struct qstr *name) >> +/** >> + * nfs4_negotiate_security - in response to an NFS4ERR_WRONGSEC on lookup, >> + * return an rpc_clnt that uses the best available security flavor with >> + * respect to the secinfo flavor list and the sec= mount options. >> + * >> + * @clnt: RPC client to clone >> + * @inode: directory inode >> + * @name: lookup name >> + * >> + * Please call rpc_shutdown_client() when you are done with this rpc client. >> + */ >> +struct rpc_clnt * >> +nfs4_negotiate_security(struct rpc_clnt *clnt, struct inode *inode, >> + struct qstr *name) >> { >> struct page *page; >> struct nfs4_secinfo_flavors *flavors; >> - rpc_authflavor_t flavor; >> + struct rpc_clnt *new = ERR_PTR(-ENOMEM); >> int err; >> >> page = alloc_page(GFP_KERNEL); >> if (!page) >> - return -ENOMEM; >> + return new; >> + >> flavors = page_address(page); >> >> err = nfs4_proc_secinfo(inode, name, flavors); >> if (err < 0) { >> - flavor = err; >> + new = ERR_PTR(err);; >> goto out; >> } >> >> - flavor = nfs_find_best_sec(NFS_SERVER(inode), flavors); >> + new = nfs_find_best_sec(clnt, NFS_SERVER(inode), flavors); >> >> out: >> put_page(page); >> - return flavor; >> -} >> - >> -/* >> - * Please call rpc_shutdown_client() when you are done with this client. >> - */ >> -struct rpc_clnt *nfs4_create_sec_client(struct rpc_clnt *clnt, struct inode *inode, >> - struct qstr *name) >> -{ >> - rpc_authflavor_t flavor; >> - >> - flavor = nfs4_negotiate_security(inode, name); >> - if ((int)flavor < 0) >> - return ERR_PTR((int)flavor); >> - >> - return rpc_clone_client_set_auth(clnt, flavor); >> + return new; >> } >> >> static struct vfsmount *try_location(struct nfs_clone_mount *mountdata, >> @@ -397,11 +415,6 @@ struct vfsmount *nfs4_submount(struct nfs_server *server, struct dentry *dentry, >> >> if (client->cl_auth->au_flavor != flavor) >> flavor = client->cl_auth->au_flavor; >> - else { >> - rpc_authflavor_t new = nfs4_negotiate_security(dir, name); >> - if ((int)new >= 0) >> - flavor = new; >> - } >> mnt = nfs_do_submount(dentry, fh, fattr, flavor); >> out: >> rpc_shutdown_client(client); >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >> index 68dd81e..fe14063 100644 >> --- a/fs/nfs/nfs4proc.c >> +++ b/fs/nfs/nfs4proc.c >> @@ -3247,7 +3247,7 @@ static int nfs4_proc_lookup_common(struct rpc_clnt **clnt, struct inode *dir, >> err = -EPERM; >> if (client != *clnt) >> goto out; >> - client = nfs4_create_sec_client(client, dir, name); >> + client = nfs4_negotiate_security(client, dir, name); >> if (IS_ERR(client)) >> return PTR_ERR(client); >> >> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c >> index 5285ead..1d97e19 100644 >> --- a/net/sunrpc/auth.c >> +++ b/net/sunrpc/auth.c >> @@ -592,6 +592,7 @@ rpcauth_lookupcred(struct rpc_auth *auth, int flags) >> put_group_info(acred.group_info); >> return ret; >> } >> +EXPORT_SYMBOL_GPL(rpcauth_lookupcred); >> >> void >> rpcauth_init_cred(struct rpc_cred *cred, const struct auth_cred *acred, > > -- > Trond Myklebust > Linux NFS client maintainer, PrimaryData > trond.myklebust@primarydata.com