|WARNING| [6/6] crypto/dpaa2_sec: add support for env variables

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

Reviewing patch series against AGENTS.md guidelines.

---

## PATCH 1/6: crypto/dpaa2_sec: fix buffer overflow in GCM decrypt

### Errors

**Buffer overflow on old_icv write (sge+8)**
The patch correctly identifies that the FLE buffer was too small (256 bytes = 8 SGEs) to hold 8 SGEs plus 16 bytes of old_icv at index 8, causing out-of-bounds access. The fix (increasing to 288 bytes = 9x32) is correct.

**Initialization does not cover sge+5 through sge+7**
The patch adds initialization for sge+4, but the comment states the layout uses "8 entries plus old_icv at index 8." This implies indices 0-7 are SGEs, and old_icv is at offset sge+8. The initialization only covers sge+0 through sge+4, leaving sge+5, sge+6, and sge+7 uninitialized if they are part of the layout. If the actual layout only uses sge+0 through sge+4 and then old_icv at sge+8, then sge+5..sge+7 are unused padding and do not require initialization. However, the commit message claims "8 entries" are occupied. This inconsistency should be verified -- if the layout truly uses 8 SGEs, all 8 must be initialized.

### Warnings

**Missing release notes**
This is a bug fix with Fixes/Cc:stable tags. It should have an entry in the release notes under "Fixed Issues" documenting the symptom (SMMU fault, descriptor corruption) and the resolution.

---

## PATCH 2/6: crypto/dpaa2_sec: fix FLE pool leak on sec FD build failure

### Errors

**Wrong loop counter after assignment**
The patch assigns `frames_to_send = loop + 1`, then immediately starts a new loop with `for (loop = 0; loop < frames_to_send; loop++)`. This reuses the `loop` variable, clobbering its value. While the reassignment sets `loop = 0` at loop initialization, this is confusing and error-prone. The cleanup loop should use a distinct counter variable (e.g., `i` or `cleanup_idx`) to avoid shared-counter issues.

```c
/* Current (confusing) */
frames_to_send = loop + 1;
for (loop = 0; loop < frames_to_send; loop++)
    free_fle(&fd_arr[loop], dpaa2_qp);

/* Suggested */
frames_to_send = loop + 1;
for (int i = 0; i < frames_to_send; i++)
    free_fle(&fd_arr[i], dpaa2_qp);
```

### Warnings

**Missing release notes**
Bug fix with Fixes/Cc:stable. Requires release notes entry.

---

## PATCH 3/6: crypto/dpaa2_sec: support AES-GMAC

### Errors

**New AEAD enum value added to public API without `__rte_experimental`**
`RTE_CRYPTO_AEAD_AES_GMAC` is added to `enum rte_crypto_aead_algorithm` in `lib/cryptodev/rte_crypto_sym.h`, which is a public API header. New API additions must be marked `__rte_experimental`. However, enum values cannot be marked experimental directly. The entire API extension should be treated as experimental, or an alternative approach (feature flag, capability check) should be used. At minimum, release notes must document this as a new API feature and state that it is experimental.

**Return -ENOTSUP without propagating error context**
The `dpaa2_sec_ipsec_proto_init()` function returns `-ENOTSUP` when `RTE_CRYPTO_AUTH_AES_GMAC` is used as an AUTH transform, but this error is returned directly without setting `op->status` or any field the caller could inspect. Verify that the caller chain properly translates `-ENOTSUP` into a session setup failure that applications can detect. If the caller does not set `op->status` or return a meaningful error, the application may receive a silent failure.

### Warnings

**Copyright year updated in multiple files**
Copyright year changes from 2025 to 2026 in `dpaa2_sec_dpseci.c` and from 2024 to 2026 in `dpaa2_sec_priv.h`. Per AGENTS.md, copyright years are not subject to AI review. However, the year 2026 is in the future relative to the patch date shown in the email headers (Mon, 10 Aug 2026 is a hypothetical future date). This is acceptable if the patch is genuinely being submitted on that date, but if the actual submission date is earlier, the year should match the real date.

**Missing release notes for new API**
New AEAD algorithm support (AES-GMAC) and new capability entries require documentation in release notes under "New Features."

