[PATCH 0/3] rpcauth_create() returns -EEXIST

[PATCH 0/3] rpcauth_create() returns -EEXIST

[Chuck Lever]

A few weeks ago I reported a problem with calling rpcauth_create()
in a loop to serially replace an RPC transport's with different
flavors.  When the loop gets up to the GSS flavors, the second
GSS flavor in the list can't be created.  rpcauth_create() fails
with -EEXIST.  See:

  http://marc.info/?t=134073471800002&r=1&w=2

rpcauth_create() invokes the rpc_auth ->create operation first then
releases the old rpc_auth.  But the old rpc_auth is holding onto
upcall pipes, which prevents the new GSS rpc_auth from creating its
own upcall pipes. 

This breaks our client's SECINFO implementation, and will also break
UCS server trunking discovery when it appears.

Trond proposed a solution in the same e-mail thread, but it was
archived uuencoded.  Here is his suggestion in human-readable form:

> The solution here would be to create a per-rpc_client, per-pipename
> shared 'rpcsec_gss_pipe' object that holds the upcall pipe data.

Here's my crack at this idea.  This post is a request for comments;
more testing is needed.

---

Chuck Lever (3):
      SUNRPC: Share upcall pipes among an rpc_clnt's rpc_auth objects
      SUNRPC: Insert a shim under gss_create()
      SUNRPC: Prepare gss_pipes_dentries_create() for sharing pipe data


 include/linux/sunrpc/clnt.h    |    1 
 net/sunrpc/auth_gss/auth_gss.c |  232 ++++++++++++++++++++++++++++++++--------
 net/sunrpc/clnt.c              |    1 
 3 files changed, 189 insertions(+), 45 deletions(-)

-- 
Chuck Lever

[PATCH 1/3] SUNRPC: Prepare gss_pipes_dentries_create() for sharing pipe data

[Chuck Lever]

We are about to make pipe data shareable. gss_pipes_dentries_create()
and gss_pipes_dentries_destroy() have to be more careful when dealing
with their dentry pointers.  Namely:

  1. When asked to create a dentry, don't write over possibly valid
     pointers, but instead preserve an existing dentry

  2. When unlinking a dentry, make sure the ->dentry pointer is NULL

  3. If creating a dentry fails, don't leak the ERR_PTR value, make
     sure the ->dentry pointer is NULL

2. and 3. are needed so that 1. can recognize when a new dentry
should be created.

This is clean up, mostly.  But it's necessary because unmounting a
pipe FS can cause pipe dentries, but not the pipe data, to be
released (via ->pipes_destroy).

The functions that manage pipe data for gss_create() have to be
prepared for finding an existing shared rpc_pipe_data object that
has a dentry, or doesn't, and then do the correct thing in either
case.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Cc: Stanislav Kinsbursky <skinsbursky@parallels.com>
---

 net/sunrpc/auth_gss/auth_gss.c |   67 +++++++++++++++++++++++++++++-----------
 1 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index f5147ef..dfe7825 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -757,42 +757,73 @@ gss_pipe_destroy_msg(struct rpc_pipe_msg *msg)
 	}
 }
 
+static void gss_pipe_dentry_destroy(struct rpc_pipe *pipe)
+{
+	if (pipe->dentry != NULL) {
+		rpc_unlink(pipe->dentry);
+		pipe->dentry = NULL;
+	}
+}
+
+/*
+ * Release existing pipe dentries, but leave the rpc_pipe data
+ * ready to receive fresh dentries if subsequently needed.
+ */
 static void gss_pipes_dentries_destroy(struct rpc_auth *auth)
 {
 	struct gss_auth *gss_auth;
 
 	gss_auth = container_of(auth, struct gss_auth, rpc_auth);
-	if (gss_auth->pipe[0]->dentry)
-		rpc_unlink(gss_auth->pipe[0]->dentry);
-	if (gss_auth->pipe[1]->dentry)
-		rpc_unlink(gss_auth->pipe[1]->dentry);
+
+	dprintk("RPC:       %s auth=%p, clnt=%p\n",
+		__func__, auth, gss_auth->client);
+
+	gss_pipe_dentry_destroy(gss_auth->pipe[0]);
+	gss_pipe_dentry_destroy(gss_auth->pipe[1]);
 }
 
