|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

## PATCH 01/16: common/cnxk: use context invalidate for cn20k

### Errors

**Error: Missing release notes for significant changes**
- The patch removes the manual context flush path (`roc_cpt_lf_ctx_flush` calls) from IPsec and TLS session destroy paths and replaces it with a context invalidate mechanism.
- This is a significant behavioral change that affects session lifecycle and should be documented in release notes.

**Error: Resource leak on error path (cn20k_tls.c:800)**
```c
if (tls->write_sa != NULL)
    rte_free(tls->write_sa);
tls->write_sa = write_sa;
```
- This code at line 800 is executed only after the successful `roc_cpt_ctx_write()`.
- However, if `roc_cpt_ctx_write()` fails earlier at line 777, the function jumps to `sa_dptr_free` which then goes to `sa_cptr_free`, freeing `write_sa` but NOT freeing the old `tls->write_sa` that was allocated in a prior call.
- The old `tls->write_sa` is leaked on the update-failure path.

**Error: Use-after-free potential in cn20k_tls.c**
- In `cn20k_tls_write_sa_create()`, the assignment `tls->write_sa = write_sa` at line 802 is now conditional on success.
- If `roc_cpt_ctx_write()` fails and returns early, `tls->write_sa` retains its old value.
- On the next `cn20k_tls_record_session_update()` call, the free at line 802-803 attempts to free the old pointer again.
- The correct pattern is to free the old SA *before* attempting to create the new one, or to track both old and new separately.

### Warnings

**Warning: Removed code may still be needed on error recovery (cn20k_ipsec.c:297)**
```c
// Removed:
// /* Trigger CTX flush to write dirty data back to DRAM */
// roc_cpt_lf_ctx_flush(lf, sa->in_sa, false);
```
- The comment claimed this was writing dirty data back. Without the flush, is dirty data guaranteed to be visible to the invalidate path?
- The patch does not explain why the flush is no longer needed.

**Warning: Removed delay on error path (cn20k_ipsec.c:323)**
```c
// Removed:
// /* Wait for 1 ms so that flush is complete */
// rte_delay_ms(1);
```
- The delay was ensuring the flush completed before attempting CTX reload.
- With the new invalidate mechanism, is a similar ordering constraint needed?

**Warning: Removed delay on TLS error path (cn20k_tls.c:892, 907)**
- Same concern: the delays ensured the flush completed before reload.
- The patch does not explain why these are safe to remove.

---

## PATCH 02/16: crypto/cnxk: return HW CPTR from cptr_get for cn20k

### Warnings

**Warning: Missing release notes**
- This changes the return value of `rte_pmd_cnxk_crypto_cptr_get()` for CN20K, which is a public API.
- Applications relying on the old behavior (returning the wrapper address) will break.
- Requires release notes and possibly an ABI notice.

---

## PATCH 03/16: crypto/cnxk: enable cq_all and allocate extra CQ line

### Errors

**Error: Off-by-one in CQ allocation sizing (roc_cpt.c:711)**
```c
len = lf->cq_size;
if (roc_model_is_cn20k() && lf->cq_all)
    len++;
```
- The patch says "allocate one extra CQ line beyond the HW ring size."
- But this adds only 1 *entry*, not 1 *line*.
- If CQ entry size is 128B (2^7), then `len * (ROC_CPT_CQ_ENTRY_SIZE_UNIT << lf->cq_entry_size)` does allocate the extra bytes.
- However, the comment and commit message say "line" when the code says "entry." Clarify the units.

**Error: Missing release notes**
- The patch enables a new hardware mode (`cq_all`) that changes completion behavior.
- This is a user-visible change and requires release notes.

---

## PATCH 04/16: common/cnxk: validate cipher key length in IPsec SA fill

### Info

