|WARNING| [v4,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/171285

_AI Code Review_

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

# DPDK Patch Review

## ERRORS

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

**Error 1: Missing error check on queue pair allocation**
Line: `struct ipsec_mb_qp *qp = dev->data->queue_pairs[0];`
In `aesni_mb_asym_session_configure()` (ipsec_mb_ops.c:997), the code accesses `queue_pairs[0]` without checking whether any queue pairs have been configured. If `dev->data->nb_queue_pairs` is zero, this is an out-of-bounds access.

```c
/* BAD - no bounds check */
struct ipsec_mb_qp *qp = dev->data->queue_pairs[0];
IMB_MGR *mb_mgr;
int ret = 0;

if (qp != NULL)
    mb_mgr = qp->mb_mgr;

/* GOOD - check nb_queue_pairs first */
if (dev->data->nb_queue_pairs == 0)
    return -EINVAL;

struct ipsec_mb_qp *qp = dev->data->queue_pairs[0];
```

**Error 2: Resource leak on session configure failure**
Line: `if (unlikely(sess == NULL)) { ... }`
In `aesni_mb_asym_session_configure()` (ipsec_mb_ops.c:1008-1012), when `qp == NULL` the code allocates `mb_mgr` via an implied call but then leaks it on the error path when `sess == NULL`. The comment says `free_mb_mgr(mb_mgr)` but this only happens if `sess == NULL`; if the later `session_configure` call fails (line 1015), `mb_mgr` is never freed when `qp == NULL`.

```c
/* BAD - mb_mgr leaked if session_configure fails */
if (qp != NULL)
    mb_mgr = qp->mb_mgr;

if (!mb_mgr)
    return -ENOMEM;

if (unlikely(sess == NULL)) {
    IPSEC_MB_LOG(ERR, "invalid session struct");
    if (qp == NULL)
        free_mb_mgr(mb_mgr);  /* only freed here */
    return -EINVAL;
}

ret = (*pmd_data->asym_session_configure)(mb_mgr, ...);
if (ret != 0) {
    IPSEC_MB_LOG(ERR, "failed configure asym session parameters");
    return ret;  /* mb_mgr not freed if qp == NULL */
}
```

**GOOD:**
```c
if (qp != NULL)
    mb_mgr = qp->mb_mgr;
else
    mb_mgr = alloc_mb_mgr(0);  /* assuming this is the missing call */

if (!mb_mgr)
    return -ENOMEM;

if (unlikely(sess == NULL)) {
    IPSEC_MB_LOG(ERR, "invalid session struct");
    ret = -EINVAL;
    goto error;
}

ret = (*pmd_data->asym_session_configure)(mb_mgr, ...);
if (ret != 0) {
    IPSEC_MB_LOG(ERR, "failed configure asym session parameters");
    goto error;
}

pmd_data->asym_session_created = true;
return 0;

error:
    if (qp == NULL)
        free_mb_mgr(mb_mgr);
    return ret;
```

**Error 3: Inconsistent conditional compilation guards**
Lines: multiple locations in pmd_aesni_mb.c
The patch uses `IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM` to guard pre-3.0 code paths and `IMB_VERSION(3, 0, 0) <= IMB_VERSION_NUM` to guard 3.0+ code paths. However, in some places (e.g., pmd_aesni_mb.c:1472-1485), the pre-3.0 `#if` block falls through to an `#else` that assumes 3.0+, but then later code (e.g., 2800-2830) uses the `<=` form separately. This is inconsistent and risks compilation failure on exactly version 3.0.0 if the comparison operators are not strict mirror images.

The issue is theoretical but the safer pattern is to use `<` and `>=` or `>` and `<=` consistently, not mixing `>` and `<=`.

**Error 4: `const IMB_MGR *` removed without ABI consideration**
Line: `aesni_mb_set_session_cipher_parameters(IMB_MGR *mb_mgr, ...`
The function signature change from `const IMB_MGR *mb_mgr` to `IMB_MGR *mb_mgr` (pmd_aesni_mb.c:348) is an ABI break if this function is exported or used in installed headers. The patch does not document this as an ABI change or guard it with versioning. If `aesni_mb_set_session_cipher_parameters` is internal-only, this is acceptable; if it's reachable from outside the PMD, it requires ABI versioning or a deprecation notice.

**Context check:** The function is declared `static int` in the same file, so it is internal. However, the review guidelines require checking whether a symbol is exported. Since the function is static, this is not an ABI break. **Retracted** -- this is correct as-is.

---

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

**Error 5: Missing NULL check after `imb_ml_kem_new` and `imb_ml_dsa_new`**
Lines: pmd_aesni_mb.c:1019, 1045
The code checks the return value `ret` after calling `imb_ml_kem_new` and `imb_ml_dsa_new`, but the IPsec MB library convention (based on the error log at line 2624) is that these functions return an error code and the context pointer is set via the `ctx` parameter. However, the code does not verify that `sess->asym.ml_kem.ctx` or `sess->asym.ml_dsa.ctx` is non-NULL before using them in subsequent operations.

**If the library sets `ctx` to NULL on allocation failure but returns 0, later calls will dereference NULL.**

```c
/* BAD - ctx not checked */
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;
}
/* sess->asym.ml_kem.ctx could be NULL here */

/* GOOD - check both ret and ctx */
ret = imb_ml_kem_new(mb_mgr, sess->asym.ml_kem.alg, &sess->asym.ml_kem.ctx);
if (ret != 0 || sess->asym.ml_kem.ctx == NULL) {
    IPSEC_MB_LOG(ERR, "Failed to create ML KEM session");
    return (ret != 0) ? ret : -ENOMEM;
}
```

**Note:** This depends on the IPsec MB library contract. If `imb_ml_kem_new` guarantees that a non-zero return implies `ctx == NULL` and zero return implies `ctx != NULL`, the existing check is sufficient. However, defensive programming dictates checking both.

**Error 6: Missing cleanup of ML-KEM/ML-DSA contexts on session configure failure**
Lines: pmd_aesni_mb.c:1000-1060
In `aesni_mb_asym_session_configure`, if `imb_ml_kem_new` succeeds (line 1019) but a later error occurs (e.g., unsupported `type` in the ML_DSA case), the function returns without calling `imb_ml_kem_free`. Similarly, if `imb_ml_dsa_new` succeeds (line 1045) but the function later fails, `imb_ml_dsa_free` is not called. This leaks the IPsec MB library's internal state.

```c
/* BAD - ML_KEM context not freed on error in ML_DSA case */
case RTE_CRYPTO_ASYM_XFORM_ML_KEM:
{
    /* ... */
    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;
    }
    /* ... */
}
break;
case RTE_CRYPTO_ASYM_XFORM_ML_DSA:
{
    /* ... */
    ret = imb_ml_dsa_new(mb_mgr, sess->asym.ml_dsa.alg, &sess->asym.ml_dsa.ctx);
    if (ret) {
        IPSEC_MB_LOG(ERR, "Failed to create ML DSA session");
        return ret;  /* OK - no cleanup needed yet */
    }
    /* ... */
}
break;
default:
    ret = -ENOTSUP;
    IPSEC_MB_LOG(ERR, "Unsupported algorithm");
    return ret;  /* BUG - if ML_KEM was initialized, its ctx is leaked */
};
```

**GOOD - cleanup on error:**
```c
    ret = imb_ml_kem_new(mb_mgr, sess->asym.ml_kem.alg, &sess->asym.ml_kem.ctx);
    if (ret != 0 || sess->asym.ml_kem.ctx == NULL) {
        IPSEC_MB_LOG(ERR, "Failed to create ML KEM session");
        return (ret != 0) ? ret : -ENOMEM;
    }
    IMB_ML_KEM_KEYGEN_PARAMS_INIT(&sess->asym.ml_kem.keygen_params);
    IMB_ML_KEM_ENCAP_PARAMS_INIT(&sess->asym.ml_kem.encap_params);
}
break;
case RTE_CRYPTO_ASYM_XFORM_ML_DSA:
{
    /* ... */
    ret = imb_ml_dsa_new(mb_mgr, sess->asym.ml_dsa.alg, &sess->asym.ml_dsa.ctx);
    if (ret != 0 || sess->asym.ml_dsa.ctx == NULL) {
        IPSEC_MB_LOG(ERR, "Failed to create ML DSA session");
        return (ret != 0) ? ret : -ENOMEM;
    }
    /* ... */
}
break;
default:
    ret = -ENOTSUP;
    IPSEC_MB_LOG(ERR, "Unsupported algorithm");
    return ret;
};

return ret;
```
The issue is that the switch statement has no fallthrough path that would cause a leak **within this function**, because each case ends with a `break` or `return`. However, the outer function `ipsec_mb_asym_session_configure` (ipsec_mb_ops.c:495) does not clean up on failure, so if this function returns an error, the caller must handle cleanup. The patch does not add such handling. **This is a resource leak at the caller level.**

**Error 7: ML-KEM/ML-DSA session cleanup not called on device close**
The patch adds `imb_ml_kem_free` and `imb_ml_dsa_free` to `aesni_mb_asym_session_clear` (line 1066-1074), but does not hook this into the device lifecycle. If a session is created but not explicitly cleared by the application, and then the device is closed, the contexts are leaked. The `aesni_mb_asym_session_clear` function is only called via the `asym_session_clear` op (line 2806), which is under application control, not PMD teardown.

**The PMD must iterate all sessions and free their resources when the device is stopped or closed.**

---

## WARNINGS

### Patch 1/4

**Warning 1: Mixed symmetric and asymmetric operation processing in dequeue path**
Lines: pmd_aesni_mb.c:2808-2829
The dequeue function now conditionally processes asymmetric operations inline if any asym session has been created. This adds control-flow overhead to every dequeue call (checking `pmd_data->asym_session_created`) even when no asym ops are enqueued. A more efficient design would separate sym and asym queue pairs, or use a per-queue-pair flag rather than a global PMD flag.

**This is not a bug, but a performance concern.**

---

## INFO

### Patch 1/4

**Info 1: Pre-existing code -- ZUC256 EIA3 removal not documented in code comments**
The removal of ZUC-256 EIA3 (guarded by `IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM` at lines 172-179 and elsewhere) is mentioned in the commit message and release notes, but the code does not include a comment explaining why the `#if` block is needed. Future readers may not understand why ZUC-256 is conditionally compiled.

**Suggestion:** Add a comment like `/* ZUC-256 EIA3 removed in IPsec MB 3.0 */` at the first occurrence.

---

### Patch 2/4

**Info 2: New AEAD algorithms do not update features matrix documentation**
The patch adds Snow5G/AES/ZUC NCA4/5/6 capabilities and updates `features/aesni_mb.ini`, but does not update the general `doc/guides/cryptodevs/features.rst` or ensure that the algorithm names are listed in the cryptodev overview. This is a documentation gap, not a code error.

---

### Patch 3/4

**Info 3: Asymmetric crypto operations bypass session-less path**
The inline asymmetric processing in `aesni_mb_dequeue_burst` (lines 2808-2829) requires a session (`CRYPTODEV_GET_ASYM_SESS_PRIV(aop->session)`), but the PMD advertises `RTE_CRYPTODEV_FF_SYM_SESSIONLESS`. The asym path does not check whether the session is sessionless or handle per-operation xform. This is a functional limitation, not a bug, but should be documented.

---

# SUMMARY

**6 Errors identified:**
1. Missing bounds check on `dev->data->queue_pairs[0]` in asym session configure (out-of-bounds access risk)
2. Resource leak of `mb_mgr` when `qp == NULL` and session configure fails
3. Inconsistent version macro comparison operators (theoretical risk of compilation failure on exactly v3.0.0)
4. ~~`const IMB_MGR *` ABI break~~ (retracted -- function is static, no ABI impact)
5. Missing NULL check on ML-KEM/ML-DSA context pointers after allocation
6. Missing cleanup of ML-KEM/ML-DSA contexts on session configure failure
7. ML-KEM/ML-DSA contexts not freed on device close if session not explicitly cleared

**1 Warning:**
1. Performance concern: mixed sym/asym processing in dequeue adds overhead to symmetric fast path

**3 Info notes:**
1. ZUC-256 removal not commented in code
2. AEAD algorithms missing from general features documentation
3. Asymmetric operations do not support sessionless mode

---

Review-Result: ERROR