Subject: Re: Coverity: frwr_unmap_sync(): Null pointer dereferences

From: Chuck Lever III <[email-masked]>
Date: Fri, 30 Apr 2021 20:12:15 +0000

> On Apr 30, 2021, at 3:09 PM, Trond Myklebust <[email-masked]> wrote:
> 
> On Fri, 2021-04-30 at 18:45 +0000, Chuck Lever III wrote:
>> 
>> 
>>> On Apr 30, 2021, at 2:26 PM, coverity-bot <[email-masked]>
>>> 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