Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump to dnsmasq 2.87 and nftables support #10820

Closed
wants to merge 5 commits into from

Conversation

ldir-EDB0
Copy link
Contributor

This could all do with a bit more testing/I'm not brave enough to push it myself 'cos anything dnsmasq tends to explode rather quickly.

@github-actions github-actions bot added the core packages pull request/issue for core (in-tree) packages label Sep 26, 2022
DragonBluep referenced this pull request Sep 28, 2022
Serge Vasilugin reports:

To improve mt7620 built-in wifi performance some changes:
1. Correct BW20/BW40 switching (see comments with mark (1))
2. Correct TX_SW_CFG1 MAC reg from v3 of vendor driver see
	https://gitlab.com/dm38/padavan-ng/-/blob/master/trunk/proprietary/rt_wifi/rtpci/3.0.X.X/mt76x2/chips/rt6352.c#L531
3. Set bbp66 for all chains.
4. US_CYC_CNT init based on Programming guide, default value was 33 (pci),
   set chipset bus clock with fallback to cpu clock/3.
5. Don't overwrite default values for mt7620.
6. Correct some typos.
7. Add support for external LNA:
    a) RF and BBP regs never be corrected for this mode
    b) eLNA is driven the same way as ePA with mt7620's pin PA
	but vendor driver explicitly pin PA to gpio mode (for forrect calibration?)
	so I'm not sure that request for pa_pin in dts-file will be enough

First 5 changes (really 2) improve performance for boards w/o eLNA/ePA.
Changes 7 add support for eLNA

Configuration w/o eLAN/ePA and with eLNA show results
tx/rx (from router point of view) for each stream:
 35-40/30-35 Mbps for HT20
 65-70/60-65 Mbps for HT40

Yes. Max results for 2T2R client is 140-145/135-140
with peaks 160/150, It correspond to mediatek driver results.
Boards with ePA untested.

Reported-by: Serge Vasilugin <vasilugin@yandex.ru>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
@@ -818,6 +818,8 @@ dnsmasq_ipset_add() {
domains="$domains/$1"
}

config_get table "$cfg" table 'fw4'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ldir-EDB0 I'm not sure I understand this code correctly, but can we not get the type and table name from the nftset option definition?
What's the suggested syntax for the nftset definition in the config file? Can we keep it the same as the dnsmasq definition, like: option nftset '/domain.com/<family>#<type>#<table>#<set>'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nftset option lives in /etc/config/firewall, not in /etc/config/dhcp.
Similarly there's no guarantee the set is defined or under control of fw4, it might be in a different table controlled by a 3rd party application. - it may not even exist at dnsmasq startup time, this is also the reason why I did the ip version heuristic based on the set name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ldir-EDB0 thank you for your prompt reply. I believe I have unintentionally created a confusion and I should have phrased things better. I'm not talking about defining a new nftset, which, similarly to defining a new ipset is done thru the firewall config.

I'm talking about the option to let the dnsmasq resolve the domain and add elements to the nftset. The similar support for ipset required entries in /etc/config/dhcp: https://openwrt.org/docs/guide-user/base-system/dhcp#all_options.

If the new OpenWrt dnsmasq-full package supports resolving domains and adding their IP addresses to the specified nft set, I believe this should also be configured in /etc/config/dhcp and that config entry should be similar to what upstream dnsmasq package requires, specifically: ip_family, table_type, table_name, set_name.

That's why I'd suggest that the entries for these should be in /etc/config/dhcp and should be:

list nftset '/domain.com/<ip_family>#<table_type>#<table_name>#<set_name>'

This way, the dnsmasq init can parse the dhcp config and obtain all the required information for the nft set from the config entry.

I'm directly affected by this PR, as I maintain two packages which would benefit from the nft set support in dnsmasq -- simple-adblock and vpn-policy-routing. The latter wil be replaced by the new package pbr which is now nft-compatible, but I'm waiting for the nft set support in dnsmasq before I submit a PR to replace the old, iptables-only package.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new dnsmasq init script will still read the existing ipset config entries in /etc/config/dhcp for backward compatibility. The same ipset entries can be used by fw3 or fw4.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe @jow- has previously said that the ipset entries from the firewall config will be created as nft sets by fw4.

