From: Chuck Lever III <chuck.lever@oracle•com>
To: Trond Myklebust <trondmy@hammerspace•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: Fri, 30 Apr 2021 20:12:15 +0000 [thread overview]
Message-ID: <84662251-DF84-4827-9D89-293BB2340433@oracle.com> (raw)
In-Reply-To: <08aa80f9bfeb1b48daf804aa2903d6217e599fa4.camel@hammerspace.com>
> 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.
--
Chuck Lever
next prev parent reply other threads:[~2021-04-30 20:12 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 [this message]
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=84662251-DF84-4827-9D89-293BB2340433@oracle.com \
--to=chuck.lever@oracle$(echo .)com \
--cc=gustavo@embeddedor$(echo .)com \
--cc=keescook@chromium$(echo .)org \
--cc=linux-next@vger$(echo .)kernel.org \
--cc=trondmy@hammerspace$(echo .)com \
/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