From: Tom Tucker Subject: Re: [PATCH] SVC: Guard call to xpo_release_rqst in svc_send Date: Fri, 29 Feb 2008 21:20:34 -0600 Message-ID: References: <20080229204030.GB6276@fieldses.org> Mime-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Cc: linux-nfs , jaschut , Steve Wise To: "J. Bruce Fields" Return-path: Received: from mail.es335.com ([67.65.19.105]:6094 "EHLO mail.es335.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752731AbYCADw1 (ORCPT ); Fri, 29 Feb 2008 22:52:27 -0500 In-Reply-To: <20080229204030.GB6276@fieldses.org> Sender: linux-nfs-owner@vger.kernel.org List-ID: 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" 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 >> --- >> 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