On Wed, Sep 15, 2021 at 02:51:55PM +0100, Kevin Laatz wrote:
> On 07/09/2021 13:56, Chengwen Feng wrote:
> > This patch introduce DMA device library implementation which includes
> > configuration and I/O with the DMA devices.
> >
> > Signed-off-by: Chengwen Feng <fengcheng...@huawei.com>
> > Acked-by: Bruce Richardson <bruce.richard...@intel.com>
> > Acked-by: Morten Brørup <m...@smartsharesystems.com>
> > Reviewed-by: Kevin Laatz <kevin.la...@intel.com>
> > Reviewed-by: Conor Walsh <conor.wa...@intel.com>
> > ---
> > config/rte_config.h | 3 +
> > lib/dmadev/meson.build | 1 +
> > lib/dmadev/rte_dmadev.c | 607 +++++++++++++++++++++++++++++++++++
> > lib/dmadev/rte_dmadev.h | 118 ++++++-
> > lib/dmadev/rte_dmadev_core.h | 2 +
> > lib/dmadev/version.map | 1 +
> > 6 files changed, 720 insertions(+), 12 deletions(-)
> > create mode 100644 lib/dmadev/rte_dmadev.c
> >
> [snip]
>
> > /**
> > * @warning
> > @@ -941,10 +1018,27 @@ rte_dmadev_completed(uint16_t dev_id, uint16_t
> > vchan, const uint16_t nb_cpls,
> > * status array are also set.
> > */
> > __rte_experimental
> > -uint16_t
> > +static inline uint16_t
> > rte_dmadev_completed_status(uint16_t dev_id, uint16_t vchan,
> > const uint16_t nb_cpls, uint16_t *last_idx,
> > - enum rte_dma_status_code *status);
> > + enum rte_dma_status_code *status)
> > +{
> > + struct rte_dmadev *dev = &rte_dmadevices[dev_id];
> > + uint16_t idx;
> > +
> > +#ifdef RTE_DMADEV_DEBUG
> > + if (!rte_dmadev_is_valid_dev(dev_id) || !dev->data->dev_started ||
> > + vchan >= dev->data->dev_conf.nb_vchans ||
> > + nb_cpls == 0 || status == NULL)
> > + return 0;
> > + RTE_FUNC_PTR_OR_ERR_RET(*dev->completed_status, 0);
> > +#endif
> > +
> > + if (last_idx == NULL)
> > + last_idx = &idx;
>
> Hi Chengwen,
>
> An internal coverity scan on the IDXD dmadev driver patches flagged a
> potential null pointer dereference when using completed_status().
>
> IMO it is a false positive for the driver code since it should be checked at
> the library API level, however the check is also not present in the library.
>
> For the v22, can you add the NULL pointer check for status here, like you
> have for last_idx, please?
>
I think the check would have to be different than that for last_idx, since
the status pointer is a pointer to an array, rather than a single value -
which procludes a simple replacement in the wrapper function that the
compiler can inline away if unnecessary.
It's probably best to add it as a check in the debug block, with an
error-return if status is NULL.
/Bruce
