|WARNING| [v3, 20/20] common/cnxk: fix TM link config selection in debug dump

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-15

# DPDK Patch Review: series.patch

## SUMMARY

Reviewed 20 patches from the cnxk driver patchset v3. The review focuses on correctness bugs (resource leaks, use-after-free, race conditions), C coding style violations, and API/documentation requirements per AGENTS.md guidelines.

**Patches with no findings:** 02, 03, 04, 05, 09, 10, 11, 14, 15, 18, 19, 20.

**Patches with findings:** 01, 06, 07, 08, 12, 13, 16, 17.

---

## PATCH 01/20: net/cnxk: update mbuf next field for multi segment

### Errors

**Use-after-free on m_next** (Correctness Bug -- Report at >=50% confidence)

In `cn9k_nix_prepare_mseg()` around line 671 of the patch:
```c
if (likely(!m_next))
    goto done;

if (!(flags & NIX_TX_OFFLOAD_MBUF_NOFF_F)) {
    m->next = NULL;
    m->nb_segs = 1;
}
m = m_next;
```

After `m = m_next`, the code enters a loop (`do { ... } while (nb_segs)`). Inside the loop:
```c
m_next = m->next;
...
if (!(flags & NIX_TX_OFFLOAD_MBUF_NOFF_F))
    m->next = NULL;
m = m_next;
```

The assignment `m->next = NULL` is executed, then `m = m_next` assigns the saved `m_next` pointer. However, if `m_next` itself had been freed or its `next` field modified between `m_next = m->next` and the `m->next = NULL` assignment, this could reference a stale pointer.

**Why this matters**: If `m->next` pointed to a segment that was freed by another thread or corrupted before `m = m_next`, the subsequent `m = m_next` assigns a freed pointer, and the loop continues to access freed memory (`m->next`, `m->buf_iova`, etc.).

**Suggested fix**: Verify that no code path between `m_next = m->next` and `m = m_next` can free `m_next`. If this is a Tx completion path and `m_next` is from an mbuf chain that should not be concurrently freed, add a comment explaining the ownership model. Otherwise, ensure `m_next` is validated before dereferencing.

**Confidence**: ~60% (plausible depending on concurrency model; DPDK Tx typically does not free mbufs until after send, but the patch modifies reset logic and could expose a race).

---

### Warnings

**Missing explanation in commit message**

The commit message states:
> "This reset these field for multi-segment mbufs on cn9k platform."

**Why this matters**: The message does not explain *why* these fields must be reset, or what bug this fixes. The patch adds conditional checks for `NIX_TX_OFFLOAD_MBUF_NOFF_F` but does not define what MBUF_NOFF means or when it is set.

**Suggested fix**: Expand the commit message to explain the requirement from `rte_mbuf_raw_reset_bulk()` (mentioned in the message but not in the code), and clarify why `next` and `nb_segs` were *not* being reset before this patch. For example: "Prior to this patch, multi-segment mbufs reused after Tx completion retained stale `next` pointers, causing MBUF_NOFF to fail validation. Now reset these fields unless MBUF_NOFF is disabled."

---

## PATCH 06/20: net/cnxk: reserve memory for lookup mem at probe

### Errors

**Resource leak on cnxk_nix_fastpath_lookup_mem_get() failure** (Correctness Bug)

In `cnxk_eth_dev_init()` around line 2224:
```c
/* Reserve memory for lookup_memory */
if (!cnxk_nix_fastpath_lookup_mem_get()) {
    plt_err("Failed to reserve lookup memory");
    rc = -ENOMEM;
    goto dev_fini;
}
```

**Why this matters**: If `cnxk_nix_fastpath_lookup_mem_get()` returns NULL, the error path is `goto dev_fini`, which presumably calls `dev_fini()`. However, there is no indication that this releases the `mbox_vfpf` and `mbox_vfpf_up` resources initialized earlier in `dev_init()`. If `dev_init()` succeeded up to the NPA LF init and `nix_lf_init()` calls, those resources would leak on this error path.

**Suggested fix**: Trace `dev_init()` error paths to ensure `goto dev_fini` properly cleans up all resources allocated before the lookup memory reservation. If `dev_fini()` does not release mailbox/VF resources, change the goto target to a cleanup label that does (e.g., `goto mbox_unregister` or add a new label that calls `mbox_fini()` for both mailboxes).

**Confidence**: ~70% (depends on what `dev_fini()` actually releases; the patch to `dev_init()` in PATCH 16 suggests mailbox cleanup was missing, supporting this finding).

