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