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

[Kevin Laatz <kevin.laatz@intel.com>]

On 15/09/2021 15:34, Bruce Richardson wrote:
> 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@huawei.com>
>>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>>> Reviewed-by: Kevin Laatz <kevin.laatz@intel.com>
>>> Reviewed-by: Conor Walsh <conor.walsh@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.
+1

/Kevin