|WARNING| [v2] net/bnxt: fix parent MAC lookup path handling

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/171203

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-13

# DPDK Patch Review

## Summary

Reviewing patch "net/bnxt: fix parent MAC lookup path handling" which addresses buffer overflow risks and resource leak in `ulp_pmd_get_mac_by_pci()`.

---

## ERRORS

### 1. Missing `globfree()` on `glob()` failure path

**File:** `drivers/net/bnxt/tf_ulp/bnxt_tf_pmd_shim.c`  
**Line:** 152

The patch adds `globfree()` only when `glob()` succeeds (`glob() == 0`), but if `glob()` returns non-zero (failure), the function returns early without calling `globfree()`. According to POSIX, `globfree()` should only be called after successful `glob()`, but the current logic is correct. However, if `glob()` fails, any previously allocated memory in `gres` from a prior failed call is not freed on the early return path.

Actually, reviewing the code more carefully: when `glob()` returns non-zero (failure), the `if` block is not entered and the function returns `-EINVAL` directly. In this case, `globfree()` should NOT be called (per POSIX spec - `globfree()` is only valid after successful `glob()`). The current code is correct for the failure case.

**Retract this - no issue here.**

---

## WARNINGS

### 1. Buffer overflow check has unreachable error path

**File:** `drivers/net/bnxt/tf_ulp/bnxt_tf_pmd_shim.c`  
**Lines:** 147-150

```c
pr = snprintf(path, sizeof(path), PARENT_PHY_INTF_PATH, pci_name);
if (unlikely((size_t)pr >= sizeof(path) || pr < 0)) {
    BNXT_DRV_DBG(ERR, "Error writing path: %s", path);
    return rc;
}
```

The error message attempts to print `path` which may be unterminated or contain garbage if `snprintf()` truncated. When `pr >= sizeof(path)`, `path` contains a truncated string (still null-terminated by `snprintf`), so this is actually safe. When `pr < 0` (encoding error), `path[0]` was set to `\0` by the earlier `memset(path, 0, sizeof(path))`, so printing it is safe but pointless. The message is misleading - it should not print the truncated path as if it were the problem, but rather indicate what was being attempted.

**Suggested fix:**
```c
if (unlikely((size_t)pr >= sizeof(path) || pr < 0)) {
    BNXT_DRV_DBG(ERR, "Path buffer too small for PCI device %s", pci_name);
    return rc;
}
```

### 2. Same buffer overflow check repeated with same issue

**File:** `drivers/net/bnxt/tf_ulp/bnxt_tf_pmd_shim.c`  
**Lines:** 158-162

```c
pr = snprintf(path, sizeof(path), ULP_PRT_MAC_PATH, pci_name, intf_name);
if (unlikely((size_t)pr >= sizeof(path) || pr < 0)) {
    BNXT_DRV_DBG(ERR, "Error writing path: %s", path);
    goto free_glob;
}
```

Same issue as above - error message prints the truncated path instead of describing what failed.

**Suggested fix:**
```c
if (unlikely((size_t)pr >= sizeof(path) || pr < 0)) {
    BNXT_DRV_DBG(ERR, "Path buffer too small for device %s/%s", pci_name, intf_name);
    goto free_glob;
}
```

---

## INFO

### 1. Variable initialization could be deferred

**File:** `drivers/net/bnxt/tf_ulp/bnxt_tf_pmd_shim.c`  
**Line:** 142

```c
int32_t rc = -EINVAL;
```

The variable `rc` is initialized to `-EINVAL` at declaration but is only returned at the end of the function. This is acceptable style per guidelines (initialization at declaration is allowed), but in this case the initial value never changes on any code path, so it could be `return -EINVAL` directly on error paths. Not a problem, just an observation.

### 2. Cast of `snprintf` return to `size_t` is safe

**File:** `drivers/net/bnxt/tf_ulp/bnxt_tf_pmd_shim.c`  
**Lines:** 148, 159

The check `(size_t)pr >= sizeof(path)` first verifies `pr < 0` in the same conditional, so the cast to `size_t` only happens after confirming `pr >= 0`. This is correct - no issue.

---

## POSITIVE OBSERVATIONS

1. The patch correctly addresses buffer overflow risks by replacing `sprintf()` with `snprintf()` and checking for truncation.

2. Resource leak fix: the `globfree()` call is correctly added on all exit paths after successful `glob()`.

3. The use of `unlikely()` on the truncation check is appropriate - these are error paths that should not occur in normal operation.

4. The `goto free_glob` pattern correctly ensures cleanup on all error paths within the `if (glob(...) == 0)` block.

---

Review-Result: WARNING