|WARNING| [4/4] test/crypto: add asym test suite for AESNI_MB

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: IPsec MB 3.0 Support Series

## Patch 1/4: crypto/ipsec_mb: add support for IPsecMB 3.0

### Errors

None identified.

### Warnings

**1. Missing error check on `rte_ring_dequeue_burst` (Patch 1/4, pmd_aesni_mb.c:1183-1203)**
The code dequeues operations from `qp->ingress_queue` without checking if the ring exists.
If `qp->ingress_queue` is NULL (e.g., due to a failed setup or teardown race), `rte_ring_dequeue_burst` will segfault.

```c
/* In pmd_aesni_mb.c around line 1188 */
nb_submit_ops = rte_ring_dequeue_burst(qp->ingress_queue,
                        (void **)deqd_ops, n, NULL);
```

**Suggested fix:**
Add a NULL check before the dequeue:
```c
if (qp->ingress_queue == NULL)
    return 0;
nb_submit_ops = rte_ring_dequeue_burst(qp->ingress_queue,
                        (void **)deqd_ops, n, NULL);
```

**2. Potential memory leak on `imb_ml_kem_new`/`imb_ml_dsa_new` failure (Patch 3/4, pmd_aesni_mb.c:999, 1025)**
In `aesni_mb_asym_session_configure`, if `imb_ml_kem_new` or `imb_ml_dsa_new` fails after the session has been partially initialized, the error path returns without cleaning up any allocated resources.

```c
ret = imb_ml_kem_new(mb_mgr, sess->asym.ml_kem.alg, &sess->asym.ml_kem.ctx);
if (ret) {
    IPSEC_MB_LOG(ERR, "Failed to create ML KEM session");
    return ret;  /* No cleanup of sess or other allocations */
}
```

**Suggested fix:**
If the session structure is zeroed on allocation (common pattern), this may be acceptable.
However, document that callers must call `aesni_mb_asym_session_clear` on error, or ensure the session is in a consistent state for later cleanup.

**3. Hardcoded buffer size in ML-KEM keygen (Patch 3/4, pmd_aesni_mb.c:2565-2567)**
The stack-allocated `seed_d_z[64]` buffer has a fixed size of 64 bytes.
If the library or future versions of the algorithm require a different seed size, this will silently fail or corrupt the stack.

```c
uint8_t seed_d_z[64];
memcpy(seed_d_z,      keygen->d.data, 32);
memcpy(&seed_d_z[32], keygen->z.data, 32);
```

**Suggested fix:**
Add compile-time or runtime checks that the combined seed size is exactly 64 bytes, or use a size constant from the library.
```c
RTE_BUILD_BUG_ON(IMB_ML_KEM_SEED_D_BYTES + IMB_ML_KEM_SEED_Z_BYTES != 64);
```

### Info

**1. Wrapper function macro pattern (Patch 1/4, pmd_aesni_gcm.c:9-144)**
The `_fn` macro used to generate wrapper functions for library versions < 3.0 is a clever way to bridge API differences.
This is correct but can be simplified if the minimum required version is bumped to 3.0 in the future.

**2. Function pointer arrays now include `mb_mgr` pointer (Patch 1/4, pmd_aesni_gcm_priv.h:131)**
Adding the `IMB_MGR *mb_mgr` field to `struct aesni_gcm_ops` is a clean way to carry the state pointer.
Ensure that `ops` is always initialized before use (appears to be the case in `aesni_gcm_set_ops`).

**3. Version-specific feature flag removal (Patch 1/4, pmd_aesni_mb.c:2589)**
The removal of `RTE_CRYPTODEV_FF_NON_BYTE_ALIGNED_DATA` for IPsec MB 3.0 is correctly conditional on the library version.
This matches the release note stating that non-byte-aligned bit length support was removed.

**4. Asym session creation flag in `ipsec_mb_internals` (Patch 3/4, ipsec_mb_private.h:377)**
The `bool asym_session_created` flag is a global per-PMD-type flag indicating whether any asym session has been created.
This is used to decide whether to process asym ops in the dequeue path.
This is a valid optimization, but consider that it is never reset if all asym sessions are destroyed.
This means the asym processing path will remain active even when no asym sessions exist.
For most deployments this is a minor inefficiency, not a bug.

**5. Asym op processing in dequeue path (Patch 3/4, pmd_aesni_mb.c:2780-2805)**
The asym op processing is done synchronously in the dequeue burst function.
This is acceptable for CPU-based crypto but means that a burst of asym ops will block all crypto processing on that queue pair.
Consider documenting this behavior or adding a note that dedicated asym queue pairs are recommended for performance.

---

## Patch 2/4: crypto/ipsec_mb: add support for 256-NxA4/5/6 algorithms

### Errors

None identified.

### Warnings

