[PATCH] ethdev: validate reserved fields

[PATCH] ethdev: validate reserved fields

[Stephen Hemminger]

The various reserved fields added to ethdev could not be
safely used for future extensions because they were never
checked on input. Therefore ABI would be broken if these
fields were added in a future DPDK release.

Fixes: 436b3a6b6e62 ("ethdev: reserve space in main structs for extension")
Cc: thomas@monjalon.net
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/ethdev/rte_ethdev.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 4d0325568322..4f937a1914c9 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -1228,6 +1228,25 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	/* Backup mtu for rollback */
 	old_mtu = dev->data->mtu;
 
+	/* fields must be zero to reserve them for future ABI changes */
+	if (dev_conf->rxmode.reserved_64s[0] != 0 ||
+	    dev_conf->rxmode.reserved_64s[1] != 0 ||
+	    dev_conf->rxmode.reserved_ptrs[0] != NULL ||
+	    dev_conf->rxmode.reserved_ptrs[1] != NULL) {
+		RTE_ETHDEV_LOG(ERR, "Rxmode reserved fields not zero\n");
+		ret = -EINVAL;
+		goto rollback;
+	}
+
+	if (dev_conf->txmode.reserved_64s[0] != 0 ||
+	    dev_conf->txmode.reserved_64s[1] != 0 ||
+	    dev_conf->txmode.reserved_ptrs[0] != NULL ||
+	    dev_conf->txmode.reserved_ptrs[1] != NULL) {
+		RTE_ETHDEV_LOG(ERR, "txmode reserved fields not zero\n");
+		ret = -EINVAL;
+		goto rollback;
+	}
+
 	ret = rte_eth_dev_info_get(port_id, &dev_info);
 	if (ret != 0)
 		goto rollback;
@@ -2003,6 +2022,14 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
 	if (*dev->dev_ops->rx_queue_setup == NULL)
 		return -ENOTSUP;
 
+	if (rx_conf->reserved_64s[0] != 0 ||
+	    rx_conf->reserved_64s[1] != 0 ||
+	    rx_conf->reserved_ptrs[0] != NULL ||
+	    rx_conf->reserved_ptrs[1] != NULL) {
+		RTE_ETHDEV_LOG(ERR, "Rx conf reserved fields not zero\n");
+		return -EINVAL;
+	}
+
 	ret = rte_eth_dev_info_get(port_id, &dev_info);
 	if (ret != 0)
 		return ret;
@@ -2206,6 +2233,12 @@ rte_eth_rx_hairpin_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
 		return -EINVAL;
 	}
 
+	if (conf->reserved != 0) {
+		RTE_ETHDEV_LOG(ERR,
+			       "Rx hairpin reserved field not zero\n");
+		return -EINVAL;
+	}
+
 	ret = rte_eth_dev_hairpin_capability_get(port_id, &cap);
 	if (ret != 0)
 		return ret;
@@ -2301,6 +2334,14 @@ rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
 	if (*dev->dev_ops->tx_queue_setup == NULL)
 		return -ENOTSUP;
 
+	if (tx_conf->reserved_64s[0] != 0 ||
+	    tx_conf->reserved_64s[1] != 0 ||
+	    tx_conf->reserved_ptrs[0] != NULL ||
+	    tx_conf->reserved_ptrs[1] != NULL) {
+		RTE_ETHDEV_LOG(ERR, "Tx conf reserved fields not zero\n");
+		return -EINVAL;
+	}
+
 	ret = rte_eth_dev_info_get(port_id, &dev_info);
 	if (ret != 0)
 		return ret;
-- 
2.39.2

Re: [PATCH] ethdev: validate reserved fields

[Bruce Richardson]

On Thu, May 25, 2023 at 01:39:42PM -0700, Stephen Hemminger wrote:
> The various reserved fields added to ethdev could not be
> safely used for future extensions because they were never
> checked on input. Therefore ABI would be broken if these
> fields were added in a future DPDK release.
> 
> Fixes: 436b3a6b6e62 ("ethdev: reserve space in main structs for extension")
> Cc: thomas@monjalon.net
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/ethdev/rte_ethdev.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [PATCH] ethdev: validate reserved fields

[Ferruh Yigit]

On 5/26/2023 9:15 AM, Bruce Richardson wrote:
> On Thu, May 25, 2023 at 01:39:42PM -0700, Stephen Hemminger wrote:
>> The various reserved fields added to ethdev could not be
>> safely used for future extensions because they were never
>> checked on input. Therefore ABI would be broken if these
>> fields were added in a future DPDK release.
>>
>> Fixes: 436b3a6b6e62 ("ethdev: reserve space in main structs for extension")
>> Cc: thomas@monjalon.net
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> ---
>>  lib/ethdev/rte_ethdev.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 41 insertions(+)
>>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

Applied to dpdk-next-net/main, thanks.

Re: [PATCH] ethdev: validate reserved fields

[Ferruh Yigit]

