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

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

[Johannes Berg]

+netdev

> IEEE802.11-2012 proposes directed multicast service (DMS) using A-
> MSDU frames and a station initiated control protocol. It has the
> advantage that the station can recover the destination multicast mac
> address, but it is not backward compatible with non QOS stations and
> does not enable the administrator of a BSS to force this mode of
> operation within a BSS. Additionally, it would require both the ap
> and the station to implement the control protocol, which is optional
> 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?

> 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"?

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:

         An ICMP error message MUST NOT be sent as the result of
         receiving:
[...]
         *    a datagram sent as a link-layer broadcast, or
[...]

since the client can no longer realize that the datagram was in fact
sent as a link-layer broadcast (or multicast).

>  include/net/cfg80211.h        |   5 ++
>  include/uapi/linux/nl80211.h  |   7 +++
>  net/mac80211/cfg.c            |  14 ++++++
>  net/mac80211/debugfs_netdev.c |  29 ++++++++++++
>  net/mac80211/ieee80211_i.h    |   1 +
>  net/mac80211/tx.c             | 103
> ++++++++++++++++++++++++++++++++++++++++++
>  net/wireless/nl80211.c        |  33 ++++++++++++++
>  net/wireless/rdev-ops.h       |  11 +++++
>  net/wireless/trace.h          |  19 ++++++++
>  9 files changed, 222 insertions(+)

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

> + * @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.

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

