[PATCH v1 1/1] baseband/acc: fix check after deref and dead code

[Chautru, Nicolas]

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 at redhat.com>; Vargas, Hernan
> <Hernan.Vargas at intel.com>
> Cc: dev at dpdk.org; gakhil at marvell.com; trix at redhat.com;
> maxime.coquelin at redhat.com; Zhang, Qi Z <qi.z.zhang at 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 at redhat.com>
> > Sent: Thursday, November 10, 2022 1:49 AM
> > To: Vargas, Hernan <hernan.vargas at intel.com>
> > Cc: dev at dpdk.org; gakhil at marvell.com; trix at redhat.com;
> > maxime.coquelin at redhat.com; Chautru, Nicolas
> > <nicolas.chautru at intel.com>; Zhang, Qi Z <qi.z.zhang at 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 at 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 at 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