[firewall] opt-in in-process nftables backend via libnftnl/libmnl

[LorbusChris]

Summary

Adds an opt-in in-process nftables backend for the OTBR firewall, speaking
netlink directly via libnftnl/libmnl. Addresses #1675, which suggested talking to
the kernel directly via libmnl/libnftnl rather than calling binaries from C++.

The legacy ipset/ip6tables shell firewall remains the default. Building with
-DOTBR_NFTABLES=ON (default OFF) switches the whole firewall in-process:
otbr-agent owns a single inet otbr table, including producing the ingress
prefix sets from Thread network data (refreshed on OT_CHANGED_THREAD_NETDATA),
replacing the OpenThread posix platform firewall.cpp ipset producer
(OT_FIREWALL is forced OFF for such builds).

table inet otbr {
    set ingress_deny_src   { type ipv6_addr; flags interval; }   # on-mesh + mesh-local, agent-produced
    set ingress_allow_dst  { type ipv6_addr; flags interval; }   # on-mesh, agent-produced

    chain forward_ingress  { type filter hook forward     priority filter; ... }
    chain nat_prerouting   { type filter hook prerouting  priority mangle; ... }  # fwmark 0x1001, ipv4 only
    chain nat_postrouting  { type nat    hook postrouting priority srcnat; ... }  # masquerade by mark
    chain nat_forward      { type filter hook forward     priority filter; ... }
}

Ingress rule semantics match the legacy ip6tables chain 1:1. Set updates are a
single atomic kernel batch (flush both sets + re-add), so there is never a
half-populated window. Firewall install failures are fatal, matching the legacy
scripts' die-on-failure: a border router that cannot install its ingress
filter must not silently forward unfiltered.

Commits (independently reviewable)

1. add INftables interface and libnftnl/libmnl backend INftables interface + libnftnl/libmnl backend + the
   OTBR_NFTABLES flag (default OFF). Focus: netlink/batch correctness.
2. add FirewallManager policy layer with unit tests FirewallManager policy layer + gmock unit tests.
   Focus: table lifecycle, rule/set construction, atomic replace.
3. wire the nftables backend into otbr-agent (opt-in) otbr-agent wiring: install on init, produce ingress sets
   from netdata, tear down on shutdown; OT_FIREWALL=OFF for nftables
   builds. Focus: producer parity with OT-core's ipset producer.
4. gate legacy firewall install on the nftables opt-in Deployment gating: Docker run/finish, OpenWrt init and
   host script/setup skip the legacy firewall/NAT44 when the in-process
   backend is in use, so the two stacks never double-install. Focus: every
   path defaults to legacy; opt-in is explicit everywhere.
5. build the nftables backend in CI A nftables-check job that configures
   -DOTBR_NFTABLES=ON and runs the test suite. Focus: the netlink backend is
   actually compiled and exercised in CI.

Opting in per platform

• CMake/host: -DOTBR_NFTABLES=ON, and OTBR_NFTABLES=1 in the
  environment for script/setup (skips the legacy otbr-firewall /
  otbr-nat44 services).
• Docker: --build-arg OTBR_OPTIONS="-DOTBR_NFTABLES=ON" and run with
  -e OTBR_NFTABLES=1 (skips the legacy blocks in the s6 scripts).
• OpenWrt: -DOTBR_NFTABLES=ON plus a manual DEPENDS swap in
  etc/openwrt/openthread-br/Makefile — drop +ip6tables +ipset +iptables-mod-extra, add +libmnl +libnftnl +kmod-nft-core +kmod-nft-nat.
  The Makefile carries a comment rather than a switch, since the package recipe
  has no visibility of the CMake option. The otbr-firewall init script is
  dropped automatically by src/openwrt/CMakeLists.txt.

Validation

