|WARNING| net/af_xdp: fix default device plugin path

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary

This patch fixes the default device plugin path for the AF_XDP PMD to use the EAL runtime directory instead of `/tmp`, with fallback to the legacy location for compatibility. The implementation has several correctness issues and style violations that must be addressed.

---

## Errors

### 1. Resource leak on `access()` failure path

**File:** `drivers/net/af_xdp/rte_eth_af_xdp.c`  
**Function:** `get_dflt_dp_path()`

The `access()` call at line 2492 does not check for errors other than `ENOENT`. If `access()` fails with a permission error (`EACCES`) or other error, the code silently falls through to the legacy path and logs a misleading warning. The caller cannot distinguish between "file doesn't exist" and "cannot determine if file exists."

```c
if (access(dp_path, F_OK) == 0)
    return 0;
/* Falls through even on EACCES, EIO, etc. */
```

**Fix:** Check `errno` after `access()` fails. Only fall back to the legacy path when `errno == ENOENT`. For other errors, log the actual error and return failure.

```c
if (access(dp_path, F_OK) == 0)
    return 0;

if (errno != ENOENT) {
    AF_XDP_LOG_LINE(ERR, "Cannot access '%s': %s", dp_path, strerror(errno));
    return -errno;
}

/* Now we know the file doesn't exist; fall back to legacy path */
ret = snprintf(dp_path, size, "%s/%s/%s", DP_LEGACY_BASE_PATH, if_name, entry);
```

---

### 2. `snprintf()` overflow check is incorrect

**File:** `drivers/net/af_xdp/rte_eth_af_xdp.c`  
**Function:** `get_dflt_dp_path()`

Lines 2488, 2496: The overflow check `(size_t)ret >= size` is correct, but the negative return check `ret < 0` is unnecessary -- `snprintf()` cannot return a negative value. However, the real issue is that when `snprintf()` truncates, the code jumps to `too_long`, but the buffer already contains a truncated (null-terminated) path. If `access()` is called on this truncated path and succeeds by chance, the function returns success with a corrupted path.

**Impact:** Could cause the PMD to connect to the wrong socket or use the wrong pinned map if a truncated path happens to exist.

**Fix:** Check the overflow before calling `access()`. Move the size check to immediately after each `snprintf()` and return error before any use of the buffer.

```c
ret = snprintf(dp_path, size, "%s/%s/%s/%s", rte_eal_get_runtime_dir(),
               DP_DIR_NAME, if_name, entry);
if (ret < 0 || (size_t)ret >= size) {
    AF_XDP_LOG_LINE(ERR, "Device plugin path for %s is too long", if_name);
    return -ENAMETOOLONG;
}

if (access(dp_path, F_OK) == 0)
    return 0;

if (errno != ENOENT) {
    AF_XDP_LOG_LINE(ERR, "Cannot access '%s': %s", dp_path, strerror(errno));
    return -errno;
}

ret = snprintf(dp_path, size, "%s/%s/%s", DP_LEGACY_BASE_PATH, if_name, entry);
if (ret < 0 || (size_t)ret >= size) {
    AF_XDP_LOG_LINE(ERR, "Device plugin path for %s is too long", if_name);
    return -ENAMETOOLONG;
}

AF_XDP_LOG_LINE(WARNING, "No '%s' entry for %s below '%s', falling back to '%s'",
    entry, if_name, rte_eal_get_runtime_dir(), DP_LEGACY_BASE_PATH);

return 0;
```

(This also removes the now-unreachable `too_long` label.)

---

### 3. Missing NULL check on `rte_eal_get_runtime_dir()`

**File:** `drivers/net/af_xdp/rte_eth_af_xdp.c`  
**Function:** `get_dflt_dp_path()`

Line 2487: `rte_eal_get_runtime_dir()` can return `NULL` if EAL is not initialized or if the runtime directory could not be determined. Passing `NULL` to `snprintf()` as the `%s` argument is undefined behavior.

**Fix:** Check for `NULL` and either return an error or skip to the legacy path:

```c
const char *runtime_dir = rte_eal_get_runtime_dir();

if (runtime_dir != NULL) {
    ret = snprintf(dp_path, size, "%s/%s/%s/%s", runtime_dir,
                   DP_DIR_NAME, if_name, entry);
    if (ret < 0 || (size_t)ret >= size) {
        AF_XDP_LOG_LINE(ERR, "Device plugin path for %s is too long", if_name);
        return -ENAMETOOLONG;
    }

    if (access(dp_path, F_OK) == 0)
        return 0;

    if (errno != ENOENT) {
        AF_XDP_LOG_LINE(ERR, "Cannot access '%s': %s", dp_path, strerror(errno));
        return -errno;
    }
}

/* Fall back to legacy path */
ret = snprintf(dp_path, size, "%s/%s/%s", DP_LEGACY_BASE_PATH, if_name, entry);
if (ret < 0 || (size_t)ret >= size) {
    AF_XDP_LOG_LINE(ERR, "Device plugin path for %s is too long", if_name);
    return -ENAMETOOLONG;
}

if (runtime_dir != NULL) {
    AF_XDP_LOG_LINE(WARNING, "No '%s' entry for %s below '%s', falling back to '%s'",
        entry, if_name, runtime_dir, DP_LEGACY_BASE_PATH);
}

return 0;
```

