diff mbox series

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

Message ID	20231101074039.3088716-3-haijie1@huawei.com (mailing list archive)
State	Changes Requested, archived
Delegated to:	Ferruh Yigit
Headers	From: Jie Hai <haijie1@huawei.com> To: <haijie1@huawei.com>, <dev@dpdk.org>, Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit <ferruh.yigit@amd.com>, Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> CC: <lihuisong@huawei.com>, <fengchengwen@huawei.com>, <liudongdong3@huawei.com> Subject: [PATCH v8 02/10] lib/ethdev: check RSS key length Date: Wed, 1 Nov 2023 15:40:31 +0800 Message-ID: <20231101074039.3088716-3-haijie1@huawei.com> In-Reply-To: <20231101074039.3088716-1-haijie1@huawei.com> References: <20230315110033.30143-1-liudongdong3@huawei.com> <20231101074039.3088716-1-haijie1@huawei.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain Precedence: list Errors-To: dev-bounces@dpdk.org
Series	support setting and querying RSS algorithms \| [v8,00/10] support setting and querying RSS algorithms [v8,01/10] ethdev: overwrite some comment related to RSS [v8,02/10] lib/ethdev: check RSS key length [v8,03/10] ethdev: support setting and querying RSS algorithm [v8,04/10] net/hns3: report RSS hash algorithms capability [v8,05/10] net/hns3: support setting and querying RSS hash function [v8,06/10] app/proc-info: fix never show RSS info [v8,07/10] app/proc-info: adjust the display format of RSS info [v8,08/10] ethdev: add API to get RSS algorithm names [v8,09/10] app/proc-info: support querying RSS hash algorithm [v8,10/10] app/testpmd: add RSS hash algorithms display

Checks

Context	Check	Description
ci/checkpatch	success	coding style OK

Commit Message

Jie Hai Nov. 1, 2023, 7:40 a.m. UTC

  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.

Signed-off-by: Jie Hai <haijie1@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

Comments

lihuisong (C) Nov. 1, 2023, 10:07 a.m. UTC | #1

在 2023/11/1 15:40, Jie Hai 写道:
> 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.
>
> 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) {
dev_conf->rx_adv_conf.rss_conf.rss_key_len != dev_info.hash_key_size, right?
otherwise, this isn't inconsistent with the comments in patch 1.
> +		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,

Ferruh Yigit Nov. 1, 2023, 1:19 p.m. UTC | #2

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,

Jie Hai Nov. 2, 2023, 3:40 a.m. UTC | #3

On 2023/11/1 21:19, Ferruh Yigit wrote:
> 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.
> 
Thanks.
Will update.
> 
>> 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"?
> 
> 
Yes, should be equal, will correct.
>> +		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,
> 
> .

diff mbox series

Patch

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) {
+		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,