Return-Path: linux-nfs-owner@vger.kernel.org Received: from mail-qa0-f43.google.com ([209.85.216.43]:46294 "EHLO mail-qa0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753809AbaKMCle (ORCPT ); Wed, 12 Nov 2014 21:41:34 -0500 Received: by mail-qa0-f43.google.com with SMTP id j7so9443578qaq.2 for ; Wed, 12 Nov 2014 18:41:33 -0800 (PST) From: Jeff Layton Date: Wed, 12 Nov 2014 21:41:28 -0500 To: Trond Myklebust Cc: Bruce Fields , linux-nfs@vger.kernel.org Subject: Re: [PATCH] SUNRPC: Fix locking around callback channel reply receive Message-ID: <20141112214128.4fc43a49@synchrony.poochiereds.net> In-Reply-To: <1415833444-48129-1-git-send-email-trond.myklebust@primarydata.com> References: <1415833444-48129-1-git-send-email-trond.myklebust@primarydata.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-nfs-owner@vger.kernel.org List-ID: On Wed, 12 Nov 2014 18:04:04 -0500 Trond Myklebust wrote: > Both xprt_lookup_rqst() and xprt_complete_rqst() require that you > take the transport lock in order to avoid races with xprt_transmit(). > > Signed-off-by: Trond Myklebust > Cc: Bruce Fields > --- > net/sunrpc/svcsock.c | 27 ++++++++++++++++----------- > 1 file changed, 16 insertions(+), 11 deletions(-) > > diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c > index 3f959c681885..f9c052d508f0 100644 > --- a/net/sunrpc/svcsock.c > +++ b/net/sunrpc/svcsock.c > @@ -1019,17 +1019,12 @@ static int receive_cb_reply(struct svc_sock *svsk, struct svc_rqst *rqstp) > xid = *p++; > calldir = *p; > > - if (bc_xprt) > - req = xprt_lookup_rqst(bc_xprt, xid); > - > - if (!req) { > - printk(KERN_NOTICE > - "%s: Got unrecognized reply: " > - "calldir 0x%x xpt_bc_xprt %p xid %08x\n", > - __func__, ntohl(calldir), > - bc_xprt, ntohl(xid)); > + if (!bc_xprt) > return -EAGAIN; > - } > + spin_lock_bh(&bc_xprt->transport_lock); > + req = xprt_lookup_rqst(bc_xprt, xid); > + if (!req) > + goto unlock_notfound; > > memcpy(&req->rq_private_buf, &req->rq_rcv_buf, sizeof(struct xdr_buf)); > /* > @@ -1040,11 +1035,21 @@ static int receive_cb_reply(struct svc_sock *svsk, struct svc_rqst *rqstp) > dst = &req->rq_private_buf.head[0]; > src = &rqstp->rq_arg.head[0]; > if (dst->iov_len < src->iov_len) > - return -EAGAIN; /* whatever; just giving up. */ > + goto unlock_eagain; /* whatever; just giving up. */ > memcpy(dst->iov_base, src->iov_base, src->iov_len); > xprt_complete_rqst(req->rq_task, rqstp->rq_arg.len); > rqstp->rq_arg.len = 0; > + spin_unlock_bh(&bc_xprt->transport_lock); > return 0; > +unlock_notfound: > + printk(KERN_NOTICE > + "%s: Got unrecognized reply: " > + "calldir 0x%x xpt_bc_xprt %p xid %08x\n", > + __func__, ntohl(calldir), > + bc_xprt, ntohl(xid)); > +unlock_eagain: > + spin_unlock_bh(&bc_xprt->transport_lock); > + return -EAGAIN; > } > > static int copy_pages_to_kvecs(struct kvec *vec, struct page **pages, int len) Nice catch. It would also be good to pair this with a lockdep_assert_held() call in xprt_lookup_rqst, but that could be added in a separate patch. Reviewed-by: Jeff Layton