From: Trond Myklebust <trondmy@hammerspace•com>
To: "chuck.lever@oracle•com" <chuck.lever@oracle•com>
Cc: "keescook@chromium•org" <keescook@chromium•org>,
"linux-next@vger•kernel.org" <linux-next@vger•kernel.org>,
"gustavo@embeddedor•com" <gustavo@embeddedor•com>
Subject: Re: Coverity: frwr_unmap_sync(): Null pointer dereferences
Date: Sat, 1 May 2021 00:26:52 +0000 [thread overview]
Message-ID: <8724e5cb46c9cbc97d3f5bef19aadd50ea8210a5.camel@hammerspace.com> (raw)
In-Reply-To: <84662251-DF84-4827-9D89-293BB2340433@oracle.com>
On Fri, 2021-04-30 at 20:12 +0000, Chuck Lever III wrote:
>
>
> > On Apr 30, 2021, at 3:09 PM, Trond Myklebust <
> > trondmy@hammerspace•com> wrote:
> >
> > On Fri, 2021-04-30 at 18:45 +0000, Chuck Lever III wrote:
> > >
> > >
> > > > On Apr 30, 2021, at 2:26 PM, coverity-bot <
> > > > keescook@chromium•org>
> > > > wrote:
> > > >
> > > > Hello!
> > > >
> > > > This is an experimental semi-automated report about issues
> > > > detected
> > > > by
> > > > Coverity from a scan of next-20210430 as part of the linux-next
> > > > scan project:
> > > > https://scan.coverity.com/projects/linux-next-weekly-scan
> > > >
> > > > You're getting this email because you were associated with the
> > > > identified
> > > > lines of code (noted below) that were touched by commits:
> > > >
> > > > Mon Apr 26 09:27:06 2021 -0400
> > > > 9a301cafc861 ("xprtrdma: Move fr_linv_done field to struct
> > > > rpcrdma_mr")
> > > >
> > > > Coverity reported the following:
> > > >
> > > > *** CID 1504556: Null pointer dereferences (FORWARD_NULL)
> > > > /net/sunrpc/xprtrdma/frwr_ops.c: 539 in frwr_unmap_sync()
> > > > 533
> > > > 534 /* Strong send queue ordering guarantees that
> > > > when
> > > > the
> > > > 535 * last WR in the chain completes, all WRs in
> > > > the
> > > > chain
> > > > 536 * are complete.
> > > > 537 */
> > > > 538 last->wr_cqe->done = frwr_wc_localinv_wake;
> > > > vvv CID 1504556: Null pointer dereferences (FORWARD_NULL)
> > > > vvv Passing null pointer "&mr->mr_linv_done" to
> > > > "reinit_completion", which dereferences it.
> > > > 539 reinit_completion(&mr->mr_linv_done);
> > > > 540
> > > > 541 /* Transport disconnect drains the receive CQ
> > > > before it
> > > > 542 * replaces the QP. The RPC reply handler won't
> > > > call us
> > > > 543 * unless re_id->qp is a valid pointer.
> > > > 544 */
> > > >
> > > > If this is a false positive, please let us know so we can mark
> > > > it
> > > > as
> > > > such, or teach the Coverity rules to be smarter.
> > >
> > > Sure, not my proudest moment here.
> > >
> > > The sole call site for frwr_unmap_sync() is this one:
> > >
> > > net/sunrpc/xprtrdma/transport.c:
> > > 606 if (unlikely(!list_empty(&req->rl_registered))) {
> > > 607 trace_xprtrdma_mrs_zap(task);
> > > 608 frwr_unmap_sync(rpcx_to_rdmax(rqst->rq_xprt),
> > > req);
> > > 609 }
> > >
> > > Thus, in the current code base, the while() loop:
> > >
> > > net/sunrpc/xprtrdma/frwr_ops.c:
> > > 514 while ((mr = rpcrdma_mr_pop(&req->rl_registered))) {
> > >
> > > Should always terminate with mr containing a non-NULL address.
> > >
> > > Seems to me the frwr_unmap_sync() code before fdf5ecb1934b
> > > ("xprtrdma: Move fr_linv_done field to struct rpcrdma_mr") has
> > > the same risk -- frwr can be NULL if rl_registered is empty.
> > >
> > > I'm open to suggestions for improvement, but I'm not seeing this
> > > rise to the level of a pervasive and high impact issue.
> > >
> >
> > Chuck, I think the point is that you can't ever exit that while()
> > loop
> > _unless_ mr == NULL. So calling reinit_completion(&mr-
> > >mr_linv_done)
> > after exiting that loop will indeed Oops.
>
> D'oh.
>
>
> > So will the call to wait_for_completion(&mr->mr_linv_done).
> >
> > IOW: I think you need to save the last non-NULL value of 'mr'
> > inside
> > the loop.
>
> I think following the while() loop with:
>
> mr = container_of(last, struct rpcrdma_mr, mr_invwr);
>
> Might also work.
>
Sounds good. Will be you sending me an updated+tested patch?
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace•com
next prev parent reply other threads:[~2021-05-01 0:27 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-30 18:26 Coverity: frwr_unmap_sync(): Null pointer dereferences coverity-bot
2021-04-30 18:45 ` Chuck Lever III
2021-04-30 19:09 ` Trond Myklebust
2021-04-30 20:12 ` Chuck Lever III
2021-05-01 0:26 ` Trond Myklebust [this message]
2021-05-01 0:49 ` Chuck Lever III
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8724e5cb46c9cbc97d3f5bef19aadd50ea8210a5.camel@hammerspace.com \
--to=trondmy@hammerspace$(echo .)com \
--cc=chuck.lever@oracle$(echo .)com \
--cc=gustavo@embeddedor$(echo .)com \
--cc=keescook@chromium$(echo .)org \
--cc=linux-next@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox