|WARNING| [16/16] crypto/cnxk: size CN20K IPsec SA by context length

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review - series.patch

I have reviewed the 16-patch series against the AGENTS.md guidelines. Below are my findings organized by severity.

---

## ERRORS

### Patch 1/16: common/cnxk: use context invalidate for cn20k

**Error 1: Missing release notes**
The patch introduces a new API behavior on CN20K (context invalidation replaces CTX flush). This is a significant change affecting IPsec and TLS session APIs and requires a release notes entry documenting the new behavior.

**Error 2: Potential use-after-free in `roc_cpt_lf_ctx_flush()`**
```c
if (inval && roc_model_is_cn20k()) {
    sa = cptr;
    /* Clear bit 58 aop_valid */
    sa[0] &= ~(1ULL << ROC_CN20K_CPT_AOP_VALID_BIT);
    plt_io_wmb();

    ctx_inval.u = 0;
    ctx_inval.s.cptr = ((uintptr_t)sa) >> 7;
    plt_write64(ctx_inval.u, lf->rbase + CPT_LF_CTX_INVAL);
}
```
The `sa` pointer is dereferenced (`sa[0] &= ...`) after being cast from `cptr`, but there is no NULL check on `cptr` before this dereference. While the existing `if (cptr == NULL)` check at the top of `roc_cpt_lf_ctx_flush()` handles the case where `cptr` is NULL for the `inval` parameter, the removed flush paths in `cn20k_ipsec.c` and `cn20k_tls.c` previously called `roc_cpt_lf_ctx_flush()` with `inval=true/false` and could have passed a NULL `cptr` if the SA pointer was invalid. The removal of those calls does not guarantee all callers have validated `cptr`. This is a potential NULL dereference.

**Recommendation:** Add a NULL check before dereferencing `sa[0]` in the CN20K invalidation path, or ensure all callers validate `cptr` and document this requirement.

---

### Patch 4/16: common/cnxk: validate cipher key length in IPsec SA fill

**Error 3: Missing release notes**
The patch rejects previously-accepted NULL-cipher SAs with oversized keys. This is a behavior change that could affect existing applications and should be documented in the release notes.

---

### Patch 6/16: crypto/cnxk: fix CN20K CPT error recovery

**Error 4: Missing Fixes tag for `cnxk_cryptodev_ops.c` change**
The patch adds a second Fixes tag for `13ce9121c3c0` (roc_cpt.c), but the change to `cnxk_cryptodev_ops.c` (resetting `pend_q.head` and `pend_q.tail`) also fixes a bug introduced in a different commit. The commit message should include a Fixes tag for the commit that introduced the pending queue (`pend_q`) or the queue pair reset logic.

**Recommendation:** Add a Fixes tag for the commit introducing the pending queue or document that this is part of the same error recovery fix.

---

### Patch 15/16: crypto/cnxk: add cq_ena devarg for CPT CQ mode

**Error 5: Missing release notes**
The patch introduces a new devarg `cq_ena` that changes the completion reporting mechanism on CN20K. This is a user-visible configuration option and must be documented in the release notes.

**Error 6: RTE_PMD_REGISTER_PARAM_STRING formatting**
```c
RTE_PMD_REGISTER_PARAM_STRING(crypto_cnxk, CNXK_MAX_QPS_LIMIT "=<1-63> " CNXK_CPT_CQ_ENA
							      "=<0-1> " CNXK_CPT_CTX_ILEN "=<0-7>");
```
The macro invocation splits the second line with leading whitespace. While this compiles, it is inconsistent with the single-line style used elsewhere in DPDK. The DPDK style prefers short lists on a single line; if a line exceeds 100 characters, wrap the argument list, not the string literal.

**Recommendation:** Reformat as:
```c
RTE_PMD_REGISTER_PARAM_STRING(crypto_cnxk,
	CNXK_MAX_QPS_LIMIT "=<1-63> " CNXK_CPT_CQ_ENA "=<0-1> " CNXK_CPT_CTX_ILEN "=<0-7>");
```

