[dpdk-dev] [PATCH v21 4/7] dmadev: introduce DMA device library implementation

[Bruce Richardson]

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 <fengchengwen at huawei.com>
> > Acked-by: Bruce Richardson <bruce.richardson at intel.com>
> > Acked-by: Morten Brørup <mb at smartsharesystems.com>
> > Reviewed-by: Kevin Laatz <kevin.laatz at intel.com>
> > Reviewed-by: Conor Walsh <conor.walsh at 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