|WARNING| [RFC,7/7] regex/hs: add Hyperscan compile flag support

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Code Review - regex/hs driver RFC patch series

## Summary

This patch series adds a new software regex PMD based on Intel Hyperscan. The implementation is generally sound and follows DPDK coding standards, with good documentation and clear structure. However, there are several correctness issues (resource leaks, error handling gaps) and a few style/process items that should be addressed.

---

## Patch 1/7: Driver skeleton and build integration

### Errors

**Resource leak on rte_hash creation failure**
The hash table created by `rte_hash_create()` in `rule_db_update` (patch 3) is never freed on error paths in `configure`. When `hs_regex_rule_db_import()` fails, the code frees `priv->qps` but does not check or free `priv->rule_id_hash` that was created earlier in the same function flow.

```c
/* In patch 3, hs_regex_rule_db_update creates the hash */
if (!priv->rule_id_hash) {
    priv->rule_id_hash = rte_hash_create(&hp);
    if (!priv->rule_id_hash) {
        HS_LOG(ERR, "Failed to create rule_id hash");
        return -ENOMEM;
    }
}

/* In hs_regex_configure, import failure path */
if (ret < 0) {
    HS_LOG(ERR, "Failed to import rule DB in configure");
    rte_free(priv->qps);
    priv->qps = NULL;
    priv->nb_queue_pairs = 0;
    priv->dev_state = HS_REGEX_DEV_CREATED;
    return ret;  /* BUG: rule_id_hash leaked if created earlier */
}
```

Fix: Add hash cleanup on configure error paths.

---

## Patch 2/7: Device configure and queue pair setup

### Errors

**Resource leak: QP ops ring not freed on scratch allocation failure**
In `hs_regex_qp_setup`, when `hs_alloc_scratch()` fails, the code returns `-ENOMEM` without freeing the `qp->ops` array that was just allocated.

```c
qp->ops = rte_zmalloc("hs_regex_qp_ops", ...);
if (!qp->ops) {
    HS_LOG(ERR, "Failed to allocate ops ring for qp %u", qp_id);
    return -ENOMEM;
}

qp->nb_desc = nb_desc;
qp->head = 0;
qp->tail = 0;
qp->count = 0;

if (priv->db) {
    err = hs_alloc_scratch(priv->db, &qp->scratch);
    if (err != HS_SUCCESS) {
        HS_LOG(ERR, "Failed to alloc scratch for qp %u", qp_id);
        /* BUG: qp->ops not freed */
        rte_free(qp->ops);  /* MISSING */
        qp->ops = NULL;
        return -ENOMEM;
    }
}
```

Fix: Free `qp->ops` before returning on scratch allocation failure.

**Configure state rollback incomplete**
In `hs_regex_configure`, when `hs_regex_rule_db_import()` fails, the cleanup frees `priv->qps` and sets `nb_queue_pairs = 0`, but does not reset other fields (`max_matches`, `nb_groups`) that were set earlier. This leaves the device in an inconsistent state between `CREATED` and `CONFIGURED`.

```c
if (ret < 0) {
    HS_LOG(ERR, "Failed to import rule DB in configure");
    rte_free(priv->qps);
    priv->qps = NULL;
    priv->nb_queue_pairs = 0;
    /* BUG: max_matches and nb_groups remain set from earlier */
    priv->dev_state = HS_REGEX_DEV_CREATED;
    return ret;
}
```

Fix: Reset `max_matches` and `nb_groups` to 0 on import failure.

---

## Patch 3/7: Rule database update and compilation

### Errors

**Use-after-free: pattern pointer freed before hash key deleted**
In `hs_regex_rule_db_update` `REMOVE` operation, the code frees `priv->rules[j].pattern` and then calls `memmove()` to shift the rules array, but the hash table key deletion happens **before** the free, using the `rule_id` from the rule that is about to be shifted. If the rule is not found in the array after hash deletion, the pattern is never freed.

```c
if (priv->rule_id_hash)
    rte_hash_del_key(priv->rule_id_hash, &rules[i].rule_id);

for (j = 0; j < priv->nb_rules; j++) {
    if (priv->rules[j].rule_id != rules[i].rule_id)
        continue;
    rte_free(priv->rules[j].pattern);  /* pattern freed */
    memmove(&priv->rules[j], &priv->rules[j + 1], ...);
    priv->nb_rules--;
    break;
}
/* If loop completes without finding rule_id in array (hash/array out of sync),
 * pattern is leaked. This is unlikely but possible if hash and array are updated
 * inconsistently in earlier operations. */
```

