diff mbox

[OpenWrt-Devel,PATCHv3,1/2] Revert "kernel: disable multicast-to-unicast translation for ipv6 neighbor solicitation (#17625)"

Message ID 1440343170-13040-5-git-send-email-linus.luessing@c0d3.blue
State Accepted
Headers show

Commit Message

Linus Lüssing Aug. 23, 2015, 3:19 p.m. UTC
This reverts commit a080e8e1943156168913d0353a2e99d1151102aa.

It did not fix the problem but just hid some symptom. The real issue was
that IGMP/MLD report suppression was not considered for the
multicast-to-unicast feature. A recent netifd which isolates IGMP/MLD
reports between STAs by utilizing AP-isolation and bridge-hairpinning
should have fixed this.

It is perfectly fine to apply multicast-to-unicast to IPv6 Neighbor
Solicitations, too (once that feature is configured correctly).

Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---
 .../645-bridge_multicast_to_unicast.patch          |   37 +++++++++-----------
 1 file changed, 17 insertions(+), 20 deletions(-)

Comments

Steven Barth Aug. 29, 2015, 2:21 p.m. UTC | #1
Hi Linus,

one of these two kernel patches break IPv6 over WiFi completely.

https://dev.openwrt.org/changeset/46719 works
https://dev.openwrt.org/changeset/46723 is broken with default settings,
but works when setting option igmp_snooping 0.

symptoms are:
"IPv6: wlan0: IPv6 duplicate address fe80::[...] detected!"
in the client's kernel log when connecting using WiFi.

Connecting through Ethernet (bridged to said WiFi) works as expected
though. Since the link-local is marked as dadfailed over WiFi,
IPv6 connectivity is completely broken, obviously the DAD failed
is a false positive.

Router is ar71xx / ath9k.
Client is iwlwfi.

Let me know if you need more info.



Cheers,

Steven
Linus Lüssing Aug. 30, 2015, 1:49 p.m. UTC | #2
Hi Steven,

Thanks for the feedback.

On Sat, Aug 29, 2015 at 04:21:50PM +0200, Steven Barth wrote:
> Let me know if you need more info.

Could you make a tcpdump for ICMPv6 on the wifi of the client?
Please start it before connecting and let it run for about three
minutes after connecting to the wifi.

On the router side, what's the output of:

* /sys/class/net/$bridged-wifi/brport/hairpin
* /sys/class/net/$bridged-wifi/brport/multicast_to_unicast
* /tmp/run/hostapd-*, ap-isolate=...

Cheers, Linus
Linus Lüssing Aug. 30, 2015, 10:54 p.m. UTC | #3
On Sun, Aug 30, 2015 at 03:49:50PM +0200, Linus Lüssing wrote:
> Hi Steven,
> 
> Thanks for the feedback.
> 
> On Sat, Aug 29, 2015 at 04:21:50PM +0200, Steven Barth wrote:
> > Let me know if you need more info.
> 
> Could you make a tcpdump for ICMPv6 on the wifi of the client?
> Please start it before connecting and let it run for about three
> minutes after connecting to the wifi.

Ok, I was able to easily reproduce the issue here. Looking at the
tcpdump the wifi client receives its own Neighbor Solicitation
again, reflected through the bridge-hairpin option.

The Linux IPv6 stack seems to interpret the reflected NS as an
address collision with another host. It seems a little weird that
the Linux kernel does that for NS - I think an IPv6 implementation
should only interpret Neighbor Advertisements as address conflicts
if I remember the RFCs correctly.


My current thought to fix this issue is to patch the
multicast-to-unicast patch in the following way:

Only transmit multicast-to-unicast'ed packets if the mangled,
now unicast MAC destination address differs from the MAC source.


We need to think about what'd happen for non-multicast-to-unicast'ed
multicast packets which get reflected back to the wifi interface
though. This should only happen if a multicast router is on one of
the wifi clients / STAs, I think. Will need to check whether the
Linux kernel interprets multicast packets without a mangled
MAC destination as duplicates, too.

Cheers, Linus
Linus Lüssing Aug. 30, 2015, 11:07 p.m. UTC | #4
PS: Out of curiousity, can someone with deeper knowledge into 802.11
tell me, when an STA sends a multicast packet to its master which
then rebroadcasts it at e.g. 1MBit/s, how does the originating STA
know that its a rebroadcast of its own frame and refrain from
forwarding it internally?
Steven Barth Aug. 31, 2015, 9:29 a.m. UTC | #5
>Ok, I was able to easily reproduce the issue here. Looking at the
>tcpdump the wifi client receives its own Neighbor Solicitation
>again, reflected through the bridge-hairpin option.

Why are we reflecting packets back to the originator anyway? Could it not be excluded?

