Hi Maxime, Akhil,
This patch is not applied yet.
Any concern to apply this as is?
Maxime, unclear whether you recommend a change for this in the context of the
coverify fix. Please kindly clarify. Do you want to keep the q == null in the
PMD?
Thanks
Nic
> -----Original Message-----
> From: Chautru, Nicolas
> Sent: Friday, November 4, 2022 10:39 AM
> To: Maxime Coquelin <maxime.coque...@redhat.com>; Vargas, Hernan
> <hernan.var...@intel.com>; dev@dpdk.org; gak...@marvell.com;
> t...@redhat.com
> Cc: Zhang, Qi Z <qi.z.zh...@intel.com>
> Subject: RE: [PATCH v1 1/1] baseband/acc: fix check after deref and dead code
>
> Hi Maxime,
>
> > -----Original Message-----
> > From: Maxime Coquelin <maxime.coque...@redhat.com>
> > Sent: Friday, November 4, 2022 1:52 AM
> > To: Vargas, Hernan <hernan.var...@intel.com>; dev@dpdk.org;
> > gak...@marvell.com; t...@redhat.com
> > Cc: 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 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.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;
> >
> > 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;
