* Coverity: frwr_unmap_sync(): Null pointer dereferences
@ 2021-04-30 18:26 coverity-bot
2021-04-30 18:45 ` Chuck Lever III
0 siblings, 1 reply; 6+ messages in thread
From: coverity-bot @ 2021-04-30 18:26 UTC (permalink / raw)
To: Chuck Lever; +Cc: Trond Myklebust, Gustavo A. R. Silva, linux-next
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 <keescook+coverity-bot@chromium.org>
Addresses-Coverity-ID: 1504556 ("Null pointer dereferences")
Fixes: 9a301cafc861 ("xprtrdma: Move fr_linv_done field to struct rpcrdma_mr")
Thanks for your attention!
--
Coverity-bot
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Coverity: frwr_unmap_sync(): Null pointer dereferences 2021-04-30 18:26 Coverity: frwr_unmap_sync(): Null pointer dereferences coverity-bot @ 2021-04-30 18:45 ` Chuck Lever III 2021-04-30 19:09 ` Trond Myklebust 0 siblings, 1 reply; 6+ messages in thread From: Chuck Lever III @ 2021-04-30 18:45 UTC (permalink / raw) To: coverity-bot; +Cc: Trond Myklebust, Gustavo A. R. Silva, linux-next > 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. > 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 <keescook+coverity-bot@chromium.org> > Addresses-Coverity-ID: 1504556 ("Null pointer dereferences") > Fixes: 9a301cafc861 ("xprtrdma: Move fr_linv_done field to struct rpcrdma_mr") > > Thanks for your attention! > > -- > Coverity-bot -- Chuck Lever ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Coverity: frwr_unmap_sync(): Null pointer dereferences 2021-04-30 18:45 ` Chuck Lever III @ 2021-04-30 19:09 ` Trond Myklebust 2021-04-30 20:12 ` Chuck Lever III 0 siblings, 1 reply; 6+ messages in thread From: Trond Myklebust @ 2021-04-30 19:09 UTC (permalink / raw) To: keescook, chuck.lever; +Cc: linux-next, gustavo On Fri, 2021-04-30 at 18:45 +0000, Chuck Lever III wrote: > > > > 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->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. -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Coverity: frwr_unmap_sync(): Null pointer dereferences 2021-04-30 19:09 ` Trond Myklebust @ 2021-04-30 20:12 ` Chuck Lever III 2021-05-01 0:26 ` Trond Myklebust 0 siblings, 1 reply; 6+ messages in thread From: Chuck Lever III @ 2021-04-30 20:12 UTC (permalink / raw) To: Trond Myklebust; +Cc: keescook, linux-next, gustavo > 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: >> >> >>> 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->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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Coverity: frwr_unmap_sync(): Null pointer dereferences 2021-04-30 20:12 ` Chuck Lever III @ 2021-05-01 0:26 ` Trond Myklebust 2021-05-01 0:49 ` Chuck Lever III 0 siblings, 1 reply; 6+ messages in thread From: Trond Myklebust @ 2021-05-01 0:26 UTC (permalink / raw) To: chuck.lever; +Cc: keescook, linux-next, gustavo On Fri, 2021-04-30 at 20:12 +0000, Chuck Lever III wrote: > > > > 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: > > > > > > > > > > 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- > > >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@hammerspace.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Coverity: frwr_unmap_sync(): Null pointer dereferences 2021-05-01 0:26 ` Trond Myklebust @ 2021-05-01 0:49 ` Chuck Lever III 0 siblings, 0 replies; 6+ messages in thread From: Chuck Lever III @ 2021-05-01 0:49 UTC (permalink / raw) To: Trond Myklebust; +Cc: keescook, linux-next, gustavo > On Apr 30, 2021, at 8:27 PM, Trond Myklebust <trondmy@hammerspace.com> 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@hammerspace.com> 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@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- >>>> 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-01 0:49 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-30 18:26 Coverity: frwr_unmap_sync(): Null pointer dereferences coverity-bot 2021-04-30 18:45 ` Chuck Lever III 2021-04-30 19:09 ` Trond Myklebust 2021-04-30 20:12 ` Chuck Lever III 2021-05-01 0:26 ` Trond Myklebust 2021-05-01 0:49 ` Chuck Lever III
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.