---

### Patch 16/16: crypto/cnxk: size CN20K IPsec SA by context length

**Error 7: Missing release notes**
The patch changes SA allocation size based on `ctx_ilen`, affecting memory usage and potentially enabling new use cases (e.g., 1 KiB SAs). This should be noted in the release notes.

---

## WARNINGS

### Patch 3/16: crypto/cnxk: enable cq_all and allocate extra CQ line

**Warning 1: Unclear CQ allocation change**
The commit message says "allocate one extra CQ line beyond the HW ring size," but the diff shows:
```c
-	len = PLT_ALIGN(lf->cq_size * (ROC_CPT_CQ_ENTRY_SIZE_UNIT << lf->cq_entry_size), ROC_ALIGN);
+	len = lf->cq_size;
+	if (roc_model_is_cn20k() && lf->cq_all)
+		len++;
+	len = PLT_ALIGN(len * (ROC_CPT_CQ_ENTRY_SIZE_UNIT << lf->cq_entry_size), ROC_ALIGN);
```
This allocates `cq_size + 1` entries *only when* `roc_model_is_cn20k() && lf->cq_all`. However, Patch 15/16 changes this:
```c
-	len = lf->cq_size;
-	if (roc_model_is_cn20k() && lf->cq_all)
-		len++;
+	len = lf->cq_size + 1;
```
The commit message in Patch 15/16 clarifies: "the pending-queue pointer (req_queue) is always positioned at cq_vaddr + 1 entry so that the hardware-managed reserved slot sits at cq_vaddr[0]. Allocate the extra entry unconditionally."

This means Patch 3/16's conditional allocation is a temporary state that is replaced by unconditional allocation in Patch 15/16. The commit message for Patch 3/16 should note this is a partial step or reference the follow-up in Patch 15/16.

**Recommendation:** Clarify in the Patch 3/16 commit message that the extra entry is allocated conditionally here and made unconditional in a later patch, or squash Patch 3/16 and Patch 15/16 allocation changes together.

---

### Patch 10/16: crypto/cnxk: enable ctx cache for cn20k crypto

**Warning 2: Comment does not match code**
```c
static __rte_always_inline bool
hw_ctx_cache_enable(void)
{
	return roc_errata_cpt_hang_on_mixed_ctx_val() || roc_model_is_cn10ka_b0() ||
	       roc_model_is_cn10kb_a0() || roc_model_is_cn20k();
}
```
The function returns `true` for CN20K, but there is no comment explaining *why* CN20K requires the context cache, unlike the errata-based reasons for CN10K. The commit message says "Enable ctx cache for cn20k lookaside crypto" but does not explain the reason.

**Recommendation:** Add a comment or expand the commit message to explain why CN20K requires the context cache (e.g., performance, hardware requirement, or to match the invalidation changes in Patch 1/16).

---

### Patch 15/16: crypto/cnxk: add cq_ena devarg for CPT CQ mode

