Re: [PATCH] SVC: Guard call to xpo_release_rqst in svc_send

[Tom Tucker <tom@opengridcomputing.com>]

Bruce:

Summary response: Mia culpa....please revert this patch. Although it made
the test failure go away, this patch just moves the problem, it doesn't fix
anything.  

More below inline....

On 2/29/08 2:40 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:


> On Wed, Feb 27, 2008 at 01:58:59PM -0600, Tom Tucker wrote:
>>     
>> The svc_send path is calling xpo_release_rqst without checking
>> the XPT_DEAD bit. It is illegal to call transport methods on a dead
>> transport. In practice, if the transport gets an error and shuts down
>> while there are still RPC in svc_process the resulting svc_send could
>> crash calling into a transport that is being shut down.
> 
> As long as we have a pointer to an xprt (say in rqstp->rq_xprt), we
> should have a reference on the corresponding xprt class, shouldn't we?
> Anything else seems like a problem.
> 

Generally yes. In particular, at entry to svc_recv, we should have a minimum
of two references. svc_xprt_enqueue gets you one, and there is a one ref
bias that comes from svc_xprt_init. The bias gets removed on close.

>>     
>> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
>> ---
>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
>> index ea377e0..467c1c0 100644
>> --- a/net/sunrpc/svc_xprt.c
>> +++ b/net/sunrpc/svc_xprt.c
>> @@ -729,9 +729,6 @@ int svc_send(struct svc_rqst *rqstp)
>> if (!xprt)
>> return -EFAULT;
>>  
>> - /* release the receive skb before sending the reply */
>> - rqstp->rq_xprt->xpt_ops->xpo_release_rqst(rqstp);
>> -
>> /* calculate over-all length */
>> xb = &rqstp->rq_res;
>> xb->len = xb->head[0].iov_len +
>> @@ -742,8 +739,11 @@ int svc_send(struct svc_rqst *rqstp)
>> mutex_lock(&xprt->xpt_mutex);
>> if (test_bit(XPT_DEAD, &xprt->xpt_flags))
>> len = -ENOTCONN;
>> - else
>> + else {
>> +  /* release the receive skb before sending the reply */
>> +  rqstp->rq_xprt->xpt_ops->xpo_release_rqst(rqstp);
>> len = xprt->xpt_ops->xpo_sendto(rqstp);
>> + }
> 
> So in the case where XPT_DEAD is set, doesn't the receive skb also need
> to be freed, or does somebody else do that?
> 

Only reboot does that :-\. I need to think this through more carefully. On
RDMA transports this function is used to return send credits to the user,
and that involves talking to the transport. Sorry for this hasty
patch...it's not right.

>> mutex_unlock(&xprt->xpt_mutex);
>> svc_xprt_release(rqstp);
> 
> Also something else is odd here: svc_xprt_release(rqstp) already calls
> rqstp->rq_xprt->xpt_ops->xpo_release_rqst(rqstp) itself, so we seem to
> have two calls to that method.
> 

Hmm. Busted. This patch is broken in three dimensions. Is that a record?

Tom
 
> --b.
> 
>>  
>> 
>> -
>> 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
> --
> 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