+/*
+ * On success, zero is returned and both elements of the rpc_pipe
+ * array are populated with valid dentries.  Otherwise an error value
+ * is returned, the dentries are freed, and both ->dentry pointers are
+ * set to NULL.
+ */
 static int gss_pipes_dentries_create(struct rpc_auth *auth)
 {
-	int err;
+	int err = 0;
 	struct gss_auth *gss_auth;
 	struct rpc_clnt *clnt;
 
 	gss_auth = container_of(auth, struct gss_auth, rpc_auth);
 	clnt = gss_auth->client;
 
-	gss_auth->pipe[1]->dentry = rpc_mkpipe_dentry(clnt->cl_dentry,
-						      "gssd",
-						      clnt, gss_auth->pipe[1]);
-	if (IS_ERR(gss_auth->pipe[1]->dentry))
-		return PTR_ERR(gss_auth->pipe[1]->dentry);
-	gss_auth->pipe[0]->dentry = rpc_mkpipe_dentry(clnt->cl_dentry,
-						      gss_auth->mech->gm_name,
-						      clnt, gss_auth->pipe[0]);
-	if (IS_ERR(gss_auth->pipe[0]->dentry)) {
-		err = PTR_ERR(gss_auth->pipe[0]->dentry);
-		goto err_unlink_pipe_1;
+	if (gss_auth->pipe[1]->dentry == NULL) {
+		gss_auth->pipe[1]->dentry =
+			rpc_mkpipe_dentry(clnt->cl_dentry, "gssd", clnt,
+							gss_auth->pipe[1]);
+		if (IS_ERR(gss_auth->pipe[1]->dentry)) {
+			err = PTR_ERR(gss_auth->pipe[1]->dentry);
+			gss_auth->pipe[1]->dentry = NULL;
+			goto out_err;
+		}
 	}
+
+	if (gss_auth->pipe[0]->dentry == NULL) {
+		gss_auth->pipe[0]->dentry =
+			rpc_mkpipe_dentry(clnt->cl_dentry,
+						gss_auth->mech->gm_name,
+						clnt, gss_auth->pipe[0]);
+		if (IS_ERR(gss_auth->pipe[0]->dentry)) {
+			err = PTR_ERR(gss_auth->pipe[0]->dentry);
+			gss_auth->pipe[0]->dentry = NULL;
+			goto out_err;
+		}
+	}
+
 	return 0;
 
-err_unlink_pipe_1:
-	rpc_unlink(gss_auth->pipe[1]->dentry);
+out_err:
+	gss_pipes_dentries_destroy(auth);
 	return err;
 }
 

[PATCH 2/3] SUNRPC: Insert a shim under gss_create()

[Chuck Lever]

We want to share rpc_pipe data between GSS-flavored rpc_auth objects.
To do this, create a shim in gss_create() and gss_free() where the
pipe data will be looked up, created, and destroyed.  The unused
arguments anticipate a per-RPC client cache of pipes.

This is a refactoring change which shouldn't have a functional
effect.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Cc: Stanislav Kinsbursky <skinsbursky@parallels.com>
---

 net/sunrpc/auth_gss/auth_gss.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index dfe7825..2264776 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -857,6 +857,18 @@ static int gss_pipes_dentries_create_net(struct rpc_clnt *clnt,
 	return err;
 }
 
+static struct rpc_pipe *gss_mkpipe_data(struct rpc_clnt *clnt,
+					const struct rpc_pipe_ops *ops,
+					char *name)
+{
+	return rpc_mkpipe_data(ops, RPC_PIPE_WAIT_FOR_OPEN);
+}
+
+static void gss_destroy_pipe_data(struct rpc_clnt *clnt, struct rpc_pipe *pipe)
+{
+	rpc_destroy_pipe_data(pipe);
+}
+
 /*
  * NOTE: we have the opportunity to use different
  * parameters based on the input flavor (which must be a pseudoflavor)
@@ -899,15 +911,13 @@ gss_create(struct rpc_clnt *clnt, rpc_authflavor_t flavor)
 	 * that we supported only the old pipe.  So we instead create
 	 * the new pipe first.
 	 */
-	gss_auth->pipe[1] = rpc_mkpipe_data(&gss_upcall_ops_v1,
-					    RPC_PIPE_WAIT_FOR_OPEN);
+	gss_auth->pipe[1] = gss_mkpipe_data(clnt, &gss_upcall_ops_v1, "gssd");
 	if (IS_ERR(gss_auth->pipe[1])) {
 		err = PTR_ERR(gss_auth->pipe[1]);
 		goto err_put_mech;
 	}
-
-	gss_auth->pipe[0] = rpc_mkpipe_data(&gss_upcall_ops_v0,
-					    RPC_PIPE_WAIT_FOR_OPEN);
+	gss_auth->pipe[0] = gss_mkpipe_data(clnt, &gss_upcall_ops_v0,
+						gss_auth->mech->gm_name);
 	if (IS_ERR(gss_auth->pipe[0])) {
 		err = PTR_ERR(gss_auth->pipe[0]);
 		goto err_destroy_pipe_1;
@@ -923,9 +933,9 @@ gss_create(struct rpc_clnt *clnt, rpc_authflavor_t flavor)
 err_unlink_pipes:
 	gss_pipes_dentries_destroy_net(clnt, auth);
 err_destroy_pipe_0:
-	rpc_destroy_pipe_data(gss_auth->pipe[0]);
+	gss_destroy_pipe_data(clnt, gss_auth->pipe[0]);
 err_destroy_pipe_1:
-	rpc_destroy_pipe_data(gss_auth->pipe[1]);
+	gss_destroy_pipe_data(clnt, gss_auth->pipe[1]);
 err_put_mech:
 	gss_mech_put(gss_auth->mech);
 err_free:
@@ -939,8 +949,8 @@ static void
 gss_free(struct gss_auth *gss_auth)
 {
 	gss_pipes_dentries_destroy_net(gss_auth->client, &gss_auth->rpc_auth);
-	rpc_destroy_pipe_data(gss_auth->pipe[0]);
-	rpc_destroy_pipe_data(gss_auth->pipe[1]);
+	gss_destroy_pipe_data(gss_auth->client, gss_auth->pipe[0]);
+	gss_destroy_pipe_data(gss_auth->client, gss_auth->pipe[1]);
 	gss_mech_put(gss_auth->mech);
 
 	kfree(gss_auth);

[PATCH 3/3] SUNRPC: Share upcall pipes among an rpc_clnt's rpc_auth objects

[Chuck Lever]

An ULP is supposed to be able to replace a GSS rpc_auth object with
another GSS rpc_auth object using rpcauth_create().  However,
rpcauth_create() in 3.5 reliably fails with -EEXIST in this case.
This is because when gss_create() attempts to create the upcall pipes,
sometimes they are already there.  For example if a pipe FS mount
event occurs, or a previous GSS flavor was in use for this rpc_clnt.

The ->pipes_destroy method works only on whatever rpc_auth is
associated with the rpc_clnt when it is destroyed.  But there is no
guarantee that this is the same rpc_auth that was created when
the rpc_clnt was created.  Thus cached pipe data must be reference
counted and destroyed when no more gss_auth's are using it.  And,
->pipes_destroy must ensure that both the dentries and the pipe
data are gone so the pipe data does not leak.

The dentries appear to be created and deleted independently of the
rpc_pipe data.  We must allow this to continue in order that mounting
and unmounting the pipe FS can work.  However, pipe dentries must
remain in place during gss_free(), since they could be shared.

In the meantime, we need to provide a way for gss_create() to find
an existing pipe of a particular name so it may be shared.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Cc: Stanislav Kinsbursky <skinsbursky@parallels.com>
---

 include/linux/sunrpc/clnt.h    |    1 
 net/sunrpc/auth_gss/auth_gss.c |  141 ++++++++++++++++++++++++++++++++++------
 net/sunrpc/clnt.c              |    1 
 3 files changed, 123 insertions(+), 20 deletions(-)

diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index 523547e..31857e0 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -62,6 +62,7 @@ struct rpc_clnt {
 	struct rpc_timeout	cl_timeout_default;
 	const struct rpc_program *cl_program;
 	char			*cl_principal;	/* target to authenticate to */
+	struct list_head	cl_pipes;	/* clnt's upcall pipes */
 };
 
 /*
diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 2264776..8ff5103 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -84,6 +84,14 @@ struct gss_auth {
 	struct rpc_pipe *pipe[2];
 };
 
+struct gss_cached_pipe {
+	struct list_head	gp_list;
+	unsigned int		gp_ref;
+	char			*gp_name;
+	struct rpc_pipe		*gp_pipe;
+};
+DEFINE_MUTEX(gss_pipe_mutex);
+
 /* pipe_version >= 0 if and only if someone has a pipe open. */
 static int pipe_version = -1;
 static atomic_t pipe_users = ATOMIC_INIT(0);
@@ -765,6 +773,20 @@ static void gss_pipe_dentry_destroy(struct rpc_pipe *pipe)
 	}
 }
 
+static void gss_pipe_dentry_destroy_net(struct rpc_clnt *clnt,
+					struct rpc_pipe *pipe)
+{
+	struct net *net = rpc_net_ns(clnt);
+	struct super_block *sb;
+
+	sb = rpc_get_sb_net(net);
+	if (sb) {
+		if (clnt->cl_dentry)
+			gss_pipe_dentry_destroy(pipe);
+		rpc_put_sb_net(net);
+	}
+}
+
 /*
  * Release existing pipe dentries, but leave the rpc_pipe data
  * ready to receive fresh dentries if subsequently needed.
@@ -827,20 +849,6 @@ out_err:
 	return err;
 }
 
-static void gss_pipes_dentries_destroy_net(struct rpc_clnt *clnt,
-					   struct rpc_auth *auth)
-{
-	struct net *net = rpc_net_ns(clnt);
-	struct super_block *sb;
-
-	sb = rpc_get_sb_net(net);
-	if (sb) {
-		if (clnt->cl_dentry)
-			gss_pipes_dentries_destroy(auth);
-		rpc_put_sb_net(net);
-	}
-}
-
 static int gss_pipes_dentries_create_net(struct rpc_clnt *clnt,
 					 struct rpc_auth *auth)
 {
@@ -857,16 +865,111 @@ static int gss_pipes_dentries_create_net(struct rpc_clnt *clnt,
 	return err;
 }
 
+static struct gss_cached_pipe *gss_alloc_cached_pipe(char *name)
+{
+	struct gss_cached_pipe *cp;
+
+	cp = kmalloc(sizeof(*cp), GFP_KERNEL);
+	if (cp == NULL)
+		return NULL;
+
+	cp->gp_name = kstrdup(name, GFP_KERNEL);
+	if (cp->gp_name == NULL) {
+		kfree(cp);
+		return NULL;
+	}
+
+	INIT_LIST_HEAD(&cp->gp_list);
+	cp->gp_ref = 1;
+	return cp;
+}
+
+static void gss_free_cached_pipe(struct gss_cached_pipe *cp)
+{
+	kfree(cp->gp_name);
+	kfree(cp);
+}
+
+/*
+ * Returns a fresh or cached pipe data object.  A cached pipe
+ * data object may already have a dentry attached to it.  If an
+ * object cannot be found or created, an ERR_PTR is returned.
+ */
 static struct rpc_pipe *gss_mkpipe_data(struct rpc_clnt *clnt,
 					const struct rpc_pipe_ops *ops,
 					char *name)
 {
-	return rpc_mkpipe_data(ops, RPC_PIPE_WAIT_FOR_OPEN);
+	struct gss_cached_pipe *cp;
+	struct rpc_pipe *pipe;
+
+	mutex_lock(&gss_pipe_mutex);
+
+	list_for_each_entry(cp, &clnt->cl_pipes, gp_list) {
+		if (strcmp(cp->gp_name, name) == 0) {
+			dprintk("RPC:       %s found '%s' for clnt %p: %p\n",
+				__func__, name, clnt, cp->gp_pipe);
+			cp->gp_ref++;
+			pipe = cp->gp_pipe;
+			goto out;
+		}
+	}
+
+	pipe = ERR_PTR(-ENOMEM);
+	cp = gss_alloc_cached_pipe(name);
+	if (cp == NULL)
+		goto out;
+
+	pipe = rpc_mkpipe_data(ops, RPC_PIPE_WAIT_FOR_OPEN);
+	if (IS_ERR(pipe)) {
+		gss_free_cached_pipe(cp);
+		goto out;
+	}
+
+	dprintk("RPC:       %s created '%s' for clnt %p: %p\n",
+		__func__, name, clnt, pipe);
+	cp->gp_pipe = pipe;
+	list_add(&cp->gp_list, &clnt->cl_pipes);
+
+out:
+	mutex_unlock(&gss_pipe_mutex);
+	return pipe;
 }
 
+/*
+ * Decrements a cached pipes reference count, and releases it if
+ * the count goes to zero.  Associated dentry is freed if present.
+ */
 static void gss_destroy_pipe_data(struct rpc_clnt *clnt, struct rpc_pipe *pipe)
 {
-	rpc_destroy_pipe_data(pipe);
+	struct gss_cached_pipe *cp;
+
+	mutex_lock(&gss_pipe_mutex);
+
+	list_for_each_entry(cp, &clnt->cl_pipes, gp_list) {
+		if (cp->gp_pipe == pipe)
+			goto found;
+	}
+
+	dprintk("RPC:       %s missing cache for pipe %p for clnt %p\n",
+		__func__, pipe, clnt);
+	WARN_ON(true);
+
+	mutex_unlock(&gss_pipe_mutex);
+	return;
+
+found:
+	if (--cp->gp_ref == 0) {
+		dprintk("RPC:       %s destroying '%s' (%p) for clnt %p\n",
+			__func__, cp->gp_name, pipe, clnt);
+		gss_pipe_dentry_destroy_net(clnt, pipe);
+		rpc_destroy_pipe_data(pipe);
+		list_del(&cp->gp_list);
+		gss_free_cached_pipe(cp);
+	} else
+		dprintk("RPC:       %s leaving '%s' (%p) for clnt %p\n",
+			__func__, cp->gp_name, pipe, clnt);
+
+	mutex_unlock(&gss_pipe_mutex);
 }
 
 /*
@@ -925,13 +1028,12 @@ gss_create(struct rpc_clnt *clnt, rpc_authflavor_t flavor)
 	err = gss_pipes_dentries_create_net(clnt, auth);
 	if (err)
 		goto err_destroy_pipe_0;
+
 	err = rpcauth_init_credcache(auth);
 	if (err)
-		goto err_unlink_pipes;
+		goto err_destroy_pipe_0;
 
 	return auth;
-err_unlink_pipes:
-	gss_pipes_dentries_destroy_net(clnt, auth);
 err_destroy_pipe_0:
 	gss_destroy_pipe_data(clnt, gss_auth->pipe[0]);
 err_destroy_pipe_1:
@@ -948,7 +1050,6 @@ out_dec:
 static void
 gss_free(struct gss_auth *gss_auth)
 {
-	gss_pipes_dentries_destroy_net(gss_auth->client, &gss_auth->rpc_auth);
 	gss_destroy_pipe_data(gss_auth->client, gss_auth->pipe[0]);
 	gss_destroy_pipe_data(gss_auth->client, gss_auth->pipe[1]);
 	gss_mech_put(gss_auth->mech);
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index f56f045..7bf6ed1 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -349,6 +349,7 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args, stru
 		if (!clnt->cl_principal)
 			goto out_no_principal;
 	}
+	INIT_LIST_HEAD(&clnt->cl_pipes);
 
 	atomic_set(&clnt->cl_count, 1);
 

Re: [PATCH 0/3] rpcauth_create() returns -EEXIST

[Stanislav Kinsbursky]

Ack on all set.

10.08.2012 01:31, Chuck Lever пишет:
> A few weeks ago I reported a problem with calling rpcauth_create()
> in a loop to serially replace an RPC transport's with different
> flavors.  When the loop gets up to the GSS flavors, the second
> GSS flavor in the list can't be created.  rpcauth_create() fails
> with -EEXIST.  See:
>
>    http://marc.info/?t=134073471800002&r=1&w=2
>
> rpcauth_create() invokes the rpc_auth ->create operation first then
> releases the old rpc_auth.  But the old rpc_auth is holding onto
> upcall pipes, which prevents the new GSS rpc_auth from creating its
> own upcall pipes.
>
> This breaks our client's SECINFO implementation, and will also break
> UCS server trunking discovery when it appears.
>
> Trond proposed a solution in the same e-mail thread, but it was
> archived uuencoded.  Here is his suggestion in human-readable form:
>
>> The solution here would be to create a per-rpc_client, per-pipename
>> shared 'rpcsec_gss_pipe' object that holds the upcall pipe data.
>
> Here's my crack at this idea.  This post is a request for comments;
> more testing is needed.
>
> ---
>
> Chuck Lever (3):
>        SUNRPC: Share upcall pipes among an rpc_clnt's rpc_auth objects
>        SUNRPC: Insert a shim under gss_create()
>        SUNRPC: Prepare gss_pipes_dentries_create() for sharing pipe data
>
>
>   include/linux/sunrpc/clnt.h    |    1
>   net/sunrpc/auth_gss/auth_gss.c |  232 ++++++++++++++++++++++++++++++++--------
>   net/sunrpc/clnt.c              |    1
>   3 files changed, 189 insertions(+), 45 deletions(-)
>


-- 
Best regards,
Stanislav Kinsbursky