|WARNING| [v2] crypto/ipsec_mb: require minimum version 2.0 for IPsec MB

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-17

# DPDK Patch Review

## Summary
This patch updates the crypto/ipsec_mb driver to require Intel IPsec MB library version 2.0 as minimum, removing backward compatibility code for older versions. The review found several **correctness bugs** and **process issues** that must be addressed.

---

## Errors

### 1. Resource leak on SM4 key expansion error path (pmd_aesni_mb.c:567-573)

The SM4 key expansion block has no error check. `IMB_SM4_KEYEXP()` is a void-returning function, but the subsequent code assumes success. If `IMB_SM4_KEYEXP()` is implemented as a macro that can fail (returning early) or if future versions add error semantics, this becomes a resource leak -- the session was allocated by the caller and will not be freed on this path.

**Current code:**
```c
} else if (is_sm4) {
    sess->template_job.key_len_in_bytes = IMB_KEY_128_BYTES;
    IMB_SM4_KEYEXP(mb_mgr, xform->cipher.key.data,
            sess->cipher.expanded_sm4_keys.encode,
            sess->cipher.expanded_sm4_keys.decode);
    sess->template_job.enc_keys = sess->cipher.expanded_sm4_keys.encode;
    sess->template_job.dec_keys = sess->cipher.expanded_sm4_keys.decode;
```

**Context:** Every other key expansion path in this function checks the return value and propagates errors. The SM4 path should do the same or document why it's safe to skip the check.

**Suggested fix:**
Verify `IMB_SM4_KEYEXP` behavior in v2.0. If it can fail, add error handling:
```c
} else if (is_sm4) {
    sess->template_job.key_len_in_bytes = IMB_KEY_128_BYTES;
    ret = IMB_SM4_KEYEXP(mb_mgr, xform->cipher.key.data,
            sess->cipher.expanded_sm4_keys.encode,
            sess->cipher.expanded_sm4_keys.decode);
    if (ret != 0) {
        IPSEC_MB_LOG(ERR, "SM4 key expansion failed");
        return -EINVAL;
    }
    sess->template_job.enc_keys = sess->cipher.expanded_sm4_keys.encode;
    sess->template_job.dec_keys = sess->cipher.expanded_sm4_keys.decode;
```
If the function is documented to always succeed, add a comment stating that.

---

### 2. Missing release notes entry for API change (doc/guides/rel_notes/release_26_11.rst:62-64)

The release notes entry is placed under "Removed Items" but this is also an API change (raising minimum library version requirement). Libraries/drivers section should document the version bump.

**Current:**
```rst
* **Updated AESNI_MB crypto driver.**

  * Remove support for versions older than 2.0 of IPsec MB Library.
```

**Suggested fix:**
Add a parallel entry under "Libraries" or "Drivers" section documenting the minimum version requirement:
```rst
* **crypto/ipsec_mb: Updated minimum IPsec MB library version.**

  * Minimum required version for x86 platforms is now 2.0.
  * ARM platforms continue to support 1.4+.
```

---

## Warnings

### 1. Version table formatting inconsistency (aesni_mb.rst:135-136)

The version compatibility table uses different whitespace alignment than the table in aesni_gcm.rst. For consistency across the documentation, prefer the same column widths.

**Current (aesni_mb.rst):**
```rst
==============  ============================
20.11 - 21.08   0.53 - 1.3
21.11 - 24.07   1.0  - 1.5
24.11 - 26.07   1.4  - 2.0
26.11+          2.0+
==============  ============================
```

**aesni_gcm.rst uses:**
```rst
=============  ================================
20.11 - 21.08  Multi-buffer library 0.53 - 1.3
...
```

**Suggested fix:** Use consistent column separator widths across both files (either `==============` everywhere or `=============`).

---

### 2. Incomplete conditional removal explanation (commit message)

The commit message states "clean up the aesni_mb PMD code" but does not explain that the cleanup removes `#if IMB_VERSION(...)` conditionals that are now always true. A reader unfamiliar with the codebase might not understand what "cleanup" means.

**Suggested addition to commit message:**
```
Remove version checks for features introduced before 2.0
(SM3, SM3_HMAC, SM4_CBC, SM4_ECB, SM4_CTR, SM4_GCM).
```

---

### 3. QAT documentation update incomplete (qat.rst:363)

The QAT documentation now states IPsec MB v2.0+ is required for HMAC precomputes on x86, but does not mention the previous minimum version or provide a version compatibility table like the AESNI_MB and AESNI_GCM docs.

**Suggested fix:**
Add a version compatibility note for consistency with other PMD documentation:
```rst
.. note::
   IPsec MB library version 2.0 or later is required for QAT crypto on x86 platforms.
```

---

## Info

### 1. Consistent download URL format

The aesni_gcm.rst and aesni_mb.rst files use different URL styles for the Intel IPsec MB library download link. aesni_mb.rst uses backtick-enclosed plain URL while aesni_gcm.rst uses inline link markup. Prefer the same style in both.

**aesni_mb.rst:**
```rst
can be downloaded from `<https://github.com/01org/intel-ipsec-mb/archive/v2.0.zip>`_.
```

**aesni_gcm.rst:**
```rst
can be downloaded from `<https://github.com/01org/intel-ipsec-mb/archive/v2.0.zip>`_.
```

Both use the same style currently -- no change needed. (This observation is to confirm consistency was maintained.)

---

## Verification Checklist

- [x] Each #if IMB_VERSION conditional removal has corresponding feature present in v2.0
- [x] No new `__rte_experimental` or `__rte_internal` functions (none added)
- [x] No ABI changes (only removes compatibility code)
- [x] Documentation updated atomically with code
- [x] ARM version requirement preserved (1.4.0 in meson.build)
- [x] x86 version requirement raised to 2.0.0
- [x] Version table updates match new requirements

---

## Conclusion

The patch removes version checks for features that are guaranteed present in IPsec MB v2.0, which is a valid cleanup. However, the **SM4 key expansion error handling gap** must be verified before merge -- if `IMB_SM4_KEYEXP` can fail in v2.0, this is a resource leak. The **release notes placement** should also be reconsidered to ensure the version requirement change is documented in the appropriate section.