diff mbox series

batman-adv: mcast: add option to forward MLD reports only to mcast routers

Message ID 20240831194528.1932-1-linus.luessing@c0d3.blue (mailing list archive)
State Changes Requested
Delegated to: Simon Wunderlich
Headers
Series batman-adv: mcast: add option to forward MLD reports only to mcast routers |

Commit Message

Linus Lüssing Aug. 31, 2024, 7:45 p.m. UTC 
  In most setups it is sufficient for us to only send MLD reports to nodes
which have a multicast router attached. Which reduces overall overhead,
especially in large batman-adv mesh networks, where broadcasts are
particularly undesirable.

However there is one specific, known issue / broken scenario with this
setting:

If the IGMP/MLD querier is configured directly on the bridge on top of
bat0. But there is no multicast router on or behind this node. Then this
bridge will be unable to detect multicast listeners on/behind other
nodes which have the MLD-RTR-ONLY setting enabled. (A workaround for this
can then in turn be to set multicast_router=2 on the bat0 bridge port
on the node with the IGMP/MLD querier.)

Therefore MLD report flooding is kept the default and MLD report to
multicast routers only forwarding is considered experimental and
warned about.

Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---

An list of scenarios with illustrations and a step-by-step
evaluation with this setting for them can be found on the batman-adv wiki:
* https://www.open-mesh.org/projects/batman-adv/wiki/Multicast-IGMP-MLD-Report-Forwarding#section-2

This patchset is applied and tested on/between Freifunk Lübeck
(180 nodes) and a small Freifunk Vogtland domain. It allowed using
layer 3 IPv6 multicast routing here without flooding the whole mesh
network with MLD reports.

 include/uapi/linux/batman_adv.h |  9 ++++++
 net/batman-adv/multicast.c      | 52 +++++++++++++++++++++++++--------
 net/batman-adv/netlink.c        | 17 +++++++++++
 net/batman-adv/soft-interface.c |  1 +
 net/batman-adv/types.h          |  7 +++++
 5 files changed, 74 insertions(+), 12 deletions(-)

Comments

Sven Eckelmann Oct. 5, 2024, 3:37 p.m. UTC | #1
Unaddressed 
On Saturday, 31 August 2024 21:45:28 CEST Linus Lüssing wrote:
> In most setups it is sufficient for us to only send MLD reports to nodes
> which have a multicast router attached. Which reduces overall overhead,
> especially in large batman-adv mesh networks, where broadcasts are
> particularly undesirable.

What is the situation before. How does this change try to modify the behavior. 
Why is this good?

It also doesn't mention anywhere that the old (multicast filter) behavior was 
just to handle IPv4/IPv6 messages and it is now expanded to 
IGMP_HOST_MEMBERSHIP_REPORT + ICMPV6_MGM_REPORT. 


> 
> However there is one specific, known issue / broken scenario with this
> setting:
> 
> If the IGMP/MLD querier is configured directly on the bridge on top of
> bat0. But there is no multicast router on or behind this node. Then this
> bridge will be unable to detect multicast listeners on/behind other
> nodes which have the MLD-RTR-ONLY setting enabled. (A workaround for this
> can then in turn be to set multicast_router=2 on the bat0 bridge port
> on the node with the IGMP/MLD querier.)
> 
> Therefore MLD report flooding is kept the default and MLD report to
> multicast routers only forwarding is considered experimental and
> warned about.

Most of the text here seems to suggest that (whatever reason you want to 
enable it), you should most likely not do it because it just creates more 
problems.


The commit message doesn't show me why I should apply this change in the first 
place - especially when it just causes more headaches in the end.


Another headache I get is the "Warning: MLD-RTR-ONLY is experimental and has 
known, broken scenarios" part. Which seems to suggest to me that there might 
be changes in the future which will cause more problems if I just apply this 
patch now - and in this process create ABI incompatibilities or "no 
regression" conflicts later.

Btw. BATADV_ATTR_MULTICAST_MLD_RTR_ONLY_ENABLED seems to suggest that it is 
about MLD - but it seems to be also about IGMP. At least I didn't expect this.

Kind regards,
	Sven
Linus Lüssing Oct. 7, 2024, 8:22 a.m. UTC | #2 
On Sat, Oct 05, 2024 at 05:37:19PM +0200, Sven Eckelmann wrote:
> On Saturday, 31 August 2024 21:45:28 CEST Linus Lüssing wrote:
> > In most setups it is sufficient for us to only send MLD reports to nodes
> > which have a multicast router attached. Which reduces overall overhead,
> > especially in large batman-adv mesh networks, where broadcasts are
> > particularly undesirable.
> 
> What is the situation before.

For Gluon the situation was that IGMP+MLD is filtered via ebtables,
because it otherwise would have a significant broadcast overhead.

For batman-adv the situation was that IGMP+MLD got flooded so far.

> How does this change try to modify the behavior. 

With this option a node would only send it to detected multicast
routers instead.

> Why is this good?

I would like to enable layer 3 IPv6 multicast routing between Freifunk
communities / Gluon domains. But without the broadcast flooding
overhead, to stay somewhat scalable.

And the reason why I would want to do layer 3 multicast routing:
Right now multicast has a chicken & egg problem: There aren't many
transmitters or receivers for it (other than ICMPv6 Neighbor Discovery or mDNS).
However one strength of multicast lies in the significantly
increased throughput when there are many simultaneous receivers.

So I'm hoping that while at Freifunk Lübeck with its ~200 nodes /
~200 clients domain I'm basically the only one using it that
if there were all 40'000 Gluon nodes connected on layer 3 that
the chances of a multicast stream being received by multiple
people would significantly increase. Also more multicast content
should then become available for everyone.

> 
> It also doesn't mention anywhere that the old (multicast filter) behavior was 
> just to handle IPv4/IPv6 messages and it is now expanded to 
> IGMP_HOST_MEMBERSHIP_REPORT + ICMPV6_MGM_REPORT. 

Ok, maybe I should have written that more directly in the first
two sentences.

> 
> 
> > 
> > However there is one specific, known issue / broken scenario with this
> > setting:
> > 
> > If the IGMP/MLD querier is configured directly on the bridge on top of
> > bat0. But there is no multicast router on or behind this node. Then this
> > bridge will be unable to detect multicast listeners on/behind other
> > nodes which have the MLD-RTR-ONLY setting enabled. (A workaround for this
> > can then in turn be to set multicast_router=2 on the bat0 bridge port
> > on the node with the IGMP/MLD querier.)
> > 
> > Therefore MLD report flooding is kept the default and MLD report to
> > multicast routers only forwarding is considered experimental and
> > warned about.
> 
> Most of the text here seems to suggest that (whatever reason you want to 
> enable it), you should most likely not do it because it just creates more 
> problems.

Right, in most cases I wouldn't suggest people to enable it. As
can be seen in the wiki page examples it is not trivial. I would
only suggest enabling it in setups where you have thoroughly
checked your use-case against the ones in the wiki. And then only
if you care about layer 3 multicast routing in the first place.

> 
> 
> The commit message doesn't show me why I should apply this change in the first 
> place - especially when it just causes more headaches in the end.
> 
> 
> Another headache I get is the "Warning: MLD-RTR-ONLY is experimental and has 
> known, broken scenarios" part. Which seems to suggest to me that there might 
> be changes in the future which will cause more problems if I just apply this 
> patch now - and in this process create ABI incompatibilities or "no 
> regression" conflicts later.

I don't have any future ABI changes for this in mind. This was
more thought as a warning that the list of examples in the wiki
might not be exhaustive. I could only find one setup where
enabling this option would not work, but despite having spent
several days thinking about these scenarios I can't guarantee right now that
it's the only one. That's why I wanted to mark it as
experimental.

> 
> Btw. BATADV_ATTR_MULTICAST_MLD_RTR_ONLY_ENABLED seems to suggest that it is 
> about MLD - but it seems to be also about IGMP. At least I didn't expect this.

Yeah, it's a bit annoying that for IPv4 it was named IGMP and for
IPv6 it was then renamed to MLD even though the protocol is 99%
the same thing. I'm not sure if it would make sense to make two
separate knobs for IGMP and MLD? (I kind of more and more have the
stance that where "legacy IP" creates extra hassle to ignore it
where possible. And multicast with IPv4 is not as much fun as with
IPv6 anyway, where you have things like embedded RP etc.)

> 
> Kind regards,
> 	Sven
diff mbox series

Patch

diff --git a/include/uapi/linux/batman_adv.h b/include/uapi/linux/batman_adv.h
index 35dc016c9bb4..11cd170036ff 100644
--- a/include/uapi/linux/batman_adv.h
+++ b/include/uapi/linux/batman_adv.h
@@ -481,6 +481,15 @@  enum batadv_nl_attrs {
 	 */
 	BATADV_ATTR_MULTICAST_FANOUT,
 
+	/**
+	 * @BATADV_ATTR_MULTICAST_MLD_RTR_ONLY_ENABLED: defines how IGMP/MLD
+	 * reports are forwarded in the mesh. If set to non-zero then IGMP/MLD
+	 * reports are only forwarded to detected multicast routers. If set to
+	 * zero then they are flooded instead.
+	 * Warning: The former is experimental and potentially unsafe!
+	 */
+	BATADV_ATTR_MULTICAST_MLD_RTR_ONLY_ENABLED,
+
 	/* add attributes above here, update the policy in netlink.c */
 
 	/**
diff --git a/net/batman-adv/multicast.c b/net/batman-adv/multicast.c
index 38fab5e46ae2..d5e3f4d90fd1 100644
--- a/net/batman-adv/multicast.c
+++ b/net/batman-adv/multicast.c
@@ -1008,8 +1008,14 @@  static int batadv_mcast_forw_mode_check_ipv4(struct batadv_priv *bat_priv,
 	if (!pskb_may_pull(skb, sizeof(struct ethhdr) + sizeof(*iphdr)))
 		return -ENOMEM;
 
-	if (batadv_mcast_is_report_ipv4(skb))
+	if (batadv_mcast_is_report_ipv4(skb)) {
+		if (atomic_read(&bat_priv->multicast_mld_rtr_only)) {
+			*is_routable = IGMP_HOST_MEMBERSHIP_REPORT;
+			return 0;
+		}
+
 		return -EINVAL;
+	}
 
 	iphdr = ip_hdr(skb);
 
@@ -1072,8 +1078,14 @@  static int batadv_mcast_forw_mode_check_ipv6(struct batadv_priv *bat_priv,
 	if (!pskb_may_pull(skb, sizeof(struct ethhdr) + sizeof(*ip6hdr)))
 		return -ENOMEM;
 
-	if (batadv_mcast_is_report_ipv6(skb))
+	if (batadv_mcast_is_report_ipv6(skb)) {
+		if (atomic_read(&bat_priv->multicast_mld_rtr_only)) {
+			*is_routable = ICMPV6_MGM_REPORT;
+			return 0;
+		}
+
 		return -EINVAL;
+	}
 
 	ip6hdr = ipv6_hdr(skb);
 
@@ -1160,17 +1172,19 @@  static int batadv_mcast_forw_want_all_ip_count(struct batadv_priv *bat_priv,
  * @protocol: the ethernet protocol type to count multicast routers for
  *
  * Return: the number of nodes which want all routable IPv4 multicast traffic
- * if the protocol is ETH_P_IP or the number of nodes which want all routable
- * IPv6 traffic if the protocol is ETH_P_IPV6. Otherwise returns 0.
+ * if the protocol is ETH_P_IP or IGMP_HOST_MEMBERSHIP_REPORT. Or the number of
+ * nodes which want all routable IPv6 traffic if the protocol is ETH_P_IPV6 or
+ * ICMPV6_MGM_REPORT. Otherwise returns 0.
  */
-
 static int batadv_mcast_forw_rtr_count(struct batadv_priv *bat_priv,
 				       int protocol)
 {
 	switch (protocol) {
 	case ETH_P_IP:
+	case IGMP_HOST_MEMBERSHIP_REPORT:
 		return atomic_read(&bat_priv->mcast.num_want_all_rtr4);
 	case ETH_P_IPV6:
+	case ICMPV6_MGM_REPORT:
 		return atomic_read(&bat_priv->mcast.num_want_all_rtr6);
 	default:
 		return 0;
@@ -1234,10 +1248,11 @@  enum batadv_forw_mode
 batadv_mcast_forw_mode(struct batadv_priv *bat_priv, struct sk_buff *skb,
 		       unsigned short vid, int *is_routable)
 {
-	int ret, tt_count, ip_count, unsnoop_count, total_count;
+	atomic_t *unsnoop_cnt_atom = &bat_priv->mcast.num_want_all_unsnoopables;
+	int ret, ip_count, rtr_count, total_count;
+	int tt_count = 0, unsnoop_count = 0;
 	bool is_unsnoopable = false;
 	struct ethhdr *ethhdr;
-	int rtr_count = 0;
 
 	ret = batadv_mcast_forw_mode_check(bat_priv, skb, &is_unsnoopable,
 					   is_routable);
@@ -1248,11 +1263,17 @@  batadv_mcast_forw_mode(struct batadv_priv *bat_priv, struct sk_buff *skb,
 
 	ethhdr = eth_hdr(skb);
 
-	tt_count = batadv_tt_global_hash_count(bat_priv, ethhdr->h_dest,
-					       BATADV_NO_FLAGS);
-	ip_count = batadv_mcast_forw_want_all_ip_count(bat_priv, ethhdr);
-	unsnoop_count = !is_unsnoopable ? 0 :
-			atomic_read(&bat_priv->mcast.num_want_all_unsnoopables);
+	if (*is_routable != IGMP_HOST_MEMBERSHIP_REPORT &&
+	    *is_routable != ICMPV6_MGM_REPORT) {
+		tt_count = batadv_tt_global_hash_count(bat_priv, ethhdr->h_dest,
+						       BATADV_NO_FLAGS);
+
+		if (is_unsnoopable)
+			unsnoop_count = atomic_read(unsnoop_cnt_atom);
+	}
+
+	ip_count = batadv_mcast_forw_want_all_ip_count(bat_priv,
+						       ethhdr);
 	rtr_count = batadv_mcast_forw_rtr_count(bat_priv, *is_routable);
 
 	total_count = tt_count + ip_count + unsnoop_count + rtr_count;
@@ -1540,8 +1561,10 @@  batadv_mcast_forw_want_rtr(struct batadv_priv *bat_priv,
 {
 	switch (ntohs(eth_hdr(skb)->h_proto)) {
 	case ETH_P_IP:
+	case IGMP_HOST_MEMBERSHIP_REPORT:
 		return batadv_mcast_forw_want_all_rtr4(bat_priv, skb, vid);
 	case ETH_P_IPV6:
+	case ICMPV6_MGM_REPORT:
 		return batadv_mcast_forw_want_all_rtr6(bat_priv, skb, vid);
 	default:
 		/* we shouldn't be here... */
@@ -1571,12 +1594,17 @@  int batadv_mcast_forw_send(struct batadv_priv *bat_priv, struct sk_buff *skb,
 {
 	int ret;
 
+	if (is_routable == IGMP_HOST_MEMBERSHIP_REPORT ||
+	    is_routable == ICMPV6_MGM_REPORT)
+		goto skip_mc_listeners;
+
 	ret = batadv_mcast_forw_tt(bat_priv, skb, vid);
 	if (ret != NET_XMIT_SUCCESS) {
 		kfree_skb(skb);
 		return ret;
 	}
 
+skip_mc_listeners:
 	ret = batadv_mcast_forw_want_all(bat_priv, skb, vid);
 	if (ret != NET_XMIT_SUCCESS) {
 		kfree_skb(skb);
diff --git a/net/batman-adv/netlink.c b/net/batman-adv/netlink.c
index 3f0003fc502c..0923117d63b2 100644
--- a/net/batman-adv/netlink.c
+++ b/net/batman-adv/netlink.c
@@ -145,6 +145,7 @@  static const struct nla_policy batadv_netlink_policy[NUM_BATADV_ATTR] = {
 	[BATADV_ATTR_LOG_LEVEL]			= { .type = NLA_U32 },
 	[BATADV_ATTR_MULTICAST_FORCEFLOOD_ENABLED]	= { .type = NLA_U8 },
 	[BATADV_ATTR_MULTICAST_FANOUT]		= { .type = NLA_U32 },
+	[BATADV_ATTR_MULTICAST_MLD_RTR_ONLY_ENABLED]	= { .type = NLA_U8 },
 	[BATADV_ATTR_NETWORK_CODING_ENABLED]	= { .type = NLA_U8 },
 	[BATADV_ATTR_ORIG_INTERVAL]		= { .type = NLA_U32 },
 	[BATADV_ATTR_ELP_INTERVAL]		= { .type = NLA_U32 },
@@ -345,6 +346,10 @@  static int batadv_netlink_mesh_fill(struct sk_buff *msg,
 	if (nla_put_u32(msg, BATADV_ATTR_MULTICAST_FANOUT,
 			atomic_read(&bat_priv->multicast_fanout)))
 		goto nla_put_failure;
+
+	if (nla_put_u8(msg, BATADV_ATTR_MULTICAST_MLD_RTR_ONLY_ENABLED,
+		       atomic_read(&bat_priv->multicast_mld_rtr_only)))
+		goto nla_put_failure;
 #endif /* CONFIG_BATMAN_ADV_MCAST */
 
 #ifdef CONFIG_BATMAN_ADV_NC
@@ -588,6 +593,18 @@  static int batadv_netlink_set_mesh(struct sk_buff *skb, struct genl_info *info)
 
 		atomic_set(&bat_priv->multicast_fanout, nla_get_u32(attr));
 	}
+
+	if (info->attrs[BATADV_ATTR_MULTICAST_MLD_RTR_ONLY_ENABLED]) {
+		u8 mld_rtr_only;
+
+		attr = info->attrs[BATADV_ATTR_MULTICAST_MLD_RTR_ONLY_ENABLED];
+		mld_rtr_only = !!nla_get_u8(attr);
+		if (mld_rtr_only)
+			batadv_info(bat_priv->soft_iface,
+				    "Warning: MLD-RTR-ONLY is experimental and has known, broken scenarios\n");
+
+		atomic_set(&bat_priv->multicast_mld_rtr_only, mld_rtr_only);
+	}
 #endif /* CONFIG_BATMAN_ADV_MCAST */
 
 #ifdef CONFIG_BATMAN_ADV_NC
diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c
index 30ecbc2ef1fd..bd9168450995 100644
--- a/net/batman-adv/soft-interface.c
+++ b/net/batman-adv/soft-interface.c
@@ -762,6 +762,7 @@  static int batadv_softif_init_late(struct net_device *dev)
 #ifdef CONFIG_BATMAN_ADV_MCAST
 	atomic_set(&bat_priv->multicast_mode, 1);
 	atomic_set(&bat_priv->multicast_fanout, 16);
+	atomic_set(&bat_priv->multicast_mld_rtr_only, 0);
 	atomic_set(&bat_priv->mcast.num_want_all_unsnoopables, 0);
 	atomic_set(&bat_priv->mcast.num_want_all_ipv4, 0);
 	atomic_set(&bat_priv->mcast.num_want_all_ipv6, 0);
diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h
index 00840d5784fe..bf8eab36e43b 100644
--- a/net/batman-adv/types.h
+++ b/net/batman-adv/types.h
@@ -1683,6 +1683,13 @@  struct batadv_priv {
 	 *  multicast-to-unicast conversion
 	 */
 	atomic_t multicast_fanout;
+
+	/**
+	 * @multicast_mld_rtr_only: bool indicating whether to send IGMP/MLD
+	 *  reports only to multicast routers or to flood them otherwise.
+	 * Warning: The former is experimental and potentially unsafe!
+	 */
+	atomic_t multicast_mld_rtr_only;
 #endif
 
 	/** @orig_interval: OGM broadcast interval in milliseconds */