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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1690671fb30f19c53c7883a5207c5721@fami-braun.de>
Date:   Wed, 05 Oct 2016 13:40:03 +0200
From:   michael-dev <michael-dev@...i-braun.de>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     linux-wireless@...r.kernel.org, projekt-wlan@....tu-ilmenau.de,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH 3/3] mac80211: multicast to unicast conversion

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