|WARNING| [v2,3/3] fib, rib: fix name prefixes

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-22

# Review of DPDK Patch Series

## Patch 1/3: app/crypto-perf: fix asymmetric op pool name size

### No Issues Found

This patch correctly fixes a potential buffer overflow by sizing the `pool_name` buffer to match the `snprintf` limit. The change from hardcoded `32` to `RTE_MEMPOOL_NAMESIZE` and using `sizeof(pool_name)` is the correct pattern, matching what the symmetric path already does.

---

## Patch 2/3: eal: increase memzone name size to 64

### Errors

**ABI break without proper versioning**

This patch changes `RTE_MEMZONE_NAMESIZE` from 32 to 64 and reorders fields in `struct rte_mempool` and `struct rte_ring`. These are ABI breaks that affect all consumers of these structures. The release notes document the changes but the patch does not use ABI versioning macros (`RTE_VERSION_SYMBOL`, `RTE_DEFAULT_SYMBOL`) to maintain backward compatibility. Unless this targets an LTS+1 release where ABI breaks are allowed, this needs proper versioning.

**Missing review of cross-library impact**

The patch increases several derived name size constants (`RTE_RING_NAMESIZE`, `RTE_MEMPOOL_NAMESIZE`, `RTE_STACK_NAMESIZE`, `RTE_RCU_QSBR_DQ_NAMESIZE`) but does not verify that all code using these constants is still correct. For example, any code that hardcodes a smaller size for buffers holding these names would silently truncate. A thorough review of all uses of these constants across the codebase should be documented.

### Warnings

**`struct rte_mempool` field reordering rationale incomplete**

The release notes state that `local_cache` and `ops_index` were previously in the second cache line and are now in the first, but the patch does not demonstrate that this ordering actually improves performance or that these fields are hot-path. The justification should include either performance measurements or a clear analysis of which code paths access these fields. The claim that they are "datapath fields" should be verified against actual usage patterns.

**`struct rte_ring` grows by one cache line**

The release notes state this growth, but the patch does not assess the memory impact. For applications creating many rings, this is a 64-byte-per-ring increase. A note about the memory cost and whether it was considered acceptable would strengthen the justification.

**Release notes formatting**

The release notes use a bullet list with bold terms followed by descriptions. Per AGENTS.md RST style guidelines, this pattern is better expressed as a definition list:

```rst
``struct rte_memzone``
   Grew by 32 bytes.

``struct rte_ring``
   Grew by one cache line.
   The ``memzone`` and ``name`` fields were moved after the size fields
   to keep the datapath fields in the first cache line.

``struct rte_mempool``
   Is unchanged in size, but the fields were reordered...
```

---

## Patch 3/3: fib, rib: fix name prefixes

### Errors

**Incorrect maximum name length computation in error messages**

The error messages in `rte_rib_create()` and `rte_rib6_create()` compute the maximum name length as:
```c
RTE_MEMPOOL_NAMESIZE - sizeof(RIB_MEM_PREFIX)
```

`sizeof()` on a string literal includes the null terminator, so `sizeof("RIB_")` is 5, not 4. The actual available space for the user's name is `RTE_MEMPOOL_NAMESIZE - strlen(RIB_MEM_PREFIX) - 1` (subtract prefix length and 1 for the null terminator). The computed limits in the commit message (53, 52, 49, 47 characters) may be off by one.

**Suggested fix:**
```c
/* RIB_MEM_PREFIX is "RIB_", length 4 */
RTE_MEMPOOL_NAMESIZE - strlen(RIB_MEM_PREFIX) - 1
/* or */
RTE_MEMPOOL_NAMESIZE - 5  /* "RIB_" (4) + null (1) */
```

And update the release notes with the corrected maximum lengths. Verify the arithmetic: if `RTE_MEMPOOL_NAMESIZE` is 64 (from patch 2), then for `RIB_` (4 chars prefix):
- 64 - 4 - 1 = 59, not 53 (unless `RTE_MEMPOOL_NAMESIZE` is still 32 in the code when this patch is applied, in which case dependencies between patches are unclear).

**Missing length check in `rte_fib_create()` and `rte_fib6_create()`**

The FIB functions prepend `"FIB_"` or `"FIB6_"` to the user's name before passing it to RIB, which then prepends its own prefix. The final mempool name is `"RIB_FIB_<name>"` or `"RIB6_FIB6_<name>"`, which could exceed `RTE_MEMPOOL_NAMESIZE` even though the FIB code checks the intermediate name length.

The check in `rte_fib_create()`:
```c
ret = snprintf(mem_name, sizeof(mem_name), FIB_MEM_PREFIX "%s", name);
if (ret < 0 || ret >= (int)sizeof(mem_name)) {
    ...
}
```
only verifies that `"FIB_<name>"` fits in `mem_name` (which is `RTE_FIB_NAMESIZE`). It does not verify that when RIB prepends `"RIB_"`, the result fits in `RTE_MEMPOOL_NAMESIZE`. The RIB code will catch it, but the error message will be confusing (it will say "RIB name FIB_<name> is too long" when the user only provided `<name>` to FIB).

**Suggested fix:** Add a check in FIB that accounts for the RIB prefix:
```c
/* FIB will prepend "FIB_", RIB will prepend "RIB_" */
#define FIB_MAX_NAME_LEN (RTE_MEMPOOL_NAMESIZE - sizeof(FIB_MEM_PREFIX) - sizeof(RIB_MEM_PREFIX))
if (strlen(name) >= FIB_MAX_NAME_LEN) {
    FIB_LOG(ERR, "FIB name %s is too long, limit is %zu characters",
            name, FIB_MAX_NAME_LEN);
    rte_errno = ENAMETOOLONG;
    return NULL;
}
```

### Warnings

**Inconsistent null-terminator accounting in `sizeof()` usage**

The `sizeof(mem_name)` is correct for the `snprintf` size argument (it's the buffer size including null terminator). The `sizeof(RIB_MEM_PREFIX)` is correct for the error message math only if you subtract 1 afterward (see Error above). This inconsistency is easy to get wrong. Using `strlen()` for the prefix length is clearer.

**Release notes list could be a definition list**

The maximum name length limits are presented as a bullet list. Per RST style guidelines, this would be clearer as a definition list:

```rst
RIB
   53 characters (verify this number per the error above).

RIB6
   52 characters.

FIB
   49 characters.

FIB6
   47 characters.
```

(But first fix the numbers if they are incorrect.)

---

## Summary

**Patch 1/3:** Clean, no issues.

**Patch 2/3:** Has ABI break concerns and incomplete justification for performance claims. Release notes formatting could be improved.

**Patch 3/3:** Has correctness bugs in maximum name length computation (`sizeof` vs `strlen`) and missing length validation in FIB for the double-prefixed name. Release notes formatting could be improved.