Re: Coverity: frwr_unmap_sync(): Null pointer dereferences

On Apr 30, 2021, at 8:27 PM, Trond Myklebust [off-list ref] wrote:

On Fri, 2021-04-30 at 20:12 +0000, Chuck Lever III wrote:

quoted

quoted

On Apr 30, 2021, at 3:09 PM, Trond Myklebust <
trondmy@hammerspace.com> wrote:

On Fri, 2021-04-30 at 18:45 +0000, Chuck Lever III wrote:

quoted

quoted

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.

Chuck, I think the point is that you can't ever exit that while()
loop
_unless_ mr == NULL. So calling reinit_completion(&mr-

quoted

mr_linv_done)

after exiting that loop will indeed Oops.

D'oh.

quoted

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.