On Tue, Sep 15, 2009 at 01:32:13PM -0400, Trond Myklebust wrote:
> On Tue, 2009-09-15 at 11:10 -0400, J. Bruce Fields wrote:
> > On Mon, Sep 14, 2009 at 05:16:22PM -0400, Trond Myklebust wrote:
> > > On Mon, 2009-09-14 at 17:09 -0400, Trond Myklebust wrote:
> > >
> > > > I suppose we could. Alternatively, why not just call it once and for all
> > > > when you start the NFSv4 server? The one cred will work on all
> > > > rpc_clients.
> > >
> > > (This is correct...)
> > >
> > > > You just have to remember to bump the reference count
> > > > before using it in an RPC call...
> > >
> > > Oops! Sorry, this is incorrect. The RPC engine will take care of bumping
> > > the ref count for you if necessary...
> >
> > OK, on another look: I create a generic cred with
> > rpc_lookup_machine_cred(). The gss cred then gets created by something
> > like:
> >
> > rpc_call_async
> > rpc_run_task
> > rpc_new_task
> > rpc_init_task
> > rpcauth_bindcred
> > cred->cr_ops->crbind == generic_bind_cred
> > gss_auth->au_ops->lookup_cred(auth, acred, 0)
> > rpcauth_lookup_credcache(gss_auth, acred, 0)
> >
> > But rpcauth_lookup_credcache ends up calling cr_init (gss_cred_init) and
> > waiting synchronously for the gss upcall. I want to avoid that. Should
> > I be setting up the cred differently, or could the rpc task
> > initialization process be tweaked somehow? (Or am I just
> > misunderstanding the code?)
>
> How about something like the following?
Looks perfect, thanks! Any objections to my submitting the following 4
patches for 2.6.32? I'm leaving in the minimal bugfix first mainly as
something simple and nonobtrusive for stable kernels.
--b.
>
> ------------------------------------------------------------------------
> SUNRPC: Defer the auth_gss upcall when the RPC call is asynchronous
>
> From: Trond Myklebust <[email protected]>
>
> Otherwise, the upcall is going to be synchronous, which may not be what the
> caller wants...
>
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
>
> include/linux/sunrpc/auth.h | 4 ++--
> net/sunrpc/auth.c | 20 ++++++++++++--------
> net/sunrpc/auth_generic.c | 4 ++--
> 3 files changed, 16 insertions(+), 12 deletions(-)
>
>
> diff --git a/include/linux/sunrpc/auth.h b/include/linux/sunrpc/auth.h
> index 3f63218..996df4d 100644
> --- a/include/linux/sunrpc/auth.h
> +++ b/include/linux/sunrpc/auth.h
> @@ -111,7 +111,7 @@ struct rpc_credops {
> void (*crdestroy)(struct rpc_cred *);
>
> int (*crmatch)(struct auth_cred *, struct rpc_cred *, int);
> - void (*crbind)(struct rpc_task *, struct rpc_cred *);
> + void (*crbind)(struct rpc_task *, struct rpc_cred *, int);
> __be32 * (*crmarshal)(struct rpc_task *, __be32 *);
> int (*crrefresh)(struct rpc_task *);
> __be32 * (*crvalidate)(struct rpc_task *, __be32 *);
> @@ -140,7 +140,7 @@ struct rpc_cred * rpcauth_lookup_credcache(struct rpc_auth *, struct auth_cred *
> void rpcauth_init_cred(struct rpc_cred *, const struct auth_cred *, struct rpc_auth *, const struct rpc_credops *);
> struct rpc_cred * rpcauth_lookupcred(struct rpc_auth *, int);
> void rpcauth_bindcred(struct rpc_task *, struct rpc_cred *, int);
> -void rpcauth_generic_bind_cred(struct rpc_task *, struct rpc_cred *);
> +void rpcauth_generic_bind_cred(struct rpc_task *, struct rpc_cred *, int);
> void put_rpccred(struct rpc_cred *);
> void rpcauth_unbindcred(struct rpc_task *);
> __be32 * rpcauth_marshcred(struct rpc_task *, __be32 *);
> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> index 0c431c2..54a4e04 100644
> --- a/net/sunrpc/auth.c
> +++ b/net/sunrpc/auth.c
> @@ -385,7 +385,7 @@ rpcauth_init_cred(struct rpc_cred *cred, const struct auth_cred *acred,
> EXPORT_SYMBOL_GPL(rpcauth_init_cred);
>
> void
> -rpcauth_generic_bind_cred(struct rpc_task *task, struct rpc_cred *cred)
> +rpcauth_generic_bind_cred(struct rpc_task *task, struct rpc_cred *cred, int lookupflags)
> {
> task->tk_msg.rpc_cred = get_rpccred(cred);
> dprintk("RPC: %5u holding %s cred %p\n", task->tk_pid,
> @@ -394,7 +394,7 @@ rpcauth_generic_bind_cred(struct rpc_task *task, struct rpc_cred *cred)
> EXPORT_SYMBOL_GPL(rpcauth_generic_bind_cred);
>
> static void
> -rpcauth_bind_root_cred(struct rpc_task *task)
> +rpcauth_bind_root_cred(struct rpc_task *task, int lookupflags)
> {
> struct rpc_auth *auth = task->tk_client->cl_auth;
> struct auth_cred acred = {
> @@ -405,7 +405,7 @@ rpcauth_bind_root_cred(struct rpc_task *task)
>
> dprintk("RPC: %5u looking up %s cred\n",
> task->tk_pid, task->tk_client->cl_auth->au_ops->au_name);
> - ret = auth->au_ops->lookup_cred(auth, &acred, 0);
> + ret = auth->au_ops->lookup_cred(auth, &acred, lookupflags);
> if (!IS_ERR(ret))
> task->tk_msg.rpc_cred = ret;
> else
> @@ -413,14 +413,14 @@ rpcauth_bind_root_cred(struct rpc_task *task)
> }
>
> static void
> -rpcauth_bind_new_cred(struct rpc_task *task)
> +rpcauth_bind_new_cred(struct rpc_task *task, int lookupflags)
> {
> struct rpc_auth *auth = task->tk_client->cl_auth;
> struct rpc_cred *ret;
>
> dprintk("RPC: %5u looking up %s cred\n",
> task->tk_pid, auth->au_ops->au_name);
> - ret = rpcauth_lookupcred(auth, 0);
> + ret = rpcauth_lookupcred(auth, lookupflags);
> if (!IS_ERR(ret))
> task->tk_msg.rpc_cred = ret;
> else
> @@ -430,12 +430,16 @@ rpcauth_bind_new_cred(struct rpc_task *task)
> void
> rpcauth_bindcred(struct rpc_task *task, struct rpc_cred *cred, int flags)
> {
> + int lookupflags = 0;
> +
> + if (flags & RPC_TASK_ASYNC)
> + lookupflags |= RPCAUTH_LOOKUP_NEW;
> if (cred != NULL)
> - cred->cr_ops->crbind(task, cred);
> + cred->cr_ops->crbind(task, cred, lookupflags);
> else if (flags & RPC_TASK_ROOTCREDS)
> - rpcauth_bind_root_cred(task);
> + rpcauth_bind_root_cred(task, lookupflags);
> else
> - rpcauth_bind_new_cred(task);
> + rpcauth_bind_new_cred(task, lookupflags);
> }
>
> void
> diff --git a/net/sunrpc/auth_generic.c b/net/sunrpc/auth_generic.c
> index 4028502..bf88bf8 100644
> --- a/net/sunrpc/auth_generic.c
> +++ b/net/sunrpc/auth_generic.c
> @@ -55,13 +55,13 @@ struct rpc_cred *rpc_lookup_machine_cred(void)
> EXPORT_SYMBOL_GPL(rpc_lookup_machine_cred);
>
> static void
> -generic_bind_cred(struct rpc_task *task, struct rpc_cred *cred)
> +generic_bind_cred(struct rpc_task *task, struct rpc_cred *cred, int lookupflags)
> {
> struct rpc_auth *auth = task->tk_client->cl_auth;
> struct auth_cred *acred = &container_of(cred, struct generic_cred, gc_base)->acred;
> struct rpc_cred *ret;
>
> - ret = auth->au_ops->lookup_cred(auth, acred, 0);
> + ret = auth->au_ops->lookup_cred(auth, acred, lookupflags);
> if (!IS_ERR(ret))
> task->tk_msg.rpc_cred = ret;
> else
>
>
From: Trond Myklebust <[email protected]>
Otherwise, the upcall is going to be synchronous, which may not be what the
caller wants...
Signed-off-by: Trond Myklebust <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
include/linux/sunrpc/auth.h | 4 ++--
net/sunrpc/auth.c | 20 ++++++++++++--------
net/sunrpc/auth_generic.c | 4 ++--
3 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/include/linux/sunrpc/auth.h b/include/linux/sunrpc/auth.h
index 3f63218..996df4d 100644
--- a/include/linux/sunrpc/auth.h
+++ b/include/linux/sunrpc/auth.h
@@ -111,7 +111,7 @@ struct rpc_credops {
void (*crdestroy)(struct rpc_cred *);
int (*crmatch)(struct auth_cred *, struct rpc_cred *, int);
- void (*crbind)(struct rpc_task *, struct rpc_cred *);
+ void (*crbind)(struct rpc_task *, struct rpc_cred *, int);
__be32 * (*crmarshal)(struct rpc_task *, __be32 *);
int (*crrefresh)(struct rpc_task *);
__be32 * (*crvalidate)(struct rpc_task *, __be32 *);
@@ -140,7 +140,7 @@ struct rpc_cred * rpcauth_lookup_credcache(struct rpc_auth *, struct auth_cred *
void rpcauth_init_cred(struct rpc_cred *, const struct auth_cred *, struct rpc_auth *, const struct rpc_credops *);
struct rpc_cred * rpcauth_lookupcred(struct rpc_auth *, int);
void rpcauth_bindcred(struct rpc_task *, struct rpc_cred *, int);
-void rpcauth_generic_bind_cred(struct rpc_task *, struct rpc_cred *);
+void rpcauth_generic_bind_cred(struct rpc_task *, struct rpc_cred *, int);
void put_rpccred(struct rpc_cred *);
void rpcauth_unbindcred(struct rpc_task *);
__be32 * rpcauth_marshcred(struct rpc_task *, __be32 *);
diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
index 0c431c2..54a4e04 100644
--- a/net/sunrpc/auth.c
+++ b/net/sunrpc/auth.c
@@ -385,7 +385,7 @@ rpcauth_init_cred(struct rpc_cred *cred, const struct auth_cred *acred,
EXPORT_SYMBOL_GPL(rpcauth_init_cred);
void
-rpcauth_generic_bind_cred(struct rpc_task *task, struct rpc_cred *cred)
+rpcauth_generic_bind_cred(struct rpc_task *task, struct rpc_cred *cred, int lookupflags)
{
task->tk_msg.rpc_cred = get_rpccred(cred);
dprintk("RPC: %5u holding %s cred %p\n", task->tk_pid,
@@ -394,7 +394,7 @@ rpcauth_generic_bind_cred(struct rpc_task *task, struct rpc_cred *cred)
EXPORT_SYMBOL_GPL(rpcauth_generic_bind_cred);
static void
-rpcauth_bind_root_cred(struct rpc_task *task)
+rpcauth_bind_root_cred(struct rpc_task *task, int lookupflags)
{
struct rpc_auth *auth = task->tk_client->cl_auth;
struct auth_cred acred = {
@@ -405,7 +405,7 @@ rpcauth_bind_root_cred(struct rpc_task *task)
dprintk("RPC: %5u looking up %s cred\n",
task->tk_pid, task->tk_client->cl_auth->au_ops->au_name);
- ret = auth->au_ops->lookup_cred(auth, &acred, 0);
+ ret = auth->au_ops->lookup_cred(auth, &acred, lookupflags);
if (!IS_ERR(ret))
task->tk_msg.rpc_cred = ret;
else
@@ -413,14 +413,14 @@ rpcauth_bind_root_cred(struct rpc_task *task)
}
static void
-rpcauth_bind_new_cred(struct rpc_task *task)
+rpcauth_bind_new_cred(struct rpc_task *task, int lookupflags)
{
struct rpc_auth *auth = task->tk_client->cl_auth;
struct rpc_cred *ret;
dprintk("RPC: %5u looking up %s cred\n",
task->tk_pid, auth->au_ops->au_name);
- ret = rpcauth_lookupcred(auth, 0);
+ ret = rpcauth_lookupcred(auth, lookupflags);
if (!IS_ERR(ret))
task->tk_msg.rpc_cred = ret;
else
@@ -430,12 +430,16 @@ rpcauth_bind_new_cred(struct rpc_task *task)
void
rpcauth_bindcred(struct rpc_task *task, struct rpc_cred *cred, int flags)
{
+ int lookupflags = 0;
+
+ if (flags & RPC_TASK_ASYNC)
+ lookupflags |= RPCAUTH_LOOKUP_NEW;
if (cred != NULL)
- cred->cr_ops->crbind(task, cred);
+ cred->cr_ops->crbind(task, cred, lookupflags);
else if (flags & RPC_TASK_ROOTCREDS)
- rpcauth_bind_root_cred(task);
+ rpcauth_bind_root_cred(task, lookupflags);
else
- rpcauth_bind_new_cred(task);
+ rpcauth_bind_new_cred(task, lookupflags);
}
void
diff --git a/net/sunrpc/auth_generic.c b/net/sunrpc/auth_generic.c
index 4028502..bf88bf8 100644
--- a/net/sunrpc/auth_generic.c
+++ b/net/sunrpc/auth_generic.c
@@ -55,13 +55,13 @@ struct rpc_cred *rpc_lookup_machine_cred(void)
EXPORT_SYMBOL_GPL(rpc_lookup_machine_cred);
static void
-generic_bind_cred(struct rpc_task *task, struct rpc_cred *cred)
+generic_bind_cred(struct rpc_task *task, struct rpc_cred *cred, int lookupflags)
{
struct rpc_auth *auth = task->tk_client->cl_auth;
struct auth_cred *acred = &container_of(cred, struct generic_cred, gc_base)->acred;
struct rpc_cred *ret;
- ret = auth->au_ops->lookup_cred(auth, acred, 0);
+ ret = auth->au_ops->lookup_cred(auth, acred, lookupflags);
if (!IS_ERR(ret))
task->tk_msg.rpc_cred = ret;
else
--
1.6.0.4
The failure here is pretty unlikely, but we should handle it anyway.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 17 ++++++++++++-----
fs/nfsd/nfssvc.c | 4 +++-
include/linux/nfsd/nfsd.h | 4 ++--
3 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 46e9ac5..11db40c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4004,7 +4004,7 @@ set_max_delegations(void)
/* initialization to perform when the nfsd service is started: */
-static void
+static int
__nfs4_state_start(void)
{
unsigned long grace_time;
@@ -4016,19 +4016,26 @@ __nfs4_state_start(void)
printk(KERN_INFO "NFSD: starting %ld-second grace period\n",
grace_time/HZ);
laundry_wq = create_singlethread_workqueue("nfsd4");
+ if (laundry_wq == NULL)
+ return -ENOMEM;
queue_delayed_work(laundry_wq, &laundromat_work, grace_time);
set_max_delegations();
+ return 0;
}
-void
+int
nfs4_state_start(void)
{
+ int ret;
+
if (nfs4_init)
- return;
+ return 0;
nfsd4_load_reboot_recovery_data();
- __nfs4_state_start();
+ ret = __nfs4_state_start();
+ if (ret)
+ return ret;
nfs4_init = 1;
- return;
+ return 0;
}
time_t
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 4472449..fcc0010 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -411,7 +411,9 @@ nfsd_svc(unsigned short port, int nrservs)
error = nfsd_racache_init(2*nrservs);
if (error<0)
goto out;
- nfs4_state_start();
+ error = nfs4_state_start();
+ if (error)
+ goto out;
nfsd_reset_versions();
diff --git a/include/linux/nfsd/nfsd.h b/include/linux/nfsd/nfsd.h
index 2812ed5..24fdf89 100644
--- a/include/linux/nfsd/nfsd.h
+++ b/include/linux/nfsd/nfsd.h
@@ -166,7 +166,7 @@ extern int nfsd_max_blksize;
extern unsigned int max_delegations;
int nfs4_state_init(void);
void nfsd4_free_slabs(void);
-void nfs4_state_start(void);
+int nfs4_state_start(void);
void nfs4_state_shutdown(void);
time_t nfs4_lease_time(void);
void nfs4_reset_lease(time_t leasetime);
@@ -174,7 +174,7 @@ int nfs4_reset_recoverydir(char *recdir);
#else
static inline int nfs4_state_init(void) { return 0; }
static inline void nfsd4_free_slabs(void) { }
-static inline void nfs4_state_start(void) { }
+static inline int nfs4_state_start(void) { }
static inline void nfs4_state_shutdown(void) { }
static inline time_t nfs4_lease_time(void) { return 0; }
static inline void nfs4_reset_lease(time_t leasetime) { }
--
1.6.0.4
Callbacks are always made using the machine's identity, so we can use a
single auth_generic credential shared among callbacks to all clients and
let the rpc code take care of the rest.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4callback.c | 33 ++++++++++-----------------------
fs/nfsd/nfs4state.c | 6 +-----
include/linux/nfsd/state.h | 2 +-
3 files changed, 12 insertions(+), 29 deletions(-)
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 4abb882..1285197 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -432,42 +432,29 @@ static const struct rpc_call_ops nfsd4_cb_probe_ops = {
.rpc_call_done = nfsd4_cb_probe_done,
};
-static struct rpc_cred *lookup_cb_cred(struct nfs4_cb_conn *cb)
+static struct rpc_cred *callback_cred;
+
+int set_callback_cred(void)
{
- struct auth_cred acred = {
- .machine_cred = 1
- };
- struct rpc_auth *auth = cb->cb_client->cl_auth;
-
- /*
- * Note in the gss case this doesn't actually have to wait for a
- * gss upcall (or any calls to the client); this just creates a
- * non-uptodate cred which the rpc state machine will fill in with
- * a refresh_upcall later.
- */
- return auth->au_ops->lookup_cred(auth, &acred, RPCAUTH_LOOKUP_NEW);
+ callback_cred = rpc_lookup_machine_cred();
+ if (!callback_cred)
+ return -ENOMEM;
+ return 0;
}
+
void do_probe_callback(struct nfs4_client *clp)
{
struct nfs4_cb_conn *cb = &clp->cl_cb_conn;
struct rpc_message msg = {
.rpc_proc = &nfs4_cb_procedures[NFSPROC4_CLNT_CB_NULL],
.rpc_argp = clp,
+ .rpc_cred = callback_cred
};
- struct rpc_cred *cred;
int status;
- cred = lookup_cb_cred(cb);
- if (IS_ERR(cred)) {
- status = PTR_ERR(cred);
- goto out;
- }
- cb->cb_cred = cred;
- msg.rpc_cred = cb->cb_cred;
status = rpc_call_async(cb->cb_client, &msg, RPC_TASK_SOFT,
&nfsd4_cb_probe_ops, (void *)clp);
-out:
if (status) {
warn_no_callback_path(clp, status);
put_nfs4_client(clp);
@@ -550,7 +537,7 @@ nfsd4_cb_recall(struct nfs4_delegation *dp)
struct rpc_message msg = {
.rpc_proc = &nfs4_cb_procedures[NFSPROC4_CLNT_CB_RECALL],
.rpc_argp = dp,
- .rpc_cred = clp->cl_cb_conn.cb_cred
+ .rpc_cred = callback_cred
};
int status;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 11db40c..0445192 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -696,10 +696,6 @@ shutdown_callback_client(struct nfs4_client *clp)
clp->cl_cb_conn.cb_client = NULL;
rpc_shutdown_client(clnt);
}
- if (clp->cl_cb_conn.cb_cred) {
- put_rpccred(clp->cl_cb_conn.cb_cred);
- clp->cl_cb_conn.cb_cred = NULL;
- }
}
static inline void
@@ -4020,7 +4016,7 @@ __nfs4_state_start(void)
return -ENOMEM;
queue_delayed_work(laundry_wq, &laundromat_work, grace_time);
set_max_delegations();
- return 0;
+ return set_callback_cred();
}
int
diff --git a/include/linux/nfsd/state.h b/include/linux/nfsd/state.h
index 70ef5f4..9bf3aa8 100644
--- a/include/linux/nfsd/state.h
+++ b/include/linux/nfsd/state.h
@@ -89,7 +89,6 @@ struct nfs4_cb_conn {
/* RPC client info */
atomic_t cb_set; /* successful CB_NULL call */
struct rpc_clnt * cb_client;
- struct rpc_cred * cb_cred;
};
/* Maximum number of slots per session. 160 is useful for long haul TCP */
@@ -362,6 +361,7 @@ extern int nfs4_in_grace(void);
extern __be32 nfs4_check_open_reclaim(clientid_t *clid);
extern void put_nfs4_client(struct nfs4_client *clp);
extern void nfs4_free_stateowner(struct kref *kref);
+extern int set_callback_cred(void);
extern void nfsd4_probe_callback(struct nfs4_client *clp);
extern void nfsd4_cb_recall(struct nfs4_delegation *dp);
extern void nfs4_put_delegation(struct nfs4_delegation *dp);
--
1.6.0.4
On setting up the callback to the client, we attempt to use the same
authentication flavor the client did. We find an rpc cred to use by
calling rpcauth_lookup_credcache(), which assumes that the given
authentication flavor has a credentials cache. However, this is not
required to be true--in particular, auth_null does not use one.
Instead, we should call the auth's lookup_cred() method.
Without this, a client attempting to mount using nfsv4 and auth_null
triggers a null dereference.
Cc: [email protected]
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4callback.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 63bb384..4abb882 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -437,6 +437,7 @@ static struct rpc_cred *lookup_cb_cred(struct nfs4_cb_conn *cb)
struct auth_cred acred = {
.machine_cred = 1
};
+ struct rpc_auth *auth = cb->cb_client->cl_auth;
/*
* Note in the gss case this doesn't actually have to wait for a
@@ -444,8 +445,7 @@ static struct rpc_cred *lookup_cb_cred(struct nfs4_cb_conn *cb)
* non-uptodate cred which the rpc state machine will fill in with
* a refresh_upcall later.
*/
- return rpcauth_lookup_credcache(cb->cb_client->cl_auth, &acred,
- RPCAUTH_LOOKUP_NEW);
+ return auth->au_ops->lookup_cred(auth, &acred, RPCAUTH_LOOKUP_NEW);
}
void do_probe_callback(struct nfs4_client *clp)
--
1.6.0.4