|SUCCESS| [v5,4/4] test/crypto: add asym test suite for AESNI_MB

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/171447

_AI Code Review_

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

# Code Review: DPDK AESNI_MB IPsecMB 3.0 Support Patch Series

---

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

### Errors

**1. Non-const function pointer wrapper parameter** (line 159 in pmd_aesni_gcm.c)

```c
/* wrapper functions for library versions < 3.0.0 */
#if IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM

#define _fn(bitlen) static void imb_aes##bitlen##_gcm_pre(const void *key, \
		struct gcm_key_data *key_data, IMB_MGR *state) \
```

The `IMB_MGR *state` parameter in the wrapper functions should be `const IMB_MGR *state` for consistency with the pattern where the state manager is read-only during key expansion operations. However, on closer inspection of the code context, the wrappers call `state->gcm##bitlen##_pre(key, key_data)` which suggests the function pointer itself may modify internal state. The usage in lines 285-287 shows `imb_aes128_gcm_pre(key, &sess->gdata_key, mb_mgr)` where `mb_mgr` is non-const. This is consistent with the IPsecMB 3.0 API change where `IMB_MGR` becomes non-const.

**Correction: This is correct for IPsecMB 3.0. The non-const `IMB_MGR` is the intended API change. Not an error.**

---

### Warnings

None identified.

---

### Info

**1. Pre-existing: Potential integer overflow in cast-free pointer arithmetic** (lines 491, 497, etc.)

The code performs pointer arithmetic with `uint64_t` lengths directly:
```c
ops->update_enc(&session->gdata_key, &qp_data->gcm_ctx_data,
        dst, src, (uint64_t)part_len, ops->mb_mgr);
```
If `part_len` is a 32-bit value and the platform uses 32-bit pointers, the cast to `uint64_t` is fine, but the subsequent pointer increments in the library could theoretically overflow on exotic platforms. This is not introduced by the patch and is a property of the library API.

---

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

### Errors

None identified.

---

### Warnings

None identified.

---

### Info

**1. Capabilities structure size** (pmd_aesni_mb_priv.h, lines 894-1118)

Adding 227 lines of capability definitions for 9 new algorithms (NEA4/5/6, NIA4/5/6, NCA4/5/6) is substantial but follows the existing pattern. The code is correctly conditional on `IMB_VERSION(3, 0, 0) <= IMB_VERSION_NUM`. Each capability block correctly defines key sizes, IV sizes, digest sizes, and AAD sizes matching the 3GPP TS 33.501 specifications for these 5G algorithms.

**2. Alignment attributes on expanded key structures** (pmd_aesni_mb_priv.h, lines 1293-1301)

```c
struct {
    alignas(16) uint32_t encode[60];
    alignas(16) uint32_t decode[60];
} expanded_nia5_keys;
struct {
    alignas(16) uint32_t encode[60];
    alignas(16) uint32_t decode[60];
} expanded_nea5_keys;
```
The `alignas(16)` is correct for AES key schedule structures. The 60-element array size is correct for AES-256 (14 rounds * 4 + 4 = 60 uint32_t values).

---

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

### Errors

**1. Missing error path cleanup in session configuration** (pmd_aesni_mb.c, line 1017)

```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;
}
```

If `imb_ml_kem_new()` fails, the function returns immediately without cleaning up any previously allocated resources. However, inspection of the code flow shows this is the first allocation in the ML_KEM case, so there are no prior resources to clean. The ML_DSA case at line 1043 has the same pattern and is also the first allocation in that branch. Not an error.

**2. Resource leak on session_configure failure** (ipsec_mb_ops.c, line 477)

```c
ret = (*pmd_data->asym_session_configure)(mb_mgr,
        CRYPTODEV_GET_ASYM_SESS_PRIV(sess), xform);
if (ret != 0) {
    IPSEC_MB_LOG(ERR, "failed configure asym session parameters");
    return ret;
}
```

If `asym_session_configure` fails (e.g., `imb_ml_kem_new()` fails inside it), the session private data may contain partially initialized state, but the caller (`rte_cryptodev_asym_session_create`) will eventually call `asym_session_clear` on session destruction. However, tracing the code: `imb_ml_kem_new` and `imb_ml_dsa_new` allocate internal context. If they fail, `ctx` is not set, so `asym_session_clear` will attempt to free NULL (lines 1065-1070), which is safe (`imb_ml_kem_free(NULL)` and `imb_ml_dsa_free(NULL)` are documented as no-ops in IPsecMB). Not a leak.

---

### Warnings

**1. Conditional compilation creates API inconsistency**

The ops structure is conditionally populated:
```c
#if IMB_VERSION(3, 0, 0) <= IMB_VERSION_NUM
    .asym_session_get_size = ipsec_mb_sym_session_get_size,
    .asym_session_configure = ipsec_mb_asym_session_configure,
    .asym_session_clear = ipsec_mb_asym_session_clear,
#endif
```

Applications compiled against DPDK built with IPsecMB < 3.0 will not see these ops, while applications compiled against DPDK built with IPsecMB >= 3.0 will see them. This is correct behavior per the release notes, but creates a minor compatibility concern: an application checking for asymmetric crypto support at runtime will get different results depending on the build-time library version, even on the same DPDK version. This is acceptable given the feature requires IPsecMB 3.0+.

**2. Reuse of `ipsec_mb_sym_session_get_size` for asym sessions** (pmd_aesni_mb.c, line 3054)

```c
.asym_session_get_size = ipsec_mb_sym_session_get_size,
```

