First rule in chain INPUT & OUTPUT for firewall4

[kvic]

hello developers

The change in this commit is an improvement. But why the first two rules not swapped instead? i.e. the "ct state" rule be the first and the "lo accept" rule the second.

I believe my suggestion is more optimal. Or was it not done because of some known issue?

@jow

ruleset: reduce ksoftirqd load by refering to looopback by numeric id

[brada4]

Hi me the original author, yes, could be, your PR welcome.

My next take on reordering: https://github.com/openwrt/firewall4/pull/22

More plans with no timeline:

• add better heuristic for soft offload to accomodate now working pppoe offload without crippling wifi back
• some heuristic for iif ? iifname ie what interfaces may not change numeric ID like those compiled-in cf mss fix
• jump ? goto cf _reject

[kvic]

@brada4

Hi me the original author, yes, could be, your PR welcome.

I'll try but don't hold your breath pls. Every time I tried to finish the Submission Guideline Wiki in the past week..I fell asleep shortly.

@brada4

My next take on reordering: https://github.com/openwrt/firewall4/pull/22

I briefly went through the changes. I didn't follow the chronological of order of changes. So some of my general impressions:

• 'flow offload @ft' is the old version of nftables. The newer version recommends 'flow add @ft' So 'add' is the keyword for the long term
• nftables will guarantee a flow is established (even including UDP flows) before adding to the flow table, and hand over to HW offload if available & enabled. So I'm not sure if explicitly only adding & accepting the flow to offload until 'ct state establish' is necessary or desirable
• If chain 'prerouting' has dropped 'invalid' packets, then no longer need to check for invalid packets in 'input' and 'forward' chains.
• I think 'ct state' with verdict map (vmap) is very fast. But I also think 'ct state established, related accept' is even faster if all actions have the same verdict i.e. 'accept' in this case.

[brada4]

