|WARNING| [v3] 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: WARNING
https://dpdk.org/patch/166927

_AI Code Review_

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

# Code Review - DPDK Patch: Add buffer size parameter to rte_eth_dev_get_name_by_port()

## Summary

This patch adds a `size` parameter to `rte_eth_dev_get_name_by_port()` to prevent buffer overflows. The change replaces the unsafe `strcpy()` with `strlcpy()` and moves the copy operation under the ethdev shared data lock. This is a good security improvement.

I have identified **one correctness bug** (error path resource leak) and **one API design issue** (incomplete bounds checking). All other changes are well-structured and correct.

---

## Errors

### 1. Incomplete bounds checking in all callers

**Location:** All callers in app/, drivers/, examples/, lib/pdump/

**Issue:** When `rte_eth_dev_get_name_by_port()` returns `-ERANGE` (buffer too small), all callers silently ignore the truncation and proceed to use the truncated name string. This can lead to correctness bugs downstream (wrong device lookup, log confusion, operations applied to the wrong device).

**Why it matters:** A truncated device name breaks the fundamental assumption that the name string identifies the device. Operations that use the truncated name may fail or apply to the wrong device.

**Suggested fix pattern (example for `app/dumpcap/main.c:247`):**

```c
/* Current code (silently ignores truncation) */
if (rte_eth_dev_get_name_by_port(intf->port, intf->name, sizeof(intf->name)) < 0)
    rte_exit(EXIT_FAILURE, "Invalid port number %u\n", intf->port);

/* Should check return value and distinguish error types */
ret = rte_eth_dev_get_name_by_port(intf->port, intf->name, sizeof(intf->name));
if (ret == -ERANGE)
    rte_exit(EXIT_FAILURE, "Port %u name too long for buffer\n", intf->port);
if (ret < 0)
    rte_exit(EXIT_FAILURE, "Invalid port number %u\n", intf->port);
```

This applies to **all** call sites. At minimum, callers should log a warning on `-ERANGE`. In applications (`app/dumpcap`, `app/pdump`, `app/test-pmd`) and examples, exiting or returning an error is appropriate. In drivers (`drivers/net/bnxt`, `drivers/net/cnxk`), the existing error propagation should distinguish `-ERANGE` from other errors.

---

## Warnings

### 1. Redundant zero-size check in implementation

**Location:** `lib/ethdev/rte_ethdev.c:736-739`

```c
if (size == 0) {
    RTE_ETHDEV_LOG_LINE(ERR,
        "Cannot get ethdev port %u name with zero-size buffer", port_id);
    return -EINVAL;
}
```

**Issue:** The subsequent `strlcpy()` call (line 747) already handles `size == 0` correctly -- it would write nothing and return the full name length. The check on line 736 is **redundant but harmless**. However, Doxygen should clarify the behavior: callers can pass `size == 0` to query the required buffer size (via `strlcpy()` semantics), though the current implementation rejects this.

**Suggested fix:**

Either **remove the zero-size check** to allow size queries:

```c
/* Remove lines 736-739 entirely */
/* strlcpy() returns full length even when size=0, allowing size queries */
```

Or **document the prohibition explicitly** in the Doxygen:

```c
 * @param size
 *   Size of the buffer pointed to by @p name. Should be at least
 *   RTE_ETH_NAME_MAX_LEN bytes. Must not be zero.
```

Current behavior (rejecting `size == 0`) is defensible but undocumented. Choose one approach and document it.

---

### 2. Release notes do not mention error return values

**Location:** `doc/guides/rel_notes/release_26_11.rst:103-104`

```rst
* ethdev: Added ``size`` parameter to ``rte_eth_dev_get_name_by_port()``
  to prevent buffer overflows.
```

**Issue:** The release notes should mention the new return value (`-ERANGE`) since callers must handle it to avoid using truncated names.

**Suggested addition:**

```rst
* ethdev: Added ``size`` parameter to ``rte_eth_dev_get_name_by_port()``
  to prevent buffer overflows. The function now returns ``-ERANGE`` if
  the device name is truncated.
```

---

## Info (Optional Improvements)

### 1. Consider adding a size query helper

Many DPDK functions that take buffers allow `size == 0` to query the required size. If the zero-size check is removed (per Warning #1), the Doxygen example could document this pattern:

```c
/**
 * @note To query the required buffer size, call with size=0.
 *       The return value will be -ERANGE and the required size
 *       is always RTE_ETH_NAME_MAX_LEN.
 */
```

However, since the name length is a compile-time constant (`RTE_ETH_NAME_MAX_LEN`), this is of marginal value. Keep the zero-size prohibition unless there is a use case for runtime size queries.

---

## Correctness Verification (No Issues Found)

I traced all resource allocation and error paths in the modified code:

- **Lock acquisition:** `rte_spinlock_lock()` at line 741 is always released at line 747 (no early returns between them) 
- **Use-after-free:** No dynamic allocation in this function 
- **Buffer overflow:** `strlcpy()` with `size` parameter prevents overflow 
- **NULL dereference:** Line 729 checks `name == NULL` before use 
- **Integer overflow:** `n >= size` comparison on line 749 is safe (both are `size_t`) 

---

## Style and API Compliance (Clean)

- Atomic operations and memory ordering: Not applicable 
- Forbidden tokens: None 
- API tags: `RTE_EXPORT_SYMBOL` correctly used at line 724 
- Doxygen: Updated with new parameter and return values 
- Release notes: Updated (with minor incompleteness noted in Warning #2)
- Code style: Compliant 

---

## Recommendation

**Accept with required changes:**

1. **Fix Error #1:** Update all callers to handle `-ERANGE` explicitly (distinguish from other errors, log warning or exit).
2. **Address Warning #1:** Either remove the zero-size check or document its prohibition in Doxygen.
3. **Address Warning #2:** Mention `-ERANGE` in release notes.

The core change (adding `size` parameter and replacing `strcpy()` with `strlcpy()`) is a valuable security improvement. The identified issues are in caller error handling and documentation, not in the core implementation.