Coverity: frwr_unmap_sync(): Null pointer dereferences

DISCUSSION coverity-bot 3 participants 6 messages Apr 30, 2021 – May 1, 2021 Linux-next tree

Share & tools

coverity-bot

author

2021-04-30 18:26 UTC #0 lore raw

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. If not, please make sure fixes get into linux-next. :) For patches fixing this, please include these lines (but double-check the "Fixes" first):

Reported-by coverity-bot <[email protected]>

Addresses-Coverity-ID: 1504556 ("Null pointer dereferences")

Fixes 9a301cafc861 ("xprtrdma: Move fr_linv_done field to struct rpcrdma_mr")

Thanks for your attention!

Signature · 1 line

Coverity-bot

Chuck Lever III

2021-04-30 18:45 UTC #1 lore raw

Quoted 35 lines · coverity-bot

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     	 */

Quoted 2 lines

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.

Quoted 4 lines

If not, please make
sure fixes get into linux-next. :) For patches fixing this, please include these lines (but double-check the "Fixes" first):

Quoted 8 lines

Reported-by coverity-bot <[email protected]>
Addresses-Coverity-ID: 1504556 ("Null pointer dereferences")
Fixes 9a301cafc861 ("xprtrdma: Move fr_linv_done field to struct rpcrdma_mr")
Thanks for your attention!
Signature · 1 line
Coverity-bot

--
Chuck Lever

Trond Myklebust

2021-04-30 19:09 UTC #2 lore raw

Quoted 65 lines · Chuck Lever III

Quoted 45 lines
On Apr 30, 2021, at 2:26 PM, coverity-bot <[email protected]> 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              */
Quoted 3 lines
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))) {

Quoted 9 lines

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.

Signature · 3 lines

Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]

Chuck Lever III

2021-04-30 20:12 UTC #3 lore raw

Quoted 76 lines · Trond Myklebust

Quoted 71 lines · Chuck Lever III
Quoted 45 lines
On Apr 30, 2021, at 2:26 PM, coverity-bot <[email protected]> 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 */
Quoted 3 lines
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.

Quoted 7 lines

Quoted 3 lines
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.

Quoted 4 lines

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

Trond Myklebust

2021-05-01 00:27 UTC #4 lore raw

Quoted 101 lines · Chuck Lever III

Quoted 86 lines
On Apr 30, 2021, at 3:09 PM, Trond Myklebust <
[email protected]> wrote:
Quoted 70 lines · Chuck Lever III
Quoted 49 lines
On Apr 30, 2021, at 2:26 PM, coverity-bot <
[email protected]>
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              */
Quoted 4 lines
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))) {
Quoted 9 lines
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.
Quoted 5 lines
Chuck, I think the point is that you can't ever exit that while() loop
_unless_ mr == NULL. So calling reinit_completion(&mr-
Quoted 1 line
mr_linv_done)
after exiting that loop will indeed Oops.
D'oh.
Quoted 5 lines
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.

Quoted 6 lines

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?

Signature · 3 lines

Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]

Chuck Lever III

2021-05-01 00:49 UTC #5 lore raw

Quoted 106 lines · Trond Myklebust

Quoted 101 lines · Chuck Lever III
Quoted 86 lines
On Apr 30, 2021, at 3:09 PM, Trond Myklebust <
[email protected]> wrote:
Quoted 70 lines · Chuck Lever III
Quoted 49 lines
On Apr 30, 2021, at 2:26 PM, coverity-bot <
[email protected]>
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 */
Quoted 4 lines
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))) {
Quoted 9 lines
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.
Quoted 5 lines
Chuck, I think the point is that you can't ever exit that while() loop
_unless_ mr == NULL. So calling reinit_completion(&mr-
Quoted 1 line
mr_linv_done)
after exiting that loop will indeed Oops.
D'oh.
Quoted 5 lines
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.

Quoted 8 lines

Quoted 6 lines
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.