Re: [PATCH 1/4] ethdev: verify queue ID when Tx done cleanup

[fengchengwen <fengchengwen@huawei.com>]

On 2024/10/12 10:27, Ferruh Yigit wrote:
> On 9/5/2024 8:33 AM, Andrew Rybchenko wrote:
>> On 9/5/24 09:46, Jie Hai wrote:
>>> From: Chengwen Feng <fengchengwen@huawei.com>
>>>
>>> Verify queue_id for rte_eth_tx_done_cleanup API.
>>
>> If I'm not mistaken the function is considered data path API (fast).
>> If so, it should not validate it's parameters as in rte_eth_tx_burst().
>> It may be done under RTE_ETHDEV_DEBUG_TX only.
>>
>> May be documentation should be fixed to highlight it.
>>
>> And yes, current implementation looks inconsistent from this point of
>> view since port_id and presence of callback are checked.
>>
>> Anyway motivation in the patch description is insufficient.
>>
> 
> Hi Chengwen,
> 
> I agree with Andrew, to not add checks to the datapath function.
> 
> Can you please explain more why this check is needed at first place?
> Is it for a specific usecase?

Hi Ferruh,

It was triggered by our internal security review.
I think it's okay to add the check under RTE_ETHDEV_DEBUG_TX. and Haijie will send v2.

Thanks

> 
>>>
>>> Fixes: 44a718c457b5 ("ethdev: add API to free consumed buffers in Tx
>>> ring")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>> ---
>>>   lib/ethdev/rte_ethdev.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>> index f1c658f49e80..998deb5ab101 100644
>>> --- a/lib/ethdev/rte_ethdev.c
>>> +++ b/lib/ethdev/rte_ethdev.c
>>> @@ -2823,6 +2823,10 @@ rte_eth_tx_done_cleanup(uint16_t port_id,
>>> uint16_t queue_id, uint32_t free_cnt)
>>>       RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>       dev = &rte_eth_devices[port_id];
>>>   +    ret = eth_dev_validate_tx_queue(dev, queue_id);
>>> +    if (ret != 0)
>>> +        return ret;
>>> +
>>>       if (*dev->dev_ops->tx_done_cleanup == NULL)
>>>           return -ENOTSUP;
>>>   
>>
>