Btw. once we are bit more stable i will try to test how the igmpv3/mldv2 querier / proxy I wrote interacts here (disabling kernel querier).
Linus Lüssing Aug. 31, 2015, 10:19 a.m. UTC | #6
On Mon, Aug 31, 2015 at 11:29:45AM +0200, Steven Barth wrote:
> 
> 
> 
> >Ok, I was able to easily reproduce the issue here. Looking at the
> >tcpdump the wifi client receives its own Neighbor Solicitation
> >again, reflected through the bridge-hairpin option.
> 
> Why are we reflecting packets back to the originator anyway? Could it not be excluded?

Currently the multicast-to-unicast patch reflects it back as it
detected that the originator itself is a listener of the multicast
group of the Neighbor Solicitation.

I had somehow expected that mac80211 would exclude it. But looks like it
doesn't. And yes, we can exclude/drop that unicast'ed multicast
packet with src==dst on the bridge side already like I said
before for a suggested fix. But it's not 100% sufficient yet, we'd
have issues again once a multicast router/querier is on an STA.

Cheers, Linus

> 
> Btw. once we are bit more stable i will try to test how the igmpv3/mldv2 querier / proxy I wrote interacts here (disabling kernel querier).

I'd be interested to hear about that :).
Linus Lüssing Aug. 31, 2015, 12:04 p.m. UTC | #7
On Mon, Aug 31, 2015 at 12:19:44PM +0200, Linus Lüssing wrote:
> I had somehow expected that mac80211 would exclude it. But looks like it
> doesn't.

