|SUCCESS| [v2] ethdev: add buffer size parameter to rte_eth_dev_get_name_by_port()

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164556

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-29

git sha: (not a git commit, reviewing patch file series.patch)
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH v2] ethdev: add buffer size parameter to rte_eth_dev_get_name_by_port()

This patch adds a size parameter to rte_eth_dev_get_name_by_port() to
prevent buffer overflows, replacing strcpy() with strlcpy() and
returning -ERANGE on truncation. The old ABI is preserved via symbol
versioning.

> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index d0273e3f7b..1638cab231 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c

[ ... ]

> @@ -721,12 +721,9 @@ rte_eth_dev_count_total(void)
>  	return count;
>  }
>  
> -RTE_EXPORT_SYMBOL(rte_eth_dev_get_name_by_port)
> -int
> -rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
> +static int
> +eth_dev_get_name_by_port(uint16_t port_id, char *name, size_t size)
>  {
> -	char *tmp;
> -
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  
>  	if (name == NULL) {

[ ... ]

> +	if (size == 0) {
> +		RTE_ETHDEV_LOG_LINE(ERR,
> +			"Cannot get ethdev port %u name with zero-size buffer", port_id);
> +		return -EINVAL;
> +	}
> +
>  	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
> -	/* shouldn't check 'rte_eth_devices[i].data',
> -	 * because it might be overwritten by VDEV PMD */
> -	tmp = eth_dev_shared_data->data[port_id].name;
> +	/*
> +	 * Use the shared data name rather than rte_eth_devices[].data->name
> +	 * because VDEV PMDs may overwrite the per-process data pointer.
> +	 */
> +	size_t n = strlcpy(name, eth_dev_shared_data->data[port_id].name, size);
>  	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());

Does this code access eth_dev_shared_data->data[port_id].name safely?
The spinlock protects the name during the strlcpy() call, but does DPDK
guarantee that the name buffer at eth_dev_shared_data->data[port_id]
itself is allocated and stable after RTE_ETH_VALID_PORTID_OR_ERR_RET()
passes? If a port can be removed between validation and the lock, does
this risk accessing freed or invalid memory?

[ ... ]

> +	if (n >= size) {
> +		RTE_ETHDEV_LOG_LINE(ERR,
> +				    "ethdev port %u name exceeds buffer size %zu",
> +				    port_id, size);
> +		return -ERANGE;
> +	}

Does this code match strlcpy() semantics correctly? The function returns
-ERANGE when n >= size, but strlcpy() returns the length of the source
string, not the number of bytes copied. If the source name in shared
data is exactly size-1 characters long, strlcpy() returns size-1, which
is less than size, but n >= size would be false. However, if the source
is size or longer, strlcpy() returns that length (not size-1), making
n >= size. This appears correct, but can you verify the boundary case
where strlen(source) == size - 1?

> +/* new ABI version: bounded copy using caller-provided size */
> +RTE_DEFAULT_SYMBOL(27, int, rte_eth_dev_get_name_by_port,
> +		   (uint16_t port_id, char *name, size_t size))
> +{
> +	return eth_dev_get_name_by_port(port_id, name, size);
> +}
> +
> +/* old ABI version: delegate to new version with max buffer size */
> +RTE_VERSION_SYMBOL(26, int, rte_eth_dev_get_name_by_port,
> +		   (uint16_t port_id, char *name))
> +{
> +	return eth_dev_get_name_by_port(port_id, name, RTE_ETH_NAME_MAX_LEN);
> +}

The RTE_DEFAULT_SYMBOL and RTE_VERSION_SYMBOL macros appear to be used
correctly for symbol versioning. However, does DPDK guarantee that the
old ABI callers always provide a buffer of at least RTE_ETH_NAME_MAX_LEN
bytes? The old API had no size parameter, so callers might have
allocated smaller buffers. By passing RTE_ETH_NAME_MAX_LEN
unconditionally, does the versioned wrapper risk buffer overflow for old
binaries with undersized buffers?

[ ... ]

> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 3f4d2438e4..17c000dbf2 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h

[ ... ]

> @@ -5691,14 +5691,18 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id);
>   * @param port_id
>   *   Port identifier of the device.
>   * @param name
> - *   Buffer of size RTE_ETH_NAME_MAX_LEN to store the name.
> + *   Buffer to store the name.
> + * @param size
> + *   Size of the buffer pointed to by @p name. Should be at least
> + *   RTE_ETH_NAME_MAX_LEN bytes.
>   * @return
>   *   - (0) if successful.
>   *   - (-ENODEV) if *port_id* is invalid.
> - *   - (-EINVAL) on failure.
> + *   - (-EINVAL) if *name* is NULL or *size* is 0.
> + *   - (-ERANGE) if the buffer is too small for the device name.
>   */
>  int
> -rte_eth_dev_get_name_by_port(uint16_t port_id, char *name);
> +rte_eth_dev_get_name_by_port(uint16_t port_id, char *name, size_t size);

The documentation states that size "should be at least
RTE_ETH_NAME_MAX_LEN bytes" but does not explicitly state whether this
is a hard requirement or a recommendation. Does the function tolerate
smaller buffers and truncate gracefully (returning -ERANGE), or should
the documentation be more explicit about the minimum required size for
successful operation?