Anyways, if dnsmasq-full supports both ipset and nft sets, it should parse both ipset and nftset entries from its config and add IP addresses to ipset and nft set respectively.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since commit acb3362 the preferred dnsmasq ipset configuration is a separate section in the dhcp config. Both of dnsmasq’s native options ipset= and nftset= will be populated from the same ipset definition in /etc/config/dhcp. There is no dedicated nftset configuration in the uci dhcp config.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add example config which hopefully makes things clearer

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since commit acb3362 the preferred dnsmasq ipset configuration is a separate section in the dhcp config. Both of dnsmasq’s native options ipset= and nftset= will be populated from the same ipset definition in /etc/config/dhcp. There is no dedicated nftset configuration in the uci dhcp config.

Could you please help me understand how this is supposed to work by providing sample configs for the following scenarios for domain google.com when the user wants the IPs from resolved domain to be added to:

  1. the ipsets named set_4_cfg000 and set_6_cfg000
  2. the nft set set_4_cfg000 of IPv4 family in fw4 table and the nft set set_6_cfg000 of IPv6 family in the same table
  3. the nft set nftset_4_cfg001 of IPv4 family in the table custom4 of type ip and the nft set nftset_6_cfg001 of IPv6 family in the table custom6 of type ip6
  4. the nft set nftset_4_cfg002 of IPv4 family in the table custom4 of type inet and the nft set nftset_6_cfg002 in the same table

Thanks!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scenarios 1,2,4 would be naming variations on this sample config for /etc/config/dhcp:

config ipset
    list name 'nftset_4_cfg002'
    list name 'nftset_6_cfg002'
    list domain 'google.com'
    option table 'custom4' # optional, defaults to fw4

Scenario 3 would break because everything has been assumed to be an inet table so far, since fw4 uses an inet table. But if you're already into making custom tables, you might not be averse to adding custom config to dnsmasq.conf.d.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate your reply @dave14305 even tho Kevin has closed this PR.

So how does a user make dnsmasq add IPs to an ipset vs nft set if configuration is the same?

ldir-EDB0 and others added 5 commits October 3, 2022 06:38
Bump dnsmasq to 2.87 & refresh patches

Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
Add build option for nftables sets.

Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>

dnsmasq: nftset: serve from ipset config

Use existing ipset configs as source for nftsets to be compatible with
existing configs. As the OS can either have iptables XOR nftables
support, it's fine to provide both to dnsmasq. dnsmasq will silently
fail for the present one. Depending on the dnsmasq compile time options,
the ipsets or nftsets option will not be added to the dnsmasq config
file.

dnsmasq will try to add the IP addresses to all sets, regardless of the
IP version defined for the set. Adding an IPv6 to an IPv4 set and vice
versa will silently fail.

Signed-off-by: Mathias Kresin <dev@kresin.me>

dnsmasq: support populating nftsets in addition to ipsets

Tell dnsmasq to populate nftsets instead of ipsets, if firewall4 is present in
the system. Keep the same configuration syntax in /etc/config/dhcp, for
compatibility purposes.

Huge thanks to Jo-Philipp Wich for basically writing the function.

Signed-off-by: Jo-Philipp Wich <jo@mein.io>
Signed-off-by: Rui Salvaterra <rsalvaterra@gmail.com>
Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
When running sysupgrade from an existing configuration, move existing
ipset definitions to a dedicated config section. Later on, it will allow
to serve ipset as well as nftable sets from the same configuration.

Signed-off-by: Mathias Kresin <dev@kresin.me>
Permit ipsets to specify an nftables table for the set.  New config
parameter is 'table'.  If not specified the default of 'fw4' is used.

eg.

config ipset
	list name 'BK_4,BK_6'
	option table 'dscpclassify'
	list domain 'ms-acdc.office.com'
	list domain 'windowsupdate.com'
	list domain 'update.microsoft.com'
	list domain 'graph.microsoft.com'
	list domain '1drv.ms'
	list domain '1drv.com'

Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
If setname ends in single 4 or 6 use that as ip family indication.

With-thanks-to: Jo-Philipp Wich <jo@mein.io> who improved my regexp.

Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
@ldir-EDB0
Copy link
Contributor Author

I'm going to close this PR - I simply don't have the time or inclination to see it through. Anyone please feel free to take the commits/code/whatever.

@Fiouz
Copy link

Fiouz commented Oct 25, 2022

New pull request: #11073

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants