|SUCCESS| [RFC] 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/162955

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-09

I'll review this patch carefully against the DPDK coding standards and guidelines.

---

**git sha:** Not available (patch from patchwork)  
**Author:** Stephen Hemminger <stephen at networkplumber.org>  
**Subject:** [RFC] 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 adding
proper bounds checking. It uses symbol versioning to maintain ABI
compatibility.

> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 2edc7a362e..586a56bd46 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) {
> @@ -735,19 +732,46 @@ rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
>  		return -EINVAL;
>  	}
>  
> +	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());

In this function, the variable declaration violates DPDK coding style.
Does this declare n in the middle of the function block? According to
the coding style, all variable declarations should appear at the
beginning of the block, not mixed with executable statements.

[ ... ]

> +/* 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);
> +}

Can these symbol versioning macros introduce symbol name conflicts? Both
RTE_DEFAULT_SYMBOL and RTE_VERSION_SYMBOL are generating functions with
the same base name. Does this require careful verification of the
generated assembly to ensure proper symbol resolution occurs?

> diff --git a/lib/pdump/rte_pdump.c b/lib/pdump/rte_pdump.c
> index ac94efe7ff..57c81d4322 100644
> --- a/lib/pdump/rte_pdump.c
> +++ b/lib/pdump/rte_pdump.c
> @@ -736,7 +736,7 @@ pdump_validate_port(uint16_t port, char *name)
>  		return -1;
>  	}
>  
> -	ret = rte_eth_dev_get_name_by_port(port, name);
> +	ret = rte_eth_dev_get_name_by_port(port, name, sizeof(name));

Does this code have an issue with sizeof(name)? Here, name is a
function parameter of type char*, so sizeof(name) will evaluate to the
size of a pointer (typically 8 bytes on 64-bit systems), not the size
of the buffer it points to. Can this cause truncation of port names
longer than 8 characters?