0 dont be afraid, you just need to reasonably explain change and sign (github removes SOB at every click, so just keep it and paste on.
1 both work, nftables still say "add" , the real syntax is flow XXX \@ft ELSE quick
2 flow initiating packet follows slowpath, quickest slowpath exit is the properly pre-filtered place to avoid that check with nice bonus that kworker gets immediately scheduled to add flowtable state...
3 it is done like that, check remains in output
4 another weird corner - vmap dispatches quick statement (jump goto accept return etc) during evaluation while normal {est,rel} sets variable then does conditional jump. Less bytecode less headaches.

[kvic]

@brada4

1 both work, nftables still say "add" , the real syntax is flow XXX \@ft ELSE quick

Use 'add' in your code and keep it simple. 'nft list ruleset' will show 'add' anyway even your code is 'offload'. Get what I meant? Keep it simple and consistent.

@brada4

2 flow initiating packet follows slowpath, quickest slowpath exit is the properly pre-filtered place to avoid that check with nice bonus that kworker gets immediately scheduled to add flowtable state...

Keep it simple in your code that works the same functionally. No need for verdict map and hop around.

@brada4

3 it is done like that, check remains in output

No idea what you're talking.

@brada4

4 another weird corner - vmap dispatches quick statement (jump goto accept return etc) during evaluation while normal {est,rel} sets variable then does conditional jump. Less bytecode less headaches.

Again keep it simple. Your answer didn't address the point I raised.

Anyway, your code you have the final say, especially you're volunteering. Users who have an opinion will make their own adjustments. I did some on my setup. But I tried to keep my change to minimal for easier admin work in the long run.

[brada4]

1. agree hope your next request will not be to write bitmasks as readable numbers as in final rendition
2. nope, thats slow, per-packet examination is memory access, a huge bottleneck on small systems.
3. output drop incalid remains...
4. yes, nftablish-simple, less bytecode = less time.

[kvic]

@brada4

agree hope your next request will not be to write bitmasks as readable numbers as in final rendition

Dude. First of all, I hope you didn't see me opening this thread as a request for your help. I would rather you see me as helping you.

Second, in your initial response, you presented some of your code changes over a couple months, back and forth. Seemingly didn't know what to decide the best way forward. I thought you were soliciting some feedback or code review from users like me. That's the reason I wrote my original response.

Actually if I knew you were the author, I won't bother to open this thread. Have a good day.

[brada4]

Hey no offence intended. Just have in mind it takes long time to get patch to main or releases

[kvic]

LOL. Peace.

I guess it's an issue of language barrier.

[brada4]

2 of your suggestions are reasonable (swap rules offload-> add), If I dont meet them next time from you Ill add them.

[kvic]

You may also consider the other three too:

• prerouting chain has dropped invalid packets. Then you don't need to add one rule to check & drop invalid packets in chain INPUT & chain FORWARD. They are redundant checks.

• 'meta l4proto { tcp, udp } flow add @ft' is good. No need for 'ct state vmap {}' to jump to a flow_handler and add to @ft. It's slower.

• 'ct state { established, related } accept' is simple, clean & fast. No need for 'ct state vmap { establish: accept, related: accept }' But I don't have a strong opinion on this one. Either way I'm fine.

[brada4]

1. it is not done, invalid is dropped and not checked in input/forward anymore, only in output.
2. if packet is rejected from flowtable due to mss or window change it will be accepted only after passing ct state established, same with icmp code 3 related (tcpudp misses that totally)
3. maybe, i discarded old slow 1043 gigabit router, fear not to add angry numbers in github

[kvic]

@brada4

it is not done, invalid is dropped and not checked in input/forward anymore, only in output.

Oh !

I think I finally see what you meant by "output" in earlier response.

In chain OUTPUT, check for invalid packets to prevent NAT leakages is what I did for my Edgerouter X running good old iptables. If you're doing something similar, then it makes sense of course.

Whether you should have discarded your 'invalid packets' check in chain PREROUTING is debatable.

I saw some of your changes but I might have mixed up with chronological order of changes. I thought your intention to drop invalid packets early & asap. That's what I will consider good. I also implement in my own "ban IP" kind of rules.

But you may leave the invalid packet check to "ban IP" kind of 3rd party packages...

@brada4

if packet is rejected from flowtable due to mss or window change it will be accepted only after passing ct state established, same with icmp code 3 related (tcpudp misses that totally)

Didn't know that's a thing. I wonder if it's true.

If a flow is added to flowtable already, it's only dropped when the flow ends. If a flow is not added to flowtable already, then it'll be added in the next possible opportunity.

[brada4]

@kvic

PREROUTING is debatable

Dont try ufw or firewalld , they debatable too.

@kvic

invalid packet check to "ban

It is different kind of invalidness, like bad checksum, out of sequence window, extra syn inside tcp stream etc.

@kvic

wonder if it's true

Add counters, be surprised.

@kvic

dropped when the flow ends

Nope, on sysctl-able timers. 30s default.

[brada4]

Download raw ruleset.uc file to device, free backup in /rom/usr/share/firewall4/templates

brada4

Probably worth adding /usr/share/ucode/fw4.uc and /usr/share/firewall4/templates/zone-mssfix.uc same way from firewall4 master (both other files changed from 23.05.4 version)
Mail me if you want proper credit added

[kvic]

@brada4

Mail me if you want proper credit added

Nah, I don't need credit for it. Just a little help.

Thanks for making the changes.

@brada4

Nope, on sysctl-able timers. 30s default.

I observed on MT7988a that HW offload drops flows instantly the second flows end. Perhaps also timers but in a second or two. Or some way to detects a flow ends and drops from hardware table.

A separate question: do you happen to know how exactly a flow enters nftables' flowtable AND then get transitioned to hardware offload ?

[brada4]

There is additional 30s timer handing states back to slowpath, besides the events normally ending flow.
Outline of operations

Flow Offload with Cisco Firepower NGFW

HW offload is not applicable for me, wire-backhaul mesh same MAC floats between wifi5 wifi2 and extender wires...

[brada4]

HW offload is smaller LRU like 256 vs 32k reviveded on forwarded packet, idler states downgrade to soft offload, back to slowpath on timer, then shortly in HW offload. The only trigers are passing packets.

[kvic]

There was an "UDP packet out of order" issue raised in another thread. A few UDP packets out of order (at the start of the flow) are not avoidable based on my reading of linux kernel document. So I'm thinking of:

• finding ways to keep the number of 'out of order' packets to minimal & more deterministic, say less than 10 packets at any time
• finding a way in nftables rules to define an exception to certain flows not being added into flowtable (and hence get offloaded to hardware).

I tried a couple of way for #2. Seems to me as of latest nftables it's not possible.

[brada4]

Nice observation, yes, the offload works by softirq scheduling flowtable add to kworker, causing some reorder. Even normal iptables do not guarantee fifo behaviour.
Finding a way would be plugging l4proto udp accept before ft add via /etc/nftables.d
No big issue setting enormous flowtable timers via sysctl. The slowpath check is accept either way.