|WARNING| [v2,7/7] net/ixgbe: refactor IPsec code

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/7: net/ixgbe: add key length check in IPsec

### Errors
None.

### Warnings
None.

### Info
None.

---

## Patch 2/7: net/ixgbe: remove unnecessary dev pointer

### Errors
None.

### Warnings
None.

### Info
None.

---

## Patch 3/7: net/ixgbe: harmonize return values in IPsec

### Errors
None.

### Warnings
None.

### Info
None.

---

## Patch 4/7: net/ixgbe: get rid of unnecessary malloc in IPsec

### Errors

**Key material not wiped with rte_memzero_explicit() in scope-exit path (error path):**
The `key` array is zeroed after successful HW write, but on error paths between key initialization and the explicit zero, the key may remain on the stack. If the function returns early (e.g., `-ENOSPC` return paths), the key is not wiped.

**Fix:** Add `rte_memzero_explicit(key, sizeof(key));` on all early return paths after `memcpy(key, ic_session->key, ic_session->key_len);` in both Rx and Tx branches.

```c
/* Example fix for Rx path */
memcpy(key, ic_session->key, ic_session->key_len);

/* ... HW writes ... */

/* Before any return, wipe key */
if (error_condition) {
    rte_memzero_explicit(key, sizeof(key));
    return -ENOSPC;
}

rte_memzero_explicit(key, sizeof(key));
```

Note: Currently the function has no error paths after the `memcpy`, but this is fragile. Best practice is to wipe on all exits.

### Warnings
None.

### Info
None.

---

## Patch 5/7: net/ixgbe: use correct index variable in IPsec

### Errors
None.

### Warnings
None.

### Info
None.

---

## Patch 6/7: net/ixgbe: fix missing HW write in IPsec

### Errors
None.

### Warnings
None.

### Info
None.

---

## Patch 7/7: net/ixgbe: refactor IPsec code

### Errors

**Missing key wipe in helper functions:**
The new helper functions `ixgbe_crypto_write_rx_key()` and `ixgbe_crypto_write_tx_key()` receive a raw key pointer and write it to HW, but the callers pass a stack-allocated `key` array that is zeroed after the helper returns. This is correct in the current call sites, but the helpers themselves do not document the caller's responsibility to wipe the key, creating a fragile contract.

**Mitigation:** While not strictly a bug in this patch (callers do wipe), add a comment in the helper function Doxygen or inline noting that the caller must zero the key buffer after use. This prevents future misuse.

```c
/* Caller must wipe key buffer with rte_memzero_explicit() after call */
static inline void
ixgbe_crypto_write_rx_key(...)
```

**SPI storage inconsistency:**
In `ixgbe_crypto_add_sa()`, the Rx path stores `rxsa->spi = ic_session->spi;` (host byte order), while the Tx path stores `txsa->spi = ic_session->spi;` (also host byte order). However, in the HW write helpers, Rx SPI is written as `rte_cpu_to_be_32(spi)`. The remove functions now compare `priv->rx_sa_tbl[i].spi == ic_session->spi` (host order) and `priv->tx_sa_tbl[i].spi == ic_session->spi` (host order).

The old code stored Rx SPI as `rte_cpu_to_be_32(ic_session->spi)` and compared it against `rte_cpu_to_be_32(ic_session->spi)`, which was consistent. The new code stores SPI in host order but the comparison is now:

```c
/* Old Rx add: */
priv->rx_sa_tbl[sa_index].spi = rte_cpu_to_be_32(ic_session->spi);

/* Old Rx remove: */
if (priv->rx_sa_tbl[i].spi == rte_cpu_to_be_32(ic_session->spi))

/* New Rx add: */
rxsa->spi = ic_session->spi;  /* host order */

/* New Rx remove: */
if (priv->rx_sa_tbl[i].spi == ic_session->spi)  /* host order comparison */
```

This changes the semantics: the table now stores SPI in host byte order, but the HW write still converts to big-endian. This is a **correctness change** not mentioned in the commit message. It may be intentional (storing in host order simplifies comparisons), but the patch should explicitly document this semantic change.

**Fix:** Either (1) note in the commit message that SPI storage format changed from network to host byte order, or (2) revert to storing SPI in network byte order and adjust comparisons.

### Warnings

**Tx SPI storage in old code:**
The old Tx path stored `priv->tx_sa_tbl[sa_index].spi = rte_cpu_to_be_32(ic_session->spi);` (big-endian), and the remove path compared `priv->tx_sa_tbl[i].spi == rte_cpu_to_be_32(ic_session->spi)`. The new code stores `txsa->spi = ic_session->spi;` (host order) and compares `priv->tx_sa_tbl[i].spi == ic_session->spi` (host order). Same semantic change as Rx.

This is consistent with the Rx change above, but should be documented.

### Info

**Global variable naming:**
The `key` local variable in helper callers could collide with a hypothetical global `key` symbol in static linking scenarios. Recommend renaming to `session_key` or `hw_key` for clarity and collision avoidance, though this is minor since it's a local variable.

---

## Summary

**Critical findings:**
1. Patch 4: Key material left on stack on error paths (though no error paths exist after key copy in current code, this is fragile).
2. Patch 7: SPI storage byte order changed from network to host without documentation; this is a semantic change that may affect on-disk/persistent structures or debugging.
3. Patch 7: Helper functions do not document caller's responsibility to wipe key.

**Recommendations:**
- Patch 4: Add explicit key wipe on all function exit paths (even if currently unreachable) for robustness.
- Patch 7: Document the SPI byte-order change in the commit message, or revert to network byte order storage if that was not intentional.
- Patch 7: Add comments to key-writing helper functions noting caller must wipe the key buffer.