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.march...@redhat.com>; Vargas, Hernan
> <hernan.var...@intel.com>
> Cc: dev@dpdk.org; gak...@marvell.com; t...@redhat.com;
> maxime.coque...@redhat.com; Zhang, Qi Z <qi.z.zh...@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.march...@redhat.com>
> > Sent: Thursday, November 10, 2022 1:49 AM
> > To: Vargas, Hernan <hernan.var...@intel.com>
> > Cc: dev@dpdk.org; gak...@marvell.com; t...@redhat.com;
> > maxime.coque...@redhat.com; Chautru, Nicolas
> > <nicolas.chau...@intel.com>; Zhang, Qi Z <qi.z.zh...@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.var...@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.var...@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