---

## PATCH 07/20: drivers: add support for devargs skip size

### Errors

**Integer overflow on skip_size_info left shift** (Correctness Bug)

In `cnxk_eventdev_devargs.c` around line 326:
```c
uint16_t skip_size_info = 0;
...
rte_kvargs_process(kvlist, CNXK_SKIP_SIZE_INFO, &parse_skip_size_info,
                   &skip_size_info);
```

Then later:
```c
dev->npc.skip_size = skip_size_info;
```

The `parse_skip_size_info()` function does:
```c
val = strtoul(value, NULL, 0);
if (errno || val > 255)
    return -EINVAL;
*(uint16_t *)extra_args = val;
```

**Why this matters**: The parsed value is stored in `uint16_t skip_size_info`, then assigned to `dev->npc.skip_size` (which is `uint8_t` per the patch adding it to `roc_npc`). However, `skip_size_info` is `uint16_t`, and the devargs string could specify a value like `0x100`, which passes the `> 255` check only if `val` is still an unsigned type at that point. But `strtoul` returns `unsigned long`, and the check `val > 255` would fail if `val` is cast to `uint16_t` before the check. This is not the bug.

Actually, re-reading: the code is correct. The check is `val > 255` where `val` is `uint32_t` (declared in the function). The assignment `*(uint16_t *)extra_args = val` truncates to 16 bits, but since `val` is guaranteed `<= 255`, no overflow occurs. Then `dev->npc.skip_size = skip_size_info` is safe because `skip_size_info <= 255`.

**Correction**: No overflow bug here. The code is correct.

---

## PATCH 08/20: net/cnxk: update inbound SA pkind for skip size

### Warnings

**Missing error handling on roc_npc_skip_size_pkind_get()** (Low confidence -- do not flag)

In `cn10k_ethdev_sec.c` around line 856:
```c
rc = roc_npc_skip_size_pkind_get(&dev->npc);
if (rc >= 0)
    inb_sa_dptr->w0.s.pkind = rc;
```

The function `roc_npc_skip_size_pkind_get()` returns `-1` on error (per PATCH 07 implementation). The code checks `rc >= 0` and proceeds only if valid. If `rc < 0`, the pkind is not updated.

**Why this might be acceptable**: If skip_size is not configured, `rc == -1` is expected, and leaving `pkind` at its default (0 or whatever was set earlier) is correct. The function is not required to succeed; it only updates the pkind when skip_size is active.

**Conclusion**: No issue here. The check is correct.

---

## PATCH 12/20: net/cnxk: fix unsigned integer underflow in LSO calculation

### Warnings

**Correctness of ternary vs. mask-based selection**

The patch replaces:
```c
mask = -(!w1.il3type);
lso_sb = (mask & w1.ol4ptr) + (~mask & w1.il4ptr) + m->l4_len;
```

with:
```c
lso_sb = (w1.il3type ? w1.il4ptr : w1.ol4ptr) + m->l4_len;
```

**Why this matters**: The original code: if `w1.il3type` is non-zero, `mask = -0 = 0`, so `lso_sb = (0 & ol4ptr) + (~0 & il4ptr)` = `il4ptr`. If `w1.il3type` is zero, `mask = -1 = 0xFFFFFFFFFFFFFFFF`, so `lso_sb = (~0 & ol4ptr) + (0 & il4ptr)` = `ol4ptr`. The ternary does: if `il3type` is non-zero, pick `il4ptr`; else `ol4ptr`. **These are semantically identical.**

**Why the patch is correct**: The `-(!w1.il3type)` assigns `-1` to a `uint64_t`, which is defined as `UINT64_MAX`, but Coverity flags this as an unsigned underflow. The ternary eliminates the `-1` constant, so Coverity is satisfied. The logic is preserved.

**Conclusion**: No issue here. The patch is a correct silent semantic no-op to suppress a static analysis warning.

---

## PATCH 13/20: net/cnxk: derive ethdev from SA for inbound CPT CQ events

### Errors

**NULL pointer dereference on eth_dev derivation** (Correctness Bug)

In `cn20k_ethdev_sec.c` around line 565:
```c
if (type == NIX_INL_INB_CPT_CQ) {
    struct cn20k_inb_priv_data *inb_priv;

    inb_priv = roc_nix_inl_ow_ipsec_inb_sa_sw_rsvd(args);
    if (inb_priv->eth_sec && inb_priv->eth_sec->eth_dev) {
        eth_dev = inb_priv->eth_sec->eth_dev;
    } else {
        plt_err("Inbound CPT CQ event: no eth_dev in SA priv");
        return;
    }
}
```

