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