Repository navigation
sqm: Replace iptables marking with nftables - #190
Conversation
|
Thank you for the patch. Did you test these changes, and if so, how? On OpenWrt, I presume? :) |
|
Yes, on openwrt. I tested |
|
this would break things for everyone who uses iptables. |
| if [ "$ZERO_DSCP_INGRESS" = "1" ]; then | ||
| sqm_debug "Squashing differentiated services code points (DSCP) from ingress." | ||
| ipt -t mangle -I PREROUTING -i $IFACE -m dscp ! --dscp 0 -j DSCP --set-dscp-class be | ||
| nft_rule " iifname \"${IFACE}\" ip dscp != cs0 ip dscp set cs0" |
There was a problem hiding this comment.
use iif ipo iifname, tc interface is totally fixed, no need to strlen() + strcmp() for each packet.
probably pre-pend meta nfproto ipv[4|6] , not visible in read-back but entered in kernel to skip unnecessary "heavy lifting" before ipX dscp for "other" protocol....
| nft_rule " }" | ||
| nft_rule " chain output {" | ||
| nft_rule " type filter hook output priority mangle; policy accept;" | ||
| nft_rule " udp dport { 123, 53 } ip dscp set af42" |
There was a problem hiding this comment.
skip payload expression by using conntrack meta 16-bits
meta nfproto ipv4 meta l4proto udp ct original proto-dst { 123 , 53 } ip dscp ...
There was a problem hiding this comment.
Thanks. I’ll update the fixed interface matches to use iif/oif, keep iifname only for the wildcard case, and add the explicit nfproto guards around the DSCP rules.
I’ll also the conntrack form for the udp output rule and retest it.
There was a problem hiding this comment.
Proposed
family 1 __set%d t 0
element 00007b00 : 0 [end] element 00003500 : 0 [end]
inet t c
[ meta load nfproto => reg 1 ]
[ cmp eq reg 1 0x00000002 ]
[ meta load l4proto => reg 1 ]
[ cmp eq reg 1 0x00000011 ]
[ ct load proto_dst => reg 1 , dir original ]
[ lookup reg 1 set __set%d ]
[ payload load 1b @ network header + 1 => reg 1 ]
[ bitwise reg 1 = ( reg 1 & 0x000000fc ) ^ 0x00000000 ]
[ cmp eq reg 1 0x00000000 ]
Current
family 1 __set%d t 0
element 00007b00 : 0 [end] element 00003500 : 0 [end]
inet t c
[ meta load l4proto => reg 1 ]
[ cmp eq reg 1 0x00000011 ]
[ payload load 2b @ transport header + 2 => reg 1 ]
[ lookup reg 1 set __set%d ]
[ meta load nfproto => reg 1 ] ^^^ till here unnecessary payload expression
[ cmp eq reg 1 0x00000002 ]
[ payload load 1b @ network header + 1 => reg 1 ]
[ bitwise reg 1 = ( reg 1 & 0x000000fc ) ^ 0x00000000 ]
[ cmp eq reg 1 0x00000000 ]
There was a problem hiding this comment.
Oh nice, I didn’t know about nft -c -d netlink -f. Thanks for pointing that out.
I checked the generated form and updated the output rules so meta nfproto comes before the UDP/ct match.
62b4b09 to
781d74d
Compare
@yhaenggi Fair point. I originally leaned toward adding nftables while keeping iptables as fallback. I made this version nft-only based on the earlier discussion here: openwrt/packages#29830 (comment) |
|
@yhaenggi addressing undefined ordering (set priority after default mangle hoks) nftables script can coexist with legacy tables, hope adding of nftables requirement is not a huge weight to carry. |
|
@tohojo question about porting tos 4 - should it be 8 ranges or X known dscp codepoints falling in them? |
|
@dhrm1k ill check back later when you get around my first sweep |
wait, your comment makes me think, you are under the impression we have kept legacy iptables intact in the code. is that the case? no, i ported it entirely to nft. |
|
Got it, I misunderstood your point. You mean even with the script fully nft-based, the hook priority should be explicit so it does not race fw4 or legacy hooks. |
Yes, exactly. There is no problem under normal usage, but 2 days into uptime some hotplug event reloads one or other firewall table and packets start running into matrix unexplainably. |
781d74d to
b9a4757
Compare
|
As much as fixed so far - perfect.
|
b9a4757 to
cc5c10e
Compare
|
@tohojo what’s your take? |
|
this is still pending.
|
|
gentle ping to the maintainers. |
tohojo
left a comment
There was a problem hiding this comment.
A few comments on the implementation, see below...
| local mark_bulk | ||
| local mark_prio | ||
|
|
||
| NFT_CLEAR_MASK=$(printf '0x%x' $((0xffffffff & ~MARK_MASK))) |
There was a problem hiding this comment.
AFAICT this is only used in nft_mark_set(), so let's just make it a local variable in there instead of this global variable that's not really declared anywhere else?
There was a problem hiding this comment.
moved the clear-mask calculation into nft_mark_set() and made it a local variable there, so it no longer leaks into global state.
| nft_rule " }" | ||
| nft_rule " chain prerouting {" | ||
| nft_rule " type filter hook prerouting priority mangle + 10; policy accept;" | ||
| nft_rule " iifname \"vtun*\" meta l4proto tcp ${mark_be}" |
There was a problem hiding this comment.
The comment explaining why this is there got dropped in the conversion
There was a problem hiding this comment.
oops! restored the comment.
| ipt -t mangle -A QOS_MARK_${IFACE} -m dscp --dscp-class AF42 -j MARK --set-mark 0x1/${IPT_MASK} | ||
| ipt -t mangle -A QOS_MARK_${IFACE} -m tos --tos Minimize-Delay -j MARK --set-mark 0x1/${IPT_MASK} | ||
| nft_rule " meta nfproto ipv4 ip dscp cs1 ${mark_bulk}" | ||
| nft_rule " meta nfproto ipv6 ip6 dscp cs1 ${mark_bulk}" |
There was a problem hiding this comment.
The iptables version encapsulates the v4/v6 duplication in the function that installs the rules; let's do the same here? Something like:
nft_dscp_rule() {
local arg
arg="$1"
nft_rule " meta nfproto ipv4 ip $arg"
nft_rule " meta nfproto ipv6 ip6 $arg"
}called like:
nft_dscp_rule "dscp cs6 ${mark_prio}"There was a problem hiding this comment.
i added the suggested nft_dscp_rule() helper and converted the duplicated IPv4/IPv6 dscp rules to use it. I kept the minimize-delay equivalent separate because that rule is IPv4-only.
There was a problem hiding this comment.
Did you forget to push the updated commits? I still see the old version in the GH interface... :)
There was a problem hiding this comment.
Ah yes. So sorry. I will do it after few hours.
There was a problem hiding this comment.
I have pushed changes
As for this bit. Yes, this is obviously true. The question is - do we have any users actively using new versions of sqm-scripts and using the |
|
Thank you for taking your time to review this. I will look into the points you raised.
I do agree with this. I want nft support, but I am open to keeping iptables intact. that's what I had in my mind until we discussed we want it to shift to nft entirely. |
|
Use case is very low cpu devices |
|
I am fine with dropping iptables, with a new sqm-scripts release, very low CPU devices likely can simply continue using older sqm-scripts, no? |
|
Use case is very low cpu devices
Right, okay, but if a device does not have enough CPU power to run
nftables, should it really be running the simple.qos script in the first
place? As opposed to running simplest.qos or piece_of_cake.qos, both of
which are completely unaffected by this change? Is anyone doing so
today?
|
|
moeller0 ***@***.***> writes:
I am fine with dropping iptables, with a new sqm-scripts release, very
low CPU devices likely can simply continue using oder sqm-scripts, no?
Yes, exactly.
|
Replace the iptables-based marking setup with native nftables rules. This keeps the existing tc fw filter model, but uses nftables to install packet marks. Signed-off-by: dharmik <dharmikparmar2004@yahoo.com>
cc5c10e to
211c7d5
Compare
|
just incase if notif got missed, I have made requested changes. |
|
Thank you for taking the time to review and work through this with me. Are you planning to tag a new sqm-scripts release now that this is merged? |
|
Dharmik Parmar ***@***.***> writes:
dhrm1k left a comment (tohojo/sqm-scripts#190)
Thank you for taking the time to review and work through this with me.
Are you planning to tag a new sqm-scripts release now that this is
merged?
Yup, done.
|
I'm running openwrt without nft stuff but iptables. In SQM i'm using "Queue setup script" "piece_of_cake.qos". Does this still works without nft? |
Yes, |
|
Thanks for clarification! |
|
@fda77 nft is available for any curent lts kernel, you can use this nft alongside iptables-legacy (due to priority skew) |
|
I know. I'm using iptables (only) |
|
Good you know, there is no conflict then |
|
omfg |
Replace the iptables-based marking setup with native nftables rules.
This keeps the existing tc fw filter model, but uses nftables to install packet marks.