Re: [PATCH v8 02/10] lib/ethdev: check RSS key length

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 11/1/2023 7:40 AM, Jie Hai wrote:
> In rte_eth_dev_rss_hash_conf_get(), the "rss_key_len" should be
> greater than or equal to the "hash_key_size" which get from
> rte_eth_dev_info_get() API. And the "rss_key" should contain at
> least "hash_key_size" bytes. If these requirements are not met,
> the query unreliable.
> 
> In rte_eth_dev_rss_hash_update() or rte_eth_dev_configure(), the
> "rss_key_len" indicates the length of the "rss_key" in bytes of
> the array pointed by "rss_key", it should be equal to the
> "hash_key_size" if "rss_key" is not NULL.
> 
> This patch checks "rss_key_len" in ethdev level.
> 

Can you please squash this patch and previous one, previous one
clarifies the API and this one adds relevant checks, so they con be in
some patch.

Can you also please update release notes, 'API Changes', explaining
'rss_conf.rss_key_len' needs to be provided by user for the case
"conf.rss_key != NULL", it won't be taken as default 40 bytes anymore.


> Signed-off-by: Jie Hai <haijie1@huawei.com>
> ---
>  lib/ethdev/rte_ethdev.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index af23ac0ad00f..07bb35833ba6 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -1500,6 +1500,16 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>  		goto rollback;
>  	}
>  
> +	if (dev_conf->rx_adv_conf.rss_conf.rss_key != NULL &&
> +	    dev_conf->rx_adv_conf.rss_conf.rss_key_len < dev_info.hash_key_size) {
>

Why check is "rss_key_len < dev_info.hash_key_size", is it allowed to
have "rss_key_len > dev_info.hash_key_size"?

Shouldn't it enforce that "rss_key_len == dev_info.hash_key_size"?


> +		RTE_ETHDEV_LOG(ERR,
> +			"Ethdev port_id=%u invalid RSS key len: %u, valid value: %u\n",
> +			port_id, dev_conf->rx_adv_conf.rss_conf.rss_key_len,
> +			dev_info.hash_key_size);
> +		ret = -EINVAL;
> +		goto rollback;
> +	}
> +
>  	/*
>  	 * Setup new number of Rx/Tx queues and reconfigure device.
>  	 */
> @@ -4698,6 +4708,14 @@ rte_eth_dev_rss_hash_update(uint16_t port_id,
>  		return -ENOTSUP;
>  	}
>  
> +	if (rss_conf->rss_key != NULL &&
> +	    rss_conf->rss_key_len != dev_info.hash_key_size) {
> +		RTE_ETHDEV_LOG(ERR,
> +			"Ethdev port_id=%u invalid RSS key len: %u, valid value: %u\n",
> +			port_id, rss_conf->rss_key_len, dev_info.hash_key_size);
> +		return -EINVAL;
> +	}
> +
>  	if (*dev->dev_ops->rss_hash_update == NULL)
>  		return -ENOTSUP;
>  	ret = eth_err(port_id, (*dev->dev_ops->rss_hash_update)(dev,
> @@ -4712,6 +4730,7 @@ int
>  rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
>  			      struct rte_eth_rss_conf *rss_conf)
>  {
> +	struct rte_eth_dev_info dev_info = { 0 };
>  	struct rte_eth_dev *dev;
>  	int ret;
>  
> @@ -4725,6 +4744,18 @@ rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
>  		return -EINVAL;
>  	}
>  
> +	ret = rte_eth_dev_info_get(port_id, &dev_info);
> +	if (ret != 0)
> +		return ret;
> +
> +	if (rss_conf->rss_key != NULL &&
> +	    rss_conf->rss_key_len < dev_info.hash_key_size) {
> +		RTE_ETHDEV_LOG(ERR,
> +			"Ethdev port_id=%u invalid RSS key len: %u, should not be less than: %u\n",
> +			port_id, rss_conf->rss_key_len, dev_info.hash_key_size);
> +		return -EINVAL;
> +	}
> +
>  	if (*dev->dev_ops->rss_hash_conf_get == NULL)
>  		return -ENOTSUP;
>  	ret = eth_err(port_id, (*dev->dev_ops->rss_hash_conf_get)(dev,