Skip to content

Add NEXT verdict for BPF program chaining#474

Open
qdeslandes wants to merge 6 commits intofacebook:mainfrom
qdeslandes:add_next_verdict
Open

Add NEXT verdict for BPF program chaining#474
qdeslandes wants to merge 6 commits intofacebook:mainfrom
qdeslandes:add_next_verdict

Conversation

@qdeslandes
Copy link
Copy Markdown
Contributor

Add a new BF_VERDICT_NEXT terminal verdict, meaning "pass to the next BPF program.".

On TC, this maps to TCX_NEXT which defers packet processing to the next program in the TCX link, distinct from TCX_PASS which accepts the packet and bypasses subsequent programs.
For NF, XDP, and cgroup_skb, NEXT maps to the same return code as ACCEPT since these hooks don't distinguish between the two.

@meta-cla meta-cla bot added the cla signed label Mar 13, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 13, 2026

Claude is re-reviewing this PR… (workflow run)

Claude review of PR #474 (982fc4e)

Must fix

  • Commit message Add missing header and definitions #2 exceeds 72-char limit — Fixed: subject is now lib: verdict: add BF_VERDICT_NEXT (36 chars).
  • Commit 4 registers e2e test file that does not yet exist — Fixed: CMakeLists.txt entry and next_tc.sh are now in the same commit.

Suggestions

  • Stale Doxygen on get_verdict implementations — Fixed in commit 3: Doxygen updated for all flavor implementations.
  • BF_VERDICT_NEXT Doxygen format (dismissed) — Author: "All the multiline struct fields documentation follow this style."
  • BF_VERDICT_NEXT placed under "non-terminal" section comment — Fixed in commit 3: enum comments restructured to per-member inline annotations.
  • E2E first test section lacks assertion (dismissed) — Author: "ping itself is an assertion as we set -e."
  • E2E counter check validates wrong rule index — Fixed: rule 0 now has counter keyword; test checks both rule 0 (count=1) and rule 1 (count=0).
  • "in the chain" wording in NEXT policy docs — Fixed: now reads "pass the packet to the next BPF program" without "in the chain."
  • Stale code example in jmp.h — Fixed in commit 6: jmp.h documentation updated to use the new get_verdict(verdict, &ret) API.
  • Unit test missing boundary values for bf_verdict_is_valid_policy — Fixed: tests now include _BF_VERDICT_MAX and -1.
  • E2E test missing coverage for NEXT on non-TC hookstests/e2e/rules/next_tc.sh — NEXT maps to ACCEPT on NF, XDP, and cgroup_skb, but no E2E test verifies this. Low priority: behavior is identical to ACCEPT, so there is no distinguishable outcome to test for.
  • bf_rpack_kv_enum range widened beyond policy needschain.c:244 — Deserialization now accepts all verdict values before bf_chain_new rejects invalid ones.
  • E2E test missing negative cases for invalid policy via CLItests/e2e/rules/next_tc.sh — No test verifies that bfcli rejects CONTINUE or REDIRECT as chain policies through the end-to-end CLI path. The parser validation changed in this PR, and while unit tests cover bf_chain_new(), the CLI rejection path is untested.

Nits

  • Commit 2 bundles library and CLI changeslib: verdict: add BF_VERDICT_NEXT modifies src/bfcli/lexer.l and parser.y (CLI component). Consider splitting or broadening the component tag.
  • Commit 4 has no body and conflates unit and e2e teststests: unit: add tests for NEXT verdict and policy validation includes a 92-line e2e test (next_tc.sh). The tests: unit: subcomponent doesn't reflect e2e content, and a body explaining test coverage would help.
  • Reformatted _bf_verdict_strs arraysrc/libbpfilter/verdict.c:43 — Most similar arrays in the codebase (_bf_hook_strs, _bf_pkthdr_strs, _bf_redirect_dir_strs) use one entry per line; the compact multi-entry layout is less scannable.
  • @param verdict wording inconsistentflavor.h:124 says "Must be a terminal verdict" while cgroup_skb.c, nf.c, tc.c, xdp.c say "Must be valid." The header wording is more precise since the implementations return -ENOTSUP for non-terminal verdicts.
  • Commit 3 bundles unrelated include cleanup — Removes #include "cgen/cgen.h" from cgroup_skb.c and tc.c, unrelated to NEXT verdict support.

Workflow run

@qdeslandes qdeslandes enabled auto-merge (rebase) March 13, 2026 21:21
The get_verdict flavor callback returns the BPF return code directly,
using negative values for errors. This will conflict with TCX_NEXT (-1)
when adding the NEXT verdict. Switch to an output parameter so that the
return value is reserved for error reporting.

No functional change.
Add a new terminal verdict BF_VERDICT_NEXT that means "pass to next BPF
program." For TC this will map to TCX_NEXT; for other flavors it maps to
the same return code as ACCEPT.

Replace the _BF_TERMINAL_VERDICT_MAX sentinel with an explicit
bf_verdict_is_valid_policy() function, which is easier to reason about
and extend. Update chain creation and the CLI parser to use it.

Add NEXT to the lexer so it is recognised as a verdict token.
Map BF_VERDICT_NEXT to flavor-specific return codes:
  - TC:         TCX_NEXT (-1), distinct from TCX_PASS
  - Netfilter:  NF_ACCEPT (same as ACCEPT)
  - XDP:        XDP_PASS (same as ACCEPT)
  - cgroup_skb: 1 (same as ACCEPT)

Handle NEXT as a terminal verdict in rule codegen, alongside ACCEPT
and DROP.
Update bfcli usage documentation to describe the NEXT verdict for both
chain policies and rule verdicts. Note that NEXT has distinct behavior
only for TC hooks (TCX_NEXT); for NF, XDP, and cgroup_skb it maps to
the same return code as ACCEPT.

- ``ACCEPT``: forward the packet to the kernel.
- ``DROP``: discard the packet.
- ``NEXT``: pass the packet to the next BPF program. For TC hooks, this maps to ``TCX_NEXT``, deferring the decision to the next program in the TCX link. For NF, XDP, and cgroup_skb hooks, ``NEXT`` behaves identically to ``ACCEPT`` since these hooks do not distinguish between "accept" and "pass to next program."
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to call out that if there are no other BPF programs then this acts like ACCEPT?

- ``counter``: optional literal. If set, the filter will counter the number of packets and bytes matched by the rule.
- ``mark``: optional, ``$MARK`` must be a valid decimal or hexadecimal 32-bits value. If set, write the packet's marker value. This marker can be used later on in a rule (see ``meta.mark``) or with a TC filter.
- ``$VERDICT``: action taken by the rule if the packet is matched against **all** the criteria: either ``ACCEPT``, ``DROP``, ``CONTINUE``, or ``REDIRECT``.
- ``$VERDICT``: action taken by the rule if the packet is matched against **all** the criteria: either ``ACCEPT``, ``DROP``, ``CONTINUE``, ``NEXT``, or ``REDIRECT``.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
- ``$VERDICT``: action taken by the rule if the packet is matched against **all** the criteria: either ``ACCEPT``, ``DROP``, ``CONTINUE``, ``NEXT``, or ``REDIRECT``.
- ``$VERDICT``: action taken by the rule if the packet is matched against **all** the criteria. One of the following:

Not sure why you want to list the verdicts and then list them again with descriptions


.. note::

``NEXT`` has distinct behavior only for TC hooks (``BF_HOOK_TC_INGRESS``, ``BF_HOOK_TC_EGRESS``), where it maps to ``TCX_NEXT`` and defers to the next BPF program in the TCX link. For all other hooks (Netfilter, XDP, cgroup_skb), ``NEXT`` produces the same return code as ``ACCEPT``.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
``NEXT`` has distinct behavior only for TC hooks (``BF_HOOK_TC_INGRESS``, ``BF_HOOK_TC_EGRESS``), where it maps to ``TCX_NEXT`` and defers to the next BPF program in the TCX link. For all other hooks (Netfilter, XDP, cgroup_skb), ``NEXT`` produces the same return code as ``ACCEPT``.
``NEXT`` has distinct behavior only for TC hooks (``BF_HOOK_TC_INGRESS``, ``BF_HOOK_TC_EGRESS``), where it maps to ``TCX_NEXT`` and defers to the next BPF program in the TCX link. For all other hooks (Netfilter, XDP, cgroup_skb), ``NEXT`` is equivalent to ``ACCEPT``.

* @return 0 on success, or a negative errno value on failure.
*/
static int _bf_cgroup_skb_get_verdict(enum bf_verdict verdict)
static int _bf_cgroup_skb_get_verdict(enum bf_verdict verdict, int *ret_code)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are internal APIs, I think you could have kept the same return mechanism without adding the ret_code param and just check for -ENOTSUP on the caller. Obviously a matter of style but less changes.

case BF_VERDICT_DROP:
case BF_VERDICT_NEXT:
return true;
default:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned on another diff, it would be nice if this is an exhaustive check instead of using default

jordalgo pushed a commit to jordalgo/bpftrace that referenced this pull request Mar 18, 2026
Integrates Claude Code as an AI assistant for reviewing pull requests. This
is experimental and relies on a subscription provided by Meta.

Example of what this review looks like:
facebook/bpfilter#474 (comment)

Signed-off-by: Jordan Rome <linux@jordanrome.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants