|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

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

### Errors

**Buffer overflow fix incomplete - missing buffer pool allocation update**

The patch increases `FLE_POOL_BUF_SIZE` from 256 to 288 bytes to accommodate 9 SGEs (8 entries + old_icv at index 8). However, the FLE pool is created with `rte_mempool_create()` somewhere in the initialization path, and this size increase requires recreating the pool with the new buffer size. The patch does not show that the pool is drained and recreated, which means existing pool buffers remain 256 bytes. When `rte_mempool_get()` returns a stale 256-byte buffer, the old_icv write at offset 256 still overflows.

**Fix**: Ensure the FLE pool is either:
1. Drained and recreated during device reconfigure when `FLE_POOL_BUF_SIZE` changes, or
2. Document that this change requires a full application restart to take effect (not just a driver reload).

### Warnings

**Hardcoded buffer size calculation**

The comment "9 x 32 bytes" hardcodes the SGE size. If `sizeof(struct dpaa2_sge)` ever changes, this calculation breaks. Consider:
```c
#define FLE_POOL_BUF_SIZE (9 * sizeof(struct dpaa2_sge))
```

**Missing `FLE_SG_MEM_SIZE` macro update consistency**

The `FLE_SG_MEM_SIZE(num)` macro adds `(num) * 32` to `FLE_POOL_BUF_SIZE`. With the new base size of 288, this macro now assumes the base accommodates 9 SGEs. If `num` is meant to be *additional* SGEs beyond the base, the macro should be reviewed for correctness. If `num` is the *total* SGE count, then callers passing `num < 9` will over-allocate. Clarify the intended semantics.

---

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

### Errors

**Loop variable reuse causes double-free or skipped cleanup**

The cleanup loop reuses the same `loop` variable that was just incremented by the outer loop. After the failed `build_sec_fd()` call, `loop` contains the index of the failed entry. The patch then does:

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

This **modifies the outer loop counter** inside the error path. When control returns to the outer loop's increment (`loop++`), the counter has already been reset to `frames_to_send - 1` by the inner loop's final iteration, causing the outer loop to skip ahead or iterate incorrectly.

**Fix**: Use a distinct cleanup loop variable:
```c
frames_to_send = loop + 1;
for (uint32_t i = 0; i < frames_to_send; i++)
    free_fle(&fd_arr[i], dpaa2_qp);
```

(Assuming the outer loop does not need to continue after `goto skip_tx`, which appears to be the case since `skip_tx` typically returns.)

**Missing error propagation in return value**

After freeing the FLEs, the code jumps to `skip_tx`, which likely returns a partial count of successfully enqueued frames. However, the calling application has no indication that `build_sec_fd()` *failed* due to `-ENOMEM` versus simply hitting a queue-full condition. Consider whether the return path should set an error flag or return a negative value to distinguish these cases.

**Same issue in `dpaa2_sec_enqueue_burst_ordered`**

The second hunk has identical loop variable reuse and the same double-free / counter corruption risk.

---

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

### Errors

**Public API change without `__rte_experimental` or ABI versioning**

The patch adds `RTE_CRYPTO_AEAD_AES_GMAC` to `enum rte_crypto_aead_algorithm` in `lib/cryptodev/rte_crypto_sym.h`, which is a **stable public API header**. Adding an enum value is an ABI break on some platforms (enum size may change) and requires either:

1. Marking the new enum value with a comment indicating it is experimental, **or**
2. Following the ABI versioning policy for the cryptodev library.

Since this is a new feature (not a fix), the enum addition must be accompanied by a release notes update and potentially an ABI version bump.

**Fix**: Add a comment next to `RTE_CRYPTO_AEAD_AES_GMAC` indicating it is available from DPDK 26.xx, and update the release notes under "New Features" to document the new AEAD algorithm.

**Incorrect comment - AES-GMAC is not NULL encryption**

The comment in `dpaa2_sec_ipsec_aead_init()` states:
```c
/**
 * AES-GMAC is an AEAD algo with NULL encryption and GMAC
 * authentication.
 */
```

This is **incorrect**. AES-GMAC is the *authentication mode* of GCM with a zero-length plaintext. It does not imply "NULL encryption" in the sense of a separate cipher -- it is simply GCM with no data to encrypt. The comment should be:

```c
/* AES-GMAC is GCM AEAD with zero-length plaintext (authentication-only). */
```