---

## PATCH 4/6: crypto/dpaa2_sec: increase ivsize range for AES-CTR

### Warnings

**Incorrect attribution**
Signed-off-by is Hemant Agrawal, but the patch author in the From: field is Gagandeep Singh. Typically the author and first Signed-off-by should match unless the patch is being forwarded. If Hemant authored this change, the From: field should be updated or an explicit Author: line should be added.

**Missing release notes**
Capability change (IV size range expansion) should be noted in release notes.

---

## PATCH 5/6: crypto/dpaa2_sec: add missing ECN capability

### Warnings

**Missing release notes**
New security capability (ECN support) should be documented in release notes under "New Features."

---

## PATCH 6/6: crypto/dpaa2_sec: add support for env variables

### Errors

**`getenv()` used in driver code (lib/drivers restriction)**
Per AGENTS.md, `getenv()` is forbidden in `lib/` and `drivers/` except for EAL. This patch adds `getenv(DRIVER_STRICT_ORDER)` and `getenv(DRIVER_DUMP_MODE)` calls in `drivers/crypto/dpaa2_sec/`. This violates the guideline. Use devargs or driver-specific configuration mechanisms instead.

**Environment variable names not defined**
`DRIVER_STRICT_ORDER` and `DRIVER_DUMP_MODE` are used but not defined anywhere visible in the patch. These must be `#define`d constants (likely prefixed with `DPAA2_SEC_` to avoid namespace collisions). If they are intended to be user-facing environment variable names, they should be documented and defined as constants.

**Return value of `atoi()` not validated**
`atoi()` returns 0 on parse failure, which may be indistinguishable from a legitimate "0" value. For `DRIVER_DUMP_MODE`, the code clamps to `DPAA2_SEC_DP_FULL_DUMP` but does not detect invalid input (e.g., negative numbers, non-numeric strings). For `DRIVER_STRICT_ORDER`, `!atoi(env)` treats any non-zero value as "loose ordered disabled." This is fragile. Use `strtol()` with error checking.

**`rte_kvargs_process()` return value checked but success path unclear**
The code does `ret = rte_kvargs_process(...); if (!ret) return;`. If `rte_kvargs_process()` returns 0 on success and non-zero on error, then `if (!ret)` means success, and the function returns early without falling through to `env_set`. However, if an error occurs, the code falls through to `env_set` as a fallback. This logic is correct only if `rte_kvargs_process()` returns 0 on success. Verify this is the intended behavior and add a comment explaining the fallback logic.

### Warnings

**Missing release notes**
New feature (environment variable configuration) requires release notes entry.

**Missing documentation**
The new environment variables (`DRIVER_STRICT_ORDER`, `DRIVER_DUMP_MODE`) must be documented in the driver's documentation (likely `doc/guides/cryptodevs/dpaa2_sec.rst`) explaining their purpose, valid values, and interaction with devargs.

---

## General Issues Across All Patches

### Warnings

**Patch series does not update MAINTAINERS if needed**
If any new files or subsystems are introduced, `MAINTAINERS` should be updated. This series only modifies existing files, so no update is needed.

**No update to driver feature matrix**
If new capabilities are added (AES-GMAC, ECN), the driver's feature list in `doc/guides/cryptodevs/features/` should be updated to reflect the new supported features.

---

## Summary

**Critical Issues (Must Fix):**
1. **Patch 1**: Verify sge+5..sge+7 initialization if 8 SGEs are actually used
2. **Patch 2**: Use distinct loop counter for cleanup
3. **Patch 3**: New AEAD enum requires experimental marking or documentation strategy
4. **Patch 6**: Remove `getenv()` calls or move to EAL-controlled configuration
5. **Patch 6**: Define `DRIVER_STRICT_ORDER` and `DRIVER_DUMP_MODE` constants
6. **Patch 6**: Validate `atoi()` return values

**Should Fix (Warnings):**
- All patches: Add release notes entries for fixes and new features
- Patch 3: Check error propagation for `-ENOTSUP` return
- Patch 4: Verify Signed-off-by vs. From: mismatch
- Patch 6: Document new environment variables
- Series: Update driver feature matrix if applicable