|WARNING| [14/14] net/cnxk: fix custom inbound SA condition check

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary

This 14-patch series primarily addresses bug fixes and platform-specific updates for the Marvell cnxk PMD (CN10K/CN20K platforms), focusing on packet handling, interrupt handling, IPsec inline processing, and CN20K platform enablement.

---

## Patch 1: net/cnxk: fix packet length handling

### Errors

1. **Packet length calculation in fragmented reassembly may be incorrect**

   At line 409 (`drivers/net/cnxk/cn20k_rx.h`):
   ```c
   mbuf->pkt_len = l4_off + (fsz_w1 & 0xFFFF) - ts_rx_off;
   ```
   This assignment computes `pkt_len` from `l4_off` and the fragment size word, but `l4_off` itself is derived from CQ words that may not be fully validated for all reassembly failure cases. If `l4_off` or `fsz_w1` are malformed due to hardware error, this could produce an incorrect packet length that downstream code (especially mbuf chaining logic) may not detect.

   **Impact**: Potential incorrect `pkt_len` leading to buffer overruns or underflows if the calculated length exceeds actual buffer capacity.

   **Recommendation**: Add bounds checks on `l4_off` and `fsz_w1` before computing `pkt_len`, especially in reassembly failure paths where hardware-supplied metadata may be unreliable.

2. **Macro rename without corresponding update in documentation or release notes**

   The patch renames several macros (e.g., `ROC_IE_OT_UCC_*` - `ROC_IE_OW_UCC_*`, `ROC_NIX_INL_OT_IPSEC_*` - `ROC_NIX_INL_OW_IPSEC_*`). These changes affect the public API surface (exported via `rte_pmd_cnxk.h` or internal headers used by other drivers).

   **Missing**: Release notes entry documenting the renamed symbols.

   **Recommendation**: Add a release notes entry under "API Changes" listing the renamed macros and stating that they reflect CN20K platform naming conventions.

---

## Patch 2: common/cnxk: disable CPT drop error in CQ

### Info

- **Functional change**: The patch sets `cq_ctx->cpt_drop_err_en = 0` (was `1`), disabling CPT drop error reporting in the completion queue context for CN20K.

- **Rationale not explained**: The commit message states "Disable CPT drop error in CQ context for cn20k platform" but does not explain *why* this error must be disabled. Is this a hardware erratum workaround? Does CPT drop reporting on CN20K produce spurious errors?

- **Recommendation**: Expand the commit message to explain the reason for disabling this error (hardware behavior, false positives, etc.).

---

## Patch 3: common/cnxk: fix NIX QINT count reset

### Errors

1. **Incorrect interrupt count drain logic (fixed by patch, verify correctness)**

   The patch changes from `plt_write64(0, nix_base + NIX_LF_QINTX_CNT(q))` to:
   ```c
   cnt = plt_read64(nix_base + NIX_LF_QINTX_CNT(q));
   plt_write64((uint64_t)(-(int64_t)cnt), nix_base + NIX_LF_QINTX_CNT(q));
   ```
   This pattern writes the negative of the current count to drain it. This is correct **if and only if** the hardware register implements signed decrement semantics. Verify that `NIX_LF_QINTX_CNT` supports signed writes that decrement the count.

   **If the register treats the written value as an unsigned wrap-around**, this could produce unexpected behavior (e.g., if count is 5, writing `-5` (0xFFFFFFFFFFFFFFFB) might be interpreted as setting the count to a very large number).

   **Recommendation**: Confirm with hardware documentation or reference manual that `NIX_LF_QINTX_CNT` supports signed decrement via negative write. If not, the drain logic is still incorrect.

2. **Type casting correctness in count negation**

   ```c
   plt_write64((uint64_t)(-(int64_t)cnt), nix_base + NIX_LF_QINTX_CNT(q));
   ```
   The intermediate cast to `int64_t` before negation is correct, but `cnt` is read as `uint64_t`. If `cnt` has the high bit set (>=2^63), casting to `int64_t` makes it negative, then negating makes it positive again, which is correct. However, if `cnt` is greater than `INT64_MAX`, the behavior is implementation-defined.

   **Recommendation**: Add a comment explaining the signed negation semantics expected by the hardware, or assert that `cnt <= INT64_MAX` if the hardware spec guarantees this.

---

## Patch 4: common/cnxk: update channel mask for cn20k

### Info

- The patch adds `|| roc_model_is_cn20k()` to the condition that sets the CPT channel mask for second-pass traffic. This is a straightforward platform enablement change.

- **No issues found.**

---

## Patch 5: common/cnxk: update macro for cn20k

### Info

- Updates `ROC_IE_OT_SA_LIFE_UNIT_*` to `ROC_IE_OW_SA_LIFE_UNIT_*` for CN20K. Consistent with Patch 1's macro renames.

- **No issues found.**

---

## Patch 6: common/cnxk: fix null deref and irq ack in CPT CQ handler

### Errors

1. **`port_id` set to `UINT32_MAX` for inbound, then used in callback without validation**

   At line 55 (`drivers/common/cnxk/roc_nix_inl_dev_irq.c`):
   ```c
   uint32_t port_id = UINT32_MAX;
   ```
   Later, the inbound branch calls:
   ```c
   inl_dev->work_cb(gw, sa, cq_type, (void *)cq_s, port_id);
   ```
   passing `UINT32_MAX` as `port_id`. If the callback (implemented in the PMD) does not expect or handle `UINT32_MAX`, it may incorrectly index an array or use it as a valid port.

   **Impact**: Potential out-of-bounds access in the PMD's work callback if it uses `port_id` without checking.

   **Recommendation**: Document in the callback API contract that `port_id == UINT32_MAX` indicates inbound traffic where the port must be derived from SA metadata, or add an assertion/check in the callback that `port_id` is valid or `UINT32_MAX`.

2. **`roc_nix` dereferenced in outbound branch without null check**

   At line 87:
   ```c
   roc_nix = (struct roc_nix *)lf->dev->roc_nix;
   if (!roc_nix) {
       plt_nix_dbg("CPT LF dev has no roc_nix");
       goto cq_ack;
   }
   ```
   This is **correct**: the patch adds the null check. However, the subsequent check at line 92:
   ```c
   if (lf->dev != &nix->dev) {
       plt_nix_dbg("CPT LF dev mismatch with nix dev");
       goto cq_ack;
   }
   ```
   dereferences `nix`, which is derived from `roc_nix` via `roc_nix_to_nix_priv(roc_nix)` at line 90. If `roc_nix` is non-NULL but invalid (dangling pointer), this dereference is unsafe.

   **Recommendation**: The code is acceptable if `roc_nix` is guaranteed to be either NULL or a valid pointer. If there is any scenario where `lf->dev->roc_nix` could be a stale pointer after cleanup, add a validity check or document the lifetime guarantees.

---

## Patch 7: common/cnxk: derive mbuf from CPT CQ in inline IRQ path

### Errors

1. **`mbuf` derived from CQ format without validation**

   At lines 111-117 (`drivers/common/cnxk/roc_nix_inl_dev_irq.c`):
   ```c
   case WQE_PTR_CPTR:
       sa = (void *)cq_s->w1.esn;
       wqe = (void *)((uintptr_t)cq_s->w3.comp_ptr & ~0x7ULL);
       break;
   case CPTR_WQE_PTR:
       sa = (void *)cq_s->w3.comp_ptr;
       wqe = (void *)((uintptr_t)cq_s->w1.esn & ~0x7ULL);
       break;
   ```
   The code casts CQ words (`w1.esn`, `w3.comp_ptr`) directly to pointers without verifying that the addresses are within valid memory ranges. If hardware produces a corrupted CQ entry (due to DMA error, hardware bug, or malicious guest in virtualized scenario), these pointers could be arbitrary values, leading to a crash or security issue when dereferenced.

   **Impact**: Potential NULL or wild pointer dereference if CQ data is corrupt.

   **Recommendation**: Add sanity checks on `sa` and `wqe` pointers (e.g., non-NULL, within known memory pool bounds) before passing them to `inl_dev->work_cb()`.

2. **Anti-replay cases set `wqe = NULL` but may still pass to callback**

   At lines 118-124:
   ```c
   case WQE_PTR_ANTI_REPLAY:
       sa = NULL;
       wqe = (void *)((uintptr_t)cq_s->w3.comp_ptr & ~0x7ULL);
       break;
   case CPTR_ANTI_REPLAY:
       sa = (void *)cq_s->w3.comp_ptr;
       wqe = NULL;
       break;
   ```
   Then at line 129:
   ```c
   gw[1] = (uint64_t)(uintptr_t)wqe;
   inl_dev->work_cb(gw, sa, cq_type, (void *)cq_s, port_id);
   ```
   If `wqe` is `NULL` for `CPTR_ANTI_REPLAY`, `gw[1]` will be `0`, and the PMD's work callback receives a NULL mbuf pointer. The callback must handle this case.

   **Verify**: Does the PMD's work callback (in Patch 8) correctly handle `gw[1] == 0` (NULL mbuf)? If not, this will cause a NULL dereference.

   **Recommendation**: Document that the callback may receive NULL mbuf and verify all callback implementations handle this.

