From: "Chautru, Nicolas" <nicolas.chautru@intel.com>
To: David Marchand <david.marchand@redhat.com>,
"Vargas, Hernan" <hernan.vargas@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"gakhil@marvell.com" <gakhil@marvell.com>,
"trix@redhat.com" <trix@redhat.com>,
"maxime.coquelin@redhat.com" <maxime.coquelin@redhat.com>,
"Zhang, Qi Z" <qi.z.zhang@intel.com>
Subject: RE: [PATCH v1 1/1] baseband/acc: fix check after deref and dead code
Date: Tue, 15 Nov 2022 15:17:08 +0000 [thread overview]
Message-ID: <BY5PR11MB445119C552667C91FDB6ED4EF8049@BY5PR11MB4451.namprd11.prod.outlook.com> (raw)
In-Reply-To: <BY5PR11MB4451CA180DA8B9BFA2AC4F2EF8019@BY5PR11MB4451.namprd11.prod.outlook.com>
David,
Reminder on this one. Ok to apply as is?
If not consensus we cannot also mark the Coverity warning as not an issue but please check notes below.
Thanks
Nic
> -----Original Message-----
> From: Chautru, Nicolas
> Sent: Thursday, November 10, 2022 1:15 PM
> To: David Marchand <david.marchand@redhat.com>; Vargas, Hernan
> <Hernan.Vargas@intel.com>
> Cc: dev@dpdk.org; gakhil@marvell.com; trix@redhat.com;
> maxime.coquelin@redhat.com; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: RE: [PATCH v1 1/1] baseband/acc: fix check after deref and dead
> code
>
> Hi David,
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Thursday, November 10, 2022 1:49 AM
> > To: Vargas, Hernan <hernan.vargas@intel.com>
> > Cc: dev@dpdk.org; gakhil@marvell.com; trix@redhat.com;
> > maxime.coquelin@redhat.com; Chautru, Nicolas
> > <nicolas.chautru@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > Subject: Re: [PATCH v1 1/1] baseband/acc: fix check after deref and
> > dead code
> >
> > On Thu, Nov 3, 2022 at 8:57 PM Hernan Vargas <hernan.vargas@intel.com>
> > wrote:
> > >
> > > Fix potential issue of dereferencing a pointer before null check.
> > > Remove null check for value that could never be null.
> > >
> > > Coverity issue: 381646, 381631
> > > Fixes: 989dec301a9 ("baseband/acc100: add ring companion address")
> > >
> > > Signed-off-by: Hernan Vargas <hernan.vargas@intel.com>
> > > ---
> > > drivers/baseband/acc/rte_acc100_pmd.c | 4 ----
> > > 1 file changed, 4 deletions(-)
> > >
> > > diff --git a/drivers/baseband/acc/rte_acc100_pmd.c
> > > b/drivers/baseband/acc/rte_acc100_pmd.c
> > > index 96daef87bc..30a718916d 100644
> > > --- a/drivers/baseband/acc/rte_acc100_pmd.c
> > > +++ b/drivers/baseband/acc/rte_acc100_pmd.c
> > > @@ -4122,15 +4122,11 @@ acc100_dequeue_ldpc_enc(struct
> > rte_bbdev_queue_data *q_data,
> > > struct rte_bbdev_enc_op *op;
> > > union acc_dma_desc *desc;
> > >
> > > - if (q == NULL)
> > > - return 0;
> >
> > I guess this protects badly written applications that would do stuff
> > like pass an incorrect queue id, or call this callback while the queue
> > has not been configured yet.
> > This is something that should be caught at the bbdev layer (arguably
> > under the RTE_LIBRTE_BBDEV_DEBUG if the performance is that much
> > affected, though I'd like to see numbers).
> > (edit: I see Maxime replied a similar comment).
>
> That is not directly to that ticket but would be good to follow up.
> From previous discussion with Maxime, the new consensus was to avoid
> special check in debug mode (try to build the same code). It would be good to
> come up to a new consensus on this.
>
> >
> > Back to this particular patch, rather than remove the check, the right
> > fix is to move acc_ring_avail_deq(q).
> > This is what Coverity reports.
> >
> > And this same pattern is used in other parts of the driver.
> > It just happens that Coverity did not report them because some avec
> > under RTE_LIBRTE_BBDEV_DEBUG...
>
> I believe that we don't want to create discrepancies : each dequeue function
> should behave the same way. Ie. acc100_dequeue_ldpc_enc should not do
> things differently from others dequeue functions.
> Currently there is a discrepancy which is being resolved in that patch.
>
> Either we remove the check as in that commit which could be approved as is,
> or we move the check under the debug as for the other functions which hides
> the Coverity issue without in reality fully addressing it, or we remove these
> check from all functions (including under debug) which is what we do for
> other PMD.
>
> That 4th option you seem to suggest would consist in effect to do thing
> differently just for the dequeue function which would lacks consistency really.
>
> Is there any concern just to approve as is, again that q == NULL is not done in
> production code anywhere else as you pointed out.
>
> I agree that in next release we can remove much of the code under DEBUG
> flag which is not adding value nor being built/used in practice.
>
> Thanks
> Nic
>
> >
> >
> >
> > > #ifdef RTE_LIBRTE_BBDEV_DEBUG
> > > if (unlikely(ops == 0))
> >
> > And I also noticed this hunk.
> >
> > DPDK coding style, ops should be compared against NULL, but see below...
> >
> >
> > > return 0;
> > > #endif
> > > desc = q->ring_addr + (q->sw_ring_tail & q->sw_ring_wrap_mask);
> > > - if (unlikely(desc == NULL))
> > > - return 0;
> > > op = desc->req.op_addr;
> > > if (unlikely(ops == NULL || op == NULL))
> > > return 0;
> >
> > ... like here, so above check is redundant.
> >
> > There is probably more cleanups to do in this driver.
> > This can be done later.
> >
> >
> > --
> > David Marchand
next prev parent reply other threads:[~2022-11-15 15:17 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-04 3:52 [PATCH v1 0/1] baseband/acc: coverity fix RC2 Hernan Vargas
2022-11-04 3:52 ` [PATCH v1 1/1] baseband/acc: fix check after deref and dead code Hernan Vargas
2022-11-04 8:51 ` Maxime Coquelin
2022-11-04 17:39 ` Chautru, Nicolas
2022-11-08 18:00 ` Chautru, Nicolas
2022-11-10 9:48 ` David Marchand
2022-11-10 21:15 ` Chautru, Nicolas
2022-11-15 15:17 ` Chautru, Nicolas [this message]
2022-11-15 15:59 ` David Marchand
2022-11-15 18:04 ` Chautru, Nicolas
2022-11-15 22:32 ` Chautru, Nicolas
2023-01-20 20:55 [PATCH v1 0/1] DPDK Coverity issue 381631, 381646 Hernan Vargas
2023-01-20 20:55 ` [PATCH v1 1/1] baseband/acc: fix check after deref and dead code Hernan Vargas
2023-02-06 15:22 ` Maxime Coquelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BY5PR11MB445119C552667C91FDB6ED4EF8049@BY5PR11MB4451.namprd11.prod.outlook.com \
--to=nicolas.chautru@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=gakhil@marvell.com \
--cc=hernan.vargas@intel.com \
--cc=maxime.coquelin@redhat.com \
--cc=qi.z.zhang@intel.com \
--cc=trix@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).