**Info: Three separate key-length checks added**
- `ot_ipsec_sa_common_param_fill()`: checks `length > ROC_CTX_MAX_CKEY_LEN` before memcpy.
- `on_fill_ipsec_common_sa()`: checks `cipher_key_len > sizeof(common_sa->cipher_key)` before memcpy.
- `ow_ipsec_sa_common_param_fill()`: checks `length > ROC_CTX_MAX_CKEY_LEN` before memcpy.
- All three are correct and necessary. The per-algorithm checks (AES, 3DES, etc.) are separate and remain unchanged.

---

## PATCH 05/16: crypto/cnxk: reject truncated TLS records in decrypt

### Info

**Info: Bounds checks added in three places**
- `cn10k_tls_ops.h:263-267` (SGv1 scatter path): checks `pkt_len < tail_len` before computing offset.
- `cn10k_tls_ops.h:329-335` (SGv2 scatter path): same check.
- `cn20k_tls_ops.h:206-212` (SG2 scatter path): same check.
- All correct. The check prevents underflow in `(pkt_len - tail_len)` when passed to `rte_pktmbuf_read()`.

---

## PATCH 06/16: crypto/cnxk: fix CN20K CPT error recovery

### Errors

**Error: Missing Fixes tag**
- The patch claims to fix two commits but does not include `Fixes:` tags.
- Add:
  ```
  Fixes: 13ce9121c3c0 ("common/cnxk: add routines to operate CPT CQ")
  Fixes: 4e8a41acaffb ("crypto/cnxk: support CPT CQ for CN20K")
  ```

**Error: Missing Cc: stable at dpdk.org**
- This is a bug fix for error recovery. It should be backported.

### Warnings

**Warning: Reprogramming CQ base/size on every enable (roc_cpt.c:1153-1163)**
```c
/* Reconfigure CQ base and size, same as cpt_lf_cq_init() */
addr = (uintptr_t)lf->cq_vaddr;
lf_cq_base.s.addr = addr >> 7;
plt_write64(lf_cq_base.u, lf->rbase + CPT_LF_CQ_BASE);
lf_cq_size.s.size = lf->cq_size;
plt_write64(lf_cq_size.u, lf->rbase + CPT_LF_CQ_SIZE);
```
- The patch reprograms the base and size on every `roc_cpt_cq_enable()`.
- Is this necessary every time, or only after a fatal error that clears the registers?
- If only after error, should there be a flag to skip this on normal enable?

---

## PATCH 07/16: crypto/cnxk: validate TLS xform and keys before SA key copy

### Errors

**Error: Inconsistent AEAD key-length validation (cn10k_tls.c:361-377)**
```c
if (crypto_xfrm->aead.algo == RTE_CRYPTO_AEAD_AES_GCM) {
    if (length != 16 && length != 32)
        return -EINVAL;
    // ...
} else if (crypto_xfrm->aead.algo == RTE_CRYPTO_AEAD_CHACHA20_POLY1305) {
    if (length != 32)
        return -EINVAL;
    // ...
} else {
    return -EINVAL;
}
```
- The patch adds per-algorithm length checks, but does not validate the length *before* the algorithm switch.
- If `length > ROC_CTX_MAX_CKEY_LEN`, the per-algorithm checks do not catch it for AES-GCM (16 or 32) or ChaCha (32).
- The bounds check must come first, then the per-algorithm check.

**Error: Missing release notes**
- The patch rejects previously-accepted malformed xforms (NULL key pointers, oversized keys).
- This is a behavioral change and should be documented.

---

## PATCH 08/16: crypto/cnxk: reject oversized SG vectors in raw datapath

### Info

**Info: Segment count validation added in two places**
- `cnxk_raw_burst_to_iov()`: checks `src_sgl[].num` and `dest_sgl[].num` against `ROC_MAX_SG_CNT`.
- `cnxk_raw_to_iov()`: checks `n_data_vecs` against `ROC_MAX_SG_CNT`.
- Both correct. The raw-dp paths now propagate errors instead of silently overflowing.

---

## PATCH 09/16: crypto/cnxk: guard trim len bounds in TLS 1.2 MAC trim

### Info

