2008-04-19 20:50:36

by Myklebust, Trond

Subject: [PATCH 01/33] SUNRPC: Fix a bug in call_decode()

call_verify() can, under certain circumstances, free the RPC slot. In that
case, our cached pointer 'req = task->tk_rqstp' is invalid. Bug was
introduced in commit 220bcc2afd7011b3e0569fc178331fa983c92c1b (SUNRPC:
Don't call xprt_release in call refresh).

Signed-off-by: Trond Myklebust <[email protected]>
---

net/sunrpc/clnt.c | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index d6701f7..0c29792 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1236,10 +1236,13 @@ call_decode(struct rpc_task *task)
task->tk_status);
return;
out_retry:
- req->rq_received = req->rq_private_buf.len = 0;
task->tk_status = 0;
- if (task->tk_client->cl_discrtry)
- xprt_force_disconnect(task->tk_xprt);
+ /* Note: call_verify() may have freed the RPC slot */
+ if (task->tk_rqstp == req) {
+ req->rq_received = req->rq_private_buf.len = 0;
+ if (task->tk_client->cl_discrtry)
+ xprt_force_disconnect(task->tk_xprt);
+ }
}

/*

2008-04-21 16:23:39

by Chuck Lever

Subject: Re: [PATCH 01/33] SUNRPC: Fix a bug in call_decode()

Hi Trond-

On Apr 19, 2008, at 4:40 PM, Trond Myklebust wrote:
> call_verify() can, under certain circumstances, free the RPC slot.
> In that
> case, our cached pointer 'req = task->tk_rqstp' is invalid. Bug was
> introduced in commit 220bcc2afd7011b3e0569fc178331fa983c92c1b (SUNRPC:
> Don't call xprt_release in call refresh).

Since you still refer to this function as "call_verify" can I assume
you rejected my patches that renamed it rpc_verify_header?

A better fix might instead move the logic in call_decode's out_retry:
arm into call_verify.

A subsequent clean-up should refactor the spaghetti in call_verify
into several simpler functions to reduce the likelihood of this kind
of defect cropping up again.

> Signed-off-by: Trond Myklebust <[email protected]>
> ---
>
> net/sunrpc/clnt.c | 9 ++++++---
> 1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index d6701f7..0c29792 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -1236,10 +1236,13 @@ call_decode(struct rpc_task *task)
> task->tk_status);
> return;
> out_retry:
> - req->rq_received = req->rq_private_buf.len = 0;
> task->tk_status = 0;
> - if (task->tk_client->cl_discrtry)
> - xprt_force_disconnect(task->tk_xprt);
> + /* Note: call_verify() may have freed the RPC slot */
> + if (task->tk_rqstp == req) {
> + req->rq_received = req->rq_private_buf.len = 0;
> + if (task->tk_client->cl_discrtry)
> + xprt_force_disconnect(task->tk_xprt);
> + }
> }
>
> /*
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs"
> in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com