|WARNING| [v1] 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/165294

_AI Code Review_

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

# DPDK Patch Review

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

### Summary
This patch transitions QAT HMAC precomputes from OpenSSL to IPsec MB library, making IPsec MB mandatory and OpenSSL optional (for DOCSIS BPI cipher fallback only). The change removes ~400 lines of OpenSSL-based partial hash computation code.

---

## Errors

### 1. Resource leak in ossl_legacy_provider_load()
**Location:** Line 49-58

The function allocates `legacy_lib` and `default_lib` providers but never releases them on error paths or shutdown. If `OSSL_PROVIDER_load()` fails for `legacy_lib` after successfully loading `default_lib`, `default_lib` is leaked.

```c
/* Current code (BAD) */
static int ossl_legacy_provider_load(void)
{
    /* If legacy_lib load fails, default_lib is leaked */
    legacy_lib = OSSL_PROVIDER_load(NULL, "legacy");
    /* ... */
    default_lib = OSSL_PROVIDER_load(NULL, "default");
    /* No cleanup on error or normal shutdown */
}
```

**Fix:**
```c
/* Add cleanup on error path */
static int ossl_legacy_provider_load(void)
{
    legacy_lib = OSSL_PROVIDER_load(NULL, "legacy");
    if (legacy_lib == NULL) {
        QAT_LOG(ERR, "Failed to load legacy provider");
        return -EINVAL;
    }
    
    default_lib = OSSL_PROVIDER_load(NULL, "default");
    if (default_lib == NULL) {
        QAT_LOG(ERR, "Failed to load default provider");
        OSSL_PROVIDER_unload(legacy_lib);  /* cleanup on error */
        legacy_lib = NULL;
        return -EINVAL;
    }
    return 0;
}

/* Add shutdown function to be called on PMD cleanup */
static void ossl_legacy_provider_unload(void)
{
    if (legacy_lib != NULL) {
        OSSL_PROVIDER_unload(legacy_lib);
        legacy_lib = NULL;
    }
    if (default_lib != NULL) {
        OSSL_PROVIDER_unload(default_lib);
        default_lib = NULL;
    }
}
```

---

## Warnings

### 1. Missing documentation for legacy provider requirement
**Location:** doc/guides/cryptodevs/qat.rst, lines 369-372

The documentation explains that OpenSSL is required on ARM for DOCSIS but does not mention that OpenSSL 3.0's legacy provider must be available and loadable. Users may have OpenSSL 3.0 installed but without the legacy provider package (common on some distributions).

**Suggested addition:**
```rst
**On ARM platforms:**

* ARM IPsec MB library from ``gitlab.arm.com/arm-reference-solutions/ipsec-mb``
  is required for HMAC precomputes.
* OpenSSL (3.0+) is required for DOCSIS BPI cipher algorithms. ARM IPsec MB does not
  implement CFB-one-byte cipher modes needed for DOCSIS. Without OpenSSL, DOCSIS
  algorithms will not be available on ARM.
* OpenSSL 3.0 legacy provider must be available (``openssl-legacy`` package on some
  distributions) as DES algorithms are disabled by default in OpenSSL 3.0.
```

### 2. Meson build: ARM-specific logic not guarded for arch check correctness
**Location:** drivers/common/qat/meson.build, lines 32-37

The `IMB_header` assignment depends on `arch_subdir`, but the arch check happens before verifying `qat_crypto` is enabled. If QAT crypto is disabled earlier (e.g., missing QAT kernel drivers), the `arch_subdir == 'arm'` check is evaluated unnecessarily.

This is minor (no functional bug), but logically the IPsec MB check should only run when `qat_crypto == true` from earlier checks.

**Current structure:**
```python
# Line 32
if arch_subdir == 'arm':
    IMB_header = '#include<ipsec-mb.h>'
else:
    IMB_header = '#include<intel-ipsec-mb.h>'

# Check for IPsec MB library (required)
libipsecmb = cc.find_library('IPSec_MB', required: false)
# ... rest of checks
```

**Suggested improvement:**
```python
# Only do IPsec MB checks if qat_crypto wasn't already disabled
if qat_crypto
    if arch_subdir == 'arm'
        IMB_header = '#include<ipsec-mb.h>'
    else
        IMB_header = '#include<intel-ipsec-mb.h>'
    endif

    # Check for IPsec MB library (required)
    libipsecmb = cc.find_library('IPSec_MB', required: false)
    # ... rest
endif
```

### 3. Missing release notes update
**Location:** Not present in patch

This patch changes the build dependency requirements for QAT PMD (IPsec MB is now mandatory, not optional) and removes support for building with OpenSSL-only. This is a significant change that affects users and should be documented in the release notes.

**Suggested addition to doc/guides/rel_notes/release_XX_XX.rst:**
```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.
```

---

## Info

### 1. Consider using RTE_LOG macro consistently
**Location:** Lines 75-79 (meson.build messages)

The meson.build uses `message()` and `warning()` for user-facing build output, which is correct. However, for consistency with DPDK conventions, consider whether the warning on line 77 should be an error (build failure) on ARM when OpenSSL is missing, since ARM requires OpenSSL for DOCSIS and the PMD will be incomplete without it.

Current behavior: build succeeds but DOCSIS is silently unavailable.
Alternative: fail the build on ARM if OpenSSL >= 3.0 is not found, or add a more prominent meson configuration-time summary.

This is minor and depends on project policy for optional features.

---

## Overall Assessment

**Correctness:** The patch has one resource leak error that must be fixed (OpenSSL provider cleanup).

**Style:** Code follows DPDK conventions. The removal of OpenSSL-specific partial hash implementations is clean.

**Documentation:** Missing release notes for dependency change. ARM OpenSSL legacy provider requirement should be documented.

**API Impact:** No public API changes. Internal implementation change only.