On Fri, 2021-04-30 at 20:12 +0000, Chuck Lever III wrote: > > > > On Apr 30, 2021, at 3:09 PM, Trond Myklebust < > > trondmy@xxxxxxxxxxxxxxx> 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@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. > > 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. > Sounds good. Will be you sending me an updated+tested patch? -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@xxxxxxxxxxxxxxx
