Skip to content

[RFC/RFT] build: switch to firewall4 by default#4642

Closed
aparcar wants to merge 2 commits intoopenwrt:masterfrom
aparcar:fw4
Closed

[RFC/RFT] build: switch to firewall4 by default#4642
aparcar wants to merge 2 commits intoopenwrt:masterfrom
aparcar:fw4

Conversation

@aparcar
Copy link
Copy Markdown
Member

@aparcar aparcar commented Oct 6, 2021

The next OpenWrt release might use firewall4, let's test it?

This commit replaces firewall aka firewall3 with its nftables based
successor firewall4.

Heads up for packages.git: openwrt/packages#16818
Heads up for routing.git: openwrt/routing#731
Heads up for luci.git: openwrt/luci#5409

Issues:

  • option zone in /etc/config/network is ignored by firewall4
  • flow offloading doesn't work if no interfaces are configured
  • rules with option _name are skipped
  • defaults section is ignored if it contains any unsupported option
  • ipsets of type ipv{4,6}_addr claim CIDR is invalid
  • ipsets don't support timeout

Working on those here

@aparcar aparcar added RFC pull request ready for comments RFT pull request ready for testing labels Oct 6, 2021
@champtar
Copy link
Copy Markdown
Member

champtar commented Oct 6, 2021

LGTM
If we want to switch we need to do it early so we have time to fix packages

@aparcar
Copy link
Copy Markdown
Member Author

aparcar commented Oct 6, 2021

@champtar do you have a list of packages at hand which will break? I'd like to give the maintainers a heads up

@tapper82
Copy link
Copy Markdown
Contributor

tapper82 commented Oct 6, 2021

Is there needed changes in LUCI?

@aparcar
Copy link
Copy Markdown
Member Author

aparcar commented Oct 6, 2021

Is there needed changes in LUCI?

Grepping through luci.git for iptables, the answer seems yes.

@neheb
Copy link
Copy Markdown
Contributor

neheb commented Oct 6, 2021

doesn't nftables come with an iptables compat utility?

@aparcar
Copy link
Copy Markdown
Member Author

aparcar commented Oct 6, 2021

doesn't nftables come with an iptables compat utility?

That would be indeed very handy.

@K900
Copy link
Copy Markdown
Contributor

K900 commented Oct 6, 2021

Is fw4 at feature parity with fw3 yet? Last time I checked, it didn't support offloading, which is kind of a big deal for me and others running OpenWRT on potato home routers.

@aparcar
Copy link
Copy Markdown
Member Author

aparcar commented Oct 6, 2021

Is fw4 at feature parity with fw3 yet?

It should be as compatible as possible before we actually set it as default.

Last time I checked, it didn't support offloading, which is kind of a big deal for me and others running OpenWRT on potato home routers.

I haven't done any tests yet but the package kmod-nft-offload suggests something like that should be possible.

@K900
Copy link
Copy Markdown
Contributor

K900 commented Oct 6, 2021

I know it should be possible, yeah, it's just not implemented as of right now (I've just checked the source).

@champtar
Copy link
Copy Markdown
Member

champtar commented Oct 6, 2021

@champtar do you have a list of packages at hand which will break? I'd like to give the maintainers a heads up

I don't have such precise list, I think we should just prepare some label on github and email every maintainer to ask if their package is using iptables to test it with latest master + fw4 and to open an issue, label it, and link to this issue (email openwrt-devel + BCC every maintainer)

In the past I've used rg to grep through all ipk and find package using iptables without having the dependency. Some packages depends on iptables and might be fine with iptables-nft, but some packages might insert rules into chains defined by fw3 that will no longer exists. Example openwrt/routing#651 / openwrt/routing#652
(I don't remember if I finished this task or not)

The cleanest solution for many cases might be procd firewall objects https://github.com/search?p=2&q=org%3Aopenwrt+json_add_array+firewall&type=Code for many simple use cases.

It would be good to have an easy way at runtime to know if fw3 or fw4 is running to use in scripts, and we need to figure out packages dependencies for packages that will support both iptables and nftables (some global symbol FW_NFT / FW_IPT)

Lots of fun ahead :)