---

### 4. Redundant length check in `init_uds_sock()`

**File:** `drivers/net/af_xdp/rte_eth_af_xdp.c`  
**Function:** `init_uds_sock()`

Line 1443: The check `strnlen(dp_path, PATH_MAX) >= sizeof(server->sun_path)` is redundant with the check at line 2625 in `rte_pmd_af_xdp_probe()`. However, the check in `probe()` only applies when `use_cni` is true. When `use_pinned_map` is true, `init_uds_sock()` is not called, so there is no UDS socket address copy and the check in `probe()` is wrong.

The check should remain in `init_uds_sock()` (where the copy actually happens), and the check in `probe()` should be removed or adjusted to only apply when `use_cni` is true.

**Fix:** The check at line 1443 is correct. The check at line 2625 should be removed because:
- When `use_cni` is true, `init_uds_sock()` is called and already validates the length.
- When `use_pinned_map` is true, `init_uds_sock()` is not called, so checking against `sockaddr_un.sun_path` length is wrong -- the path is for a BPF map file, not a socket.

Remove lines 2622-2628.

---

## Warnings

### 1. `strnlen()` called with `PATH_MAX` on already-bounded string

**File:** `drivers/net/af_xdp/rte_eth_af_xdp.c`  
**Lines:** 2574, 2599, 2605

The variables `if_name` and `dp_path` are stack arrays of size `IFNAMSIZ` and `PATH_MAX` respectively. Calling `strnlen(if_name, PATH_MAX)` is unnecessary -- `strlen()` is safe here because the string is already bounded by the array size and the argument parsing has null-terminated it.

Similarly, `strnlen(dp_path, PATH_MAX)` is checking if a `PATH_MAX`-sized buffer contains an empty string. This is better expressed as `dp_path[0] == '\0'` or `strlen(dp_path) == 0` (safe because argument parsing ensures null-termination).

**Suggested fix:**

```c
/* Line 2574 */
if (if_name[0] == '\0') {

/* Lines 2599, 2605 */
if (use_cni && dp_path[0] == '\0') {
if (use_pinned_map && dp_path[0] == '\0') {
```

---

### 2. Missing release notes for behavior change

The patch changes the default `dp_path` lookup behavior, which affects existing deployments. While backward compatibility is maintained via the `/tmp` fallback, this is a significant behavioral change that should be documented in release notes.

**Suggested action:** Add a note to `doc/guides/rel_notes/release_XX_XX.rst` (current release) under "New Features" or "Changes":

```rst
* **af_xdp: Changed default device plugin path lookup.**

  The AF_XDP PMD now looks for device plugin endpoints below the EAL
  runtime directory first, falling back to ``/tmp/afxdp_dp`` for
  compatibility with existing Kubernetes deployments.
```

---

### 3. Documentation does not explain EAL runtime directory location

**File:** `doc/guides/nics/af_xdp.rst`, `doc/guides/howto/af_xdp_dp.rst`

The documentation tells users to "place the socket or pinned map below the EAL runtime directory" but does not explain where that directory is or how to find it. Users unfamiliar with DPDK will not know that this is typically `/var/run/dpdk/rte/<prefix>/` or can be overridden with `--file-prefix`.

**Suggested addition:** Add a note explaining how to determine the runtime directory path, or at least mention that it defaults to `/var/run/dpdk/rte/<file-prefix>/`.

---

## Info

### 1. Consider logging the final path on success

When the PMD builds the default path and finds the endpoint, it does not log which path was actually used. This makes debugging difficult when both the runtime directory and `/tmp` locations exist.

**Suggestion:** Add an `AF_XDP_LOG_LINE(INFO, ...)` after successfully finding the endpoint to show which path is being used.

```c
if (access(dp_path, F_OK) == 0) {
    AF_XDP_LOG_LINE(INFO, "Using device plugin endpoint: %s", dp_path);
    return 0;
}
```

---

### 2. The `too_long` label is now unreachable

After fixing the `snprintf()` checks as suggested in Error #2, the `too_long` label and the error message duplication can be removed. The inline checks are clearer.

---

## Style

All style issues are acceptable. The patch follows DPDK coding conventions for indentation, bracing, and comments. No style violations found.