|WARNING| [v3,6/6] crypto/dpaa2_sec: support AES-GMAC

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

**Buffer overflow fix incomplete - IVP branch missing sge+4 initialization**

The patch adds `DPAA2_SET_FLE_BPID(sge + 4, bpid);` in the bpid branch but adds `DPAA2_SET_FLE_IVP((sge + 4));` in the IVP branch. However, the commit message states "the upfront bpid/IVP initialization only covered sge+3, leaving sge+4 (the input data SGE when AAD is present) without a valid bpid or IVP assignment."

This means the original code was missing initialization of sge+4 in both branches. The patch correctly adds it to both branches, so this is actually correct. (Retracting potential issue after verification.)

### Warnings

**Missing release notes entry for correctness bug fix**

This patch fixes a buffer overflow that causes SMMU translation faults. This is a significant correctness bug affecting production deployments and should be documented in the release notes. Add an entry to `doc/guides/rel_notes/release_26_XX.rst` (where XX is the current minor version) under a "Fixed Issues" section for dpaa2_sec.

---

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

### Errors

None identified. The fix correctly clamps `frames_to_send` and iterates from 0 to free all allocated FLE buffers when `build_sec_fd` fails.

### Warnings

**Missing release notes entry**

This patch fixes a resource leak that causes silent FLE pool exhaustion and application hangs. Add an entry to the release notes documenting this fix.

---

## Patch 3/6: crypto/dpaa2_sec: increase ivsize range for AES-CTR

### Errors

None.

### Warnings

**Missing release notes entry**

This patch changes the supported IV size range for AES-CTR, which is an API-level behavioral change. Document this in the release notes under the dpaa2_sec driver section, noting that 96-bit (12-byte) IVs are now supported in addition to 128-bit.

---

## Patch 4/6: crypto/dpaa2_sec: add missing ECN capability

### Errors

None.

### Warnings

**Missing release notes entry**

This patch advertises a new capability (ECN support in IPsec tunnel mode). Document this in the release notes as a new feature or capability enhancement for the dpaa2_sec driver.

---

## Patch 5/6: crypto/dpaa2_sec: add support for env variables

### Errors

**Use of getenv() in library/driver code**

The patch adds calls to `getenv("drv_strict_order")` and `getenv("drv_dump_mode")` in `dpaa2_sec_get_devargs()`. According to the AGENTS.md Forbidden Tokens section, `getenv()` is prohibited in `lib/` and `drivers/` code.

DPDK drivers should use devargs for configuration, not environment variables. Environment variables bypass the standard DPDK configuration mechanism and create hidden dependencies that are not visible in application code or EAL arguments.

**Suggested fix:**

Remove the `getenv()` calls. If fallback configuration is needed when devargs are absent, use DPDK's internal configuration mechanisms or document that users must provide devargs. The current devargs mechanism is the correct way to configure drivers.

```c
/* Remove this entire env_set: section */
env_set:
	env = getenv(DRIVER_STRICT_ORDER);
	if (env)
		internals->en_loose_ordered = !atoi(env);

	env = getenv(DRIVER_DUMP_MODE);
	if (env) {
		dpaa2_sec_dp_dump = atoi(env);
		if (dpaa2_sec_dp_dump > DPAA2_SEC_DP_FULL_DUMP)
			dpaa2_sec_dp_dump = DPAA2_SEC_DP_FULL_DUMP;
	}
```

**Undefined macros DRIVER_STRICT_ORDER and DRIVER_DUMP_MODE**

The macros `DRIVER_STRICT_ORDER` and `DRIVER_DUMP_MODE` are used as string literals in `getenv()` but are not defined in this patch or visible in the existing code. These should either be defined as string constants or the literal strings should be used directly (though the entire `getenv()` approach should be removed as noted above).

### Warnings

**Documentation update without corresponding code that should be present**

The documentation in `dpaa2_sec.rst` describes using environment variables `drv_dump_mode` and `drv_strict_order`, but the code uses macro names `DRIVER_DUMP_MODE` and `DRIVER_STRICT_ORDER` which would expand to different strings. The documentation and code variable names are inconsistent.

---

## Patch 6/6: crypto/dpaa2_sec: support AES-GMAC

### Errors

None. The AES-GMAC implementation follows the existing driver patterns correctly.

### Warnings

**Missing release notes entry**

This patch adds support for a new algorithm (AES-GMAC) in both symmetric auth-only and IPsec lookaside protocol modes. This is a significant new feature that should be documented in the release notes under "New Features" for the dpaa2_sec driver.

**Features matrix update is correct**

The patch correctly updates `doc/guides/cryptodevs/features/dpaa2_sec.ini` to add `AES GMAC = Y`.

---

## Summary

**Errors to fix:**
1. **Patch 5**: Remove `getenv()` calls - forbidden in drivers/lib code
2. **Patch 5**: Define or remove undefined macros `DRIVER_STRICT_ORDER` and `DRIVER_DUMP_MODE`

**Warnings to address:**
1. **Patches 1, 2, 3, 4, 6**: Add release notes entries for all significant changes (fixes, API changes, new features)
2. **Patch 5**: Fix documentation/code inconsistency in environment variable names (or remove entirely per Error #1)

**Correctness bugs found:** None in patches 1-4 and 6. Patch 5 has a process violation (forbidden token) rather than a correctness bug.