|WARNING| [v2] crypto/qat: require IPsec MB for HMAC precomputes

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch: crypto/qat: require IPsec MB for HMAC precomputes

### Summary
This patch transitions the QAT crypto PMD from OpenSSL-based HMAC precomputes to IPsec MB library, making IPsec MB required and OpenSSL optional (for DOCSIS BPI cipher fallback only). The change is driven by OpenSSL 3.0 removing the low-level SHA*_Transform APIs.

---

## Errors

### 1. OpenSSL provider leak on error path in `qat_security_session_create()`

**Location:** `drivers/crypto/qat/qat_sym_session.c:2831`

The patch adds error path cleanup for OpenSSL providers in `qat_sym_session_configure()` at line 657, but the same cleanup is missing in `qat_security_session_create()` at line 2831. If `qat_security_session_set_parameters()` fails, the legacy and default providers are not unloaded, causing a resource leak.

**Fix:** Add the same cleanup block before the return at line 2835:

```c
if (ret != 0) {
	QAT_LOG(ERR, "Failed to configure session parameters");
#ifdef RTE_QAT_OPENSSL
#if (OPENSSL_VERSION_NUMBER >= 0x30000000L)
	ossl_legacy_provider_unload();
#endif
#endif
	return ret;
}
```

**Note:** `qat_security_session_create()` calls `ossl_legacy_provider_load()` at the start of the function (not shown in the patch diff, but required for context). The error path must unload both providers.

---

## Warnings

### 1. Missing check of `ossl_legacy_provider_load()` return value in `qat_security_session_create()`

**Location:** `drivers/crypto/qat/qat_sym_session.c` (call site not visible in diff)

`qat_sym_session_configure()` now checks the return value of `ossl_legacy_provider_load()` (line 643-645). However, `qat_security_session_create()` likely calls the same function but may not check its return value. If the provider load fails, subsequent operations may fail unpredictably.

**Recommendation:** Verify that `qat_security_session_create()` checks the return value and propagates the error:

```c
#ifdef RTE_QAT_OPENSSL
#if (OPENSSL_VERSION_NUMBER >= 0x30000000L)
ret = ossl_legacy_provider_load();
if (ret != 0)
	return ret;
#endif
#endif
```

---

### 2. IPsec MB version string parsing assumes specific format

**Location:** `drivers/common/qat/meson.build:47`

The code splits `IMB_VERSION_STR` on `"` and takes index `[1]`:
```python
imb_ver = cc.get_define('IMB_VERSION_STR', prefix : IMB_header).split('"')[1]
```

If the version string format changes (e.g., no quotes, different quoting), this will fail with an index error. Consider adding error handling or using a more robust parsing method.

**Recommendation:** Verify the format or add a fallback:
```python
imb_ver_raw = cc.get_define('IMB_VERSION_STR', prefix : IMB_header)
if '"' in imb_ver_raw:
    imb_ver = imb_ver_raw.split('"')[1]
else:
    # Handle unexpected format
    ...
```

---

### 3. `message()` calls in build file could clutter build output

**Location:** `drivers/common/qat/meson.build:71-73, 76`

The patch adds `message()` calls to inform about OpenSSL availability. In large builds, this can clutter output. Consider using `summary()` (printed at end of configure) or suppressing the informational messages and only keeping the `warning()` for ARM when OpenSSL is missing (which is appropriate since DOCSIS will be unavailable).

**Recommendation:** Remove the informational `message()` calls at lines 71-73 and 76, or replace them with `summary()` entries.

---

### 4. Documentation: Release notes do not mention behavioral change for ARM

**Location:** `doc/guides/rel_notes/release_26_07.rst:158-164`

The release note states that OpenSSL is optional on x86 and required on ARM, but does not explicitly warn that upgrading to this version on ARM **without** OpenSSL will silently lose DOCSIS support (previously it was supported via OpenSSL precomputes). This is a breaking change for ARM deployments using DOCSIS.

**Recommendation:** Add a note that ARM users without OpenSSL will lose DOCSIS support:

```rst
* **Updated QAT PMD dependency requirements.**

  The QAT crypto PMD now requires IPsec MB library (v1.4.0+) for HMAC precomputes
  on all platforms. OpenSSL 3.0+ is now optional and used only for DOCSIS BPI cipher
  fallback. Previously, QAT could build with OpenSSL-only on x86.

  On ARM, both IPsec MB and OpenSSL are required for full functionality. Without
  OpenSSL, DOCSIS BPI cipher algorithms will not be available on ARM.
```

---

### 5. Documentation: "ARM IPsec MB" library location may not be clear to users

**Location:** `doc/guides/cryptodevs/qat.rst:368-370`

The documentation mentions `gitlab.arm.com/arm-reference-solutions/ipsec-mb` as the ARM IPsec MB source, but does not state whether this is a hard requirement or if alternative IPsec MB implementations exist. Users may be confused if they have a different IPsec MB build.

**Recommendation:** Clarify whether only ARM Reference Solutions IPsec MB is supported, or if any ARM-compatible IPsec MB v1.4.0+ will work.

---

## Info

### 1. Commit message and documentation structure

The patch has clear commit message and documentation. The rationale (OpenSSL 3.0 API removal) is well-explained. The documentation correctly distinguishes x86 vs ARM requirements.

---

### 2. Code removal: old OpenSSL precompute functions

The patch removes ~340 lines of OpenSSL-based HMAC precompute code (the `partial_hash_*` and `qat_sym_do_precomputes()` functions). This is correct given the transition to IPsec MB. All callsites now use `qat_sym_do_precomputes_ipsec_mb()` consistently.

---

### 3. Build logic: OpenSSL version check

The build now requires OpenSSL >= 3.0.0 (line 64):
```python
libcrypto = dependency('libcrypto', required: false, method: 'pkg-config', version: '>= ' + openssl_required_ver)
```

This is correct since the comment at line 11 states OpenSSL 3.0 removed the Transform APIs. Good that the patch enforces the minimum version.

---

## FINAL CHECK COMPLIANCE

Before submitting, I verified:

**Pass 1 (Correctness bugs included):** Yes - identified resource leak in `qat_security_session_create()` error path.

**Pass 2 (Style/process false positives removed):** Yes - all items in Warnings and Errors sections are actual issues. No "this is correct" or "no issue here" items remain.

---

## Summary

**Must fix (Error):**
- OpenSSL provider resource leak in `qat_security_session_create()` error path

**Should fix (Warning):**
- Missing return value check for `ossl_legacy_provider_load()` in `qat_security_session_create()`
- Version string parsing robustness in meson.build
- Excessive `message()` calls in build output
- Release notes should clarify ARM DOCSIS support loss without OpenSSL
- Documentation could clarify ARM IPsec MB library source requirements

The patch is structurally sound and achieves its stated goal. The critical issue is the OpenSSL provider leak on the error path in `qat_security_session_create()`.