The immediate code path is safe, but relying on the loop to always find the rule creates fragility. A safer pattern would ensure the hash and array stay in sync or handle the not-found case explicitly.

Fix: Move hash deletion inside the loop after confirming the rule exists in the array, or add explicit handling for the not-found case.

**Scratch allocation rollback on partial failure incomplete**
In `hs_regex_rule_db_compile_activate`, when scratch allocation fails for queue pair `i`, the cleanup loop frees scratch for `qp[0..i-1]` but does not reset the database pointer or the `db_compiled` flag. If the user then calls `start()`, the device will attempt to use a database with no scratch space.

```c
for (j = 0; j < i; j++) {
    if (priv->qps[j].scratch) {
        hs_free_scratch(priv->qps[j].scratch);
        priv->qps[j].scratch = NULL;
    }
}
hs_free_database(priv->db);
priv->db = NULL;
return -ENOMEM;
/* BUG: db_compiled flag remains 1, inconsistent with db = NULL */
```

Fix: Set `priv->db_compiled = 0` after freeing the database.

**Same issue in hs_regex_rule_db_import**
The import function has the same bug: on scratch allocation failure, it frees the database but does not clear `db_compiled`.

```c
hs_free_database(priv->db);
priv->db = NULL;
/* BUG: db_compiled remains 1 */
return -ENOMEM;
```

Fix: Set `priv->db_compiled = 0` after freeing the database.

**Buffer size validation in rule_db_export**
When the user calls `rule_db_export(dev, rule_db)` with a non-NULL `rule_db`, the code does not verify that the user's buffer is large enough. The first call with `NULL` returns the required size, but the second call blindly copies without checking the buffer size.

```c
memcpy(rule_db, buf, len);  /* BUG: no size check */
free(buf);
return 0;
```

This is a latent API misuse bug: the regexdev API does not provide a buffer size parameter to `dev_db_export`, so the PMD cannot validate it. The current implementation trusts the application to allocate the correct size based on the first call. This should at minimum be documented as a caller responsibility.

Suggested mitigation: Add a comment in `hs_regex_rule_db_export` stating that the caller must allocate a buffer of size returned by the first call (with `rule_db = NULL`).

---

## Patch 4/7: Enqueue and dequeue burst paths

### Errors

**Match callback does not check for NULL op->matches**
The match callback `hs_match_cb` dereferences `op->matches` without checking if it is NULL. If the application provides an `rte_regex_ops` with `matches = NULL`, the callback will crash.

```c
if (op->nb_matches < ctx->max_matches) {
    struct rte_regexdev_match *m = &op->matches[op->nb_matches];
    /* BUG: if op->matches is NULL, this is a NULL pointer dereference */
    m->rule_id = id;
    ...
}
```

Fix: Add a NULL check for `op->matches` at the start of the callback.

**Incomplete error handling on linearization failure**
When `rte_pktmbuf_linearize()` fails, the code marks the op with `RESOURCE_LIMIT_REACHED_F` but does not verify that the mbuf data pointer remains valid. If linearization partially succeeded before failing, `rte_pktmbuf_mtod()` may return stale data.

The current DPDK implementation of `rte_pktmbuf_linearize()` returns 0 on success or -1 on failure (if the first segment cannot hold the entire packet). On failure, the mbuf is not modified, so `rte_pktmbuf_mtod()` is safe. However, the code assumes this behavior without documenting the assumption.

Suggested improvement: Add a comment explaining the assumption that linearization failure leaves the mbuf unchanged.

---

## Patch 5/7: Extended statistics

No correctness issues identified.

---

## Patch 6/7: Start, stop, close, and dump

### Errors

**Stop resets ring pointers without checking for in-flight ops**
The `hs_regex_stop()` function sets `qp->head = qp->tail = qp->count = 0` without verifying that all enqueued ops have been dequeued. If the application stops the device with ops still in the ring, those ops are silently dropped.

```c
for (i = 0; i < priv->nb_queue_pairs; i++) {
    struct hs_regex_qp *qp = &priv->qps[i];
    /* BUG: no check that qp->count == 0 */
    qp->head = 0;
    qp->tail = 0;
    qp->count = 0;
}
```

This is not necessarily a bug if the regexdev API allows stopping with in-flight ops, but the documentation should clarify the behavior. If the API requires draining, this is an error.

Suggested fix: Log a warning if `qp->count != 0` when stopping, and document that stop drops in-flight ops.

**Close does not zero max_matches and nb_groups**
The `hs_regex_close()` function resets most fields but leaves `max_matches` and `nb_groups` at their configured values. This is inconsistent with the state transition to `HS_REGEX_DEV_CREATED` and could cause issues if the device is reconfigured without restarting the process.

