Re: [PATCH] ethdev: validate reserved fields

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

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?