The interval-set element encoding (two-element start/INTERVAL_END) and the
atomic flush-and-refill batches were validated against live kernels twice
(6.8/nftables 1.0.9 in a VM, 7.1/nftables 1.1.6 in a container), including
verifying that nft renders the libnftnl-populated sets as the expected
prefixes. An end-to-end on-device test with a real RCP (join, ingress
filtering, NAT44) is still outstanding.

Notes for reviewers

• ND-proxy/DUA: upstream removed the DUA routing feature and nd_proxy.cpp
  ([dua] remove DUA routing feature #3360/[build] remove all references to DUA #3377) while this PR was in flight. The firewall library still carries
  the ND-proxy NFQUEUE redirect API (EnableNdProxyRedirect /
  dua_prerouting), currently unconsumed — kept for when ND-proxy returns;
  happy to drop it if preferred.
• With OTBR_NFTABLES=OFF the installed firewall/NAT behaviour is unchanged from
  main — every new runtime path is OTBR_ENABLE_NFTABLES-gated. The build
  does differ slightly even when off: script/bootstrap and the Dockerfile
  install the libnftnl/libmnl headers unconditionally, and the otbr-firewall
  policy layer plus its gmock test binary are always compiled. nftables_impl.cpp
  and the libnftnl/libmnl link are what the option actually gates.
• The nat_prerouting/nat_postrouting/nat_forward chains are installed only
  when a backbone/infra interface is configured; that interface is the masquerade
  upstream, matching the legacy OT_INFRA_IF.
• Wiring is RCP-mode only. NCP mode is untouched and gets no in-process firewall.
• A nftables-check CI job builds with -DOTBR_NFTABLES=ON and runs the test
  suite, so nftables_impl.cpp is compiled and the firewall tests run in the
  configuration they describe. The FirewallManager tests also run in the default
  build, since that target is unconditional.

🤖 Generated with Claude Code

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[gemini-code-assist]

Code Review

This pull request introduces an in-process firewall management system using nftables (via libnftnl and libmnl), replacing the legacy shell-script-based iptables and ipset configuration. It adds a new FirewallManager class to handle ingress filtering, NAT44 masquerading, and ND-proxy redirects directly within otbr-agent. The changes include build system updates, Dockerfile modifications, and integration into the Application and BackboneAgent components. Unit tests for the new firewall logic are also provided. I have no feedback to provide.

[LorbusChris]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces an in-process nftables firewall backend for the OpenThread Border Router (otbr-agent), replacing the legacy shell-based ipset/ip6tables setup. It adds a FirewallManager and an INftables interface with a concrete implementation using libnftnl and libmnl. The review feedback recommends adding an AbortBatch() method to the INftables interface and its implementation to safely discard half-built batches and prevent transaction leaks. Additionally, the feedback highlights a state desynchronization bug in EnableNdProxyRedirect where mNdRuleHandle is cleared before a successful commit, and suggests replacing an unsafe memset on Ip6Prefix with C++ value-initialization.

[LorbusChris]

Thanks — the batch-leak finding is correct and is now fixed, with a couple of adjustments.

What was actually broken. Every FirewallManager operation wraps its work in BeginBatch()/CommitBatch() with SuccessOrExit in between, so any intermediate failure jumped to exit: without committing. Because BeginBatch() guards on VerifyOrExit(!mInBatch, ...), a single mid-batch failure wedged the backend for the lifetime of the process — every later firewall operation returned OTBR_ERROR_INVALID_STATE — and leaked mBatchBuffer. Fixed by adding AbortBatch() and calling it on the error path of all nine functions that open a batch.

One correction: CommitBatch() was already cleaning up unconditionally in its own exit: block, so a failed commit did not leave the backend stuck. Rather than duplicate that logic, CommitBatch() now delegates to AbortBatch(), and AbortBatch() is documented as safe to call when no batch is open, so callers needn't track whether one was started.

mNdRuleHandle desync — correct, and fixed: the handle is only updated after CommitBatch() succeeds, so a rolled-back batch leaves the manager still able to delete the rule installed earlier.

memset on Ip6Prefix — good catch, and it was redundant rather than merely unsafe: Ip6Prefix has a default constructor that calls Clear(). Dropped the memset outright instead of replacing it with {}.

Tests. Added two regression tests and checked they actually fail without the fix — on a tree with only the behavioural changes reverted: 17 passed / 2 failed; with them: 19/19.

• FailedOperationAbortsTheBatch — an intermediate failure must abort and must not commit.
• EnableNdProxyKeepsRuleHandleWhenCommitFails — after a failed commit, a later call must still delete the previously installed rule.

CI. Also added a nftables-check job so nftables_impl.cpp is compiled and the firewall tests run in that configuration; previously only the policy layer was covered, since that target builds unconditionally.

All fixes are folded into the commits that introduced the code, so the history stays bisectable.

[LorbusChris]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces an in-process nftables firewall backend (OTBR_NFTABLES) as an opt-in alternative to the legacy shell-based ipset/ip6tables firewall. It adds a new FirewallManager and Nftables wrapper using libnftnl and libmnl to manage ingress filtering, NAT44 masquerading, and ND-proxy redirects in-process. Feedback on the changes highlights several robustness issues: ignoring the return value of mnl_nlmsg_batch_next could lead to heap corruption on batch overflows, a potential null-pointer dereference crash exists in Application::UpdateIngressPrefixes if the OpenThread instance is uninitialized, and EnableNat44Masquerade lacks a check to prevent invalid rules when called with an empty upstream interface name.

[LorbusChris]

Thanks — all three are addressed.

mnl_nlmsg_batch_next() return value (critical). Correct, and it was ignored at all ten call sites. The buffer is twice the batch limit, so the message that crosses the limit is written safely into the overflow page, but nothing may be added after that — which the code did not enforce. In practice the exposure was bounded, since the number of queued messages comes from Thread network data rather than anything unbounded, but that is an implicit invariant that nothing documents or checks, so it should not be relied on.

Rather than repeat the check ten times, added a small AdvanceBatch() helper that every queueing path now goes through, failing with ENOBUFS:

VerifyOrExit(mnl_nlmsg_batch_next(mBatch), errno = ENOBUFS, error = OTBR_ERROR_ERRNO);

I deliberately did not auto-flush and reset on overflow, which is the other way libmnl supports: that would split one logical change across two kernel transactions and lose the atomicity the batch exists to provide. Failing the whole operation — and, with the abort path added earlier, discarding the batch — keeps it all-or-nothing.

GetInstance() may be null. Added the guard. Both current call sites run after Init(), so this is defensive rather than a live crash, but it costs nothing and matches how the rest of the file is written.

Empty upstream interface name. Added VerifyOrExit(!aUpstreamInterfaceName.empty(), error = OTBR_ERROR_INVALID_ARGS). The only caller already guards on !mBackboneInterfaceName.empty(), but this is a public method and should validate its own input. Covered by a new test asserting it is rejected before any batch is opened.

Firewall suite is 20/20 with the nftables backend enabled, and all changes are folded into the commits that introduced the code.

[LorbusChris]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces an opt-in, in-process nftables firewall backend (OTBR_NFTABLES) for the OpenThread Border Router, replacing the legacy shell-based ipset/ip6tables firewall with a robust C++ implementation using libnftnl and libmnl. The review feedback highlights several critical improvements for robustness and portability, including: preventing potential null pointer dereferences during expression allocation, ensuring proper 4-byte alignment of stack-allocated Netlink buffers to avoid bus errors on alignment-sensitive architectures like MIPS, using the IfNameToWire helper for consistency, and adopting pkg_check_modules in CMake to better support cross-compilation environments.

[LorbusChris]

All six addressed.

Unchecked expression allocation (high). Correct. The eleven Make* helpers return nullptr when nftnl_expr_alloc() fails, and all 44 nftnl_rule_add_expr() call sites passed the result straight through, where it would have been dereferenced. Rather than repeat the check 44 times, every site now goes through a helper that fails with ENOMEM:

otbrError AddExpr(struct nftnl_rule *aRule, struct nftnl_expr *aExpr)
{
    VerifyOrExit(aExpr != nullptr, errno = ENOMEM, error = OTBR_ERROR_ERRNO);
    nftnl_rule_add_expr(aRule, aExpr);
    ...

Combined with the abort path added earlier, a failed allocation now unwinds the whole operation and discards the batch instead of building a partial rule.

Netlink buffer alignment. Correct, and worth taking seriously here: OpenWrt targets MIPS, which is exactly where an under-aligned buffer cast to struct nlmsghdr bites. Added alignas(struct nlmsghdr) to all three stack buffers.

IfNameToWire. Fixed — that site hand-rolled the memset/memcpy while the other two already used the helper.

pkg_check_modules. Adopted, and it matches the file, which already does find_package(PkgConfig) and pkg_check_modules(DBUS ...). I also wired up *_INCLUDE_DIRS and the library dirs, since switching the lookup without consuming them would not actually have helped cross-compilation, which is the point of the change.

One deviation: the suggestion's natural companion, target_link_directories(), requires CMake 3.13, while this project sets cmake_minimum_required(VERSION 3.10.2). I used the generator-expression idiom already used for D-Bus in src/dbus/common/CMakeLists.txt instead:

$<$<BOOL:${LIBNFTNL_LIBRARY_DIRS}>:-L$<JOIN:${LIBNFTNL_LIBRARY_DIRS}," -L">>

Verified with libnftnl 1.2.4 / libmnl 1.0.4 found via pkg-config, full build clean, firewall suite 20/20. All changes folded into the commits that introduced the code.

[LorbusChris]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces an opt-in, Linux-only in-process nftables firewall backend (OTBR_NFTABLES) for the OpenThread Border Router, replacing the legacy ipset/ip6tables shell scripts. It adds a new FirewallManager and Nftables implementation using libnftnl and libmnl, integrates them into the main application, and updates build configurations, Dockerfiles, and OpenWrt packages. A high-severity memory leak was identified in Nftables::BeginBatch where a failure in AdvanceBatch() leaks the allocated mnl_nlmsg_batch structure because mnl_nlmsg_batch_stop is not called during cleanup.

[LorbusChris]

Correct, and thanks — that leak was one I introduced in the previous round. Before AdvanceBatch() was added there was no error path after mnl_nlmsg_batch_start() succeeded, so mBatch = nullptr in the exit block was only ever reached when mBatch was already null. Adding the check created a path where a live batch was dropped without mnl_nlmsg_batch_stop().

Fixing it surfaced a second problem on the same exit path, which is the more serious of the two: VerifyOrExit(!mInBatch, ...) jumps to that block as well, and the block unconditionally did free(mBatchBuffer). So calling BeginBatch() while a batch was already open freed the live batch's buffer while leaving mInBatch set and mBatch dangling — the guard corrupted the in-flight batch instead of rejecting the call.

Both are fixed by cleaning up only what the call itself allocated:

exit:
    // Only clean up what this call allocated. When the failure is that a batch
    // is already open, mInBatch is still set and the buffer belongs to that
    // batch, so it must be left alone.
    if (error != OTBR_ERROR_NONE && !mInBatch)
    {
        AbortBatch();
    }
    return error;

mInBatch is set only on the success path, so every failure that allocated something sees it clear and goes through AbortBatch(), which stops the batch, frees the buffer and clears the pending-handle map. The already-in-a-batch case skips cleanup entirely. AbortBatch() is null-safe, so the early failures (no socket, calloc failure) are fine too.

One caveat: this is not covered by the unit tests. They drive FirewallManager through the mocked INftables, so they never enter the real Nftables::BeginBatch(), and reaching these paths needs a live netlink socket. Verified by inspection rather than by test; the suite is still 20/20 and the full build is clean.

Folded into the commit that introduces the backend.

[LorbusChris]

/gemini review

[LorbusChris]

Both taken.

Deinit() and mPendingHandles. Correct. Those entries point at caller stack variables — FirewallManager passes &handle for locals — so keeping them across a Deinit()/Init() cycle risks a later reply matching a stale sequence number and writing through a dangling pointer.

Rather than adding the missing line, Deinit() now calls AbortBatch(), which already stops the batch, frees the buffer, clears mInBatch and clears the map. Deinit() was duplicating three of those four steps by hand, which is how the fourth came to be missing, so this removes the duplication as well as the bug.

EINTR in DrainSocket(). Also correct, and it undercut the change it belonged to: a signal would end the drain early and leave behind exactly the stale replies the drain exists to remove. It now continues on EINTR and stops on EAGAIN, matching CommitBatch().

Worth saying plainly: that hole was avoidable. EINTR handling was added to CommitBatch() and SendStandalone() a few rounds ago, and I wrote a new receive loop without applying the same rule. Thanks for catching it.

Firewall suite 22/22, full build clean, clang-format clean, folded into the commit that introduces the backend. CI on the previous head came back green for nftables-check (Debug and Release) and pretty.

[LorbusChris]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces an opt-in, Linux-only in-process nftables firewall backend (OTBR_NFTABLES) for the OpenThread Border Router, which manages ingress filtering, NAT44 masquerading, and ND-proxy redirection directly within otbr-agent using libnftnl and libmnl. This modern implementation replaces the legacy shell-script-based ipset/ip6tables setup. The review feedback highlights two safety improvements: explicitly zero-initializing the prefix structure in Application::UpdateIngressPrefixes to prevent uninitialized memory issues, and adding a bounds check on the length parameter in MakeBitwiseMask to avoid potential out-of-bounds reads.

[LorbusChris]

One taken, one declined.

MakeBitwiseMask() bounds. Taken. The XOR operand is read out of a fixed 16-byte buffer at aLen bytes, so it now refuses anything longer. The only caller passes a literal 16, so this is defence in depth on a length-parameterised helper rather than a live bug.

Ip6Prefix initialisation. Not applying this, and I want to lay out why, because this is the fourth time it has come round and the guidance has now reversed:

• Originally the code was Ip6Prefix mlPrefix; followed by a memset. Review asked for the memset to go, on the grounds that memset on a non-POD is unsafe. It was removed.
• Then review asked for Ip6Prefix mlPrefix{}, on the grounds that the bare declaration leaves stack garbage. That was applied.
• Now review asks to drop {} and add the memset back, on the grounds that {} may not zero the member.

Those cannot all hold, and in fact none of the three states differ:

class Ip6Prefix
{
public:
    Ip6Prefix(void) { Clear(); }
    ...
    void Clear(void) { memset(reinterpret_cast<void *>(this), 0, sizeof(*this)); }

src/common/types.hpp:337, :343, :396. Ip6Prefix has a user-provided default constructor, so both Ip6Prefix mlPrefix; and Ip6Prefix mlPrefix{} run it, and it zeroes the entire object including mPrefix. There is no uninitialised memory for the memcpy to leave behind, and the suggested memset would be dead code — the same dead code removed earlier.

Leaving it as Ip6Prefix mlPrefix{}. Happy to be overruled by a maintainer, but I would rather not churn the line a third time on reasoning that reverses each round.

Firewall suite 22/22, build clean, clang-format clean, folded into the commits that introduce the code.

[LorbusChris]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces an opt-in, in-process nftables firewall backend (OTBR_NFTABLES) for the OpenThread Border Router, replacing the legacy shell-based ipset/ip6tables firewall. This change adds libnftnl and libmnl as dependencies and implements a new FirewallManager and Nftables wrapper to manage ingress filtering and NAT44 masquerading directly within otbr-agent. Build scripts, Dockerfiles, OpenWrt configurations, and tests have been updated to support this new backend. Feedback on the implementation suggests reducing code duplication in src/firewall/nftables_impl.cpp by reusing the ApplyPrefixMask helper function within AttachPrefixInterval.

[LorbusChris]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces an opt-in, in-process nftables firewall backend (OTBR_NFTABLES) for the OpenThread Border Router, replacing the legacy shell-based ipset/ip6tables setup. The implementation adds FirewallManager and Nftables classes to manage firewall rules and NAT44 masquerading directly within otbr-agent, alongside corresponding build, Docker, and OpenWrt configuration updates. Feedback on the changes suggests utilizing target_link_directories instead of manually constructing -L linker flags with generator expressions in CMake to align with modern CMake best practices.

[LorbusChris]

Folded a testability change into the first two commits.

The netlink socket now sits behind an INetlinkSocket interface (MnlNetlinkSocket in production), so Nftables can be constructed against a fake. The error paths raised across the review rounds were previously unreachable from a unit test — they all depend on how the socket behaves, and there was no way to make a real one time out, return short, or fail mid-batch on demand.

Six cases in otbr-gtest-firewall now cover them:

• retrying a receive interrupted by a signal, and not retrying a genuine timeout
• rejecting a zero-length reply instead of reading past it
• draining stale replies before sending a new batch
• a failed operation not leaving the backend wedged for the process lifetime
• a nested BeginBatch() being rejected without tearing down the batch already open

Each was checked by reverting the corresponding fix and confirming the matching test fails; all six do. Counts are 28 tests with -DOTBR_NFTABLES=ON and 22 with it off, where the backend cases compile out.

[LorbusChris]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces an in-process nftables firewall backend for the OpenThread Border Router (otbr-agent), replacing the legacy shell-script-based ipset/ip6tables setup. It adds a new FirewallManager and Nftables implementation using libnftnl and libmnl to manage ingress filtering, NAT44 masquerading, and ND-proxy redirection directly in-process, along with updates to the build configuration, bootstrap scripts, and unit tests. The review feedback suggests improving memory management by using MakeUnique instead of direct new allocation in Nftables constructors, and simplifying FirewallManager by removing unused handle variables where rule handles are not needed.

[LorbusChris]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces an opt-in, in-process nftables firewall backend (OTBR_NFTABLES) for the OpenThread Border Router, replacing the legacy shell-based ipset/ip6tables setup. It adds a new firewall module featuring FirewallManager and Nftables implementations, integrates them into the main Application lifecycle, and updates build configurations, Dockerfiles, and OpenWrt packages to support the new libmnl and libnftnl dependencies. The review feedback highlights a potential vulnerability where mSocket could be dereferenced when null in the Send, Recv, and Drain methods of MnlNetlinkSocket, and suggests adding appropriate null checks to prevent crashes.

[LorbusChris]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces an opt-in, in-process nftables firewall backend (OTBR_NFTABLES) for the OpenThread Border Router (otbr-agent), replacing the legacy shell-based ipset/ip6tables firewall setup. It implements a new FirewallManager and Nftables backend using libnftnl and libmnl to manage the Thread ingress filter, NAT44 masquerading, and ND-proxy redirects directly within the agent process. The changes also include updates to build configurations, Docker scripts, OpenWrt packages, and bootstrap scripts to support the new dependencies, alongside a comprehensive suite of unit tests using a scriptable fake netlink socket. No review comments were provided, so there is no additional feedback to address.

[LorbusChris]

Friendly ping for a CI approval / first review pass. Two notes that may help planning:

1. This PR is self-contained (nftables backend behind an opt-in flag; legacy iptables path untouched by default).
2. I am about to submit a series adding ubus methods for Matter border-router integration (refactor + pending-dataset support + provision/deprovision methods). The only file both touch is src/agent/application.cpp, and the overlap is trivial to rebase in either order, so review order does not matter.

The backend has been running in production on an OpenWrt (Turris Omnia) border router.