**Warning 3: Missing documentation update**
The patch adds a new devarg `cq_ena` and documents it in `doc/guides/cryptodevs/cnxk.rst`, which is correct. However, the RTE_PMD_REGISTER_PARAM_STRING in `cnxk_cryptodev_devargs.c` also adds `CNXK_CPT_CTX_ILEN` to the param string:
```c
RTE_PMD_REGISTER_PARAM_STRING(crypto_cnxk, CNXK_MAX_QPS_LIMIT "=<1-63> " CNXK_CPT_CQ_ENA
							      "=<0-1> " CNXK_CPT_CTX_ILEN "=<0-7>");
```
But `CNXK_CPT_CTX_ILEN` was already supported (introduced in Patch 1/16's context), yet the `cnxk.rst` documentation does not describe it. This is a pre-existing documentation gap, not introduced by this patch, but the param string change highlights it.

**Recommendation:** Document `CNXK_CPT_CTX_ILEN` in `cnxk.rst` in a follow-up patch or as part of this series.

---

### General: Missing functional tests for new devargs

**Warning 4:** Patches 3/16, 10/16, and 15/16 introduce or modify behavior controlled by devargs (`cq_all`, `ctx_ilen`, `cq_ena`), but there are no corresponding updates to `app/test` or functional test coverage mentioned in the commit messages. While not strictly required for bug fixes, new configuration options should have test coverage.

**Recommendation:** Add test cases exercising the new devargs or note in the commit message if manual testing was performed.

---

## INFO

### Patch 1/16: Removal of rte_delay_ms(1) calls

The patch removes several `rte_delay_ms(1)` calls that were waiting for CTX flush to complete before invalidating the context. The new CN20K invalidation path does not include these delays, which suggests either:
1. The delays were unnecessary (the flush completed synchronously), or
2. The invalidation mechanism does not require the wait.

This is likely correct (the commit message implies the new mechanism is cleaner), but it is worth noting as a behavioral change that could affect timing in edge cases.

---

### Patch 4/16: Key length validation order

The patch adds a top-level check `if (length > ROC_CTX_MAX_CKEY_LEN)` before the per-algorithm validation. This catches oversized keys for algorithms not explicitly validated (e.g., NULL cipher), which is correct. However, the per-algorithm checks (AES 16/32, 3DES 24) are still performed *after* this check. Since the top-level check is `>` and the per-algorithm checks are `!=`, a key of exactly `ROC_CTX_MAX_CKEY_LEN` (32 bytes) that is not a valid length for the algorithm (e.g., 3DES which requires 24) would pass the top check but fail the per-algorithm check. This is correct behavior, but the order of checks could be confusing.

**Recommendation:** Consider adding a comment clarifying that the top-level check is an overflow guard and per-algorithm checks validate exact lengths.

---

### Patch 15/16: Comment in roc_cpt.c

```c
/* Set completion queue (CQ) base address.
 * On CN20K the pending-queue pointer (req_queue) is always positioned
 * at cq_vaddr + 1 entry so that the hardware-managed reserved slot
 * sits at cq_vaddr[0].  Allocate the extra entry unconditionally.
 */
len = lf->cq_size + 1;
```
This comment is excellent and explains the unconditional allocation. However, it introduces a slight inconsistency with Patch 3/16, where the allocation was conditional. This is not an error (Patch 15/16 supersedes Patch 3/16's allocation logic), but it highlights that Patch 3/16's commit message should have noted the follow-up.

---

### Patches 5/16, 7/16-9/16, 11/16-14/16: Correctness bug fixes

These patches fix correctness bugs (truncated TLS records, oversized SG vectors, trim length bounds, NULL key pointers). The fixes are correct and necessary. No issues identified beyond the missing release notes noted in the Errors section.

---

## SUMMARY

The series contains several **high-value correctness fixes** (Patches 5, 7-9, 11-14) that address real security and stability issues. However, several patches introduce user-visible changes or new features without corresponding release notes, which is a **process violation**.

**Required actions:**
1. Add release notes for Patches 1, 4, 6, 15, 16 documenting the behavior changes and new devargs.
2. Fix the NULL dereference risk in Patch 1/16 `roc_cpt_lf_ctx_flush()`.
3. Add missing Fixes tags for Patch 6/16 (`cnxk_cryptodev_ops.c` change).
4. Fix the RTE_PMD_REGISTER_PARAM_STRING formatting in Patch 15/16.

**Recommended actions:**
1. Clarify the CQ allocation evolution between Patches 3/16 and 15/16 (or squash).
2. Add a comment explaining why CN20K requires ctx cache (Patch 10/16).
3. Document `CNXK_CPT_CTX_ILEN` in `cnxk.rst`.
4. Add test coverage for new devargs.

---

Review-Result: ERROR