hook: accept interface names in ifindex= option by Bodlux · Pull Request #508 · facebook/bpfilter

Bodlux · 2026-04-15T07:22:13Z

Use bf_if_index_from_str() to parse the ifindex= hook option value, which accepts both numeric indices and interface names (e.g. eth0). This is consistent with how the REDIRECT verdict already resolves interface references.

Closes #409

meta-cla · 2026-04-15T07:22:20Z

Hi @Bodlux!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

qdeslandes · 2026-04-15T10:59:15Z

Hi, thanks for your first contribution! ifindex is more suited for an interface index (number). I would suggest introducing iface= which would allow either an interface index or an interface name.

qdeslandes

Repeating my comment:
Hi, thanks for your first contribution! ifindex is more suited for an interface index (number). I would suggest introducing iface= which would allow either an interface index or an interface name.

Also, could you get rid of the comment to trigger the CLA check.

Bodlux · 2026-04-15T11:51:24Z

Thanks for the suggestion. I've renamed the option from ifindex= to iface= so it accepts either an interface name or index.

qdeslandes

This change is incomplete, as it's only modifying the code and not the doc. Additionally, renaming an existing option will break use cases. An alternative would be to create a new iface option which handles interface index and interface name, then make ifindex as deprecated in the doc.

Bodlux · 2026-04-16T12:46:52Z

Thanks for the detailed feedback, @qdeslandes. I've reworked the PR along the lines you suggested and force-pushed a single, clean commit.

Summary of the new approach:

ifindex= is preserved. It still accepts only a numeric index, so no existing ruleset, script or serialized chain breaks. It now emits a deprecation warning pointing users to iface=.
A new iface= option is introduced. It accepts either an interface name (resolved via if_nametoindex, reusing the existing bf_if_index_from_str() helper from src/libbpfilter/if.c) or a numeric interface index.
Both options share the same BF_HOOKOPTS_IFINDEX slot in used_opts. This means either one satisfies the XDP/TC requirement, and specifying both in the same chain (or either one twice) is rejected as a duplicate.
The on-wire msgpack key stays "ifindex", so the serialization format is unchanged.
doc/usage/bfcli.rst: the hook option table now documents iface=$INTERFACE as the preferred option and flags ifindex=$IFINDEX as deprecated.
Added a new hookopts_parse_iface unit test covering numeric input, interface name (lo), invalid names, duplicate detection, and the ifindex= + iface= cross-duplicate case.
Added e2e duplicate-detection cases in tests/e2e/cli/hookopts.sh for iface=,iface= and ifindex=,iface=.
History squashed to a single commit (the retrigger-CLA commit is gone).

Let me know if you'd like any further adjustments.

qdeslandes

Small changes, otherwise LGTM.

qdeslandes · 2026-04-24T11:03:05Z

+/* Alias table: alternative names that map to an existing bf_hookopts_type but
+ * use a different parser. The `type` field determines which bit in `used_opts`
+ * is set, so aliases share duplicate-detection and flavor
+ * requirement/support checks with their primary entry. */
+static struct bf_hookopts_ops _bf_hookopts_alias_ops[] = {
+    {.name = "iface",
+     .type = BF_HOOKOPTS_IFINDEX,
+     .required_by = 0,
+     .supported_by = BF_FLAGS(BF_FLAVOR_XDP, BF_FLAVOR_TC),
+     .parse = _bf_hookopts_iface_parse,
+     .dump = _bf_hookopts_ifindex_dump},
+};


No need for a dedicated table here, this entry should be in _bf_hookopts_ops, and you should add a check for incompatibility in their respective parsers:

_bf_hookopts_iface_parse: error if ifindex is defined, and error if iface is already defined

_bf_hookopts_ifindex_parse: error if iface is defined, and error if ifindex is already defined

Bodlux · 2026-04-26T15:03:08Z

Thanks for the review. Pushed a force-update with the requested changes:

Removed the dedicated alias table; iface is now a regular entry in _bf_hookopts_ops with its own BF_HOOKOPTS_IFACE slot in enum bf_hookopts_type.
_bf_hookopts_iface_parse first errors when BF_HOOKOPTS_IFACE is already set ("iface= is defined multiple times"), then when BF_HOOKOPTS_IFINDEX is set ("iface= can't be set when ifindex= is already defined"). On success it sets both bits, so the XDP/TC ifindex requirement in bf_hookopts_validate is still satisfied.
_bf_hookopts_ifindex_parse first errors when BF_HOOKOPTS_IFACE is set, then when BF_HOOKOPTS_IFINDEX is already set.
Unit tests now assert the expected BF_HOOKOPTS_IFACE / BF_HOOKOPTS_IFINDEX bit state in every path and add the reverse iface= then ifindex= rejection case; the e2e test gains the matching duplicate.

The msgpack key is still ifindex, so on-wire compatibility is unchanged.

qdeslandes · 2026-04-28T10:40:27Z

@claude review

github-actions · 2026-04-28T10:40:36Z

Claude review of PR #508 (`ef4d564`)

Must fix

Doc columns swapped for iface= and ifindex= — doc/usage/bfcli.rst:384 — "Required by" and "Supported by" are transposed for both entries relative to what the code declares in _bf_hookopts_ops

Suggestions

Missing INT_MAX bounds check — src/libbpfilter/hook.c:255 — _bf_hookopts_iface_parse casts uint32_t to int without the > INT_MAX guard present in the sibling _bf_hookopts_ifindex_parse
Dual-bit side effects in print/dump — src/libbpfilter/hook.c:257 — Setting both IFACE and IFINDEX bits causes bfcli to round-trip iface=eth0 as ifindex=N and bf_hookopts_dump to print both entries
Enum insertion shifts existing values — src/libbpfilter/include/bpfilter/hook.h:156 — Prepending BF_HOOKOPTS_IFACE changes all existing enum ordinals; appending before _BF_HOOKOPTS_MAX would avoid this
Commit message format — hook: should be lib: hook: per project convention; title also describes modifying ifindex= rather than adding iface=

Nits

Missing trailing newline — tests/e2e/cli/hookopts.sh — pre-existing, but since the file is being modified consider fixing it

Workflow run

github-actions · 2026-04-28T10:56:06Z

     - Supported by
     - Notes
-   * - ``ifindex=$IFINDEX``
+   * - ``iface=$INTERFACE``


Claude: must-fix: The "Required by" and "Supported by" columns are swapped for both iface= and ifindex=.

In the code, iface= has .required_by = 0 and .supported_by = BF_FLAGS(BF_FLAVOR_XDP, BF_FLAVOR_TC), but this row lists BF_HOOK_XDP, BF_HOOK_TC under "Required by" (unchanged context lines 385-386 inherited from the old ifindex= row).

Conversely, the new ifindex= entry (line 389-390) shows "Required by: N/A" and "Supported by: BF_HOOK_XDP, BF_HOOK_TC", but the code retains .required_by = BF_FLAGS(BF_FLAVOR_XDP, BF_FLAVOR_TC) and .supported_by = 0.

Fix: iface= should show "Required by: N/A" / "Supported by: BF_HOOK_XDP, BF_HOOK_TC". ifindex= should show "Required by: BF_HOOK_XDP, BF_HOOK_TC" / "Supported by: N/A".

github-actions · 2026-04-28T10:56:07Z

+    if (r)
+        return bf_err_r(r, "failed to parse iface from '%s'", raw_opt);
+
+    hookopts->ifindex = (int)ifindex;


Claude: suggestion: Missing INT_MAX bounds check before the uint32_t -> int narrowing cast. bf_if_index_from_str() accepts values up to UINT32_MAX (see if.c:62), and the cast (int)ifindex is implementation-defined for values exceeding INT_MAX. The sibling _bf_hookopts_ifindex_parse has an explicit guard at line 216:

if (ifindex > INT_MAX) return bf_err_r(-E2BIG, ...);

Consider adding the same check here for consistency.

github-actions · 2026-04-28T10:56:08Z

+
+    hookopts->ifindex = (int)ifindex;
+    hookopts->used_opts |= BF_FLAG(BF_HOOKOPTS_IFACE);
+    hookopts->used_opts |= BF_FLAG(BF_HOOKOPTS_IFINDEX);


Claude: suggestion: Setting both BF_HOOKOPTS_IFACE and BF_HOOKOPTS_IFINDEX is a pragmatic workaround to satisfy the required_by validation for XDP/TC, but it has two side effects:

bfcli/print.c:112 only checks BF_HOOKOPTS_IFINDEX and will output ifindex=N even when the user specified iface=eth0 — the round-trip uses the deprecated keyword.

bf_hookopts_dump iterates all entries, so both _bf_hookopts_iface_dump and _bf_hookopts_ifindex_dump will fire, printing both iface: N and ifindex: N for the same value.

Consider updating print.c to check BF_HOOKOPTS_IFACE first (printing iface= when set), and having one of the dump functions skip output when the other flag is also set.

