|WARNING| [v4,3/3] raw/ntb: add AMD NTB support

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK NTB Patch Review

## Patch 1: raw/ntb: validate vendor ops at probe

### ERRORS

**ntb_vendor_ops_check macro safety**
```c
#define NTB_OPS_REQ(member) do { \
	if (hw->ntb_ops->member == NULL) { \
		NTB_LOG(ERR, "Vendor ops lacks mandatory ." #member); \
		return -EINVAL; \
	} \
} while (0)
```
Missing NULL pointer check on `hw->ntb_ops` before dereferencing it.
If `hw->ntb_ops` is NULL, the macro will crash on the first check.
Add `if (hw->ntb_ops == NULL) return -EINVAL;` before the macro expansion loop.

**Resource leak in ntb_init_hw on ntb_vendor_ops_check failure**
```c
ret = ntb_vendor_ops_check(hw);
if (ret)
	return ret;

ret = (*hw->ntb_ops->ntb_dev_init)(dev);
if (ret) {
	NTB_LOG(ERR, "Unable to init ntb dev.");
	return ret;
}
```
At line 1367, `ntb_vendor_ops_check()` can fail and return after `hw->mw_size` was allocated in the device-specific init (e.g., `intel_ntb_dev_init()` called before the ops check in the old code, now called after).
Actually, reviewing the diff more carefully: the old code called `ntb_dev_init` first (line 1390 in old code), then did the ops check.
The new code does the ops check first (line 1366), then calls `ntb_dev_init` (line 1369).
So the order changed.
The `mw_size` allocation happens inside `ntb_dev_init`.
If `ntb_dev_init` fails after the ops check, the error path at line 1373 returns without freeing anything.
But `ntb_dev_init` itself is vendor code and may allocate `mw_size` there.
Looking at the AMD code in patch 3, `amd_ntb_dev_init()` allocates `mw_size` with `rte_zmalloc` and returns on error without freeing it if allocation succeeds but later steps fail.
However, in patch 3's `amd_ntb_dev_init()`, if the `rte_zmalloc()` succeeds, the only failure path is `return -EINVAL` without freeing `ntb->mw_size`.
But wait, the code does not have a later failure after the `rte_zmalloc()` in `amd_ntb_dev_init()`.
So this is not a leak in patch 1.

Re-reading patch 1: the removal of scattered NULL checks is correct because the ops are now validated once at probe.
However, the `ntb_vendor_ops_check` macro dereferences `hw->ntb_ops` without checking it first.
That is the error.

**mw_set_trans removed but still called in patch 1**
Looking at the ops check in patch 1:
```c
NTB_OPS_REQ(mw_set_trans);
```
Line 1228 in patch 1 adds `mw_set_trans` to the required ops list.
But patch 2 removes it.
So patch 1 still requires `mw_set_trans`, which is correct for Intel (it is present in `intel_ntb_ops` in patch 1), but patch 2 will remove this requirement.
This is not an error in patch 1 in isolation.

### WARNINGS

None.

---

## Patch 2: raw/ntb: generalize framework for multiple vendors

### ERRORS

**Resource leak: rte_pmd_ntb_get_mem_align does not validate dev**
```c
RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pmd_ntb_get_mem_align, 26.11)
uint64_t
rte_pmd_ntb_get_mem_align(uint16_t dev_id, uint32_t mw_id, uint64_t mw_len)
{
	struct rte_rawdev *dev;
	struct ntb_hw *hw;

	if (dev_id >= RTE_RAWDEV_MAX_DEVS)
		return 0;
	dev = rte_rawdev_pmd_get_dev(dev_id);
	if (dev->dev_private == NULL)
		return 0;
```
At line 724, `dev` is not checked for NULL before dereferencing `dev->dev_private`.
`rte_rawdev_pmd_get_dev()` can return NULL if the device is not allocated.
Add `if (dev == NULL) return 0;` before the `dev->dev_private` check.

