On Fri, 2021-04-30 at 18:45 +0000, Chuck Lever III wrote: > > > > On Apr 30, 2021, at 2:26 PM, coverity-bot <keescook@xxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxx
