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

gluon-mesh-batman-adv-brmldproxy: add package #2995

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

T-X
Copy link
Contributor

@T-X T-X commented Sep 23, 2023

Now that we have general support for routable IPv6 multicast address in Gluon main thanks to the newer Linux (bridge) and batman-adv versions it becomes more interesting to also support layer 3 IPv6 multicast routers.

So far this was also not possible with the default settings in Gluon due to filtering MLD into the mesh. This now adds support for brmldproxy, a daemon which proxies MLD reports between bridge ports.

For the Gluon scenario this package adds brmldproxy proxying configuration for the mesh side bat0 bridge port.

The configuration is tuned in a way to enable the usage of layer 3 IPv6 multicast routers for routable IPv6 multicast address ranges. But with a lot smaller MLD overhead compared to the filter_membership_reports=false site.conf option.

If a node has no multicast listener for a routable IPv6 multicast address then this node will emit no MLD report into the mesh. Furthermore, if a node has multiple multicast listening hosts for routable IPv6 multicast addresses then the node will act in deputy and respond with combined, aggregated MLD reports on behalf.

This package is currently incompatible with a
filter_membership_reports=true site.conf option.


This pull-request requires: freifunk-gluon/packages#264

@github-actions github-actions bot added 3. topic: docs Topic: Documentation 3. topic: package Topic: Gluon Packages 3. topic: batman-adv labels Sep 23, 2023
@T-X T-X force-pushed the pr-gluon-mesh-batman-adv-brmldproxy branch from f8c26e7 to f1ff246 Compare September 23, 2023 23:08
@T-X
Copy link
Contributor Author

T-X commented Sep 23, 2023

Changelog v2:

  • added necessary firewall rules for the brmldp0 interface through package/gluon-mesh-batman-adv-brmldproxy/luasrc/lib/gluon/upgrade/400-brmldproxy-firewall (not much, as the tc rules that are set up by brmldproxy are applied before iptables)

@T-X
Copy link
Contributor Author

T-X commented Sep 26, 2023

Changelog v3:

  • Added a patch / config option to batman-adv to forward MLD reports only to multicast routers instead of always flooding MLD reports, to further reduce overhead.
    • That way, when installing this package it makes no difference in overhead, still no MLD reports in the mesh then if no multicast router is present (as I assume no one has installed any multicast routers yet). On the other hand the batman-adv patch should be inactive / make no difference when gluon-mesh-batman-adv-brmldproxy is not installed.

@ecsv
Copy link
Contributor

ecsv commented Oct 24, 2023

Regarding the batman-adv patch: I think this a patch which could be integrated in upstream when the real world effects are verified/tested in actual setups.

@T-X
Copy link
Contributor Author

T-X commented Nov 30, 2023

As discussed during the Gluon meeting back then it would be great to gather a bit of real world experiences with it.

I've now prepared some packages with all the upcoming batman-adv / Gluon multicast features integrated into Gluon package feeds. So far tested on VMs only, but real hardware tests coming up next. It should be enough to just add it to your site/modules file and update all nodes with it. Should compile+run for Gluon v2021.1.x, Gluon v2023.1 and Gluon master: https://github.com/T-X/gluon-batman-adv-next/

If any community would be interested in testing IPv6 multicast routing between communities (on their own Gluon domains), let me know.

@T-X
Copy link
Contributor Author

T-X commented Feb 14, 2024

With the last firmware update at Freifunk Lübeck the latest gluon-batman-adv-next package feed was rolled out, with brmldproxy integration and the latest batman-adv release (v2024.0). So far we didn't notice any issues/regressions.

Haven't found a second community to test an IPv6 multicast router (pim6sd) on a Gluon based network with, but with using mrdisc (https://github.com/troglobit/mrdisc) on one wifi client I can see the MLD reports for (mostly) routable multicast addresses arriving on the local Gluon node from other nodes in tcpdump. And when stopping mrdisc, I don't see these MLD reports anymore.

So to me seems to work as expected/intended on a real setup.

Comment on lines 28 to 29
This package is currently incompatible with a
filter_membership_reports=true site.conf option.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix this, thanks!

@T-X
Copy link
Contributor Author

T-X commented Feb 23, 2024

Two more notes / observations thanks to @ecsv :

  1. When "filter_membership_reports=true" if we were adding brmldproxy by default, I wanted people to think/check what they actually want: I wanted to avoid that people will see MLD reports over their mesh without expecting it.

Unfortunately, with the current need_value(in_site({'mesh', 'filter_membership_reports'}), true, false) in need_value(in_site({'mesh', 'filter_membership_reports'}), true, false) the error message is actually inverted:

need_value(in_site({'mesh', 'filter_membership_reports'}), true, false)

(Not quite sure how to fix that. Would I need to implement a new need_value() variant?)

  1. When "filter_membership_reports=false" I would interpret this as people wanting MLD reports of all listeners over the mesh. And if brmldproxy were added by default and not unselected via -brmldproxy expclicitly, that brmldproxy would still proxy all these reports, to reduce their overhead.

However I forgot to implement to dynamically generate gluon-mesh-batman-adv-brmldproxy/files/etc/config/brmldproxy then from the Gluon Lua update scripts, with "list excludefilter 'ff02::/ff0f::'" removed in that "filter_membership_reports=false" case. Will fix that.


With "filter_membership_reports" not set in a site.conf I'd assume that people want to have the behaviour which Gluon maintainers would suggest for most cases. Which in my opinion would be, and what is currently implemented, to have brmldproxy installed by default. With MLD reports only forwarded if a client listening to routeable multicast addresses and an IPv6 multicast router is present.

@T-X T-X force-pushed the pr-gluon-mesh-batman-adv-brmldproxy branch from c8d3fcc to 0858812 Compare March 26, 2024 21:50
@T-X T-X force-pushed the pr-gluon-mesh-batman-adv-brmldproxy branch from 0858812 to cf37b5d Compare March 26, 2024 21:58
@T-X
Copy link
Contributor Author

T-X commented Mar 26, 2024

Changelog v4:

  • rebased to current master
  • added option to also use filter_membership_reports=false
  • for this removed the static bpfcountd UCI config file and generate it via a Gluon upgrade Lua script instead
  • for that renamed /lib/gluon/upgrade/400-brmldproxy-firewall to /lib/gluon/upgrade/400-brmldproxy
  • absence of filter_membership_reports now defaults to "true" for gluon-mesh-batman-adv-brmldproxy
  • updated Makefile+docs description
  • moved "batctl multicast_mld_rtr_only 1" call addition from netifd gluon_bat0.sh to a separate hotplug.d script
  • added patch: "gluon-ebtables: don't filter incoming MLD Reports via ebtables"
  • added patch: "gluon-mesh-batman-adv-brmldproxy: add to mesh-batman-adv-15 by default"

The bpfcountd concept has now also successfully been tested between Freifunk Lübeck and a domain at Freifunk Vogtland. With pim6sd and bpfcountd multicast audio could be transmitted on one and listened to in the other Gluon site/domain.

package/features Outdated
@@ -38,6 +38,7 @@ when(_'web-advanced' and _'autoupdater', {

when(_'mesh-batman-adv-15', {
'gluon-ebtables-limit-arp',
'gluon-mesh-batman-adv-brmldproxy',
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the feature should be included by default.


[ "$(gluon-show-site | jsonfilter -e "@.mesh.filter_membership_reports")" = "false" ] && exit 0

[ -x /etc/init.d/brmldproxy ] && /etc/init.d/brmldproxy enabled && \
Copy link
Member

Choose a reason for hiding this comment

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

  • There is no need to check for the executability, gluon-mesh-batman-adv-brmldproxy depends on brmldproxy
  • There is no need to check for the enabled status (on Gluon, all installed services are always enabled; disabling is not preserved on ugprades, so it is not supported). If disabling should be possible, a /etc/config/gluon flag must be introduced.


([ "$INTERFACE" != "bat0" ] || [ "$ACTION" != "ifup" ]) && exit 0

[ "$(gluon-show-site | jsonfilter -e "@.mesh.filter_membership_reports")" = "false" ] && exit 0
Copy link
Member

@neocturne neocturne Apr 10, 2024

Choose a reason for hiding this comment

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

Printing the whole site and then filtering it is inefficient. You can copy the lookup_site helper from package/gluon-mesh-batman-adv/files/lib/netifd/proto/gluon_bat0.sh.

@@ -13,8 +13,4 @@ if site.mesh.filter_membership_reports(true) then
rule('MULTICAST_OUT_ICMPV6 -p IPv6 --ip6-protocol ipv6-icmp --ip6-icmp-type 131 -j DROP') -- MLDv1 Report
rule('MULTICAST_OUT_ICMPV6 -p IPv6 --ip6-protocol ipv6-icmp --ip6-icmp-type 132 -j DROP') -- MLDv1 Done
rule('MULTICAST_OUT_ICMPV6 -p IPv6 --ip6-protocol ipv6-icmp --ip6-icmp-type 143 -j DROP') -- MLDv2 Report

rule('MULTICAST_IN_ICMPV6 -p IPv6 --ip6-protocol ipv6-icmp --ip6-icmp-type 131 -j DROP', 'nat') -- MLDv1 Report
Copy link
Member

Choose a reason for hiding this comment

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

Should these be allowed unconditionally, or only when the feature is enabled?

@T-X T-X force-pushed the pr-gluon-mesh-batman-adv-brmldproxy branch from cf37b5d to 3895867 Compare April 17, 2024 00:40
@T-X
Copy link
Contributor Author

T-X commented Apr 17, 2024

Changelog v5:

  • rebased to current master (no conflicts)
  • removed the patch "gluon-mesh-batman-adv-brmldproxy: add to mesh-batman-adv-15 by default" to not add brmldproxy by default with batman-adv
  • /etc/hotplug.d/iface/50-gluon-mesh-batman-adv-brmldproxy:
    • removed executable/enabled checks on /etc/init.d/brmldproxy, as not required due to dependencies and we don't allow manually disabling it for now
    • use Lua JSON filtering/checking instead of piping the full site through the jsonfilter process
  • only remove incoming MLD report ebtables filters if using brmldproxy, to avoid any functional changes to any setups without brmldproxy

@rotanid
Copy link
Member

rotanid commented Jul 28, 2024

@T-X have you seen the comments by neocturne? these are without answers, at least as far as i can see

Now that we have general support for routable IPv6 multicast address in
Gluon master thanks to the newer Linux (bridge) and batman-adv versions
it becomes more interesting to also support layer 3 IPv6 multicast
routers.

So far this was also not possible with the default settings in Gluon
due to filtering MLD into the mesh. This now adds support for
brmldproxy, a daemon which proxies MLD reports between bridge ports.

For the Gluon scenario this package adds brmldproxy proxying
configuration for the mesh side bat0 bridge port.

The configuration is tuned in a way to enable the usage of
layer 3 IPv6 multicast routers for routable IPv6 multicast
address ranges. But with a lot smaller MLD overhead
compared to the filter_membership_reports=false site.conf option.

If a node has no multicast listener for a routable IPv6
multicast address then this node will emit no MLD report
into the mesh. Furthermore, if a node has multiple multicast
listening hosts for routable IPv6 multicast addresses then the
node will act in deputy and respond with combined, aggregated
MLD reports on behalf.

Signed-off-by: Linus Lüssing <[email protected]>
@T-X T-X force-pushed the pr-gluon-mesh-batman-adv-brmldproxy branch 2 times, most recently from 2c68ca3 to 16c7ac7 Compare December 16, 2024 08:11
T-X added 2 commits December 16, 2024 09:14
So far batman-adv flooded all MLD reports. However in our use-case, with
the limitations we already have (*) it is safe to send MLD reports to
detected multicast routers only.

This reduces MLD report overhead even further than brmldproxy alone
already does. And in particular results in no MLD reports in the mesh
if no multicast router is present.

This should, after some more testing from others, potentially make the
gluon-mesh-batman-adv-brmldproxy package suitable for being included by
default. As overhead downsides should then be negligible.

(*): non-Gluon nodes still need to manually set multicast_router=2 on
the bat0 bridge port.

Signed-off-by: Linus Lüssing <[email protected]>
If there is no multicast router behind a bridge port then the Linux
bridge multicast snooping code itself will refrain from forwarding a
report, as recommended/required by RFC4541
("Considerations for Internet Group Management Protocol (IGMP)
  and Multicast Listener Discovery (MLD) Snooping Switches).
So these rules are in most cases redundant.

On the other hand, removing them allows to actually run an IPv6
multicast router behind a Gluon node. Since OpenWrt 23.05 it will allow
detecting multicast routers via Multicast Router Discovery (RFC4286).
And removing these ebtables rules will allow a layer 3 multicast router
to then receive MLD reports from the mesh properly and by that to learn
about others listeners in the mesh.

These incoming MLD report filtering rules are only removed when
gluon-mesh-batman-adv-brmldproxy is installed, to avoid any other
functional changes otherwise.

Signed-off-by: Linus Lüssing <[email protected]>
@T-X T-X force-pushed the pr-gluon-mesh-batman-adv-brmldproxy branch from 16c7ac7 to 8245234 Compare December 16, 2024 08:14
@T-X
Copy link
Contributor Author

T-X commented Dec 16, 2024

Changelog v6:

  • rebased to current main branch (no conflicts)
  • replaced the batman-adv mld-rtr-only configuration option with additional tc-DNAT'ing for now, to be able to use brmldproxy with low MLD report overhead even without any changes to batman-adv
    • note: we have to temporarily change the ICMPv6 type from MLD report to something else, here "100 - Private experimentation", to avoid that batman-adv always floods it
  • simplified the "don't filter incoming MLD Reports" patch by moving the change from gluon-ebtables to gluon-mesh-batman-adv-brmldproxy
    • these two changes allow to use gluon-mesh-batman-adv-brmldproxy with both a vanilla Gluon and vanilla batman-adv now, making it suitable for any external feed as is
  • replaced ( [ ... ] || [ ... ] ) && ... in gluon-mesh-batman-adv-brmldproxy with { [ ... ] || [ ...]; } && ... to avoid a sub-shell invocation, to make the linter happy

@T-X
Copy link
Contributor Author

T-X commented Dec 16, 2024

@rotanid thanks for the comment, but I think all of @neocturne 's previous remarks were addressed in v5 already

@@ -0,0 +1,37 @@
#!/bin/sh

{ [ "$INTERFACE" != "client" ] || [ "$ACTION" != "ifup" ]; } && exit 0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ [ "$INTERFACE" != "client" ] || [ "$ACTION" != "ifup" ]; } && exit 0
if [ "$INTERFACE" != "client" ] || [ "$ACTION" != "ifup" ]; then exit 0; fi

@@ -0,0 +1,37 @@
#!/bin/sh

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
set -e

lua -e "print(require('gluon.site').$path('$default'))"
}

[ "$(lookup_site 'mesh.filter_membership_reports' 'true')" = "false" ] && exit 0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[ "$(lookup_site 'mesh.filter_membership_reports' 'true')" = "false" ] && exit 0
if [ "$(lookup_site 'mesh.filter_membership_reports' 'true')" = "false" ]; then exit 0; fi

# => only send report to IPv6 multicast routers
tc filter add dev bat0 parent fffe: protocol ipv6 prio 4221 handle 11: u32 divisor 1
tc filter add dev bat0 parent fffe: protocol ipv6 prio 4221 u32 ht 11: match u8 131 0xff at 48 match u8 0 0xff at 49 action pedit ex munge eth dst set 33:33:42:4e:f3:14 munge offset 0x30 u16 set 0x6483 action pipe classid 1:1
tc filter add dev bat0 parent fffe: protocol ipv6 prio 4221 u32 ht 11: match u8 132 0xff at 48 match u8 0 0xff at 49 action pedit ex munge eth dst set 33:33:42:4e:f3:14 munge offset 0x30 u16 set 0x6484 action pipe classid 1:1
Copy link
Member

Choose a reason for hiding this comment

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

Maybe some helpers could be added for the repetitive parts of these commands?

bridge mdb "$1" dev br-client port local-port grp "$BRMLDPROXY_RTR_GROUP" permanent 2> /dev/null
}

if batctl mcast_flags | grep -q "^Multicast flags (own flags: \[.*R6\.*\]"; then
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if batctl mcast_flags | grep -q "^Multicast flags (own flags: \[.*R6\.*\]"; then
if batctl mcast_flags | grep -q '^Multicast flags (own flags: \[.*R6\.*\]'; then

Also, maybe the JSON output should be used here?

@@ -0,0 +1,13 @@
#!/bin/sh

BRMLDPROXY_RTR_GROUP="ff12::424e:f314"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment regarding the source of this group address?

Using a group prefix derived from prefix6 (as specified in some RFC) might also be a an alternative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. topic: batman-adv 3. topic: docs Topic: Documentation 3. topic: package Topic: Gluon Packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants