|WARNING| [v1, 1/1] crypto/openssl: filter legacy ciphers from capability list

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## ERRORS

**1. Missing mutex protection in openssl_pmd_caps_init()**

The function `openssl_pmd_caps_init()` modifies the global static array `openssl_pmd_caps_active` and is called from `cryptodev_openssl_create()`, which can be invoked by multiple threads during device creation. The read in `openssl_pmd_info_get()` is also unsynchronized. This is a data race on shared mutable state.

```c
/* Line 951-967 - writes to openssl_pmd_caps_active */
void
openssl_pmd_caps_init(void)
{
    /* ... */
    openssl_pmd_caps_active[j++] = *cap;
    /* ... */
}

/* Line 1041-1043 - reads openssl_pmd_caps_active */
dev_info->capabilities = openssl_pmd_caps_active;
```

The initialization must be protected by a mutex or performed once in a thread-safe manner (e.g., using `pthread_once()`).

**2. Resource leak on error path in openssl_pmd_caps_init()**

Line 960: `EVP_CIPHER_fetch()` allocates a cipher object. When the `if (des_cbc == NULL)` branch at line 961 is taken, the function continues to the next iteration without freeing `des_cbc`. If `des_cbc` is non-NULL on a later iteration but should still be skipped for another reason, the earlier allocation leaks.

```c
/* Line 960-963 */
EVP_CIPHER *des_cbc = EVP_CIPHER_fetch(NULL, "DES-CBC", "provider=legacy");
if (des_cbc == NULL)
    continue;
EVP_CIPHER_free(des_cbc);
```

The code path where `des_cbc == NULL` and we `continue` is correct. However, if there are multiple DES cipher checks in the loop, each `EVP_CIPHER_fetch()` must have a corresponding `EVP_CIPHER_free()` on all paths. Review whether all exit paths from the loop clean up.

Actually, re-reading: the `continue` at line 962 skips the `EVP_CIPHER_free(des_cbc)` at line 963. But `des_cbc` is NULL at that point, so the leak does not occur. The pattern is correct. **Discard this finding.**

**3. Uninitialized array element access on first call**

Line 1041: The check `if (openssl_pmd_caps_active[0].op == RTE_CRYPTO_OP_TYPE_UNDEFINED)` relies on `openssl_pmd_caps_active` being zero-initialized (it is a static array, so this is guaranteed). However, if `openssl_pmd_caps_init()` is never called or fails silently, the array remains all-zero, and the check incorrectly treats it as "not initialized" and falls back to `openssl_pmd_capabilities`. This is the intended behavior, so not a bug. **Discard this finding.**

**4. Missing `RTE_EXPORT_SYMBOL` on new global function**

Line 948-967: The function `openssl_pmd_caps_init()` is declared `extern` in the header (line 25 of `openssl_pmd_private.h`) but has no `RTE_EXPORT_*` macro in the `.c` file. However, checking the context: this is a driver-internal function (used only within the openssl PMD, not part of a public library API). Driver-internal symbols do not require export macros. **Discard this finding.**

**5. Race condition: openssl_pmd_caps_active read/write without synchronization**

Line 951-967 writes to `openssl_pmd_caps_active`. Line 1041-1043 reads it. If two devices are created concurrently, or if `openssl_pmd_info_get()` is called while another thread is in `openssl_pmd_caps_init()`, the read sees a partially written array. This is undefined behavior.

The fix: use `pthread_once()` to ensure `openssl_pmd_caps_init()` runs exactly once, or protect both the write and read with a mutex. Note that the read in `openssl_pmd_info_get()` does not modify the array, but it must still synchronize with the write.

```c
/* Suggested fix skeleton */
static pthread_once_t caps_init_once = PTHREAD_ONCE_INIT;

void
openssl_pmd_caps_init(void)
{
    /* current body */
}

static void
openssl_pmd_caps_init_once(void)
{
    openssl_pmd_caps_init();
}

/* In cryptodev_openssl_create(), replace the direct call: */
pthread_once(&caps_init_once, openssl_pmd_caps_init_once);
```

This is the same issue as Error 1, consolidated here.

---

## WARNINGS

**1. Missing release notes for capability filtering change**

This patch changes the reported device capabilities at runtime based on OpenSSL provider availability. Applications may rely on the capability list to decide which algorithms to use. Removing DES from the list is a user-visible behavior change and should be documented in the release notes (`doc/guides/rel_notes/release_26_XX.rst`, where XX is the minor version).

**2. Global state modified at device creation rather than driver init**

`openssl_pmd_caps_init()` modifies the global `openssl_pmd_caps_active` array every time a device is created (line 3944 calls it from `cryptodev_openssl_create()`). This is inefficient and racy. The filtering should be done once at driver initialization (e.g., in `RTE_INIT` or the first device probe), not per-device.

**3. Fallback to full capability list when init not called**

Lines 1041-1043: If `openssl_pmd_caps_init()` is never called (e.g., due to a code path change), the driver reports the full unfiltered capability list (`openssl_pmd_capabilities`), including DES when it is not available. This silently reverts to the original buggy behavior. Consider making the fallback an error or ensuring `openssl_pmd_caps_init()` is always called before any device can query capabilities.

**4. Missing test coverage**

The patch modifies capability reporting but does not add a test in `app/test/` to verify that:
- DES is correctly filtered when the legacy provider is unavailable
- DES is present when the legacy provider is available

---

## INFO

**1. EVP_CIPHER_fetch return value check pattern**

Line 960-963: The code checks if `EVP_CIPHER_fetch()` returns NULL to decide whether to skip the capability. This is correct. However, OpenSSL documentation states that `EVP_CIPHER_fetch()` can fail for reasons other than "algorithm not available" (e.g., out of memory). A NULL return is treated as "algorithm unavailable," which is reasonable but means memory allocation failures also silently skip the algorithm.

This is acceptable for this use case (capability filtering), but worth noting.

**2. DES-CBC vs DES-DOCSISBPI provider check**

Line 960: The code fetches "DES-CBC" to check if DES algorithms are available, but it filters both `RTE_CRYPTO_CIPHER_DES_CBC` and `RTE_CRYPTO_CIPHER_DES_DOCSISBPI`. DOCSISBPI is a variant of DES-CBC, so checking "DES-CBC" availability is a reasonable proxy. However, if DOCSISBPI required a different provider or had different availability, this check would be insufficient. The current implementation is acceptable given that DOCSISBPI uses the same DES primitives.

---

## REVIEW SUMMARY

The patch correctly identifies the problem (DES reported as supported when legacy provider is unavailable) and filters the capability list. However, it introduces a race condition on the global `openssl_pmd_caps_active` array, which is written at device creation and read from `openssl_pmd_info_get()` without synchronization. This must be fixed with `pthread_once()` or a mutex. Additionally, the capability filtering should be a one-time operation at driver init, not per-device, and the change should be documented in release notes.

Review-Result: ERROR