[PATCH] SUNRPC: Fix potential memory scribble in xprt_free_bc_request()

[PATCH] SUNRPC: Fix potential memory scribble in xprt_free_bc_request()

[Trond Myklebust]

The call to xprt_free_allocation() will call list_del() on
req->rq_bc_pa_list, which is not attached to a list.
This patch moves the list_del() out of xprt_free_allocation()
and into those callers that need it.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 net/sunrpc/backchannel_rqst.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c
index 890a29912d5a..e860d4f7ed2a 100644
--- a/net/sunrpc/backchannel_rqst.c
+++ b/net/sunrpc/backchannel_rqst.c
@@ -64,7 +64,6 @@ static void xprt_free_allocation(struct rpc_rqst *req)
 	free_page((unsigned long)xbufp->head[0].iov_base);
 	xbufp = &req->rq_snd_buf;
 	free_page((unsigned long)xbufp->head[0].iov_base);
-	list_del(&req->rq_bc_pa_list);
 	kfree(req);
 }
 
@@ -168,8 +167,10 @@ out_free:
 	/*
 	 * Memory allocation failed, free the temporary list
 	 */
-	list_for_each_entry_safe(req, tmp, &tmp_list, rq_bc_pa_list)
+	list_for_each_entry_safe(req, tmp, &tmp_list, rq_bc_pa_list) {
+		list_del(&req->rq_bc_pa_list);
 		xprt_free_allocation(req);
+	}
 
 	dprintk("RPC:       setup backchannel transport failed\n");
 	return -ENOMEM;
@@ -198,6 +199,7 @@ void xprt_destroy_backchannel(struct rpc_xprt *xprt, unsigned int max_reqs)
 	xprt_dec_alloc_count(xprt, max_reqs);
 	list_for_each_entry_safe(req, tmp, &xprt->bc_pa_list, rq_bc_pa_list) {
 		dprintk("RPC:        req=%p\n", req);
+		list_del(&req->rq_bc_pa_list);
 		xprt_free_allocation(req);
 		if (--max_reqs == 0)
 			break;
-- 
1.8.5.3

Re: [PATCH] SUNRPC: Fix potential memory scribble in xprt_free_bc_request()

[shaobingqing]

2014-02-12 3:43 GMT+08:00 Trond Myklebust <trond.myklebust@primarydata.com>:
> The call to xprt_free_allocation() will call list_del() on
> req->rq_bc_pa_list, which is not attached to a list.

Since the type of req->rq_bc_pa_list is struct list_head,  I think it
is right on the tmp_list or
the xprt->bc_pa_list.  Do I misunderstand  sth?

> This patch moves the list_del() out of xprt_free_allocation()
> and into those callers that need it.
>
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
>  net/sunrpc/backchannel_rqst.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c
> index 890a29912d5a..e860d4f7ed2a 100644
> --- a/net/sunrpc/backchannel_rqst.c
> +++ b/net/sunrpc/backchannel_rqst.c
> @@ -64,7 +64,6 @@ static void xprt_free_allocation(struct rpc_rqst *req)
>         free_page((unsigned long)xbufp->head[0].iov_base);
>         xbufp = &req->rq_snd_buf;
>         free_page((unsigned long)xbufp->head[0].iov_base);
> -       list_del(&req->rq_bc_pa_list);
>         kfree(req);
>  }
>
> @@ -168,8 +167,10 @@ out_free:
>         /*
>          * Memory allocation failed, free the temporary list
>          */
> -       list_for_each_entry_safe(req, tmp, &tmp_list, rq_bc_pa_list)
> +       list_for_each_entry_safe(req, tmp, &tmp_list, rq_bc_pa_list) {
> +               list_del(&req->rq_bc_pa_list);
>                 xprt_free_allocation(req);
> +       }
>
>         dprintk("RPC:       setup backchannel transport failed\n");
>         return -ENOMEM;
> @@ -198,6 +199,7 @@ void xprt_destroy_backchannel(struct rpc_xprt *xprt, unsigned int max_reqs)
>         xprt_dec_alloc_count(xprt, max_reqs);
>         list_for_each_entry_safe(req, tmp, &xprt->bc_pa_list, rq_bc_pa_list) {
>                 dprintk("RPC:        req=%p\n", req);
> +               list_del(&req->rq_bc_pa_list);
>                 xprt_free_allocation(req);
>                 if (--max_reqs == 0)
>                         break;
> --
> 1.8.5.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] SUNRPC: Fix potential memory scribble in xprt_free_bc_request()

[Trond Myklebust]

On Feb 11, 2014, at 21:12, shaobingqing <shaobingqing@bwstor.com.cn> wrote:

> 2014-02-12 3:43 GMT+08:00 Trond Myklebust <trond.myklebust@primarydata.com>:
>> The call to xprt_free_allocation() will call list_del() on
>> req->rq_bc_pa_list, which is not attached to a list.
> 
> Since the type of req->rq_bc_pa_list is struct list_head,  I think it
> is right on the tmp_list or
> the xprt->bc_pa_list.  Do I misunderstand  sth?

Not when xprt_free_bc_request() calls xprt_free_allocation().

>> This patch moves the list_del() out of xprt_free_allocation()
>> and into those callers that need it.

…and the point is that we do not add the list_del() to xprt_free_bc_request().

_________________________________
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com