> @@ -2261,6 +2266,8 @@ enum nl80211_attrs {
>  
>  	NL80211_ATTR_MESH_PEER_AID,
>  
> +	NL80211_ATTR_UNICAST,

missing docs, but likely doesn't matter after the comment above

> +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?

> +static ssize_t
> +ieee80211_if_fmt_unicast(const struct ieee80211_sub_if_data *sdata,
> +			 char *buf, int buflen)
> +{
> +	const struct ieee80211_if_ap *ifap = &sdata->u.ap;
> +
> +	return snprintf(buf, buflen, "0x%x\n", ifap->unicast);
> +}
> +
> +static ssize_t
> +ieee80211_if_parse_unicast(struct ieee80211_sub_if_data *sdata,
> +			   const char *buf, int buflen)
> +{
> +	struct ieee80211_if_ap *ifap = &sdata->u.ap;
> +	u8 val;
> +	int ret;
> +
> +	ret = kstrtou8(buf, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	ifap->unicast = val ? 1 : 0;
> +
> +	return buflen;
> +}
> +
> +IEEE80211_IF_FILE_RW(unicast);

No need for this, at least the setter, any more.

> +/* 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 :)

> +static int
> +ieee80211_tx_multicast_to_unicast(struct ieee80211_sub_if_data
> *sdata,
> +				  struct sk_buff *skb,
> u32  info_flags)
> +{
> +	struct ieee80211_local *local = sdata->local;
> +	const struct ethhdr *eth = (void *)skb->data;
> +	const struct vlan_ethhdr *ethvlan = (void *)skb->data;
> +	struct sta_info *sta, *prev = NULL;
> +	struct sk_buff *cloned_skb;
> +	u16 ethertype;
> +
> +	/* multicast to unicast conversion only for AP interfaces */
> +	switch (sdata->vif.type) {
> +	case NL80211_IFTYPE_AP_VLAN:
> +		sta = rcu_dereference(sdata->u.vlan.sta);
> +		if (sta) /* 4addr */
> +			return 0;
> +	case NL80211_IFTYPE_AP:
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	/* check runtime toggle for this bss */
> +	if (!sdata->bss->unicast)
> +		return 0;
> +
> +	/* check if this is a multicast frame */
> +	if (!is_multicast_ether_addr(eth->h_dest))
> +		return 0;

That should probably come first, would make this far easier to read.

> +		if (unlikely(!memcmp(eth->h_source, sta->sta.addr,
> ETH_ALEN)))
> +			/* do not send back to source */
> +			continue;

ether_addr_something, instead of memcmp?

> +		if (unlikely(is_multicast_ether_addr(sta-
> >sta.addr))) {
> +			WARN_ONCE(1, "sta with multicast address
> %pM",
> +				  sta->sta.addr);
> +			continue;
> +		}

Err, no, remove this... it cannot happen. We could move the check into
cfg80211 from mac80211, but we surely shouldn't add it into the TX
hotpath!

> +		if (prev) {
> +			cloned_skb = skb_clone(skb, GFP_ATOMIC);
> +			if (likely(!ieee80211_change_da(cloned_skb,
> prev)))
> +				ieee80211_subif_start_xmit(cloned_sk
> b,
> +							   cloned_sk
> b->dev);

I'm not very happy with this recursion, but I guess it can't be avoided
easily. However, you can easily call the more
sensible __ieee80211_subif_start_xmit() instead of this one.

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

What's this supposed to mean?

johannes

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

[michael-dev]

Am 05.10.2016 12:19, schrieb Johannes Berg:
>> 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.

> 
>> 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

>> + * @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


>> +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.

>> +/* 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?

>> +	unicast = nla_data(info->attrs[NL80211_ATTR_UNICAST]);
> 
> What's this supposed to mean?

this was supposed to be nla_get_u8.

michael

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

[Johannes Berg]

On Wed, 2016-10-05 at 13:40 +0200, michael-dev wrote:
> Am 05.10.2016 12:19, schrieb Johannes Berg:
> > 
> > > 
> > > 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.

> > How did you determine that it "works fine"?
> 
> First, I tested this manually using my own devices or asked friends. 
[snip

Thanks!

> > 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.

> > > + * @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?

> > 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.

> > 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)

> > 
> > > 
> > > +/* 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. :)

> > > +	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

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

[michael-dev]

Am 05.10.2016 13:58, schrieb Johannes Berg:
> 
> 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...

as I see it, that is exactly how DMS is standarized.

IEEE 802.11-2012 section 10.23.15 DMS procedures:

"If the requested DMS is accepted by the AP, the AP shall send 
subsequent group addressed MSDUs that
match the frame classifier specified in the DMS Descriptors to the 
requesting STA as A-MSDU subframes
within an individually addressed A-MSDU frame (see 8.3.2.2 and 9.11)."

  -> so the multicast packets shall go out as unicast A-MSDU frames to 
stations that requested this

"The AP shall continue to transmit the matching frames as group 
addressed frames (see 9.3.6, and 10.2.1.16) if at least one associated 
STA has not requested DMS for these frames."

  -> so it will continue to send it as multicast frames as well.

As with DMS the station requested DMS for a specific multicast address, 
it could then drop multicast frames addressed to the multicast address 
it registered for DMS.

Regards,
M. Braun

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

[Johannes Berg]

On Thu, 2016-10-06 at 13:53 +0200, michael-dev wrote:
> Am 05.10.2016 13:58, schrieb Johannes Berg:
> > 
> > 
> > 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...
> 
> as I see it, that is exactly how DMS is standarized.
> 
> IEEE 802.11-2012 section 10.23.15 DMS procedures:
> 
> "If the requested DMS is accepted by the AP, the AP shall send 
> subsequent group addressed MSDUs that
> match the frame classifier specified in the DMS Descriptors to the 
> requesting STA as A-MSDU subframes
> within an individually addressed A-MSDU frame (see 8.3.2.2 and
> 9.11)."
> 
>   -> so the multicast packets shall go out as unicast A-MSDU frames
> to  stations that requested this

Correct.

> "The AP shall continue to transmit the matching frames as group 
> addressed frames (see 9.3.6, and 10.2.1.16) if at least one
> associated 
> STA has not requested DMS for these frames."
> 
>   -> so it will continue to send it as multicast frames as well.
> 
> As with DMS the station requested DMS for a specific multicast
> address, it could then drop multicast frames addressed to the
> multicast address it registered for DMS.

Yes, the DMS spec tells it to do this. However, we can't implement non-
DMS similarly, because then the station won't request it and won't drop
the duplicates.

So for this non-standard multicast-to-unicast, it's all or nothing, it
can't be done for some stations only.

johannes