**Malformed rx stream descriptor overflow - incomplete check**
```c
for (nb_rx = 0; nb_rx < count; nb_rx++) {
	i = 0;
	while (true) {
		if (unlikely(nb_mbufs >= rxq->nb_rx_desc)) {
			NTB_LOG(ERR, "Malformed rx stream (no EOP); "
				"aborting to avoid desc overflow.");
			goto end_of_rx;
		}
		rx_item = rxq->rx_used_ring + rxq->last_used;
```
At line 608, the check `nb_mbufs >= rxq->nb_rx_desc` guards against overflow, but `nb_mbufs` is only incremented later in the same loop (line not shown in diff context, but implied by the fact that this is a new guard).
However, `nb_mbufs` is declared at the start of the function and is used to count the total descriptors consumed across all received packets in the outer loop.
The check needs to be `if (unlikely(nb_mbufs >= count))` to prevent overflowing the caller's `bufs` array, not `rxq->nb_rx_desc`.
The descriptor ring size is `rxq->nb_rx_desc`, but the caller passed `count` mbufs, so the overflow is in the `bufs` array, not the descriptor ring.
Re-reading the code: `nb_mbufs` is incremented each time a descriptor is consumed, and `bufs[nb_mbufs]` is written.
If `nb_mbufs` reaches `count`, the next write to `bufs[nb_mbufs]` is out of bounds.
So the check should be `if (unlikely(nb_mbufs >= count))`, not `rxq->nb_rx_desc`.
The current check is wrong.

**Missing error check on mem_align op result**
In `ntb_fwd.c` at line 1162:
```c
align = rte_pmd_ntb_get_mem_align(dev_id, mz_id, mz_len);
mz = rte_memzone_reserve_aligned(mz_name, mz_len, socket_id,
			RTE_MEMZONE_IOVA_CONTIG, align);
```
`rte_pmd_ntb_get_mem_align()` returns 0 on error (invalid `dev_id`, `mw_id`, or NULL dev).
An alignment of 0 is invalid for `rte_memzone_reserve_aligned()` and will cause undefined behavior.
Add a check: `if (align == 0) { error; }` before calling `rte_memzone_reserve_aligned()`.

### WARNINGS

**Missing release notes for ntb_fwd example change**
The patch modifies `examples/ntb/ntb_fwd.c` to use the new `rte_pmd_ntb_get_mem_align()` API.
This is an API usage change in an example that should be mentioned in the release notes under "Updated Examples" or similar, but the release notes only mention the API change itself, not the example update.

---

## Patch 3: raw/ntb: add AMD NTB support

### ERRORS

**Resource leak in amd_ntb_dev_init on sideinfo check failure**
```c
ntb->mw_size = rte_zmalloc("ntb_mw_size",
			   ntb->mw_cnt * sizeof(uint64_t), 0);
if (ntb->mw_size == NULL) {
	NTB_LOG(ERR, "Cannot allocate memory for mw size.");
	return -ENOMEM;
}
```
At line 63, `ntb->mw_size` is allocated.
All paths after this point that return an error must free `ntb->mw_size`.
However, there are no error paths after this allocation in `amd_ntb_dev_init()` as written, so this is not a leak in this patch.

**File descriptor leak in amd_ntb_read_lnksta_sysfs**
```c
fd = open(config_path, O_RDONLY);
if (fd < 0)
	return -errno;

if (pread(fd, &status, sizeof(status), RTE_PCI_STATUS) !=
    sizeof(status))
	goto out;
...
out:
	close(fd);
	return ret;
```
At line 250, `open()` is called.
All paths must close `fd`.
The error path at line 254 does `goto out`, which closes `fd` at line 279.
All other paths also goto out, so this is correct.
No leak.

**amd_ntb_mw_set_trans hardcoded memzone check**
```c
if (size > mw_size) {
	NTB_LOG(ERR, "Set translation size 0x%" PRIx64 " exceeds mw "
		"size 0x%" PRIx64, size, mw_size);
	return -EINVAL;
}
```
At line 114, the check compares `size` against `mw_size` (the BAR length).
But the function is called with `mw_len = ntb->mz[i]->len` from the handshake, which is the memzone length, not the BAR length.
If the memzone is smaller than the BAR, this check is too strict.
However, the memzone length is what the application requested, and the translation should be set to the memzone length, not the full BAR length.
So the check is correct: the translation size (memzone length) must not exceed the BAR size.
Not an error.

