ethdev: validate reserved fields
Checks
Commit Message
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(+)
Comments
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>
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?
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.
Recheck-request: iol-unit-testing, iol-x86_64-unit-testing
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.
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.
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.
@@ -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;