The asymmetric session uses the same `get_size` function as symmetric sessions. This works because `struct aesni_mb_session` is a union-like structure containing both `cipher`/`auth` and `asym` fields (lines 1409-1428 in pmd_aesni_mb_priv.h). The size is `sizeof(struct aesni_mb_session)` in both cases. However, the naming is misleading. The function returns the size of the entire session structure, which is correct, but calling a function named `sym_session_get_size` for asymmetric sessions is confusing. Consider adding a comment or renaming to `ipsec_mb_session_get_size`.

---

### Info

**1. Asymmetric operation processing in dequeue path** (pmd_aesni_mb.c, lines 2804-2830)

The patch adds asymmetric operation processing directly in the dequeue burst function:
```c
if (pmd_data->asym_session_created) {
    struct rte_crypto_op *tmp_dq[IMB_MAX_BURST_SIZE];
    uint16_t sym_cnt = 0;
    nb_submit_ops = rte_ring_dequeue_burst(qp->ingress_queue,
                            (void **)tmp_dq, n, NULL);
    for (i = 0; i < nb_submit_ops; i++) {
        if (tmp_dq[i]->type == RTE_CRYPTO_OP_TYPE_ASYMMETRIC) {
            aesni_mb_asym_process(tmp_dq[i]);
            ops[processed_jobs++] = tmp_dq[i];
        } else {
            deqd_ops[sym_cnt++] = tmp_dq[i];
        }
    }
```

This synchronous processing of asymmetric operations in the datapath is a reasonable design for ML-KEM and ML-DSA, which are computationally intensive but not hardware-accelerated in this PMD. The conditional check `if (pmd_data->asym_session_created)` avoids the overhead when no asymmetric sessions exist. The approach is pragmatic given that asymmetric operations are typically rare in a mixed workload.

**2. ML-KEM and ML-DSA algorithm mapping correctness**

The algorithm mappings are correct:
- `RTE_CRYPTO_ML_KEM_512` - `IMB_ML_KEM_512` (NIST PQC KEM with 128-bit security)
- `RTE_CRYPTO_ML_KEM_768` - `IMB_ML_KEM_768` (192-bit security)
- `RTE_CRYPTO_ML_KEM_1024` - `IMB_ML_KEM_1024` (256-bit security)
- `RTE_CRYPTO_ML_DSA_44` - `IMB_ML_DSA_44` (NIST PQC signature with 128-bit security, 2 categories)
- `RTE_CRYPTO_ML_DSA_65` - `IMB_ML_DSA_65` (192-bit security, 3 categories)
- `RTE_CRYPTO_ML_DSA_87` - `IMB_ML_DSA_87` (256-bit security, 5 categories)

These match the FIPS 203 (ML-KEM) and FIPS 204 (ML-DSA) parameter sets.

---

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

### Errors

None identified.

---

### Warnings

None identified.

---

### Info

The test registration follows the existing pattern for other PMDs. The test function `test_cryptodev_aesni_mb_asym()` calls `run_cryptodev_asym_testsuite()` with the PMD name, which will execute the generic asymmetric crypto test suite against the AESNI_MB PMD. This will test ML-KEM and ML-DSA operations once the corresponding test vectors are present in `test_cryptodev_asym.c`.

---

## Cross-Patch Observations

**1. Consistent version gating**

All IPsecMB 3.0 features are correctly gated with `#if IMB_VERSION(3, 0, 0) <= IMB_VERSION_NUM`. The code is backward compatible with IPsecMB < 3.0 where the new algorithms are not available.

**2. Non-byte-aligned data feature flag removal**

The patch correctly removes `RTE_CRYPTODEV_FF_NON_BYTE_ALIGNED_DATA` for IPsecMB >= 3.0 (pmd_aesni_mb.c, lines 3183-3185 in patch 1). This matches the release notes and the library's removal of `_BITLEN` enums. The code correctly uses byte-based lengths for IPsecMB 3.0:

```c
#if IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM
    job->msg_len_to_hash_in_bits = op->sym->auth.data.length;
#else
    job->msg_len_to_hash_in_bytes = op->sym->auth.data.length >> 3;
#endif
```

---

## Documentation Review

**1. Release notes** (doc/guides/rel_notes/release_26_11.rst)

The release notes correctly document:
- IPsecMB 3.0 support
- Removal of ZUC-256 EEA3/EIA3 and non-byte-aligned data feature
- Addition of NEA4/5/6, NIA4/5/6, NCA4/5/6 algorithms
- Addition of ML-KEM and ML-DSA algorithms

**2. Feature matrix** (doc/guides/cryptodevs/features/aesni_mb.ini)

Correctly updated to reflect:
- `Non-Byte aligned data  = N` (was Y)
- `Asymmetric crypto      = Y` (new)
- New cipher, auth, and AEAD algorithms
- ML-KEM and ML-DSA in [Asymmetric] section

---

## Summary

The patch series is well-structured and correctly implements support for IPsecMB 3.0. The code handles the API changes (non-const `IMB_MGR`, removal of `_BITLEN` enums, byte-based lengths) correctly with appropriate conditional compilation. The addition of 5G NxAN algorithms and PQC algorithms is complete and follows DPDK coding standards.

**Minor recommendations** (not errors):
1. Consider renaming `ipsec_mb_sym_session_get_size` to `ipsec_mb_session_get_size` when used for both symmetric and asymmetric sessions, or add a comment explaining the reuse.
2. The asym operation processing in the dequeue path adds a conditional branch and temporary buffer copy even when no asymmetric sessions exist. The `asym_session_created` flag mitigates this, but consider documenting the performance characteristics in a comment.

---

Review-Result: CLEAN