trafficshaper: add nftables firewall backend#29830
Conversation
| command -v nft &>/dev/null || return 0 | ||
|
|
||
| d "Cleaning nftables" | ||
| $NFT delete table inet "$NFT_TABLE" &>/dev/null || : |
There was a problem hiding this comment.
changed it to use destroy table now.
| printf '%s\n' "delete table inet $NFT_TABLE" | ||
| printf '%s\n' \ | ||
| "add table inet $NFT_TABLE" \ | ||
| "add chain inet $NFT_TABLE forward { type filter hook forward priority -150; policy accept; }" \ |
There was a problem hiding this comment.
Given iptables code does -A you have to use priority mangle + (1...10) to perform the processing after the default hook. Even better you can make it completely independent from fw4 or fw3 or their mere presence altogether.
There was a problem hiding this comment.
Ah, ok, i changed the nft hook to use priority mangle + 10, so it runs after
the default mangle instead of at the same priority. It also checks just nft.
dhrm1k
force-pushed
the
trafficshaper-nft-backend
branch
2 times, most recently
from
4a9716f to
5efcd9b
Compare
| CATEGORY:=Network | ||
| TITLE:=WAN traffic shaper based on LAN addresses | ||
| DEPENDS:=+tc +kmod-sched-core +kmod-sched-connmark +kmod-ifb +iptables +IPV6:ip6tables +kmod-sched-cake +iptables-mod-conntrack-extra | ||
| DEPENDS:=+tc +kmod-sched-core +kmod-sched-connmark +kmod-ifb +nftables +iptables +IPV6:ip6tables +kmod-sched-cake +iptables-mod-conntrack-extra |
There was a problem hiding this comment.
Please use "complex dependency" to shorten installed package list like
+(nftables||(iptables IP6:6tables ipt-mod-whatever)) /// syntax may not be top correct...
https://openwrt.org/docs/guide-developer/packages#dependency_types
In short - if nftables are satisfied like in 99% cases nothing is added, in other cases spmething additional is installed.
There was a problem hiding this comment.
I had no idea about this. This is good. I have now changed it to conditional instead of having both firewall stacks.
There was a problem hiding this comment.
A note for next reviewers - since priority of nft code path is skewed it can coexist (oredered predictably) with legacy tables hooks like fw3 or shorewalls. There is no need for more heuristics in default auto mode.
| *) die 2 "Unknown address family of network $class_net in class $class!" | ||
| esac | ||
| if [ "$class_reserved_uplink" ]; then | ||
| printf '%s\n' "add rule inet $NFT_TABLE classify meta mark & $mark_mask == 0x0 $nft_family saddr $class_net counter meta mark set (meta mark & $mark_mask_inv) | $xi comment \"$APPNAME-$class up\"" |
There was a problem hiding this comment.
Totally optional but you can compact multiple count(class_net) lines by using map ipo $xi
There was a problem hiding this comment.
Thanks, that would surely help, but I’ll leave it out for now to keep this PR
closer to the existing iptables path and easier to review.
I’ll certainly look into doing it as a follow-up once the basic nft backend
looks okay.
There was a problem hiding this comment.
Agree, while filter length before potential branching logic looks gross it consists of generally efficient meta integer comparisons.
There was a problem hiding this comment.
Got it, thanks. I’ll keep it simple in this PR then.
dhrm1k
force-pushed
the
trafficshaper-nft-backend
branch
from
5efcd9b to
1795a12
Compare
brada4
left a comment
brada4
left a comment
There was a problem hiding this comment.
LGTM (including directly applying to stable trains)
|
Thanks @dhrm1k for this PR. It looks pretty good. I'll test it in my lab and return ASAP. |
|
It is still installing iptables compatible tools: Openwrt has two nftables flavors: nftables-json-1.1.6-r4 x86_64 {feeds/base/network/utils/nftables} (GPL-2.0) [installed] Both provides nftables but Provides is not used during config but only during installation. This is menuconfig info: You'll need something like this to use either one of them: I also tested shaping and it is working as expected. |
| DEPENDS:=+tc +kmod-sched-core +kmod-sched-connmark +kmod-ifb \ | ||
| +PACKAGE_nftables:nftables +!PACKAGE_nftables:iptables \ | ||
| +(IPV6&&!PACKAGE_nftables):ip6tables \ | ||
| +kmod-sched-cake +!PACKAGE_nftables:iptables-mod-conntrack-extra |
There was a problem hiding this comment.
Use something like this as PACKAGE_nftables does not exist as config variable.
DEPENDS:=+tc +kmod-sched-core +kmod-sched-connmark +kmod-ifb \
+(PACKAGE_nftables-json||PACKAGE_nftables-nojson):nftables \
+!(PACKAGE_nftables-json||PACKAGE_nftables-nojson):iptables \
+(IPV6&&!(PACKAGE_nftables-json||PACKAGE_nftables-nojson)):ip6tables \
+kmod-sched-cake \
+!(PACKAGE_nftables-json||PACKAGE_nftables-nojson):iptables-mod-conntrack-extra
There was a problem hiding this comment.
I didn’t know there were two nftables flavours. Thanks.
I updated the dependency check to use the concrete nftables-json /
nftables-nojson package symbols instead of PACKAGE_nftables.
dhrm1k
force-pushed
the
trafficshaper-nft-backend
branch
from
1795a12 to
0c81cb3
Compare
Thanks for putting your time into this. Since you already tested shaping, after you check the dependency part, if it |
|
Maybe this https://openwrt.org/docs/guide-developer/dependencies#using_boolean_operators |
|
I tested the current form with both In both cases the generated I think this is probably as compact as it can reasonably get, because the |
I tested shaping with both iptables-nft and pure nft. It’s great to see this package working smoothly after 8 years! Regarding the dependencies: it is still pulling some kmod-ipt-* packages, but this is an upstream inheritance from kmod-sched-connmark: define KernelPackage/sched-connmark
SUBMENU:=$(NETWORK_SUPPORT_MENU)
TITLE:=Traffic shaper conntrack mark support
DEPENDS:=+kmod-sched-core +kmod-ipt-core +kmod-ipt-conntrack-extraWhile we could theoretically drop kmod-sched-connmark, it is required by our script's architecture to restore conntrack marks at the WAN ingress stage before redirecting traffic to the IFB device. This is currently the only viable way to share a global QoS rule across multiple LAN/VLAN interfaces. Once the OpenWrt core drops legacy ipt- kernel modules for good, a native alternative will surely take over. For now, this is as optimized as it gets: userspace is 100% free of iptables binaries, and a few passive kernel modules won't hurt performance. Again, thanks for taking care of it. |
Remove iptables kmod dependency from kmod-sched-connmark. Both dependents package/network/config/qos-scripts ../packages/net/trafficshaper Already pull in ipt kmods via iptables-mod-conntrack-extra Permits "clean" nftables migration at openwrt/packages#29830 Signed-off-by: Andris PE <[email protected]>
|
@luizluca this can pass independently from main repo commit, just that ad interim kmod-sched- will pull those unnecessary ipt modules. iptables side will pull those modules just once. |
|
@BKPepe whenever you get time, please look into this. |
BKPepe
force-pushed
the
trafficshaper-nft-backend
branch
from
0c81cb3 to
b50b4c3
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds an nftables-based firewall backend to trafficshaper for packet marking (used by tc), while keeping the existing iptables-based backend available for older/non-nft systems, aligning the package with the ongoing firewall4/nftables migration.
Changes:
- Make the package dependencies conditional so
nftablesoriptablesis pulled in depending on the build selection. - Extend the init script to support
firewall_backendselection (auto/nftables/iptables) and implement nftables rule installation/cleanup. - Add
option firewall_backend 'auto'to the default UCI config and validation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| net/trafficshaper/Makefile | Updates release and makes firewall backend dependencies conditional (nftables vs iptables). |
| net/trafficshaper/files/trafficshaper.init | Adds backend selection logic and a new nftables ruleset backend for marking + cleanup paths. |
| net/trafficshaper/files/trafficshaper.conf | Introduces a configurable firewall_backend defaulting to auto. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "add chain inet $NFT_TABLE forward { type filter hook forward priority mangle + 10; policy accept; }" \ | ||
| "add chain inet $NFT_TABLE classify" \ | ||
| "# Keep ct and packet marks in sync for tc fw filters." \ | ||
| "add rule inet $NFT_TABLE forward counter meta mark set ct mark comment \"Get previous class\"" \ |
There was a problem hiding this comment.
1/ proposed syntax does not work
2/ the exact original bits are copied back
| : $((i++)) | ||
| done | ||
|
|
||
| printf '%s\n' "add rule inet $NFT_TABLE classify counter ct mark set meta mark comment \"Save class\"" |
| stop_firewall_iptables() { | ||
| d "Cleaning iptables" | ||
| for IPT in "$IP4T" "$IP6T"; do | ||
| $IPT -t mangle -D FORWARD -j $IPT_CHAIN &>/dev/null || : | ||
| $IPT -t mangle -F $IPT_CHAIN &>/dev/null || : | ||
| $IPT -t mangle -X $IPT_CHAIN &>/dev/null || : | ||
| $IPT -t mangle -F $IPT_CHAIN-classify &>/dev/null || : | ||
| $IPT -t mangle -X $IPT_CHAIN-classify &>/dev/null || : | ||
| done | ||
| } |
|
@dhrm1k iptables-translate was wrong in this case ;-) With nftables you have to destroy one of 2 marks.... |
dhrm1k
force-pushed
the
trafficshaper-nft-backend
branch
from
b50b4c3 to
4ce07e9
Compare
|
@dhrm1k I have tried quick port of sqm script port and it went "very well" https://bugzilla.netfilter.org/show_bug.cgi?id=1840 |
|
Yeah, I also have a first pass for sqm-scripts now. I have a PR open. I'd love if you'd take a look at that. Thanks for the DSCP / inet table note. I’ll look into splitting the ip/ip6 handling over the weekend. |
|
I think the review comments on the nft backend are addressed now. |
|
As for mine - yes, all resoved, I can not tap them away, just mark them resolved yourself.... |
|
You fell for clankers |
brada4
left a comment
brada4
left a comment
There was a problem hiding this comment.
clankers broke the stuff
trafficshaper[INFO]: Starting trafficshaper
/dev/stdin:7:1-131: Error: Could not process rule: Not supported
add rule inet trafficshaper forward counter meta mark set (meta mark & 0xFFFFFF00) | (ct mark & 0xFF) comment "Get previous class"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/dev/stdin:21:1-122: Error: Could not process rule: Not supported
add rule inet trafficshaper classify counter ct mark set (ct mark & 0xFFFFFF00) | (meta mark & 0xFF) comment "Save class"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| "add chain inet $NFT_TABLE classify" \ | ||
| "# Keep ct and packet marks in sync for tc fw filters." \ | ||
| "add rule inet $NFT_TABLE forward counter meta mark set ct mark comment \"Get previous class\"" \ | ||
| "add rule inet $NFT_TABLE forward meta mark & $mark_mask == 0x0 counter jump classify comment \"If no class, try to classify\"" |
There was a problem hiding this comment.
+ meta mark & 0x000000ff ne 0x0 return
+ goto classify comment "If no class, try to classify"
- meta mark & 0x000000ff == 0x00000000 counter packets 0 bytes 0 jump classify comment "If no class, try to classify"
}
chain classify {
- meta mark & 0x000000ff == 0x00000000 ip saddr 192.168.1.0/24 counter packets 0 bytes 0 meta mark set meta mark & 0xffffff01 | 0x00000001 comment "trafficshaper-corp up"
+ ip saddr 192.168.1.0/24 counter packets 0 bytes 0 meta mark set meta mark & 0xffffff01 | 0x00000001 comment "trafficshaper-corp up"There was a problem hiding this comment.
(i can do retouching later, this shows iptable decorative logic remains)
| "add chain inet $NFT_TABLE forward { type filter hook forward priority mangle + 10; policy accept; }" \ | ||
| "add chain inet $NFT_TABLE classify" \ | ||
| "# Keep ct and packet marks in sync for tc fw filters." \ | ||
| "add rule inet $NFT_TABLE forward counter meta mark set ct mark comment \"Get previous class\"" \ |
There was a problem hiding this comment.
1/ proposed syntax does not work
2/ the exact original bits are copied back
dhrm1k
force-pushed
the
trafficshaper-nft-backend
branch
2 times, most recently
from
ef66fe8 to
3340285
Compare
|
Ah, yes, I fell for that one. The masked I reworked this now. |
|
IMO could do away without extra chain. wdyt? we dont need a sane return to calling chain in iptables... counter packets 0 bytes 0 goto classify comment "If no class, try to classify"
}
chain classify { |
brada4
left a comment
brada4
left a comment
There was a problem hiding this comment.
Great job, besides maintaining semantics identical to ip(6) tables also optimises away some slack processing present in ip(6) tables.
|
@BKPepe call in the machines, looks (very) nice to us humans. |
|
@GeorgeSapkin I am looking into the Test via Docker and I see there failure: Not sure how we are going to test this package, because it says: Anyway, I'd love to summon the machines, but you know, I got a bit carried away and blew through my entire new credit allowance in 4 days, and now I'm out. :-D If humans are okay with this, I will merge this soon. |
Remove iptables kmod dependency from kmod-sched-connmark. Both dependents package/network/config/qos-scripts ../packages/net/trafficshaper Already pull in ipt kmods via iptables-mod-conntrack-extra Permits "clean" nftables migration at openwrt/packages#29830 Signed-off-by: Andris PE <[email protected]> Link: openwrt#24019 Signed-off-by: Jonas Jelonek <[email protected]>
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit. One inline concern about the nftables backend; commit message and CI are fine.
Generated by Claude Code
| "add chain inet $NFT_TABLE forward { type filter hook forward priority mangle + 10; policy accept; }" \ | ||
| "add chain inet $NFT_TABLE classify" \ | ||
| "add rule inet $NFT_TABLE forward meta mark & $mark_mask != 0x0 counter return" \ | ||
| "add rule inet $NFT_TABLE forward counter goto classify comment \"If no class, try to classify\"" |
There was a problem hiding this comment.
Downlink shaping looks like it may be broken with this backend, because the connmark save/restore step was dropped rather than reworked.
The iptables backend saves the class into the connection mark at the end of the classify chain (CONNMARK --save-mark, L268-L269) and restores it at the top of FORWARD. Downlink is shaped on the IFB device, and IFB packets are classified at WAN ingress via tc ... action connmark (L377) — that is, from the connection mark only. The ingress redirect runs before the FORWARD hook, so the FORWARD daddr rules never reach the IFB copy; the connmark is the only channel that carries the class to the downlink path.
The nftables forward/classify chains here set meta mark but never write ct mark, so the connection mark stays 0 and the ingress action connmark restores nothing. Uplink (WAN egress, marked in FORWARD before the qdisc) should still work, but the downlink fw filters on the IFB will never match and all downlink traffic falls into the default class.
Note the earlier revision did carry ct mark set (ct mark & mask_inv) | (meta mark & mask), but it was removed after nft rejected that mixed expression ("Could not process rule: Not supported") instead of being reworked into a form nft accepts. Could you confirm downlink shaping was actually verified on the nftables backend (not just uplink)? If so, please disregard.
Generated by Claude Code
There was a problem hiding this comment.
The iptables backend saves the class into the connection mark at the end
That is lost now....
clanker: instead of being reworked into a form nft accepts.
How do you do it IRL? bit-addressed marks are lost in nftables, and iptables-translate return incomplete translation
|
Suggest 2 and a half changes: 1/ check ct mark's bits for return (whethet packet is recognized from ct) |
|
turning page upside down .... we dont need to touch meta mark at all? |
|
Ah, that really is upside down. gotta understand. I am away for the weekend, so I will look at it again after that. I really appreciate the time you put into reviewing/explaining this. I learned a lot from this PR already. |
|
Thank you for the effort so far, actually old ipt code could use some untangling ... |
Remove iptables kmod dependency from kmod-sched-connmark. Both dependents package/network/config/qos-scripts ../packages/net/trafficshaper Already pull in ipt kmods via iptables-mod-conntrack-extra Permits "clean" nftables migration at openwrt/packages#29830 Signed-off-by: Andris PE <[email protected]> Link: openwrt/openwrt#24019 Signed-off-by: Jonas Jelonek <[email protected]>
|
I think I do get what you mean by turning it upside down. I changed the nft keep the class in |
dhrm1k
force-pushed
the
trafficshaper-nft-backend
branch
from
3340285 to
16443b3
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit. This revision resolves the earlier downlink concern: the backend now writes ct mark and adds an upload-side action connmark restore, so both directions carry the class via the connection mark. Commit message and CI (no failures) are fine. One remaining parity question inline about last-match vs first-match class assignment.
Generated by Claude Code
| *) die 2 "Unknown address family of network $class_net in class $class!" | ||
| esac | ||
| if [ "$class_reserved_uplink" ]; then | ||
| printf '%s\n' "add rule inet $NFT_TABLE forward $nft_family saddr $class_net counter ct mark set (ct mark & $mark_mask_inv) | $xi comment \"$APPNAME-$class up\"" |
There was a problem hiding this comment.
These per-class rules set ct mark unconditionally, so within one packet's traversal every matching rule fires and the last match wins. The iptables backend guards each classify rule with -m mark --mark 0x0/$mark_mask (L254/L258), so there the first matching class wins. For a packet whose source falls in one class network and whose destination falls in another, the two backends will therefore assign different classes — a parity gap versus the iptables path this PR aims to mirror.
Was this divergence intended? If not, mirroring the iptables guard by prefixing the match with ct mark & $mark_mask == 0x0 on this line and the down rule below (L325) would restore first-match semantics:
add rule inet $NFT_TABLE forward $nft_family saddr $class_net ct mark & $mark_mask == 0x0 counter ct mark set (ct mark & $mark_mask_inv) | $xi comment ...
Generated by Claude Code
|
@dhrm1k i trust your coin toss, will work either way - re-checking mask at every step or returning once mask is set.... I vote for first as it is smaller in text and less bytecode in kernel. |
|
yes, the latter will be cleaner. I will do per-rule mask check and remove standalone return. |
Add an nftables firewall backend while keeping the existing iptables backend available. Use nftables on fw4 systems and fall back to iptables otherwise. Signed-off-by: Dharmik Parmar <[email protected]>
dhrm1k
force-pushed
the
trafficshaper-nft-backend
branch
from
16443b3 to
9683ebb
Compare
| "add chain inet $NFT_TABLE forward { type filter hook forward priority mangle + 10; policy accept; }" \ | ||
| "add chain inet $NFT_TABLE classify" \ | ||
| "add rule inet $NFT_TABLE forward meta mark & $mark_mask != 0x0 counter return" \ | ||
| "add rule inet $NFT_TABLE forward counter goto classify comment \"If no class, try to classify\"" |
There was a problem hiding this comment.
The iptables backend saves the class into the connection mark at the end
That is lost now....
clanker: instead of being reworked into a form nft accepts.
How do you do it IRL? bit-addressed marks are lost in nftables, and iptables-translate return incomplete translation
| *) die 2 "Unknown address family of network $class_net in class $class!" | ||
| esac | ||
| if [ "$class_reserved_uplink" ]; then | ||
| printf '%s\n' "add rule inet $NFT_TABLE forward $nft_family saddr $class_net counter ct mark set (ct mark & $mark_mask_inv) | $xi comment \"$APPNAME-$class up\"" |
There was a problem hiding this comment.
Old logic avoids writing mark twice (checking whether it is still zero before each rule)
could it be just return after each filter match setting zone?
(overlapping zones are king of not well envisioned, or make much sense)
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the new changes since the last review; no new issues found.
This revision resolves the first-match/last-match parity concern from the previous review: the chain-level ct mark & mask != 0x0 counter return shortcut is dropped in favor of a per-rule ct mark & mask == 0x0 guard on each classify rule, which mirrors the iptables backend's per-rule -m mark --mark 0x0/$mask guard, so the first matching class wins in both backends. The ct mark set (ct mark & mask_inv) | xi expression correctly clears and sets the class bits. Commit message matches the diff and CI is green.
Generated by Claude Code
Add an nftables firewall backend while keeping the existing iptables backend
available.
Trafficshaper still works the same way: firewall rules mark forwarded packets,
and
tcuses those marks for the shaping classes.Part of the nftables migration work discussed in #16818.
📦 Package Details
Maintainer: Luiz Angelo Daros de Luca [email protected] (@luizluca)
Description:
Add nftables support for the packet-marking part of trafficshaper. The existing
iptables backend remains available for older systems.
🧪 Run Testing Details
Tested on an OpenWrt VM with an Alpine LAN client behind it.
✅ Formalities
If your PR contains a patch: