Re: [PATCH 3/3] mac80211: multicast to unicast conversion

Am 05.10.2016 12:19, schrieb Johannes Berg:

quoted

on both ends. Furthermore, I've seen a few mobile phone stations
locally that indicate qos support but won't complete DHCP if their
broadcasts are encapsulated as A-MSDU. Though they work fine with
this series approach.

Presumably those phones also don't even try to use DMS, right?

When I traced this two years ago, almost no device indicated DMS 
support, even though almost all seem to accepted multicast in unicast 
a-msdu frames.

quoted

This patch therefore does not opt to implement DMS but instead just
replicates the packet and changes the destination address. As this
works fine with ARP, IPv4 and IPv6, it is limited to these protocols
and normal 802.11 multicast frames are send out for all other payload
protocols.

How did you determine that it "works fine"?

First, I tested this manually using my own devices or asked friends. I 
think this covered at least a recent debian x64 with an intel wireless 
card, a windows 7 x64 with an intel wireless card, an android phone, an 
ios phone and some recent macbook. Manually testing included visiting an 
IPv6 only website (this network uses IPv6 router advertismentens (RA) 
but no DHCPv6), so RA is accepted and ND working. Additionally, 
arping'ing these station using broadcast arp request worked fine, so 
broadcast arp requests are working. Finally, DHCP worked fine and UPNP 
multicast discovery for some closed source media streaming wireless 
device was reported working.

Next, that change was rolled out. It is now in use for at least three 
years with about 300 simulatenously online stations and >2000 currently 
registered devices and there hasn't been a single problem report that 
could be related to that change. Though, e.g. our samsung galaxy users 
report consistently that their devices refuse to connect using WPA-PSK 
as our network advertises FT-PSK next to WPA-PSK and I learned that 
there was at least one device there that did not like the 
multicast-as-unicast-amsdu packets due to a user problem report.

I see at least one undesirable impact of this, which DMS doesn't have;
it breaks a client's MUST NOT requirement from RFC 1122:

Okay, so this cannot go into linux, right?

The thing I dislike most about DMS is that it is client driven, that is 
an AP will only apply unicast conversion if a station actively requests 
so.

You should split the patch into cfg80211 and mac80211, IMHO it's big
enough to do that.

ok

quoted

+ * @set_ap_unicast: set the multicast to unicast flag for a AP
interface

That API name isn't very descriptive, I'm sure we can do something
better there.

proposal: "request multicast packets to be trasnmitted as unicast" ?

Also, perhaps we should structure this already like we would DMS, with
a per-station toggle or even list of multicast addresses?

should be possible, yes

quoted

+static int ieee80211_set_ap_unicast(struct wiphy *wiphy, struct
net_device *dev,
+				    const bool unicast)
+{
+	struct ieee80211_sub_if_data *sdata =
IEEE80211_DEV_TO_SUB_IF(dev);
+
+	if (sdata->vif.type != NL80211_IFTYPE_AP)
+		return -1;

Was this not documented but also intended to apply to its dependent
VLANs?

it was intended as a per per-BSS toggle, so it applies to all dependent 
VLANs automatically.

quoted

+/* Check if multicast to unicast conversion is needed and do it.
+ * Returns 1 if skb was freed and should not be send out. */

wrong comment style :)

you mean the */ at end of line instead of on a new line?

quoted

+	unicast = nla_data(info->attrs[NL80211_ATTR_UNICAST]);

What's this supposed to mean?

this was supposed to be nla_get_u8.

michael

On Wed, 2016-10-05 at 13:40 +0200, michael-dev wrote:

Am 05.10.2016 12:19, schrieb Johannes Berg:

quoted

quoted

on both ends. Furthermore, I've seen a few mobile phone stations
locally that indicate qos support but won't complete DHCP if
their broadcasts are encapsulated as A-MSDU. Though they work
fine with this series approach.

Presumably those phones also don't even try to use DMS, right?

When I traced this two years ago, almost no device indicated DMS 
support, even though almost all seem to accepted multicast in unicast
a-msdu frames.

Right, that's what I suspected. I'm a bit surprised they accepted
multicast in unicast A-MSDU too, though I don't actually see any big
problem with it.

quoted

How did you determine that it "works fine"?

First, I tested this manually using my own devices or asked friends. 

[snip

Thanks!

quoted

I see at least one undesirable impact of this, which DMS doesn't
have; it breaks a client's MUST NOT requirement from RFC 1122:

Okay, so this cannot go into linux, right?

I'm not necessarily saying that, I just think we need to be careful
documenting possibly unexpected/undesired side-effects.

quoted

quoted

+ * @set_ap_unicast: set the multicast to unicast flag for a AP
interface

That API name isn't very descriptive, I'm sure we can do something
better there.

proposal: "request multicast packets to be trasnmitted as unicast" ?

I was thinking more of the function name ("set_ap_unicast") which by
itself makes no sense - set_multicast_to_unicast or something like that
would be better, no?

quoted

Also, perhaps we should structure this already like we would DMS,
with a per-station toggle or even list of multicast addresses?

should be possible, yes

I'm mostly handwaving though, haven't really looked at what DMS really
would require from the API, even assuming that hostapd would implement
all the action frame handling etc.

It's quite possible that on the *client* side, mac80211 should
implement the DMS client, if supported, and perhaps only if enabled by
some kind of configuration knob.

quoted

Was this not documented but also intended to apply to its dependent
VLANs?

it was intended as a per per-BSS toggle, so it applies to all
dependent VLANs automatically.

makes sense, but you should document it in the API documentation, which
today says "for a AP interface" or so (see above)

(btw - writing that out I see that it should be "an AP interface" too)

quoted

quoted

+/* Check if multicast to unicast conversion is needed and do it.
+ * Returns 1 if skb was freed and should not be send out. */

wrong comment style :)

you mean the */ at end of line instead of on a new line?

yeah, no big deal though.

I've also mostly gone back to non-davem style with /* also on its own
line, but it's not so important. :)

quoted

quoted

+	unicast = nla_data(info->attrs[NL80211_ATTR_UNICAST]);

What's this supposed to mean?

this was supposed to be nla_get_u8.

Shouldn't it just be nla_get_flag()? I mean, why do you have a u8 with
values 0/1 rather than just flag attribute absent/present?

Anyway, perhaps this needs to change to take DMS/per-station into
account?

Then again, this kind of setting - global multicast-to-unicast -
fundamentally *cannot* be done on a per-station basis, since if you
enable it for one station and not for another, the first station that
has it enabled would get the packets twice...

johannes