```c
priv->dev_state = HS_REGEX_DEV_CREATED;
/* BUG: max_matches and nb_groups not reset */
```

Fix: Set `priv->max_matches = 0` and `priv->nb_groups = 0` in `hs_regex_close()`.

---

## Patch 7/7: Hyperscan compile flag support

No correctness issues identified.

---

## Warnings

**Missing validation: qp_id out of range after device stop**
In `hs_regex_enqueue_burst` and `hs_regex_dequeue_burst`, the code validates `qp_id >= priv->nb_queue_pairs`, but if the device is stopped and then reconfigured with fewer queue pairs, the validation may pass while `priv->qps[qp_id]` is uninitialized. The current code checks `priv->qps == NULL` but does not verify that `qp_id` is valid for the new configuration.

This is unlikely in practice because the device must be closed to change `nb_queue_pairs`, but it is a potential edge case.

Suggested fix: Add a check in configure that closes all QPs if `nb_queue_pairs` changes.

**Auto-start in enqueue_burst hides application errors**
In patch 7, the enqueue function auto-starts the device if a compiled database exists but the device is not started:

```c
if (unlikely(priv->dev_state != HS_REGEX_DEV_STARTED)) {
    if (priv->db_compiled) {
        priv->dev_state = HS_REGEX_DEV_STARTED;
        HS_LOG(NOTICE, "enqueue: auto-started device");
    } else {
        HS_LOG(ERR, "enqueue: device not started and no DB");
        return 0;
    }
}
```

This is a convenience feature but violates the regexdev API contract: the application should explicitly call `rte_regexdev_start()`. Auto-starting in the data path hides application bugs and makes state transitions non-deterministic.

Suggested fix: Remove the auto-start path and return an error if the device is not started.

**Missing release notes for internal API**
The release notes mention the new PMD, but the changelog does not clarify that this is a virtual device (vdev) PMD. The notes should mention the `--vdev regex_hs` usage.

Suggested fix: Add a sentence to the release notes explaining vdev usage.

---

## Process and Style

**Documentation: RST list formatting**
In `doc/guides/regexdevs/hs.rst`, the "Supported Regex Rule Flags" section uses bullet lists where definition lists would improve readability:

```rst
Standard DPDK flags (advertised via ``info->rule_flags``):

- ``RTE_REGEX_PCRE_RULE_ALLOW_EMPTY_F`` (maps to ``HS_FLAG_ALLOWEMPTY``)
- ``RTE_REGEX_PCRE_RULE_CASELESS_F``    (maps to ``HS_FLAG_CASELESS``)
```

Suggested change:

```rst
Standard DPDK flags (advertised via ``info->rule_flags``):

RTE_REGEX_PCRE_RULE_ALLOW_EMPTY_F
   Maps to ``HS_FLAG_ALLOWEMPTY``.

RTE_REGEX_PCRE_RULE_CASELESS_F
   Maps to ``HS_FLAG_CASELESS``.
```

**Comment style: unnecessary noise**
Several functions have multi-line comments that restate what the function name and code already show:

```c
/* Configure */
static int
hs_regex_configure(...)
```

These "section header" comments are acceptable but should only be used for major sections, not every function. The single-word comments add no information and can be removed.

**Variable initialization: unnecessary zero-initialization**
In several places, variables are initialized to 0 and then unconditionally assigned before use:

```c
int ret;  /* declaration without initialization is preferred */
ret = some_function();
```

This is correct. Do not change.

**Explicit comparisons in fast path**
The enqueue/dequeue functions use `unlikely()` with explicit comparisons:

```c
if (unlikely(dev == NULL || ops == NULL))
```

This is correct per DPDK style (explicit comparison on pointers). The `unlikely()` wrapper does not change the requirement for explicit comparison.

**Missing const on static function pointer array**
In patch 4, the `hs_xstat_suffixes` array is correctly declared `const`:

```c
static const char * const hs_xstat_suffixes[HS_XSTATS_PER_QP] = { ... };
```

This is good practice. No issue.

---

## Summary of Required Fixes

### Errors (must fix before merge)

1. **Patch 1/3**: Resource leak: free `rule_id_hash` on configure error paths
2. **Patch 2**: Resource leak: free `qp->ops` on scratch allocation failure in `hs_regex_qp_setup`
3. **Patch 2**: Reset `max_matches`/`nb_groups` on configure import failure
4. **Patch 3**: Set `db_compiled = 0` on scratch allocation failure in `hs_regex_rule