---

## Patch 8: net/cnxk: resolve mbuf from CPT CQ format in SSO work cb

### Errors

1. **`free_mbuf` returned but not always acted upon by caller**

   The function `cn20k_eth_sec_post_event()` now returns `bool free_mbuf` (line 450). At line 595 in `cn20k_eth_sec_sso_work_cb()`:
   ```c
   free_mbuf = cn20k_eth_sec_post_event(eth_dev, args, type,
                       (uint16_t)cqs->w0.s.uc_compcode,
                       (uint16_t)cqs->w0.s.compcode, mbuf);

   if (free_mbuf)
       cnxk_pktmbuf_free_no_cache(mbuf);
   ```
   This is correct **if `mbuf` is non-NULL**. But the code at line 592 derives `mbuf` from `gw[1]`, which may be NULL (see Patch 7 issue #2). If `mbuf` is NULL and `free_mbuf` is true, `cnxk_pktmbuf_free_no_cache(NULL)` is called.

   **Check**: Does `cnxk_pktmbuf_free_no_cache()` handle NULL input safely? Looking at the definition (line 414 in the same file), it calls `rte_mbuf_raw_free(mbuf)` without NULL check. `rte_mbuf_raw_free(NULL)` behavior is undefined.

   **Impact**: Potential NULL dereference if `mbuf` is NULL and `free_mbuf` is true.

   **Recommendation**: Add `if (mbuf != NULL && free_mbuf)` check before calling `cnxk_pktmbuf_free_no_cache(mbuf)`.

2. **Missing error check on `eth_dev` in `cn20k_eth_sec_post_event()`**

   At line 509:
   ```c
   if (eth_dev)
       rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_IPSEC, &desc);
   ```
   The function checks `eth_dev` before calling the callback, which is good. However, the function computes `desc.metadata` and other fields even when `eth_dev` is NULL (inbound anti-replay case). If `sa` is NULL and `inb_priv` is NULL, accessing `inb_priv->userdata` would crash.

   **Check**: At line 458, `inb_priv = sa ? roc_nix_inl_ow_ipsec_inb_sa_sw_rsvd(sa) : NULL;`. If `sa` is NULL, `inb_priv` is NULL. Then at line 459, `desc.metadata = inb_priv ? (uint64_t)inb_priv->userdata : 0;`. This is safe.

   **No issue** on closer inspection--the ternary operator handles NULL `inb_priv`.

---

## Patch 9: common/cnxk: update bpid config for cn20k

### Info

- Updates BPID (backpressure ID) mask from 9 bits (`0x1FF`) to 11 bits (`0x7FF`) for CN20K, reflecting expanded hardware field width.

- Adds `cq_ctx->bpid_ext` assignment (line 1261, `drivers/common/cnxk/roc_nix_queue.c`) to populate the extended BPID bits.

- **No issues found.** Changes are consistent with hardware register layout expansion.

---

## Patch 10: net/cnxk: add MSNS inb SA and CN20K CPT result struct

### Errors

1. **Static asserts may fail on compilers without C11 support**

   Lines 513-533 (`drivers/net/cnxk/rte_pmd_cnxk.h`) use `static_assert()`, which is C11. DPDK targets C11, so this is acceptable. However, if the code is ever built with a pre-C11 compiler, these will fail.

   **Recommendation**: No action needed if DPDK enforces C11. If C99 compatibility is required, use `_Static_assert` or compiler-specific macros.

2. **Union `rte_pmd_cnxk_ipsec_hw_sa` size may exceed expectations**

   At line 688, the union adds `struct rte_pmd_cnxk_ipsec_inb_msns_sa inb_msns;`, which is 1KB (per static assert). If code elsewhere assumes the union is the size of the largest non-MSNS member (256 bytes for `inb`/`outb`), this will break.

   **Impact**: Any code that allocates or copies this union may under-allocate if it uses `sizeof(struct rte_pmd_cnxk_ipsec_inb_sa)` instead of `sizeof(union rte_pmd_cnxk_ipsec_hw_sa)`.

   **Recommendation**: Audit all allocations of `union rte_pmd_cnxk_ipsec_hw_sa` to ensure they use the union size, not the non-