On 9/21/2023 4:12 PM, Ferruh Yigit wrote:
> On 5/26/2023 9:15 AM, Bruce Richardson wrote:
>> On Thu, May 25, 2023 at 01:39:42PM -0700, Stephen Hemminger wrote:
>>> The various reserved fields added to ethdev could not be
>>> safely used for future extensions because they were never
>>> checked on input. Therefore ABI would be broken if these
>>> fields were added in a future DPDK release.
>>>
>>> Fixes: 436b3a6b6e62 ("ethdev: reserve space in main structs for extension")
>>> Cc: thomas@monjalon.net
>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>> ---
>>>  lib/ethdev/rte_ethdev.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 41 insertions(+)
>>>
>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>>
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
> 
> Applied to dpdk-next-net/main, thanks.
>

some unit tests are failing with this patch, both iol and github actions
reports it, need to investigate the root cause.

Based on findings, we may need to drop the patch from next-net, fyi.

Re: [PATCH] ethdev: validate reserved fields

[Ferruh Yigit]

On 9/21/2023 5:33 PM, Ferruh Yigit wrote:
> On 9/21/2023 4:12 PM, Ferruh Yigit wrote:
>> On 5/26/2023 9:15 AM, Bruce Richardson wrote:
>>> On Thu, May 25, 2023 at 01:39:42PM -0700, Stephen Hemminger wrote:
>>>> The various reserved fields added to ethdev could not be
>>>> safely used for future extensions because they were never
>>>> checked on input. Therefore ABI would be broken if these
>>>> fields were added in a future DPDK release.
>>>>
>>>> Fixes: 436b3a6b6e62 ("ethdev: reserve space in main structs for extension")
>>>> Cc: thomas@monjalon.net
>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>>> ---
>>>>  lib/ethdev/rte_ethdev.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 41 insertions(+)
>>>>
>>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>>>
>>
>> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
>>
>> Applied to dpdk-next-net/main, thanks.
>>
> 
> some unit tests are failing with this patch, both iol and github actions
> reports it, need to investigate the root cause.
> 
> Based on findings, we may need to drop the patch from next-net, fyi.
> 

Unit test failures caused by segfault, because in
'rte_eth_rx_queue_setup()' & 'rte_eth_tx_queue_setup()' accepts
'rx_conf' & 'tx_conf' to be NULL, but checks doesn't take this into account.

Adding "rx_conf != NULL && (..)" check for Rx, and similar for Tx.

I will update in next-net, and force push.

Re: [PATCH] ethdev: validate reserved fields

[Ferruh Yigit]

On 5/25/2023 9:39 PM, Stephen Hemminger wrote:
> The various reserved fields added to ethdev could not be
> safely used for future extensions because they were never
> checked on input. Therefore ABI would be broken if these
> fields were added in a future DPDK release.
> 
> Fixes: 436b3a6b6e62 ("ethdev: reserve space in main structs for extension")
> Cc: thomas@monjalon.net
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/ethdev/rte_ethdev.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 4d0325568322..4f937a1914c9 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -1228,6 +1228,25 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>  	/* Backup mtu for rollback */
>  	old_mtu = dev->data->mtu;
>  
> +	/* fields must be zero to reserve them for future ABI changes */
> +	if (dev_conf->rxmode.reserved_64s[0] != 0 ||
> +	    dev_conf->rxmode.reserved_64s[1] != 0 ||
> +	    dev_conf->rxmode.reserved_ptrs[0] != NULL ||
> +	    dev_conf->rxmode.reserved_ptrs[1] != NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Rxmode reserved fields not zero\n");
> +		ret = -EINVAL;
> +		goto rollback;
> +	}
> +
> +	if (dev_conf->txmode.reserved_64s[0] != 0 ||
> +	    dev_conf->txmode.reserved_64s[1] != 0 ||
> +	    dev_conf->txmode.reserved_ptrs[0] != NULL ||
> +	    dev_conf->txmode.reserved_ptrs[1] != NULL) {
> +		RTE_ETHDEV_LOG(ERR, "txmode reserved fields not zero\n");
> +		ret = -EINVAL;
> +		goto rollback;
> +	}
> +
>


No objection on validating reserved fields, but if any application
passing not zero values before (I think this was possible), it will
break with this change, so can we get this patch in this release?

Or should it need to wait ABI break release?
If we will wait v23.11, perhaps this should be applied to all structs
with reserved fields, and may be good to add a deprecation notice in
this release, what do you think?

Re: [PATCH] ethdev: validate reserved fields

[Stephen Hemminger]

On Tue, 6 Jun 2023 16:24:36 +0100
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> No objection on validating reserved fields, but if any application
> passing not zero values before (I think this was possible), it will
> break with this change, so can we get this patch in this release?
> 
> Or should it need to wait ABI break release?
> If we will wait v23.11, perhaps this should be applied to all structs
> with reserved fields, and may be good to add a deprecation notice in
> this release, what do you t

Yes, do this in 23.11 (early in merge cycle).
Did ethdev because it is heavily used, and easy to test and validate.

Re: [PATCH] ethdev: validate reserved fields

[Ferruh Yigit]

Recheck-request: iol-unit-testing, iol-x86_64-unit-testing