Looks like mac80211 on an STA does that for frames with a
multicast destination (to prevent getting the own multicast
packets echo'd back once the AP rebroadcasts them, I guess). E.g.
ap-isolation + bridge-hairpin without multicast-to-unicast works
fine. So maybe the simple fix for the multicast-to-unicast patch
is sufficient.

The unicast-to-multicast patch mangled the destination to a
unicast one and mac80211 does not seem to consider their source
address.

Going to make and try a patch for the bridge now.

Cheers, Linus
diff mbox

Patch

diff --git a/target/linux/generic/patches-3.18/645-bridge_multicast_to_unicast.patch b/target/linux/generic/patches-3.18/645-bridge_multicast_to_unicast.patch
index 00ad14f..e8be1fd 100644
--- a/target/linux/generic/patches-3.18/645-bridge_multicast_to_unicast.patch
+++ b/target/linux/generic/patches-3.18/645-bridge_multicast_to_unicast.patch
@@ -87,19 +87,16 @@ 
  {
  	struct br_ip br_group;
  
-@@ -736,7 +758,10 @@ static int br_ip6_multicast_add_group(st
+@@ -736,7 +758,7 @@ static int br_ip6_multicast_add_group(st
  	br_group.proto = htons(ETH_P_IPV6);
  	br_group.vid = vid;
  
 -	return br_multicast_add_group(br, port, &br_group);
-+	if (ipv6_addr_is_solict_mult(group))
-+		src = NULL;
-+
 +	return br_multicast_add_group(br, port, &br_group, src);
  }
  #endif
  
-@@ -965,6 +990,7 @@ static int br_ip4_multicast_igmp3_report
+@@ -965,6 +987,7 @@ static int br_ip4_multicast_igmp3_report
  					 struct sk_buff *skb,
  					 u16 vid)
  {
@@ -107,7 +104,7 @@ 
  	struct igmpv3_report *ih;
  	struct igmpv3_grec *grec;
  	int i;
-@@ -1008,7 +1034,7 @@ static int br_ip4_multicast_igmp3_report
+@@ -1008,7 +1031,7 @@ static int br_ip4_multicast_igmp3_report
  			continue;
  		}
  
@@ -116,7 +113,7 @@ 
  		if (err)
  			break;
  	}
-@@ -1022,6 +1048,7 @@ static int br_ip6_multicast_mld2_report(
+@@ -1022,6 +1045,7 @@ static int br_ip6_multicast_mld2_report(
  					struct sk_buff *skb,
  					u16 vid)
  {
@@ -124,7 +121,7 @@ 
  	struct icmp6hdr *icmp6h;
  	struct mld2_grec *grec;
  	int i;
-@@ -1070,7 +1097,7 @@ static int br_ip6_multicast_mld2_report(
+@@ -1070,7 +1094,7 @@ static int br_ip6_multicast_mld2_report(
  		}
  
  		err = br_ip6_multicast_add_group(br, port, &grec->grec_mca,
@@ -133,7 +130,7 @@ 
  		if (err)
  			break;
  	}
-@@ -1406,7 +1433,8 @@ br_multicast_leave_group(struct net_brid
+@@ -1407,7 +1431,8 @@ br_multicast_leave_group(struct net_brid
  			 struct net_bridge_port *port,
  			 struct br_ip *group,
  			 struct bridge_mcast_other_query *other_query,
@@ -143,7 +140,7 @@ 
  {
  	struct net_bridge_mdb_htable *mdb;
  	struct net_bridge_mdb_entry *mp;
-@@ -1456,7 +1484,7 @@ br_multicast_leave_group(struct net_brid
+@@ -1457,7 +1482,7 @@ br_multicast_leave_group(struct net_brid
  		for (pp = &mp->ports;
  		     (p = mlock_dereference(*pp, br)) != NULL;
  		     pp = &p->next) {
@@ -152,7 +149,7 @@ 
  				continue;
  
  			rcu_assign_pointer(*pp, p->next);
-@@ -1490,7 +1518,7 @@ br_multicast_leave_group(struct net_brid
+@@ -1491,7 +1516,7 @@ br_multicast_leave_group(struct net_brid
  	for (p = mlock_dereference(mp->ports, br);
  	     p != NULL;
  	     p = mlock_dereference(p->next, br)) {
@@ -161,7 +158,7 @@ 
  			continue;
  
  		if (!hlist_unhashed(&p->mglist) &&
-@@ -1508,8 +1536,8 @@ out:
+@@ -1509,8 +1534,8 @@ out:
  
  static void br_ip4_multicast_leave_group(struct net_bridge *br,
  					 struct net_bridge_port *port,
@@ -172,7 +169,7 @@ 
  {
  	struct br_ip br_group;
  	struct bridge_mcast_own_query *own_query;
-@@ -1524,14 +1552,14 @@ static void br_ip4_multicast_leave_group
+@@ -1525,14 +1550,14 @@ static void br_ip4_multicast_leave_group
  	br_group.vid = vid;
  
  	br_multicast_leave_group(br, port, &br_group, &br->ip4_other_query,
@@ -189,7 +186,7 @@ 
  {
  	struct br_ip br_group;
  	struct bridge_mcast_own_query *own_query;
-@@ -1546,7 +1574,7 @@ static void br_ip6_multicast_leave_group
+@@ -1547,7 +1572,7 @@ static void br_ip6_multicast_leave_group
  	br_group.vid = vid;
  
  	br_multicast_leave_group(br, port, &br_group, &br->ip6_other_query,
@@ -198,7 +195,7 @@ 
  }
  #endif
  
-@@ -1555,6 +1583,7 @@ static int br_multicast_ipv4_rcv(struct
+@@ -1556,6 +1581,7 @@ static int br_multicast_ipv4_rcv(struct
  				 struct sk_buff *skb,
  				 u16 vid)
  {
@@ -206,7 +203,7 @@ 
  	struct sk_buff *skb2 = skb;
  	const struct iphdr *iph;
  	struct igmphdr *ih;
-@@ -1628,7 +1657,7 @@ static int br_multicast_ipv4_rcv(struct
+@@ -1629,7 +1655,7 @@ static int br_multicast_ipv4_rcv(struct
  	case IGMP_HOST_MEMBERSHIP_REPORT:
  	case IGMPV2_HOST_MEMBERSHIP_REPORT:
  		BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
@@ -215,7 +212,7 @@ 
  		break;
  	case IGMPV3_HOST_MEMBERSHIP_REPORT:
  		err = br_ip4_multicast_igmp3_report(br, port, skb2, vid);
-@@ -1637,7 +1666,7 @@ static int br_multicast_ipv4_rcv(struct
+@@ -1638,7 +1664,7 @@ static int br_multicast_ipv4_rcv(struct
  		err = br_ip4_multicast_query(br, port, skb2, vid);
  		break;
  	case IGMP_HOST_LEAVE_MESSAGE:
@@ -224,7 +221,7 @@ 
  		break;
  	}
  
-@@ -1655,6 +1684,7 @@ static int br_multicast_ipv6_rcv(struct
+@@ -1656,6 +1682,7 @@ static int br_multicast_ipv6_rcv(struct
  				 struct sk_buff *skb,
  				 u16 vid)
  {
@@ -232,7 +229,7 @@ 
  	struct sk_buff *skb2;
  	const struct ipv6hdr *ip6h;
  	u8 icmp6_type;
-@@ -1764,7 +1794,8 @@ static int br_multicast_ipv6_rcv(struct
+@@ -1765,7 +1792,8 @@ static int br_multicast_ipv6_rcv(struct
  		}
  		mld = (struct mld_msg *)skb_transport_header(skb2);
  		BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
@@ -242,7 +239,7 @@ 
  		break;
  	    }
  	case ICMPV6_MLD2_REPORT:
-@@ -1781,7 +1812,7 @@ static int br_multicast_ipv6_rcv(struct
+@@ -1782,7 +1810,7 @@ static int br_multicast_ipv6_rcv(struct
  			goto out;
  		}
  		mld = (struct mld_msg *)skb_transport_header(skb2);