|WARNING| [v3] 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/167538

_AI Code Review_

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

# DPDK Patch Review

## Summary
This patch fixes the default AF_XDP device plugin path lookup to prefer the EAL runtime directory over the legacy `/tmp` location. Overall the implementation is sound, but there are several issues to address.

---

## Errors

### 1. Error path resource leak in `get_dflt_dp_path()`

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

The function calls `access()` which can fail with various errno values (EACCES, ENOENT, etc.). While `access()` does not allocate resources, the function returns -ENAMETOOLONG when snprintf indicates truncation, but this error code may not accurately reflect the actual failure mode when the runtime directory path itself is too long but a valid legacy path exists.

More critically, when the runtime directory candidate is too long, the code silently tries the legacy path. If the legacy path *also* truncates, the function returns -ENAMETOOLONG. However, the dp_path buffer at this point contains a truncated string from the snprintf call. The caller may use this truncated path, which is exactly what the patch aims to prevent.

**Suggested fix:**

```c
static int
get_dflt_dp_path(char *dp_path, size_t size, const char *if_name,
		 const char *entry)
{
	int ret;

	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) {
		/* Runtime dir path doesn't fit, will try legacy */
	} else if (access(dp_path, F_OK) == 0) {
		return 0;
	}

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

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

	return 0;
}
```

---

## Warnings

### 1. Missing check of `get_dflt_dp_path()` return value context

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

The patch correctly checks the return value of `get_dflt_dp_path()` and returns on error. However, the second call (for `use_pinned_map`) uses `sizeof(dp_path)` as the size, while the first call (for `use_cni`) uses the sun_path size. This inconsistency should be documented or made consistent.

For the pinned map case, the path is used with BPF map pinning, which may have different length constraints than unix socket paths. Consider documenting why different sizes are used, or use a consistent bound and check it appropriately at the point of use.

**Suggested addition:**

Add a comment explaining the size difference:

```c
if (use_cni && !strnlen(dp_path, PATH_MAX)) {
	/* UDS path is bounded by sun_path field size */
	ret = get_dflt_dp_path(dp_path,
			RTE_SIZEOF_FIELD(struct sockaddr_un, sun_path),
			if_name, DP_UDS_SOCK);
	...
}

if (use_pinned_map && !strnlen(dp_path, PATH_MAX)) {
	/* Pinned map path is bounded by kernel BPF_OBJ_NAME_LEN (PATH_MAX for pin path) */
	ret = get_dflt_dp_path(dp_path, sizeof(dp_path), if_name, DP_XSK_MAP);
	...
}
```

### 2. Late validation of explicit dp_path for unix sockets

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

The new validation that checks if an explicit `dp_path` fits in `sun_path` occurs *after* the implicit path construction, but it only applies to the `use_cni` case. An explicit path for `use_pinned_map` that is too long would not be caught by this check and could cause issues downstream.

While pinned map paths may have different constraints, the validation should either apply to both cases or document why `use_pinned_map` paths are not length-checked.

**Suggested fix:**

Either add similar validation for the pinned map case, or add a comment explaining why it's not needed:

```c
/*
 * Validate explicit dp_path length for socket addresses.
 * Pinned map paths use filesystem limits, not sun_path.
 */
if (use_cni && strnlen(dp_path, PATH_MAX) >=
	       RTE_SIZEOF_FIELD(struct sockaddr_un, sun_path)) {
	AF_XDP_LOG_LINE(ERR, "'%s' value '%s' is too long for a unix socket address",
			ETH_AF_XDP_DP_PATH_ARG, dp_path);
	return -ENAMETOOLONG;
}
```

### 3. Race condition on `access()` check (TOCTOU)

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

The `access(dp_path, F_OK)` check creates a time-of-check-time-of-use (TOCTOU) race: the file could be deleted between the access() call and when the path is actually used. However, this is a benign race in this context - if the file disappears, the subsequent socket connect or map open will fail with a clear error. The fallback to the legacy path is an optimization, not a security boundary.

This is noted as a warning because while it's not a serious issue in this specific use case, it's worth documenting that the access() check is best-effort and the caller should handle connection failures gracefully.

**Suggested addition:**

Add a comment in the function:

```c
/*
 * Check if runtime directory candidate exists (best effort).
 * TOCTOU is acceptable here: if the file disappears after this check,
 * socket connect or map open will fail cleanly.
 */
if (ret >= 0 && (size_t)ret < size && access(dp_path, F_OK) == 0)
	return 0;
```

---

## Info

### 1. Documentation clarity on bind mount requirement

The new documentation in `af_xdp.rst` and `af_xdp_dp.rst` correctly notes that pinned maps must be bind-mounted into the runtime directory. Consider adding an example command showing how to do this in a Kubernetes deployment, as this is a non-obvious requirement for users migrating from the `/tmp` location.

### 2. Consider logging the selected path

When the runtime directory path is successfully used, consider logging it at INFO level (similar to when the legacy path is used) so administrators can verify which path was selected without enabling debug logging.

**Suggested addition:**

```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 && access(dp_path, F_OK) == 0) {
	AF_XDP_LOG_LINE(INFO, "Using device plugin endpoint: %s", dp_path);
	return 0;
}
```

---

## Positive Observations

1.  Proper use of `RTE_SIZEOF_FIELD` for sun_path size
2.  Release notes correctly document the behavior change
3.  Moved interface name validation before path construction (good defensive ordering)
4.  Appropriate error code (ENAMETOOLONG) for oversized paths
5.  Documentation thoroughly explains the fallback mechanism and upgrade path
6.  Fixes tag references both original commits that established the default path