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

["Chautru, Nicolas" <nicolas.chautru@intel.com>]

Hi Maxime, 

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, November 4, 2022 1:52 AM
> To: Vargas, Hernan <hernan.vargas@intel.com>; dev@dpdk.org;
> gakhil@marvell.com; trix@redhat.com
> Cc: 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 11/4/22 04:52, Hernan Vargas 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;
> 
> Can we be sure it can never be NULL?
> 
> static inline uint16_t
> rte_bbdev_dequeue_ldpc_enc_ops(uint16_t dev_id, uint16_t queue_id,
> 		struct rte_bbdev_enc_op **ops, uint16_t num_ops) {
> 	struct rte_bbdev *dev = &rte_bbdev_devices[dev_id];
> 	struct rte_bbdev_queue_data *q_data = &dev->data-
> >queues[queue_id];
> 	return dev->dequeue_ldpc_enc_ops(q_data, ops, num_ops); }
> 
> If the application passes an invalid queue_id or dev_id you can easily get
> garbage.
> 
> It may be worth adding some checks in all the helpers, to be sure dev_id is
> valid, and same for queue_id. We do that in Vhost library to improve
> robustness.
> 
> I know there is this comment:
> "
>   * This function does not provide any error notification to avoid the
>   * corresponding overhead.
> "
> 
> But to me this is not a good justification, the overhead would be minimal.
> 

Thanks. 
The rational is that this function needs to be very lightweight since this is a called in loop and hence this is was captured
explicitly in bbdev. 
More generally I don’t believe that a change to bbdev would be relevant in that ticket, ok to move that discussion for later on in any case?

This ticket is purely about a Coverity fix for the ACC100 PMD. Note that we don’t check for q null during dequeue in most baseband PMD (including both intel and non-intel ones), this one was not required either, only historical. 
Does that sound fair in the context of that Coverity fix?
Thanks
Nic


> Regards,
> Maxime
> 
> >   #ifdef RTE_LIBRTE_BBDEV_DEBUG
> >   	if (unlikely(ops == 0))
> >   		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;