**Info: Bounds check added in cn10k_cpt_tls12_trim_mac()**
```c
if (trim_len > m_len) {
    cop->status = RTE_CRYPTO_OP_STATUS_ERROR;
    cop->aux_flags = res->uc_compcode;
    return;
}
```
- Prevents underflow in `mac_offset = m_len - trim_len`.
- Correct.

---

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

### Warnings

**Warning: Missing release notes**
- The patch enables context cache on CN20K, which is a performance and behavior change.
- Requires release notes.

---

## PATCH 11/16: crypto/cnxk: guard trim len in TLS 1.2 MAC trim for CN20K

### Info

**Info: Same bounds check as patch 09, for CN20K**
- Correct.

---

## PATCH 12/16: crypto/cnxk: validate TLS 1.2 MAC trim lengths

### Info

**Info: Additional check `m_len > mbuf->pkt_len` added**
```c
if (trim_len > m_len || m_len > mbuf->pkt_len) {
```
- Prevents walk past end of mbuf chain when `m_len` (from hardware) exceeds actual packet length.
- Correct.

---

## PATCH 13/16: crypto/cnxk: bound SG segment count in packet iov builders

### Errors

**Error: prepare_iov_from_pkt() segment walk may skip NULL check (cnxk_se.h:2685-2695)**
```c
while (start_offset >= pkt->data_len) {
    start_offset -= pkt->data_len;
    pkt = pkt->next;
    nb_segs--;

    if (unlikely(pkt == NULL)) {
        plt_dp_err("Start offset beyond end of packet");
        return 1;
    }
}
```
- The NULL check is inside the loop, but `pkt->next` is dereferenced *before* the check.
- If `pkt->next` is NULL and `start_offset >= pkt->data_len`, the loop advances to `pkt = NULL`, then the *next* iteration dereferences `NULL->data_len`.
- The NULL check must come *before* the `while` condition.

**Error: Missing release notes**
- The patch rejects previously-accepted (but incorrect) segment counts exceeding `ROC_MAX_SG_CNT`.
- Requires release notes.

---

## PATCH 14/16: crypto/cnxk: bound SG iovec index in mbuf walk

### Info

**Info: Belt-and-suspenders check added in two places**
- `prepare_iov_from_pkt()`: checks `index >= ROC_MAX_SG_CNT` inside the loop.
- `prepare_iov_from_pkt_inplace()`: same check.
- These are redundant with the `nb_segs` check added in patch 13, but are a useful defense-in-depth against a chain longer than `nb_segs` reports.

---

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

### Errors

**Error: Missing release notes**
- The patch adds a new devarg `cq_ena` that changes completion behavior on CN20K.
- Requires release notes with usage example.

**Error: Incorrect RTE_PMD_REGISTER_PARAM_STRING (cnxk_cryptodev_devargs.c:165-166)**
```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>");
```
- This overwrites the previous `RTE_PMD_REGISTER_PARAM_STRING` that only listed `CNXK_MAX_QPS_LIMIT`.
- Should include all three parameters in one string.
- The patch does this correctly (concatenates all three).

---

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

### Errors

**Error: Missing release notes**
- The patch changes SA allocation size based on `ctx_ilen`.
- Requires release notes explaining the new sizing behavior.

---

## Summary

**Errors:** 9
- Patch 01: Missing release notes, resource leak on error path (cn20k_tls.c:800), use-after-free potential in cn20k_tls.c
- Patch 06: Missing Fixes tag, missing Cc: stable
- Patch 07: Inconsistent AEAD key-length validation, missing release notes
- Patch 13: NULL check after dereference in segment walk
- Patch 15: Missing release notes

**Warnings:** 7
- Patch 01: Removed flushes may still be needed (3 instances)
- Patch 02: Missing release notes
- Patch 03: Off-by-one units (entry vs line), missing release notes
- Patch 06: CQ base/size reprogrammed on every enable

**Info:** Multiple patches have correct implementations but significant behavioral changes that require release notes.

Review-Result: ERROR