### Warnings

**Missing release notes update**

The patch adds a new AEAD algorithm (`RTE_CRYPTO_AEAD_AES_GMAC`) and support for it in the dpaa2_sec PMD. This is a new feature and requires an update to the current release notes (`doc/guides/rel_notes/release_26_03.rst` or equivalent) under "New Features."

**Missing capability test**

New AEAD algorithms should have test vectors added to `app/test/test_cryptodev_aead_test_vectors.h` and test cases in `app/test/test_cryptodev.c` to validate the driver's implementation. The patch does not add these.

---

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

### Warnings

**Release notes missing**

Changing the advertised IV size range for AES-CTR is a user-visible capability change and should be documented in the release notes under "Resolved Issues" or "New Features" (depending on whether this fixes a known limitation or is an enhancement).

---

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

### Warnings

**Release notes missing**

Adding ECN capability is a user-visible feature addition and should be documented in the release notes.

---

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

### Errors

**`getenv()` forbidden in drivers**

The patch uses `getenv()` in a driver (not in `lib/eal/`), which is forbidden per the guidelines. Environment variables should be exposed through EAL command-line arguments or device arguments, not read directly by drivers.

**Fix**: Remove the `getenv()` calls. If runtime configuration is needed without devargs, consider:
1. Adding EAL-level flags that the driver can query via EAL APIs, **or**
2. Documenting that users must use devargs (`--vdev` or `--allow` with parameters).

**Undefined `DRIVER_STRICT_ORDER` and `DRIVER_DUMP_MODE` macros**

The code references `DRIVER_STRICT_ORDER` and `DRIVER_DUMP_MODE`, which are not defined anywhere in the patch. These must be:
```c
#define DRIVER_STRICT_ORDER "DPAA2_SEC_STRICT_ORDER"
#define DRIVER_DUMP_MODE "DPAA2_SEC_DUMP_MODE"
```
or similar. Without these definitions, the code will not compile.

**Potential NULL pointer dereference from `atoi()`**

`atoi(env)` is called without checking if `env` is NULL. While `getenv()` returns NULL if the variable is not set, the `if (env)` check guards the call, so this is not a bug. However, `atoi()` on an empty string (`""`) returns 0, which may not be the intended default. Consider using `strtol()` with error checking.

**Memory leak on `rte_kvargs_parse()` failure path**

If `rte_kvargs_parse()` returns NULL, the function jumps to `env_set` without freeing `kvlist` (which is already NULL, so no leak), but the *intent* of the code is unclear. If `devargs->args` is malformed, should the driver fall back to environment variables, or should it fail? The current code silently falls back, which may hide configuration errors.

### Warnings

**Mixing devargs and environment variables is confusing**

The function first processes devargs, then unconditionally checks environment variables. If a user sets both a devarg and an environment variable, which takes precedence? The code suggests environment variables override devargs (since `env_set` runs after `rte_kvargs_process()`), but this is counterintuitive -- command-line arguments typically override environment variables.

**Suggest**: Either:
1. Only check environment variables if devargs are not provided, **or**
2. Document the precedence order clearly in the driver documentation.

**`dpaa2_sec_dp_dump` clamping logic duplicates validation**

The clamping:
```c
if (dpaa2_sec_dp_dump > DPAA2_SEC_DP_FULL_DUMP)
    dpaa2_sec_dp_dump = DPAA2_SEC_DP_FULL_DUMP;
```
should likely also apply to the devargs path (not shown in the patch). If the devargs handler does not clamp, then setting `dump_mode=999` via devargs could bypass the limit, while setting `DRIVER_DUMP_MODE=999` is clamped. Ensure both paths validate the same way.

---

## Summary

| Patch | Errors | Warnings | Recommendation |
|-------|--------|----------|----------------|
| 1/6 | 1 | 2 | Needs revision (buffer pool update) |
| 2/6 | 3 | 0 | Needs revision (loop variable reuse) |
| 3/6 | 2 | 2 | Needs revision (public API change process) |
| 4/6 | 0 | 1 | Acceptable with release notes |
| 5/6 | 0 | 1 | Acceptable with release notes |
| 6/6 | 3 | 3 | Needs revision (forbidden `getenv()`) |

**Overall**: Patches 1, 2, 3, and 6 require changes before merging. Patches 4 and 5 need release notes but are otherwise acceptable.