@adschm adschm added the core packages pull request/issue for core (in-tree) packages label Oct 6, 2021
@Ansuel
Copy link
Copy Markdown
Member

Ansuel commented Oct 6, 2021

I remember testing this and got no connection on my custom image. Can you describe the needed package for this to work correctly?

@aparcar aparcar mentioned this pull request Oct 6, 2021
3 tasks
@aparcar aparcar force-pushed the fw4 branch 2 times, most recently from 3e142f2 to b0b73d0 Compare October 7, 2021 00:05
@john-tho
Copy link
Copy Markdown
Contributor

john-tho commented Oct 7, 2021

To get this easily buildable sooner, could we default to firewall4 with CONFIG_EXPERIMENTAL 8663072, and/or a specific firewall4 config option? So that we can configure an image to be built with either firewall3 or firewall4, rather than needing to change DEFAULT_PACKAGES manually?

@SkewedZeppelin
Copy link
Copy Markdown
Contributor

SkewedZeppelin commented Oct 8, 2021

Maybe irrelevant, but when firewalld switched to nftables there was a noticeable increase in RAM usage when loading ipsets due to handling in libnftables.
firewalld/firewalld#738

Does that apply here?

Would break packages like banip if so.

@aparcar
Copy link
Copy Markdown
Member Author

aparcar commented Oct 15, 2021

I added a patch from @stintel which upgrades nftables to version 1.0.0. The CI patch is only for my testing and won't make it upstream.

Follow the link below to find CI builds for all targets. I'm currently testing this on a test device and it works fine:
https://github.com/aparcar/openwrt/actions/runs/1342946939

There are also compiled packages containing iptables-nft. Please consider installing that for backwards compatibility with iptables based scripts.

@aparcar aparcar marked this pull request as ready for review October 15, 2021 20:28
@aparcar aparcar requested a review from hauke October 15, 2021 20:28
@K900
Copy link
Copy Markdown
Contributor

K900 commented Jan 6, 2022

I'll have to retest it later tonight, rolled back to fw3 for now.

@K900
Copy link
Copy Markdown
Contributor

K900 commented Jan 6, 2022

Another issue I just found, I can't open the zone editor in LuCI, at least with theme-openwrt-2020.

Any error reported in the browser console?

All I'm getting is TypeError: can't convert null to object, looks like this line.

@K900
Copy link
Copy Markdown
Contributor

K900 commented Jan 6, 2022

https://git.openwrt.org/?p=project/firewall4.git;a=commitdiff;h=b68cf67019452527d6a5a967b823a39dd1ba1edd;hp=be5f4e33c6388935651e6a87c4e5348ade0bd714

It's an easy fix so you could do that manually until I push the fix and bump firewall4 in openwrt.git.

The change worked as in a flowtable entry gets created, but it doesn't look like it's actually being used... I'm still getting massive CPU spikes under heavy load, and the bandwidth is severely limited.

@stintel
Copy link
Copy Markdown
Member

stintel commented Jan 6, 2022

https://git.openwrt.org/?p=project/firewall4.git;a=commitdiff;h=b68cf67019452527d6a5a967b823a39dd1ba1edd;hp=be5f4e33c6388935651e6a87c4e5348ade0bd714
It's an easy fix so you could do that manually until I push the fix and bump firewall4 in openwrt.git.

The change worked as in a flowtable entry gets created, but it doesn't look like it's actually being used... I'm still getting massive CPU spikes under heavy load, and the bandwidth is severely limited.

Running perf top shows nf_flow_table calls being made when I do a speedtest, so I conclude it is working. Same speedtest shows double the download speed when flow offloading is enabled.

@K900
Copy link
Copy Markdown
Contributor

K900 commented Jan 6, 2022

I am getting about 150mbps on Speedtest.net with fw4, and 600+ with fw3...

@stintel
Copy link
Copy Markdown
Member

stintel commented Jan 6, 2022

I am getting about 150mbps on Speedtest.net with fw4, and 600+ with fw3...

