Attachment 301324 Details for Bug 432867 – patch -- add new reference counted response container for portmap calls

[patch] patch -- add new reference counted response container for portmap calls

0016-BZ-432867-SUNRPC-prevent-memory-corruption-from-su.patch (text/plain), 5.55 KB, created by Jeff Layton on 2008-04-04 18:56:57 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeff Layton

Created: 2008-04-04 18:56:57 UTC

Size: 5.55 KB

patch

obsolete

>From 1d7461dd3fd4102bbe805f4c3c454e62ce18a91c Mon Sep 17 00:00:00 2001
>From: Jeff Layton <jlayton@redhat.com>
>Date: Fri, 4 Apr 2008 13:11:18 -0400
>Subject: [PATCH] BZ#432867: SUNRPC: prevent memory corruption from successful portmap call with dead parent task
>
>There is possible memory corruption when a RPC client is destroyed while
>it still has a portmap child task in flight. The portmap routines are
>set up to write the result directly to the cl_port field of the rpc_clnt.
>In the right situation, it's possible for this struct to have been freed.
>If the outstanding portmap call subsequently succeeds, it will write the
>result to freed memory.
>
>The following patch fixes this by changing the portmap routines to
>allocate a new "rpc_pmap_result" struct and to write the result there.
>The parent task then copies the result out of that struct and into its
>own when needed. The struct is managed via a kref, which prevents us
>from freeing the result container before all users have finished with
>it.
>---
> include/linux/sunrpc/clnt.h |    5 +++++
> net/sunrpc/clnt.c           |    4 ++++
> net/sunrpc/pmap_clnt.c      |   43 ++++++++++++++++++++++++++++++++++++++++---
> 3 files changed, 49 insertions(+), 3 deletions(-)
>
>diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
>index 36fc573..fab1ec4 100644
>--- a/include/linux/sunrpc/clnt.h
>+++ b/include/linux/sunrpc/clnt.h
>@@ -31,6 +31,7 @@ struct rpc_portmap {
> };
> 
> struct rpc_inode;
>+struct rpc_pmap_result;
> 
> /*
>  * The high-level client handle
>@@ -67,6 +68,9 @@ struct rpc_clnt {
> 	struct rpc_portmap	cl_pmap_default;
> 	char			cl_inline_name[32];
> 	struct rpc_program *	cl_program;
>+#ifndef __GENKSYMS__
>+	struct rpc_pmap_result	*cl_pmap_result;
>+#endif /* !__GENKSYMS__ */
> };
> #define cl_timeout		cl_xprt->timeout
> #define cl_prog			cl_pmap->pm_prog
>@@ -124,6 +128,7 @@ struct rpc_clnt *rpc_clone_client(struct rpc_clnt *);
> int		rpc_shutdown_client(struct rpc_clnt *);
> int		rpc_destroy_client(struct rpc_clnt *);
> void		rpc_release_client(struct rpc_clnt *);
>+void		pmap_result_put(struct rpc_pmap_result *pr);
> void		rpc_getport(struct rpc_task *, struct rpc_clnt *);
> int		rpc_register(u32, u32, int, unsigned short, int *);
> 
>diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>index da81c76..33a05bd 100644
>--- a/net/sunrpc/clnt.c
>+++ b/net/sunrpc/clnt.c
>@@ -343,6 +343,10 @@ rpc_destroy_client(struct rpc_clnt *clnt)
> 
> 	dprintk("RPC: destroying %s client for %s\n",
> 			clnt->cl_protname, clnt->cl_server);
>+	if (clnt->cl_pmap_result) {
>+		pmap_result_put(clnt->cl_pmap_result);
>+		clnt->cl_pmap_result = NULL;
>+	}
> 	if (clnt->cl_auth) {
> 		rpcauth_destroy(clnt->cl_auth);
> 		clnt->cl_auth = NULL;
>diff --git a/net/sunrpc/pmap_clnt.c b/net/sunrpc/pmap_clnt.c
>index fab1f97..bcf7a93 100644
>--- a/net/sunrpc/pmap_clnt.c
>+++ b/net/sunrpc/pmap_clnt.c
>@@ -15,6 +15,7 @@
> #include <linux/sunrpc/clnt.h>
> #include <linux/sunrpc/xprt.h>
> #include <linux/sunrpc/sched.h>
>+#include <linux/kref.h>
> 
> #ifdef RPC_DEBUG
> # define RPCDBG_FACILITY	RPCDBG_PMAP
>@@ -30,6 +31,25 @@ static void			pmap_getport_done(struct rpc_task *);
> static struct rpc_program	pmap_program;
> static DEFINE_SPINLOCK(pmap_lock);
> 
>+struct rpc_pmap_result {
>+	__u16			pr_port;
>+	struct kref		pr_refcount;
>+};
>+
>+static void
>+pmap_result_release(struct kref *ref)
>+{
>+	struct rpc_pmap_result *pr = container_of(ref, struct rpc_pmap_result,
>+						  pr_refcount);
>+	kfree(pr);
>+}
>+
>+void
>+pmap_result_put(struct rpc_pmap_result *pr)
>+{
>+	kref_put(&pr->pr_refcount, pmap_result_release);
>+}
>+
> /*
>  * Obtain the port for a given RPC service on a given host. This one can
>  * be called for an ongoing RPC request.
>@@ -42,7 +62,6 @@ rpc_getport(struct rpc_task *task, struct rpc_clnt *clnt)
> 	struct rpc_message msg = {
> 		.rpc_proc	= &pmap_procedures[PMAP_GETPORT],
> 		.rpc_argp	= map,
>-		.rpc_resp	= &clnt->cl_port,
> 		.rpc_cred	= NULL
> 	};
> 	struct rpc_clnt	*pmap_clnt;
>@@ -64,6 +83,14 @@ rpc_getport(struct rpc_task *task, struct rpc_clnt *clnt)
> 	map->pm_binding = 1;
> 	spin_unlock(&pmap_lock);
> 
>+	clnt->cl_pmap_result = kzalloc(sizeof(*clnt->cl_pmap_result),
>+					GFP_KERNEL);
>+	if (clnt->cl_pmap_result == NULL)
>+		goto bailout;
>+
>+	kref_init(&clnt->cl_pmap_result->pr_refcount);
>+	msg.rpc_resp = clnt->cl_pmap_result;
>+	
> 	pmap_clnt = pmap_create(clnt->cl_server, sap, map->pm_prot, 0);
> 	if (IS_ERR(pmap_clnt)) {
> 		task->tk_status = PTR_ERR(pmap_clnt);
>@@ -81,11 +108,16 @@ rpc_getport(struct rpc_task *task, struct rpc_clnt *clnt)
> 	rpc_call_setup(child, &msg, 0);
> 
> 	/* ... and run the child task */
>+	kref_get(&clnt->cl_pmap_result->pr_refcount);
> 	task->tk_xprt->stat.bind_count++;
> 	rpc_run_child(task, child, pmap_getport_done);
> 	return;
> 
> bailout:
>+	if (clnt->cl_pmap_result) {
>+		pmap_result_put(clnt->cl_pmap_result);
>+		clnt->cl_pmap_result = NULL;
>+	}
> 	spin_lock(&pmap_lock);
> 	map->pm_binding = 0;
> 	rpc_wake_up(&map->pm_bindwait);
>@@ -142,6 +174,10 @@ pmap_getport_done(struct rpc_task *task)
> 	dprintk("RPC: %4d pmap_getport_done(status %d, port %d)\n",
> 			task->tk_pid, task->tk_status, clnt->cl_port);
> 
>+	clnt->cl_port = clnt->cl_pmap_result->pr_port;
>+	pmap_result_put(clnt->cl_pmap_result);
>+	clnt->cl_pmap_result = NULL;
>+
> 	xprt->ops->set_port(xprt, 0);
> 	if (task->tk_status < 0) {
> 		clnt->cl_port = 0;
>@@ -248,9 +284,10 @@ xdr_encode_mapping(struct rpc_rqst *req, u32 *p, struct rpc_portmap *map)
> }
> 
> static int
>-xdr_decode_port(struct rpc_rqst *req, u32 *p, unsigned short *portp)
>+xdr_decode_port(struct rpc_rqst *req, u32 *p, struct rpc_pmap_result *pr)
> {
>-	*portp = (unsigned short) ntohl(*p++);
>+	pr->pr_port = (unsigned short) ntohl(*p++);
>+	pmap_result_put(pr);
> 	return 0;
> }
> 
>-- 
>1.5.3.6
>

Actions: View | Diff

Attachments on bug 432867: 301324 | 306361 | 306393 | 306475 | 306479 | 312372