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 <[email protected]>; Vargas, Hernan > <[email protected]>; [email protected]; [email protected]; > [email protected] > Cc: Zhang, Qi Z <[email protected]> > Subject: RE: [PATCH v1 1/1] baseband/acc: fix check after deref and dead code > > Hi Maxime, > > > -----Original Message----- > > From: Maxime Coquelin <[email protected]> > > Sent: Friday, November 4, 2022 1:52 AM > > To: Vargas, Hernan <[email protected]>; [email protected]; > > [email protected]; [email protected] > > Cc: Chautru, Nicolas <[email protected]>; Zhang, Qi Z > > <[email protected]> > > 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 <[email protected]> > > > --- > > > 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;