**1. Unchecked key expansion in NEA4/NEA5/NEA6/NCA4/NCA5/NCA6 setup (Patch 2/4, pmd_aesni_mb.c:645-647, 816-817, 836-837, 856-857)**
The calls to `IMB_AES_KEYEXP_256` do not check the return value.
While this function may always succeed on valid inputs, if it can fail (e.g., due to NULL `mb_mgr` or internal allocation failure), the error is silently ignored.

**Suggested fix:**
Check if `IMB_AES_KEYEXP_256` returns a status or modifies `mb_mgr` error state, and handle errors appropriately.
If it cannot fail, add a comment stating that.

**2. Digest size validation hardcoded (Patch 2/4, pmd_aesni_mb.c:829-832, 848-851, 868-871)**
The digest size check `if (sess->auth.req_digest_len < 4 || sess->auth.req_digest_len > 16)` is repeated for NCA4, NCA5, and NCA6.
If the valid range changes in a future library version, this becomes error-prone.

**Suggested fix:**
Extract the validation into a helper function or use library-provided constants if available.

### Info

**1. Documentation update (Patch 2/4, aesni_mb.rst:39-80)**
The documentation correctly lists the new algorithms under their respective categories (Cipher, Hash, AEAD).
The feature matrix update in `features/aesni_mb.ini` is also correct.

**2. Release notes (Patch 2/4, release_26_11.rst:39-43)**
The release notes correctly document the new algorithms.
The note format matches DPDK conventions.

---

## Patch 3/4: crypto/ipsec_mb: add support for ML-KEM and ML-DSA

### Errors

None identified.

### Warnings

**1. No bounds check on `siggen->sign.length` before `imb_ml_dsa_sign` (Patch 3/4, pmd_aesni_mb.c:2662-2665)**
The `imb_ml_dsa_sign` function writes to `siggen->sign.data` and updates `siggen->sign.length`.
If the buffer is too small, this could overflow.

```c
rc = imb_ml_dsa_sign(sess->asym.ml_dsa.ctx,
    siggen->sign.data, &siggen->sign.length,
    md, md_len, &sess->asym.ml_dsa.sign_params);
```

**Suggested fix:**
Validate that `siggen->sign.length` is at least `aesni_mb_ml_dsa_sig_sz(sess->asym.ml_dsa.alg)` before calling the library function.

**2. ML-KEM decap ignores `imb_ml_kem_set_privkey` failure (Patch 3/4, pmd_aesni_mb.c:2633-2639)**
The code checks `rc == 0` before calling `imb_ml_kem_decap`, but `rc` is only set by `imb_ml_kem_set_privkey`.
If that fails, `rc` is non-zero and `imb_ml_kem_decap` is skipped, but the operation status is set based on `rc` without distinguishing between key setup failure and decap failure.

**Suggested fix:**
Set `op->status` explicitly based on which operation failed, or at minimum log which step failed.

### Info

**1. Asym session size reuse (Patch 3/4, pmd_aesni_mb.c:3030)**
The code reuses `ipsec_mb_sym_session_get_size` for asym sessions:
```c
.asym_session_get_size = ipsec_mb_sym_session_get_size,
```
This works because `aesni_mb_session` is a union that covers both sym and asym session data, and the size is the same.
However, this is subtle and could break if the structures diverge in the future.
Consider adding a comment or a `RTE_BUILD_BUG_ON` to enforce that the sizes remain equal.

**2. Inline helper functions for size lookups (Patch 3/4, pmd_aesni_mb.c:2485-2542)**
The `aesni_mb_ml_kem_*_sz` and `aesni_mb_ml_dsa_*_sz` functions are static inline and use switch statements to map algorithm IDs to library constants.
These are correct but could be replaced with lookup tables for better performance.
For a small number of cases (3 per function), the switch is acceptable.

---

## Patch 4/4: test/crypto: add asym test suite for AESNI_MB

### Errors

None identified.

### Warnings

None identified.

### Info

**1. Test registration (Patch 4/4, test_cryptodev_asym.c:6184-6187, 6198)**
The test function `test_cryptodev_aesni_mb_asym` simply calls `run_cryptodev_asym_testsuite` with the PMD name.
This is the standard pattern for driver-specific test registration and is correct.

**2. No PMD-specific test cases added**
The patch only enables the existing generic asym test suite for AESNI_MB.
This is acceptable for initial support, but consider adding PMD-specific tests for ML-KEM and ML-DSA corner cases (e.g., invalid key sizes, algorithm mismatch) in a follow-up.

---

## Summary

The patch series is well-structured and correctly implements support for IPsec MB 3.0.
The main concerns are:

- **Warning-level issues**: Missing error checks on dequeue paths, potential buffer overflows in asym ops, and missing cleanup on session creation failures.
- **Info-level observations**: Asym session creation flag never resets, asym ops processed synchronously in dequeue, and some hardcoded sizes that could use library constants.

No correctness bugs (use-after-free, resource leaks on error paths) were identified.
The version conditionals are consistently applied throughout.