|WARNING| [v2,2/2] app/crypto-perf: support ML DSA

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-09

# DPDK Crypto-Perf Patches Review

## Patch 1/2: app/crypto-perf: support ML KEM

### Errors

1. **Missing error checks on `rte_cryptodev_asym_session_create()`**
   - File: `cperf_ops.c`, lines 1278-1283
   - The function can return negative error codes, but only NULL is checked.
   ```c
   ret = rte_cryptodev_asym_session_create(dev_id, &xform, sess_mp, &asym_sess);
   if (ret < 0) {
       RTE_LOG(ERR, USER1, "ML-KEM Asym session create failed\n");
       return NULL;
   }
   ```
   - The check is correct for error condition but the success path doesn't verify `asym_sess != NULL`. The API returns error code in `ret` and session handle in `asym_sess`. Need to check both or document that `asym_sess` is guaranteed non-NULL on success.

2. **Use of `rte_panic()` in library code path**
   - File: `cperf_ops.c`, line 249
   ```c
   rte_panic("Unsupported ML-KEM operation type %d\n", options->asym_op_type);
   ```
   - `rte_panic()` terminates the process. In a performance test tool this may be acceptable, but it would be better to return an error code and let the caller handle it gracefully.

### Warnings

1. **Inconsistent error handling between ML-KEM and earlier asymmetric ops**
   - File: `cperf_ops.c`
   - The SM2 session creation (lines 1253-1267) doesn't check the return value at all, while ML-KEM does. This inconsistency should be noted, though not introduced by this patch.

2. **Large static test vector arrays**
   - File: `cperf_test_vectors.c`
   - Arrays `mlkem_512_dk`, `mlkem_512_ek`, `mlkem_512_message`, `mlkem_512_cipher`, `mlkem_512_sk` are defined but the data origin/test vector source is not documented.
   - For test vectors this is acceptable, but consider adding a comment referencing the test vector source (e.g., NIST test vectors, RFC, etc.).

3. **Potential for `options->mlkem_data` NULL dereference**
   - File: `cperf_ops.c`, function `cperf_set_ops_asym_mlkem()`
   - The function directly accesses `options->mlkem_data->*` without NULL check.
   - The calling context should guarantee this is non-NULL, but defensive programming would add a check.

### Info

1. **Magic number array sizes**
   - File: `cperf_test_vectors.c`
   - The array sizes are implicit from the initialization. Consider adding explicit size constants or comments indicating the expected sizes per ML-KEM-512 specification (e.g., dk=1632 bytes, ek=800 bytes per FIPS 203).

---

## Patch 2/2: app/crypto-perf: support ML DSA

### Errors

1. **Same `rte_panic()` issue as Patch 1**
   - File: `cperf_ops.c`, line 292
   ```c
   rte_panic("Unsupported ML-DSA operation type %d\n", options->asym_op_type);
   ```
   - Same concern: terminates the process instead of returning an error.

2. **Missing error check on `rte_cryptodev_asym_session_create()`**
   - File: `cperf_ops.c`, lines 1329-1337
   - Same pattern as ML-KEM: checks `ret < 0` but doesn't verify `asym_sess != NULL` on success path.

### Warnings

1. **Very large test vector arrays with no source attribution**
   - File: `cperf_test_vectors.c`
   - Arrays `mldsa_44_seed` through `mldsa_44_extmu` total over 5KB of data.
   - The deterministic signature test vector `mldsa_44_sign_dtrm` is 2420 bytes.
   - No comment indicates the source (NIST ACVP, FIPS 204 test vectors, etc.).

2. **Potential NULL dereference of `options->mldsa_data`**
   - File: `cperf_ops.c`, function `cperf_set_ops_asym_mldsa()`
   - Direct access to `options->mldsa_data->*` without NULL check.

3. **Unused fields in test data structure**
   - File: `cperf_test_vectors.c`
   - The `mldsa_sign_perf_data` and `mldsa_verify_perf_data` structures define fields `ctx`, `seed`, `mu`, and `hash` but these are not initialized in the test data.
   - This may be intentional (leaving them NULL/zero), but consider documenting which fields are required vs. optional for each operation type.

### Info

1. **Missing initialization of optional ML-DSA parameters**
   - File: `cperf_ops.c`, lines 274-285 (siggen) and 286-297 (sigver)
   - The code accesses `ctx`, `seed`, and `mu` fields but the test vectors leave these NULL.
   - If these are optional, the PMD implementation must handle NULL pointers gracefully.
   - If they are required, the test vectors should initialize them.

2. **Asymmetry in test vector naming**
   - The test vectors use suffixes like `_noseed` (e.g., `mldsa_44_privkey_noseed`) but no corresponding `_withseed` variants exist in this patch.
   - This naming suggests there may be future test vectors with seeds, but it's currently misleading.

---

## General Observations

### Cross-Patch Issues

1. **Consistent pattern of missing NULL checks**
   - Both patches add new `cperf_set_ops_asym_*` functions that access `options->*_data` pointers without validation.
   - Consider adding assertions or NULL checks at the beginning of these functions.

2. **Documentation gap**
   - Neither patch updates `doc/guides/rel_notes/` with the new features.
   - Release notes are **required** for new features (AGENTS.md section "Documentation").

3. **No functional tests demonstrated**
   - The patches add test vectors but don't show that the operations actually execute successfully.
   - Consider mentioning in commit message whether manual testing was performed and on which PMD.

### Style Compliance

- Code style is generally correct (indentation, bracing, naming).
- Test vector arrays could benefit from comments indicating their source.

---

## Summary

**Must Fix (Errors):**
- Add proper error handling instead of `rte_panic()` (or document why process termination is acceptable for this tool).
- Verify `asym_sess` is non-NULL after successful session creation, or document the API guarantee.

**Should Fix (Warnings):**
- Add release notes entries for both ML-KEM and ML-DSA support.
- Document test vector sources (NIST, FIPS 203/204, etc.).
- Add NULL checks or assertions for `options->mlkem_data` and `options->mldsa_data`.

**Consider (Info):**
- Clarify which ML-DSA fields (ctx, seed, mu) are optional vs. required.
- Remove `_noseed` suffix if no `_withseed` variants are planned, or add explanatory comment.