Re: Coverity: frwr_unmap_sync(): Null pointer dereferences

[Chuck Lever III <chuck.lever@oracle.com>]

> 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.


> If not, please make
> sure fixes get into linux-next. :) For patches fixing this, please
> include these lines (but double-check the "Fixes" first):
> 
> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
> Addresses-Coverity-ID: 1504556 ("Null pointer dereferences")
> Fixes: 9a301cafc861 ("xprtrdma: Move fr_linv_done field to struct rpcrdma_mr")
> 
> Thanks for your attention!
> 
> -- 
> Coverity-bot

--
Chuck Lever