**Why this matters**: If `inb_priv` is NULL (because `roc_nix_inl_ow_ipsec_inb_sa_sw_rsvd(args)` returns NULL when `args` is invalid), the subsequent access `inb_priv->eth_sec` will dereference NULL.

**Suggested fix**: Check `if (inb_priv == NULL)` before accessing `inb_priv->eth_sec`.

```c
inb_priv = roc_nix_inl_ow_ipsec_inb_sa_sw_rsvd(args);
if (inb_priv == NULL) {
    plt_err("Inbound CPT CQ event: NULL SA private data");
    return;
}
if (inb_priv->eth_sec == NULL || inb_priv->eth_sec->eth_dev == NULL) {
    plt_err("Inbound CPT CQ event: no eth_dev in SA priv");
    return;
}
eth_dev = inb_priv->eth_sec->eth_dev;
```

**Confidence**: ~75% (depends on whether `roc_nix_inl_ow_ipsec_inb_sa_sw_rsvd()` can return NULL; if `args` is guaranteed valid, this is a false positive, but the error message "no eth_dev in SA priv" suggests the code already expects this pointer to be potentially invalid, so the NULL check on `eth_sec` should also cover `inb_priv`).

---

## PATCH 16/20: common/cnxk: fix Klocwork static analysis issues

### Errors

**Resource leak on dev_init() early error paths** (Correctness Bug)

The patch adds cleanup to `dev_init()` in `roc_dev.c`:
```c
vf_flr_unregister:
    if (!is_vf)
        dev_vf_flr_unregister_irqs(pci_dev, dev);
stop_msg_thrd:
    /* Exiting the mbox sync thread */
    if (dev->sync.start_thread) {
        dev->sync.start_thread = false;
        plt_thread_join(dev->sync.pfvf_msg_thread, NULL);
    }
thread_fail:
    if (pci_dev->max_vfs > 0) {
        pthread_mutex_destroy(&dev->sync.mutex);
        pthread_cond_destroy(&dev->sync.pfvf_msg_cond);
    }
iounmap:
    dev_vf_mbase_put(pci_dev, vf_mbase);
    mbox_fini(&dev->mbox_vfpf);
    mbox_fini(&dev->mbox_vfpf_up);
```

**Why this matters**: The original code did not call `mbox_fini()` on error paths after mailbox init. If `npa_lf_init()` or `dev_lmt_setup()` failed, the mailboxes were left initialized, leaking their resources. The patch correctly adds these calls.

**However**, the patch also adds:
```c
goto vf_flr_unregister;
```
on the `npa_lf_init()` and `dev_lmt_setup()` error paths. But `vf_flr_unregister` calls `dev_vf_flr_unregister_irqs()` only if `!is_vf`. If `is_vf` is true, this is a no-op, and the code falls through to `stop_msg_thrd`. This is correct *if* VFs do not register FLR IRQs. If they do, and the registration happened before this error, the IRQs leak.

**Confidence**: ~60% (depends on whether VFs register FLR IRQs; the patch adds the cleanup, so assume it's correct unless VF FLR registration exists but is not cleaned up).

**Conclusion**: The patch is likely correct. The addition of `mbox_fini()` calls is a clear correctness fix. The VF FLR unregister path is conditional and depends on code not shown in the patch.

---

## PATCH 17/20: common/cnxk: add auth key length check in inbound SA

### Warnings

**Incorrect max size for SHA1/MD5 HMAC key check**

In `cnxk_on_ipsec_inb_sa_create()` around line 1202:
```c
case RTE_CRYPTO_AUTH_MD5_HMAC:
case RTE_CRYPTO_AUTH_SHA1_HMAC:
    if (auth_key_len > (int)sizeof(in_sa->sha1_or_gcm.hmac_key)) {
        plt_err("Auth key len %d exceeds max %zu for algo %u", auth_key_len,
                sizeof(in_sa->sha1_or_gcm.hmac_key), auth_xform->auth.algo);
        return -EINVAL;
    }
```

**Why this might be incorrect**: The `hmac_key` field in `in_sa->sha1_or_gcm` is presumably sized for the maximum key length of SHA1/MD5 HMAC. For SHA1, this is typically 64 bytes (the HMAC block size); for MD5, also 64 bytes. However, the check uses `sizeof(in_sa->sha1_or_gc