From: Trond Myklebust <trondmy@hammerspace•com>
To: "keescook@chromium•org" <keescook@chromium•org>,
"chuck.lever@oracle•com" <chuck.lever@oracle•com>
Cc: "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: Fri, 30 Apr 2021 19:09:12 +0000 [thread overview]
Message-ID: <08aa80f9bfeb1b48daf804aa2903d6217e599fa4.camel@hammerspace.com> (raw)
In-Reply-To: <B431253D-E874-45BA-9303-79C7FA950D1B@oracle.com>
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.
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.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace•com
next prev parent reply other threads:[~2021-04-30 19:09 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 [this message]
2021-04-30 20:12 ` Chuck Lever III
2021-05-01 0:26 ` Trond Myklebust
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=08aa80f9bfeb1b48daf804aa2903d6217e599fa4.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