github-actions · 2026-04-28T10:56:09Z


 enum bf_hookopts_type
 {
+    BF_HOOKOPTS_IFACE,


Claude: suggestion: Inserting BF_HOOKOPTS_IFACE before BF_HOOKOPTS_IFINDEX shifts the numeric values of all existing enum members (BF_HOOKOPTS_IFINDEX changes from 0 to 1, BF_HOOKOPTS_CGPATH from 1 to 2, etc.). Since these values are used as bit positions via BF_FLAG(), any external code compiled against the old header will silently misinterpret bitmasks.

At SOVERSION 0.0.0 this may be acceptable, but appending BF_HOOKOPTS_IFACE just before _BF_HOOKOPTS_MAX would avoid shifting existing values with no functional downside.

github-actions · 2026-04-28T10:56:10Z

 (! ${BFCLI} ruleset set --dry-run --from-str "chain ifindex BF_HOOK_XDP{ifindex=2,ifindex=3} ACCEPT")
+(! ${BFCLI} ruleset set --dry-run --from-str "chain iface BF_HOOK_XDP{iface=lo,iface=lo} ACCEPT")
+(! ${BFCLI} ruleset set --dry-run --from-str "chain iface_ifindex BF_HOOK_XDP{ifindex=2,iface=lo} ACCEPT")
+(! ${BFCLI} ruleset set --dry-run --from-str "chain ifindex_iface BF_HOOK_XDP{iface=lo,ifindex=2} ACCEPT")


Claude: nit: The file lacks a trailing newline (pre-existing, visible as \ No newline at end of file in the diff). Since this file is being modified, consider adding one to satisfy POSIX text file conventions.

Add a new hook option `iface=` that accepts either an interface name (resolved via `if_nametoindex`) or a numeric interface index. The existing `ifindex=` option is kept for backward compatibility but is now marked as deprecated: it continues to accept numeric indices only, and emits a runtime warning pointing users to `iface=`. `iface=` is added as its own `BF_HOOKOPTS_IFACE` entry in `_bf_hookopts_ops`, alongside `BF_HOOKOPTS_IFINDEX`. Each parser cross-checks the other slot's bit and rejects any combination of `iface=` and `ifindex=` in the same chain (in either order, or repeated). When `iface=` succeeds it sets both bits, so the XDP/TC `ifindex` requirement is satisfied. The on-wire (msgpack) serialization key remains `ifindex`, so existing serialized chains continue to load unchanged. Docs, unit tests and e2e duplicate-detection tests are updated. Relates to facebook#409.

Bodlux · 2026-05-01T19:08:05Z

Thanks for the bot review @qdeslandes. Pushed a force-update addressing all five points and rebased onto current main.

doc/usage/bfcli.rst: swapped the Required by / Supported by columns for the iface= and ifindex= rows so the doc matches the code (iface= supports XDP/TC, ifindex= is required by XDP/TC).
src/libbpfilter/hook.c _bf_hookopts_iface_parse: added the same INT_MAX guard the sibling _bf_hookopts_ifindex_parse has before narrowing uint32_t -> int, with -E2BIG.
src/bfcli/print.c: when BF_HOOKOPTS_IFACE is set the round-trip now prints iface=N instead of ifindex=N; ifindex=N is still printed in the deprecated path when only BF_HOOKOPTS_IFINDEX is set.
src/libbpfilter/hook.c _bf_hookopts_ifindex_dump: returns early when BF_HOOKOPTS_IFACE is also set, so bf_hookopts_dump no longer emits both iface: N and ifindex: N for the same value. A short comment explains the dual-bit invariant from _bf_hookopts_iface_parse.
src/libbpfilter/include/bpfilter/hook.h: moved BF_HOOKOPTS_IFACE to the slot just before _BF_HOOKOPTS_MAX, so BF_HOOKOPTS_IFINDEX / CGPATH / FAMILY / PRIORITIES keep their original bit positions.
tests/e2e/cli/hookopts.sh: added trailing newline.

Bodlux requested a review from qdeslandes as a code owner April 15, 2026 07:22

meta-cla Bot added the cla signed label Apr 15, 2026

qdeslandes requested changes Apr 15, 2026

View reviewed changes

qdeslandes requested changes Apr 16, 2026

View reviewed changes

Bodlux force-pushed the feat/ifindex-accept-interface-names branch from 8d8081d to 55647eb Compare April 16, 2026 12:29

qdeslandes requested changes Apr 24, 2026

View reviewed changes

Bodlux force-pushed the feat/ifindex-accept-interface-names branch from 55647eb to 85b9f30 Compare April 26, 2026 15:02

qdeslandes force-pushed the feat/ifindex-accept-interface-names branch from 85b9f30 to ef4d564 Compare April 28, 2026 10:40

github-actions Bot reviewed Apr 28, 2026

View reviewed changes

Bodlux force-pushed the feat/ifindex-accept-interface-names branch from ef4d564 to 556179e Compare May 1, 2026 19:07

Conversation

Bodlux commented Apr 15, 2026

Uh oh!

meta-cla Bot commented Apr 15, 2026

Action Required

Process

Uh oh!

qdeslandes commented Apr 15, 2026

Uh oh!

qdeslandes left a comment

Choose a reason for hiding this comment

Uh oh!

Bodlux commented Apr 15, 2026

Uh oh!

qdeslandes left a comment

Choose a reason for hiding this comment

Uh oh!

Bodlux commented Apr 16, 2026

Uh oh!

qdeslandes left a comment

Choose a reason for hiding this comment

Uh oh!

qdeslandes Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Bodlux commented Apr 26, 2026

Uh oh!

qdeslandes commented Apr 28, 2026

Uh oh!

github-actions Bot commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude review of PR #508 (ef4d564)

Must fix

Suggestions

Nits

Uh oh!

github-actions Bot Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Bodlux commented May 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Apr 28, 2026 •

edited

Loading

Claude review of PR #508 (`ef4d564`)