> On Apr 30, 2021, at 8:27 PM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote: > > 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? I’ll post a fix by Sunday evening. You can squash it into the original if you like.