**amd_ntb_read_upstream_link_status path traversal**
```c
snprintf(path, sizeof(path),
	 "/sys/bus/pci/devices/%04x:%02x:%02x.%x",
	 ntb->pci_dev->addr.domain, ntb->pci_dev->addr.bus,
	 ntb->pci_dev->addr.devid, ntb->pci_dev->addr.function);

if (realpath(path, real) == NULL)
	return -errno;

for (i = 0; i < 2; i++) {
	p = strrchr(real, '/');
	if (p == NULL || p == real)
		return -ENOENT;
	*p = '\0';
}

snprintf(path, sizeof(path), "%s/config", real);
return amd_ntb_read_lnksta_sysfs(path, link_status);
```
At line 295, the path is constructed from PCI address components.
At line 299, `realpath()` is called, which prevents symlink attacks.
At line 302, the path is walked up two levels by stripping trailing components.
At line 308, `/config` is appended.
This is safe because `realpath()` has already resolved symlinks and canonicalized the path, and the only appended string is a literal constant.
No path traversal vulnerability.

**amd_handle_hw_event fall-through annotation**
```c
case AMD_PEER_D0_EVENT:
	ack = rte_read32((char *)ntb->hw_addr + AMD_PMESTAT_OFFSET);
	if (ack & 0x1)
		NTB_LOG(INFO, "D0 wakeup completed for NTB.");
	/* fall through to ack the SMU */
	/* Falls through. */
case AMD_PEER_RESET_EVENT:
```
At line 582, two fall-through comments are present: one in the old style, one in the new style.
Only one is needed.
The `/* Falls through. */` comment is the DPDK standard (see coding guidelines).
Remove the `/* fall through to ack the SMU */` comment as redundant.
Actually, the first comment explains *why* the fall-through happens, and the second is the mechanical annotation.
Both are acceptable per DPDK style.
Not an error.

**amd_handle_hw_event missing break in default case**
```c
default:
	NTB_LOG(ERR, "Unknown interrupt event 0x%" PRIx32, event);
	break;
```
At line 592, the `default` case logs an error and breaks.
This is correct.

### WARNINGS

**Use of __rte_unused in function parameters**
```c
static uint64_t
amd_ntb_get_mem_align(const struct rte_rawdev *dev __rte_unused,
		      uint32_t mw_id __rte_unused, uint64_t mw_len)
```
At line 609, `dev` and `mw_id` are marked `__rte_unused`.
The function does not use these parameters.
This is correct and not a warning.

**AMD vendor ID defined in both driver and usertools**
The patch adds `NTB_AMD_VENDOR_ID` in `ntb_hw_amd.h` and also updates `usertools/dpdk-devbind.py` with the same vendor ID `1022`.
This is correct and not a warning.

---

## Patch 1 (revisited): Missing ops validation error

Re-reading patch 1, the `ntb_vendor_ops_check()` function is added, and it dereferences `hw->ntb_ops` immediately:
```c
static int
ntb_vendor_ops_check(const struct ntb_hw *hw)
{
#define NTB_OPS_REQ(member) do { \
	if (hw->ntb_ops->member == NULL) {
```
At line 1318, `hw->ntb_ops->member` is accessed.
If `hw->ntb_ops` is NULL, this will crash.
Add a check before the macro expansion:
```c
if (hw->ntb_ops == NULL) {
	NTB_LOG(ERR, "Vendor ops table is NULL");
	return -EINVAL;
}
```

---

## Summary of Findings

### Patch 1/3: ERRORS

1. **ntb_vendor_ops_check missing NULL check on hw->ntb_ops**
   Add `if (hw->ntb_ops == NULL) return -EINVAL;` before dereferencing `hw->ntb_ops->member`.

### Patch 2/3: ERRORS

1. **rte_pmd_ntb_get_mem_align missing NULL check on dev**
   Line 724: Add `if (dev == NULL) return 0;` before `dev->dev_private`.

2. **Malformed rx stream check uses wrong bound**
   Line 608: The check should be `if (unlikely(nb_mbufs >= count))` to prevent overflowing the caller's `bufs` array, not `rxq->nb_rx_desc`.

3. **ntb_fwd.c missing error check on rte_pmd_ntb_get_mem_align**
   Line 1162 in ntb_fwd.c: `align` can be 0 on error, which is invalid for `rte_memzone_reserve_aligned()`.
   Add `if (align == 0) { error; }`.

### Patch 2/3: WARNINGS

1. **Missing release notes for example update**
   The `examples/ntb/ntb_fwd.c` change should be mentioned in release notes under "Updated Examples" or similar.

### Patch 3/3: No errors found in code correctness.

The `/* fall through to ack the SMU */` comment is explanatory and acceptable alongside `/* Falls through. */`.

---

Review-Result: ERROR