(4) dpdk rte acl (FFI only)#1546
Conversation
3f4c614 to
3659d09
Compare
af1e229 to
563013b
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new dpdk::acl module providing a (safe-ish) Rust abstraction over DPDK’s librte_acl, and fixes dpdk::eal::init so rte_eal_init receives a proper argv[0] program name. It also enables and links the DPDK ACL library in the Nix + dpdk-sys build plumbing.
Changes:
- Add
dpdk::acl(typestateAclContext<N, State>, validated config types, rule/field wrappers, and algorithm selection) plus tests. - Fix
eal::initto prepend a dummyargv[0]so the first real EAL flag isn’t skipped byrte_eal_init. - Enable/build-link ACL support end-to-end (Nix DPDK build, wrapper header include, and
dpdk-syslink list).
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| nix/pkgs/dpdk/default.nix | Enable DPDK acl library in the Nix build. |
| nix/pkgs/dpdk-wrapper/src/dpdk_wrapper.h | Include rte_acl.h for bindgen exposure. |
| dpdk-sys/build.rs | Link rte_acl alongside other DPDK libs. |
| dpdk/src/lib.rs | Export new dpdk::acl module. |
| dpdk/src/eal.rs | Prepend dummy argv[0] when calling rte_eal_init. |
| dpdk/src/acl/mod.rs | New ACL module entry, docs, and re-exports (plus smoke test). |
| dpdk/src/acl/classify.rs | New ClassifyAlgorithm enum + conversions/tests. |
| dpdk/src/acl/config.rs | New validated AclCreateParams / AclBuildConfig<N> + tests. |
| dpdk/src/acl/context.rs | New typestate AclContext<N, State> wrapper + lifecycle methods. |
| dpdk/src/acl/error.rs | New typed error enums for ACL operations. |
| dpdk/src/acl/field.rs | New FieldDef / FieldType / FieldSize wrappers + tests. |
| dpdk/src/acl/rule.rs | New Rule<N> / RuleData / AclField types + layout checks + tests. |
| dpdk/Cargo.toml | Add bolero dev-dependency for property tests. |
| Cargo.lock | Lockfile update for new dev dependency. |
Comments suppressed due to low confidence (2)
dpdk/src/acl/rule.rs:325
confidence: 9
tags: [logic]
Same issue as `from_u8`: assigning to `rte_acl_field_types` union members (`u16_`) requires `unsafe`, so these writes will fail to compile.
Wrap the union field assignments in `unsafe { ... }` (keeping the prior zero-initialization so Debug/Hash/PartialEq remain deterministic).
pub fn from_u16(value: u16, mask_range: u16) -> Self {
let mut field = Self::default();
field.value.u16_ = value;
field.mask_range.u16_ = mask_range;
field
}
**dpdk/src/acl/rule.rs:338**
* ```yaml
confidence: 9
tags: [logic]
Same union-write compile error as from_u8/from_u16: writing to rte_acl_field_types.u32_ requires unsafe, so these assignments will not compile.
Please wrap the writes in an unsafe block (keeping the zero-initialization so later u64_ reads are not reading uninitialized bytes).
pub fn from_u32(value: u32, mask_range: u32) -> Self {
let mut field = Self::default();
field.value.u32_ = value;
field.mask_range.u32_ = mask_range;
field
}
1598d11 to
80c9f40
Compare
80c9f40 to
e6148db
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
dpdk/src/acl/rule.rs:453
confidence: 9
tags: [logic]
`dpdk_sys::rte_acl_field_types` is a bindgen union; writing `field.value.u16_` / `field.mask_range.u16_` requires `unsafe`. This constructor currently performs union-field access in safe code and should be adjusted (e.g., write through the `u64_` member, or add an `unsafe` block around the union writes).
pub fn from_u16(value: u16, mask_range: u16) -> Self {
let mut field = Self::default();
field.value.u16_ = value;
field.mask_range.u16_ = mask_range;
field
**dpdk/src/acl/rule.rs:466**
* ```yaml
confidence: 9
tags: [logic]
Same issue as the other from_u* constructors: assigning to field.value.u32_ / field.mask_range.u32_ is union-field access and requires unsafe. Rework this constructor to avoid union-field writes in safe code (e.g., initialise via u64_ or use an unsafe { ... } block).
pub fn from_u32(value: u32, mask_range: u32) -> Self {
let mut field = Self::default();
field.value.u32_ = value;
field.mask_range.u32_ = mask_range;
field
7f3ba61 to
661907d
Compare
4c6fe2d to
c0d3245
Compare
3c2121b to
b6d4002
Compare
ba7d909 to
eb7cfb0
Compare
| use core::ffi::CStr; | ||
| use core::fmt::{self, Debug, Display}; | ||
| use core::marker::PhantomData; | ||
| use core::num::NonZero; | ||
|
|
||
| use std::ffi::CString; | ||
|
|
eb7cfb0 to
ab1fdf0
Compare
| /// `RTE_ACL_MAX_FIELDS` (= [`MAX_FIELDS`] = 64) entries -- about 1 KiB | ||
| /// on the stack at 16 bytes per `rte_acl_field_def`. Build is a cold |
| return false; | ||
| } | ||
|
|
||
| // First field: size = One, offset = 0, input_index = 0. |
mvachhar
left a comment
There was a problem hiding this comment.
Please read my comments first, no changes required because of them unless you want to make the changes.
| // `u32::MAX`, so the cast is exact, and certainly non-zero. | ||
| // `new_unchecked` avoids a panic path (`unreachable!()`) in | ||
| // library code. | ||
| let rule_size = unsafe { NonZero::new_unchecked(Rule::<N>::RULE_SIZE) }; |
There was a problem hiding this comment.
I'm not going to reject the commit over this but the argument for unsafe here is specious. It says it avoids a panic, but it does so in favor of possible undefined behavior if the safety analysis is wrong or ever becomes wrong. The unreachable! is preferable here IMHO.
There was a problem hiding this comment.
yeah, I can see the spooky-action-at-a-distance arg. That being said, it is a compile time assert, and if that assert is wrong DPDK will malfunction anyway. It can't deal with zero sized rules
| /// has access to the context for the duration of the call). | ||
| #[cold] | ||
| pub fn dump(&mut self) { | ||
| // SAFETY: ctx is guaranteed non-null by the NonNull invariant. |
There was a problem hiding this comment.
Again, I am not going to block the PR on this basis but a bunch of these presumably AI generated comments have some clutter. ctx is non-null because it's type is NonNull, there is no need to repeat it in every safety analysis, it is a red herring. This happens repeatedly in this file.
There was a problem hiding this comment.
this is a stronger example of where we really could stand to have an unreachable!()
rte_eal_init treats argv[0] as the program name and skips it during option parsing. The wrapper was passing the caller's args verbatim, so the first flag was silently swallowed on every call -- including in production callers and in any test that hands eal::init a list of flags. Fix by prepending an owned `c"dataplane".to_owned()` CString as the first entry in the argv vector. Backing the placeholder with heap-allocated, writable storage (rather than pointing into .rodata) keeps the wrapper sound against any future DPDK that follows `rte_eal_init`'s public contract and modifies argv strings in place (e.g. for setproctitle-style program-name manipulation or getopt-style optarg rewrites). The `cast_mut` on `as_ptr` is sound because every CString in the vector owns its own writable allocation for the duration of the call. Also reserves one slot in `ValidatedEalArgs`'s `c_int::MAX` check so the argv array can hold the placeholder plus the validated user args without overflowing the `argc` cast. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Enable DPDK's ACL library in the nix build, add `rte_acl` to the dpdk-sys link list, and include `<rte_acl.h>` in the bindgen wrapper so that downstream Rust crates can call the rte_acl_* API. Prepares the ground for the safe rte_acl wrapper that follows; this commit on its own only widens what dpdk-sys exposes and does not yet add any Rust consumers. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Adds an empty acl module to the dpdk crate, the build-dependencies it will need (concurrency facade, bolero/nix/id for tests), and the field-layout primitives that the rest of the wrapper depends on: - `FieldDef`: a validated DPDK `rte_acl_field_def` (type / size / field_index / input_index / offset). - `FieldType`: Mask / Range / Bitmask, matching the C enum. - `FieldSize`: One / Two / Four-byte field widths. These types carry no DPDK runtime state and have no cross-module dependencies, so they can land independently. Subsequent commits will introduce error types, rules, build configuration, the context typestate, and the SIMD classify algorithm enum on top. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Each fallible ACL operation gets its own dedicated error type, with variants that match DPDK's documented error codes plus the Rust-side validation failures the wrapper catches up front: - `InvalidAclName`: not-ASCII / too-long / empty / interior NUL. - `AclCreateError`: invalid name, duplicate name (AlreadyExists), EINVAL, ENOMEM, Unknown. - `AclAddRulesError`: too many rules, per-rule `InvalidRule` (carries the rule index in the caller's slice), DPDK ENOMEM/EINVAL/Unknown. - `InvalidRule`: per-rule wrapper-side validation -- mask-prefix-length out of range (prevents UB in DPDK's `RTE_ACL_MASKLEN_TO_BITMASK`), reversed range bounds, category_mask bits beyond `num_categories`. - `AclBuildError`: ENOMEM, invalid config, exceeded max_size (ERANGE), Unknown. - `AclClassifyError`: invalid args, NotSupported (ENOTSUP from the per-algorithm path), Unknown. - `AclSetAlgorithmError`: invalid params, NotSupported, Unknown. No DPDK runtime is required to construct these; they are pure value types. Other ACL sub-modules will consume them in subsequent commits. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Wraps DPDK's `rte_acl_classify_alg` enum as a strongly-typed Rust enum [`ClassifyAlgorithm`], with `From<ClassifyAlgorithm> for u32` and a fallible `TryFrom<u32>`/`from_u32` round-trip for parsing values the caller did not produce. Variants mirror DPDK 1:1: Default, Scalar, NEON, SSE, AVX2, AVX512X16, AVX512X32, Altivec. Lives in its own module because the rest of the wrapper does not need to know about SIMD selection, and because the round-trip property is worth a dedicated bolero test. Subsequent commits wire the enum into `AclContext::classify_with_algorithm` and `AclContext::set_default_algorithm`. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
08e23f9 to
09fab17
Compare
ab1fdf0 to
5dd3ea1
Compare
The heart of the safe rte_acl wrapper. These three modules land together because they have unavoidable mutual dependencies at the type level: - `config::AclCreateParams<N>::new` calls `Rule::<N>::RULE_SIZE` (compile-time stride for `rte_acl_add_rules`). - `Rule::validate` takes `&AclBuildConfig<N>` and uses it to enforce the per-rule invariants (prefix length, category mask, range bounds). - `context::AclContext<N, State>` owns both an `AclCreateParams<N>` and an `AclBuildConfig<N>` and drives them through DPDK's lifecycle. Highlights: - `Rule<N>` / `AclField` / `Priority` / `CategoryMask` / `RuleData` enforce non-zero invariants on userdata / rule size / max_rule_num and validate fields against the declared `FieldSize` so the validator is endian-safe. - `AclBuildConfig<N>` validates the input-index grouping rules DPDK imposes (first field 1 byte, others in 4-byte groups; contiguous; offset-ordered). `min_input_size` is computed from the group offsets so the `classify` safety contract is exact. - `AclContext<N, Configuring<N>>` accepts rule mutations through `&mut self`; `AclContext<N, Built<N>>` exposes `classify` through `&self`, matching DPDK's documented thread-safety guarantees. - Process-wide `ACL_CREATE_LOCK` (`OnceLock<Mutex<()>>` through the concurrency facade) serialises any operation that touches DPDK's global ACL registry: `new`, `Drop`, and `dump_all_contexts`. - `AclBuildFailure<N>` carries the original `Configuring<N>` context back to the caller after a failed `build`, preserving the rule list for retry. - bolero property tests for `AclBuildConfig::new` validators (num_categories, name, field_defs) and the `AclField` upper-byte invariants. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Finishes the safe rte_acl wrapper by promoting the most commonly used types out of their sub-modules into the `acl` namespace and adding the end-to-end lifecycle tests that exercise the full create / add_rules / build / classify / reset pipeline against a live EAL. The integration tests live in `acl::tests` (gated on `#[cfg(test)]`) and funnel every test through a shared `OnceLock<Eal>` so the mandatory once-per-process `rte_eal_init` is invoked at most once under any nextest configuration. Coverage includes: - `classify_smoke`: build a tiny ACL, classify two buffers, verify match / no-match. - `reset_round_trip`: build, classify, reset back to `Configuring`, add a new rule, rebuild, re-classify -- and assert the build config survives across the transition. - `add_rules_rejects_out_of_range_prefix_length`, `add_rules_rejects_category_mask_beyond_num_categories`: confirm wrapper-side `Rule::validate` rejects soundness-critical input before it reaches DPDK. - `classify_categories_validated_before_ffi`: classifier rejects out-of-range categories before forwarding to DPDK. - `set_default_algorithm_then_classify`, `classify_with_algorithm_scalar`: exercise the algorithm-selection paths. - `duplicate_name_rejected`: the process-wide create lock catches the duplicate-name TOCTOU. - `add_rules_after_overflow_failure`, `build_failure_returns_usable_context`: confirm the recoverable failure paths really do recover. - `classify_concurrent_arc_shared`: shows that the per-state Sync impl on `Built<N>` is not vacuous -- four threads classify concurrently through `Arc<AclContext>` and observe consistent results. Run with `cargo nextest run -p dataplane-dpdk acl::tests` after `just setup-roots` and re-entering nix-shell so that `DATAPLANE_SYSROOT` picks up the rte_acl-enabled sysroot. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
5dd3ea1 to
637cb7e
Compare
Safe-ish Rust wrapper around
librte_acl, plus a soundness fix toeal::initthat came up while wiring things in.The const-generic
AclContext<N, State>is the load-bearing abstraction.N(number of fields per rule) is shared betweenAclContext<N>,Rule<N>,AclCreateParams<N>, andAclBuildConfig<N>, so any field-count mismatch is a compile-time error. TheConfiguring->Builttypestate transition enforces DPDK's "rules cannot be added after build" and "classification is&self(thread-safe)" invariants at the type level.Commit layout
The wrapper is split into six commits so each lands as a digestible unit, with one prerequisite (the EAL fix). Every commit compiles on its own and passes
cargo fmt --check+cargo clippy --all-targets -p dataplane-dpdk-sys -p dataplane-dpdk -- -D warnings.fix(dpdk): prepend program-name placeholder in eal::initrte_eal_inittreats argv[0] as the program name and skips it during option parsing; the wrapper was silently swallowing the first real flag.into_raw/from_rawround-trip on the argv strings keeps pointer provenance mut-clean under Stacked/Tree Borrows.chore(dpdk-sys): link rte_acl and bind its headersaclin the nix DPDK build, addrte_aclto the link list, include<rte_acl.h>in the bindgen wrapper.chore(dpdk): scaffold acl module with field-layout typesdpdk::aclmodule;FieldDef/FieldType/FieldSize(no DPDK runtime state).feat(dpdk/acl): typed error enumsAclCreateError,AclAddRulesError,AclBuildError,AclClassifyError,AclSetAlgorithmError, plusInvalidAclName/InvalidRule/InvalidAclBuildConfig).feat(dpdk/acl): SIMD classify algorithm enumClassifyAlgorithmwith round-trippingFrom<u32>/TryFrom<u32>.feat(dpdk/acl): rules, build config, and the typestate contextRule<N>,AclField,Priority,CategoryMask,AclBuildConfig<N>,AclCreateParams<N>,AclContext<N, State>, plusACL_CREATE_LOCKanddump_all_contexts. These three files have mutual type-level dependencies and ship together.feat(dpdk/acl): public re-exports and integration testsSoundness surface
AclContext::classifyandclassify_with_algorithmareunsafe fn. Each*const u8in the input slice must reference a buffer valid for at leastctx.build_config().min_input_size()bytes. The bound is wider thanmax(offset + size): DPDK's classify loop does 4-byte aligned loads starting at the lowest offset within eachinput_indexgroup, so the per-buffer extent ismax(group_offset + 4)across groups.classify_with_algorithm's safety contract additionally requires CPU support for the chosen variant --rte_acl_classify_algdoes not validate against the per-CPU capability table (onlyrte_acl_set_ctx_classifydoes), so a real (non-stub) SIMD slot on an unsupported host executes unsupported instructions.DefaultandScalarare always safe; everything else needs anis_x86_feature_detected!(or arch-equivalent) gate at the call site.Rule::validatereads each field through the union member matching the declaredFieldSize, so the validator is endian-safe on big-endian hosts. It catches: mask-prefix-length out of range (would be UB in DPDK'sRTE_ACL_MASKLEN_TO_BITMASKC shift), reversed range bounds, andcategory_maskbits beyondnum_categories(DPDK would silently mask those off).AclField::value_u*/mask_range_u*are safe -- every member of the underlying union is a plain integer, so no bit pattern is invalid. Constructors guarantee the wider union members are zero-initialised.AclContext::as_raw_ptr/as_raw_mut_ptrare safe -- returning a raw pointer is sound; only its later use is unsafe. The&mut selfreceiver onas_raw_mut_ptrcarries the typestate's mutability discipline into raw FFI code.ACL_CREATE_LOCK: OnceLock<Mutex<()>>(via the concurrency facade) serialises every operation that touches DPDK's global ACL TAILQ:AclContext::new,Drop, anddump_all_contexts. This closes the duplicate-name TOCTOU (rte_acl_createreturns the existing context pointer for a duplicate name, which would otherwise produce two owners of the same DPDK handle).Sendis blanket overAclState;Syncis per-state with separateunsafe impls forConfiguring<N>andBuilt<N>, so adding a new typestate later forces a fresh reentrancy audit rather than silently inheritingSync.Rule<0>andAclBuildConfig<0>are rejected at compile time;N > MAX_FIELDSis rejected inAclCreateParams::newto preventRule::<N>::RULE_SIZE'su32cast from truncating.#[repr(C)]types carry compile-timesize_of/align_ofasserts against the matching bindgen structs.Test coverage
Unit tests live next to the code they test. bolero property tests cover
ClassifyAlgorithmround-trip,AclBuildConfig::newfield-defs validation against an independent oracle, thenum_categoriesand context-name validators, andAclField::from_u*upper-byte invariants.Integration tests (
acl::tests, gated on#[cfg(test)]) run against a live EAL initialised through a sharedOnceLock<Eal>:classify_smoke,reset_round_tripadd_rules_rejects_out_of_range_prefix_lengthadd_rules_rejects_category_mask_beyond_num_categoriesclassify_categories_validated_before_ffiset_default_algorithm_then_classify,classify_with_algorithm_scalarduplicate_name_rejectedadd_rules_after_overflow_failure,build_failure_returns_usable_contextclassify_concurrent_arc_shared-- four threads classify concurrently throughArc<AclContext>to confirm the per-stateSyncclaim isn't vacuous.