It is most definitely working for me, as evidenced by these iperf3 runs:

Flow offloading disabled:

[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec   449 MBytes   377 Mbits/sec   32             sender
[  5]   0.00-10.00  sec   446 MBytes   374 Mbits/sec                  receiver

Flow offloading enabled:

[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec  1.08 GBytes   929 Mbits/sec   44             sender
[  5]   0.00-10.01  sec  1.08 GBytes   927 Mbits/sec                  receiver

Does your nft ruleset contain the correct devices in the flowtable? Also, to rule out other issues, test using 2 local endpoints.

@K900
Copy link
Copy Markdown
Contributor

K900 commented Jan 6, 2022

I've got br-lan and eth0.2 (which is my WAN interface). I'll have to retest later again.

With the switch to nftables via firewall4 the utility iptables-nft
should be enabled. It allows users to seamlessly use `iptables` as
before while the nftables Kernel API is used.

Also select the `iptables` package automatically when selecting
`iptables-nft`. This allows a clearer dependency list in `target.mk`.

Signed-off-by: Paul Spooren <mail@aparcar.org>
This commit replaces firewall aka firewall3 with its nftables based
successor firewall4.

Signed-off-by: Paul Spooren <mail@aparcar.org>
@stintel
Copy link
Copy Markdown
Member

stintel commented Jan 6, 2022

@aparcar I've got 2 more changes pending in https://git.openwrt.org/?p=project/firewall4.git;a=shortlog;h=refs/heads/staging/stintel/fixes - waiting for review of the newest one by jow. Once these are in I'm OK with switching to firewall4 by default.

@aparcar
Copy link
Copy Markdown
Member Author

aparcar commented Jan 6, 2022

This commit does not automatically add the transparent wrapper for legacy iptables. Should it be added by default or would you make it a user choice?

For the next release it could make sense to add the wrapper to break less user setups. We should check the size impact, right?

@stintel
Copy link
Copy Markdown
Member

stintel commented Jan 6, 2022

I've hit some very nasty issue on a CentOS 8 box from mixing the transparent wrapper with nft for which I could not find a solution other than to reboot the machine, so I would prefer not to use the transparent wrapper at all.

@champtar
Copy link
Copy Markdown
Member

champtar commented Jan 6, 2022

I would say do not ship the wrapper by default, have package maintainer test if it actually make their package work and add the dependency as needed.

@aparcar
Copy link
Copy Markdown
Member Author

aparcar commented Jan 6, 2022

Exciting, please ping me once the last commits are pushed, I'll then merge this.

I'll also write another heads up to the sub issues in each repo.

@Ansuel
Copy link
Copy Markdown
Member

Ansuel commented Jan 6, 2022 via email

@Ansuel
Copy link
Copy Markdown
Member

Ansuel commented Jan 6, 2022

I am getting about 150mbps on Speedtest.net with fw4, and 600+ with fw3...

It is most definitely working for me, as evidenced by these iperf3 runs:

Flow offloading disabled:

[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec   449 MBytes   377 Mbits/sec   32             sender
[  5]   0.00-10.00  sec   446 MBytes   374 Mbits/sec                  receiver

Flow offloading enabled:

[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec  1.08 GBytes   929 Mbits/sec   44             sender
[  5]   0.00-10.01  sec  1.08 GBytes   927 Mbits/sec                  receiver

Does your nft ruleset contain the correct devices in the flowtable? Also, to rule out other issues, test using 2 local endpoints.

This is strange... I'm currently testing this and I have 50mbps with a 150mbps connection... no flow offload... Can we investigate this perf regression?

@K900
Copy link
Copy Markdown
Contributor

K900 commented Jan 6, 2022

Just out of curiosity, what hardware are you testing on? Is hardware offload on? I'm on ath79, so no hardware offload for me. Is there a chance Stijn got those results with hardware offloading?

@Ansuel
Copy link
Copy Markdown
Member

Ansuel commented Jan 6, 2022

No hardware offload ipq806x

@stintel
Copy link
Copy Markdown
Member

stintel commented Jan 6, 2022

I'm on qoriq, also no HW offloading.

@rsalvaterra
Copy link
Copy Markdown
Member

Should we care that fw3 doesn't work with 5.15 now that fw4 will be default?

There are still components which don't work with fw4. A very, very important one (in my humble opinion, since I use it professionally) is mwan3. For now, we're using Linux 5.10, and people who need mwan3 can choose fw3 instead of fw4, so we should be fine for the next release. That said (and I know we're still in very early stages), when we upgrade to 5.15, I envision three possibilities:

  1. Fw4 ships by default and fw3 is fixed to work with 5.15, allowing people to choose fw3 if they want to use mwan3;
  2. Fw4 ships by default, fw3 stays broken in 5.15, people won't be able to use mwan3 (trainwreck scenario);
  3. Somewhere, along the way, a wild mwan4 appears (the ideal, though I wager it's going to take a lot of work and time);

The obvious interim solution is, of course, to fix fw3. Thoughts?

@Ansuel
Copy link
Copy Markdown
Member

Ansuel commented Jan 6, 2022

@rsalvaterra IMHO the only correct way is fw3 fixed but don't know if it's worth... currently fw3 doesn't work in 5.15 and with the revert patch pppoe doesn't work. Using fw4 fix all these problem at once.

@champtar
Copy link
Copy Markdown
Member

champtar commented Jan 7, 2022

@rsalvaterra mwan3 is also a blocker for me, but I still think we should go ahead with the fw4 switch so that if there are too many issues we can switch back before rc0. mwan4 will need some work for sure but there is a chance the logic is pretty similar. Until we switch nobody will look into it ;)

@stintel
Copy link
Copy Markdown
Member

stintel commented Jan 7, 2022

Until we switch nobody will look into it ;)

That's the general feeling I'm getting, yes. All the more reason to switch early.

@rmandrad
Copy link
Copy Markdown
Contributor

rmandrad commented Jan 7, 2022

I am getting about 150mbps on Speedtest.net with fw4, and 600+ with fw3...

It is most definitely working for me, as evidenced by these iperf3 runs:
Flow offloading disabled:

[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec   449 MBytes   377 Mbits/sec   32             sender
[  5]   0.00-10.00  sec   446 MBytes   374 Mbits/sec                  receiver

Flow offloading enabled:

[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec  1.08 GBytes   929 Mbits/sec   44             sender
[  5]   0.00-10.01  sec  1.08 GBytes   927 Mbits/sec                  receiver

Does your nft ruleset contain the correct devices in the flowtable? Also, to rule out other issues, test using 2 local endpoints.

This is strange... I'm currently testing this and I have 50mbps with a 150mbps connection... no flow offload... Can we investigate this perf regression?

I haven't noticed any impact with wrt3200acm with fw4 either with flow offload or not ... unsure if there is any impact but I changed 660-fq_codel_defaults.patch (generic hack) to have the same definition as CONFIG_X86_64

@dave14305
Copy link
Copy Markdown

dave14305 commented Jan 9, 2022

What happens to Luci’s Status -> Firewall page under firewall4?

Edit: Already brought up by hnyman in the proper repo:
openwrt/luci#5409 (comment)

@aparcar
Copy link
Copy Markdown
Member Author

aparcar commented Jan 19, 2022

As discussed at yesterdays meeting I merged the changes. The next release will use firewall4 as default and all packages incompatible (e.g. using ipset) should add a negative dependency. Our considerations are that the default (WiFI home router) setup works fine wir firewall4 and special cases can always replace firewall4 with firewall3, which should work at least until the upcoming 5.15 Kernels.

@aparcar aparcar closed this Jan 19, 2022
@aparcar aparcar deleted the fw4 branch January 19, 2022 08:30
@K900
Copy link
Copy Markdown
Contributor

K900 commented Jan 20, 2022

Turns out my perf regression wasn't rooted in fw4, but in SQM/qosify. I'm guessing it now works more correctly (?) with offloaded flows, which completely kills the CPU...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core packages pull request/issue for core (in-tree) packages RFC pull request ready for comments RFT pull request ready for testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.