|WARNING| [v2,2/2] net/nfp: move socket from /tmp to runtime dir

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-07-23

# DPDK Patch Review

## Patch 1/2: net/cnxk: move socket from /tmp to runtime dir

### Errors

None.

### Warnings

**1. Missing release notes**

Changing the socket path is a user-visible behavior change that affects deployments and scripts. This should be documented in the release notes.

**Suggested fix:**
Add an entry to `doc/guides/rel_notes/release_26_07.rst` (or the appropriate current release notes file) under a "Drivers" or "net/cnxk" section:

```rst
* **Updated cnxk PMD control socket location.**

  Moved the eswitch control message socket from ``/tmp/cxk_rep_ctrl_msg_sock``
  to ``<runtime_dir>/cnxk_ctrl_msg_sock`` (e.g., ``/run/dpdk/<prefix>/cnxk_ctrl_msg_sock``)
  to follow the configured EAL prefix and avoid conflicts between DPDK instances.
```

**2. Missing error check on `remove()` in `close_socket()`**

In `cnxk_rep_msg.c`, the `close_socket()` function calls `cnxk_eswitch_ctrl_msg_sock_addr()` but does not check its return value. If path construction fails, `unlink()` is called on an uninitialized `un.sun_path`, which could be arbitrary memory.

**Suggested fix:**
```c
static void
close_socket(int fd)
{
	struct sockaddr_un un;

	close(fd);
	if (cnxk_eswitch_ctrl_msg_sock_addr(&un) == 0)
		unlink(un.sun_path);
}
```

This is already correct in the patch as written (the `if` check is present). Marking this as "no issue" - no further action needed.

---

## Patch 2/2: net/nfp: move socket from /tmp to runtime dir

### Errors

**1. Missing error check on `snprintf()` before `unlink()`**

After the `snprintf()` call that builds `address.sun_path`, if the path is too long, the code logs an error and returns `-EINVAL`. However, the `unlink(address.sun_path)` call occurs **before** the length check. If `snprintf()` truncates, `unlink()` is called on the truncated path, which could remove an unintended file.

**Suggested fix:**
Move the `unlink()` call after the length check:

```c
ret = snprintf(address.sun_path, sizeof(address.sun_path), "%s/nfp_%s",
		rte_eal_get_runtime_dir(), pci_name);
if (ret < 0 || (size_t)ret >= sizeof(address.sun_path)) {
	PMD_CPP_LOG(ERR, "Socket path too long. Service failed.");
	return -EINVAL;
}

unlink(address.sun_path);  /* Move here, after validation */
```

### Warnings

**1. Missing release notes**

Similar to patch 1/2, this changes the socket path in a user-visible way and should be documented.

**Suggested fix:**
Add an entry to the current release notes:

```rst
* **Updated nfp PMD CPP bridge socket location.**

  Moved the CPP bridge service socket from ``/tmp/<pci>`` to
  ``<runtime_dir>/nfp_<pci>`` (e.g., ``/run/dpdk/<prefix>/nfp_<pci>``)
  to follow the configured EAL prefix and avoid conflicts between DPDK instances.
```

**2. PCI name format assumption**

The code assumes `strchr(pf_dev->pci_dev->name, ':')` will always return non-NULL and does not check the result before incrementing the pointer. If the PCI device name does not contain a colon (which should never happen in practice but is not guaranteed by the API), this causes a NULL pointer dereference.

**Suggested fix:**
```c
const char *pci_name = strchr(pf_dev->pci_dev->name, ':');
if (pci_name == NULL) {
	PMD_CPP_LOG(ERR, "Invalid PCI device name. Service failed.");
	return -EINVAL;
}
pci_name++;  /* Skip the ':' */
```

---

## Summary

### Patch 1/2
- **Errors:** None
- **Warnings:** Missing release notes

### Patch 2/2
- **Errors:** `unlink()` called before validating path length
- **Warnings:** Missing release notes; missing NULL check on `strchr()` return value