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

From: Trond Myklebust <[email-masked]>
Date: Fri, 30 Apr 2021 19:09:12 +0000

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.

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
[email-masked]