RE: [RFC] cfg80211: Implement Multiple BSSID capability in scanning
> Is a new version of this patch hinging only on this? We'd love to have a > fixed- > up one :-) The new version covers the use cases mentioned by Jouni as the OUIs of the vendor commands has the format of OUI + type + subtype. For other format of vendor commands, they will be treated as different vendor commands and are added in the new IE as well. Jouni, could you chime in to explain your view on this? Thanks, Peng
Re: [RFC] cfg80211: Implement Multiple BSSID capability in scanning
Hi, > As you pointed out the use cases for having vendor elements in MBSSID > subelements, what is > your suggestion to identify and process the vendor elements? Is a new version of this patch hinging only on this? We'd love to have a fixed-up one :-) johannes
RE: [RFC] cfg80211: Implement Multiple BSSID capability in scanning
> On Tue, Nov 14, 2017 at 02:39:31PM +, Peng Xu wrote: > > > I'm not even sure why this is necessary anyway though, do we really > > > think vendors will expect to be able to put vendor IEs inside the > > > subelements and override the ones outside? > > > > > > Is there even any point for the WFA ones? It seems WMM really ought > > > to be the same for all anyway, for example. > > > > I don't have any use case for such scenario. If vendor elements are > > unlikely to be present in subelement, this logic can be removed. > > As far as WMM element is concerned, I'd note that IEEE 802.11 standard does > not list EDCA Parameter Set element as one of the items that has to be same > for all the BSSs in a multiple BSSID set and as such, I'd consider it to be > allowed behavior for an AP to advertise different WMM parameters for > different nontransmitted BSSIDs. > > As far as other vendor specific elements are concerned, there could certainly > be use cases for using different values for WPA element, Hotspot 2.0 > element, MBO/OCE element. > Hi Jouni, As you pointed out the use cases for having vendor elements in MBSSID subelements, what is your suggestion to identify and process the vendor elements? Thanks, Peng
Re: [RFC] cfg80211: Implement Multiple BSSID capability in scanning
On Tue, Nov 14, 2017 at 02:39:31PM +, Peng Xu wrote: > > I'm not even sure why this is necessary anyway though, do we really think > > vendors will expect to be able to put vendor IEs inside the subelements and > > override the ones outside? > > > > Is there even any point for the WFA ones? It seems WMM really ought to be > > the same for all anyway, for example. > > I don't have any use case for such scenario. If vendor elements are unlikely > to > be present in subelement, this logic can be removed. As far as WMM element is concerned, I'd note that IEEE 802.11 standard does not list EDCA Parameter Set element as one of the items that has to be same for all the BSSs in a multiple BSSID set and as such, I'd consider it to be allowed behavior for an AP to advertise different WMM parameters for different nontransmitted BSSIDs. As far as other vendor specific elements are concerned, there could certainly be use cases for using different values for WPA element, Hotspot 2.0 element, MBO/OCE element. -- Jouni MalinenPGP id EFC895FA
RE: [RFC] cfg80211: Implement Multiple BSSID capability in scanning
> > Right, not really. > > I'm not even sure why this is necessary anyway though, do we really think > vendors will expect to be able to put vendor IEs inside the subelements and > override the ones outside? > > Is there even any point for the WFA ones? It seems WMM really ought to be > the same for all anyway, for example. > I don't have any use case for such scenario. If vendor elements are unlikely to be present in subelement, this logic can be removed. Thanks, Peng
Re: [RFC] cfg80211: Implement Multiple BSSID capability in scanning
> > > > This may be right for the WFA/Microsoft OUI, but not necessary anything > > else? > > > > Right, I only considered the most common cases since I did not find a > generic way to compare two vendor elements. Any suggestion? Right, not really. I'm not even sure why this is necessary anyway though, do we really think vendors will expect to be able to put vendor IEs inside the subelements and override the ones outside? Is there even any point for the WFA ones? It seems WMM really ought to be the same for all anyway, for example. johannes
RE: [RFC] cfg80211: Implement Multiple BSSID capability in scanning
> > > > It is OUI + type + subTye. > > Ah, right, type/subtype. > > Still, this is problematic, because there's nothing that says that the vendor > IE > must have OUI + type + subtype, the spec only says OUI + vendor specific > data. > > This may be right for the WFA/Microsoft OUI, but not necessary anything > else? > Right, I only considered the most common cases since I did not find a generic way to compare two vendor elements. Any suggestion? Thanks, Peng
Re: [RFC] cfg80211: Implement Multiple BSSID capability in scanning
On Tue, 2017-11-14 at 14:20 +, Peng Xu wrote:
> >
> > > + if (tmp_old[0] == WLAN_EID_VENDOR_SPECIFIC) {
> > > + if (!memcmp(tmp_old + 2, tmp + 2, 5)) {
> > > + /* same vendor ie, copy from new ie
> >
> > */
> > > + memcpy(pos, tmp, tmp[1] + 2);
> > > + pos += tmp[1] + 2;
> > > + } else {
> > > + memcpy(pos, tmp_old, tmp_old[1] +
> >
> > 2);
> > > + pos += tmp_old[1] + 2;
> >
> > This seems really strange. What's 5? Should it be 4, so you have
> > OUI+subelement ID?
> >
>
> It is OUI + type + subTye.
Ah, right, type/subtype.
Still, this is problematic, because there's nothing that says that the
vendor IE must have OUI + type + subtype, the spec only says OUI +
vendor specific data.
This may be right for the WFA/Microsoft OUI, but not necessary anything
else?
johannes
RE: [RFC] cfg80211: Implement Multiple BSSID capability in scanning
>
> > + if (tmp_old[0] == WLAN_EID_VENDOR_SPECIFIC) {
> > + if (!memcmp(tmp_old + 2, tmp + 2, 5)) {
> > + /* same vendor ie, copy from new ie
> */
> > + memcpy(pos, tmp, tmp[1] + 2);
> > + pos += tmp[1] + 2;
> > + } else {
> > + memcpy(pos, tmp_old, tmp_old[1] +
> 2);
> > + pos += tmp_old[1] + 2;
>
> This seems really strange. What's 5? Should it be 4, so you have
> OUI+subelement ID?
>
It is OUI + type + subTye.
Thanks,
Peng
Re: [RFC] cfg80211: Implement Multiple BSSID capability in scanning
> + if (tmp_old[0] == WLAN_EID_VENDOR_SPECIFIC) {
> + if (!memcmp(tmp_old + 2, tmp + 2, 5)) {
> + /* same vendor ie, copy from new ie */
> + memcpy(pos, tmp, tmp[1] + 2);
> + pos += tmp[1] + 2;
> + } else {
> + memcpy(pos, tmp_old, tmp_old[1] + 2);
> + pos += tmp_old[1] + 2;
This seems really strange. What's 5? Should it be 4, so you have
OUI+subelement ID?
johannes
Re: [RFC] cfg80211: Implement Multiple BSSID capability in scanning
Hi,
Thanks for working on this.
> +u8 *gen_new_bssid(const u8 *bssid, u8 max_bssid, u8 mbssid_index, gfp_t gfp)
I looks like this should be static? I'm also pretty sure you leak the
result, I guess you shouldn't allocate memory here. Just passing in a
pointer to a buffer (that could be on the stack in the caller) would
work fine, afaict.
> +{
> + int i;
> + u64 bssid_a, bssid_b, bssid_tmp = 0;
> + u64 lsb_n;
> + u8 *new_bssid;
> +
> + for (i = 0; i < 5; i++) {
> + bssid_tmp = bssid_tmp | bssid[i];
> + bssid_tmp = bssid_tmp << 8;
> + }
ether_addr_to_u64()
> + bssid_tmp = bssid_tmp | bssid[5];
> +
> + lsb_n = bssid_tmp & ((1 << max_bssid) - 1);
> + bssid_b = (lsb_n + mbssid_index) % (1 << max_bssid);
> + bssid_a = bssid_tmp & ~((1 << max_bssid) - 1);
> + bssid_tmp = bssid_a | bssid_b;
This seems like a somewhat roundabout way of getting this, even if this
is how the spec says it's done. How about just
new_bssid = bssid_tmp;
new_bssid &= ~((1 << max_bssid) - 1);
new_bssid |= (lsb_n + mbssid_index) % (1 << max_bssid);
> + new_bssid = kzalloc(6, gfp);
> + if (!new_bssid)
> + return NULL;
don't allocate
> + for (i = 0; i < 6; i++) {
> + new_bssid[5 - i] = bssid_tmp & 0xFF;
> + bssid_tmp >>= 8;
> + }
u64_to_ether_addr()
Also, wouldn't it be better to handle both the index and list option
from the Co-Located BSSID List subelement in the same function?
> +size_t calculate_new_ie_len(const u8 *ie, size_t ielen, const u8 *subelement,
> + size_t subie_len)
static
> +{
> + size_t new_ie_len;
> + const u8 *old_ie, *new_ie;
> + int delta;
> +
> + new_ie_len = ielen;
> +
> + /* calculate length difference between ssid ie */
> + old_ie = cfg80211_find_ie(WLAN_EID_SSID, ie, ielen);
> + new_ie = cfg80211_find_ie(WLAN_EID_SSID, subelement, subie_len);
> + if (old_ie && new_ie) {
> + delta = new_ie[1] - old_ie[1];
> + new_ie_len += delta;
> + }
No need for the delta variable. Also, this goes wrong if old_ie is
missing but new_ie is present. Probably can't happen, but still.
> + /* calculate length difference between FMS Descriptor ie */
> + old_ie = cfg80211_find_ie(WLAN_EID_FMS_DESCRIPTOR, ie, ielen);
> + new_ie = cfg80211_find_ie(WLAN_EID_FMS_DESCRIPTOR, subelement,
> + subie_len);
> + if (!old_ie && new_ie)
> + new_ie_len += new_ie[1];
> + else if (old_ie && new_ie)
> + new_ie_len += (new_ie[1] - old_ie[1]);
Technically that's missing teh old_ie && !new_ie case. Perhaps you
should pull that out into a small separate function:
new_ie_len += ie_len_delta(WLAN_EID_SSID, ie, ielen,
subelement, subie_len);
or so.
> + /* calculate length difference for same vendor ie */
> + new_ie = cfg80211_find_ie(WLAN_EID_VENDOR_SPECIFIC, subelement,
> + subie_len);
> + while (new_ie && (new_ie - subelement) < subie_len) {
> + old_ie = cfg80211_find_ie(WLAN_EID_VENDOR_SPECIFIC, ie, ielen);
> + while (old_ie && ((old_ie - ie) < ielen)) {
> + if (!memcmp(new_ie + 2, old_ie + 2, 5)) {
> + /* same vendor ie, calculate length difference*/
> + new_ie_len += (new_ie[1] - old_ie[1]);
> + break;
> + }
> + old_ie = cfg80211_find_ie(
> + WLAN_EID_VENDOR_SPECIFIC,
> + old_ie + old_ie[1] + 2,
> + ielen - (old_ie + old_ie[1] + 2 - ie));
> + }
> + new_ie = cfg80211_find_ie(WLAN_EID_VENDOR_SPECIFIC,
> + new_ie + new_ie[1] + 2,
> + subie_len -
> + (new_ie + new_ie[1] + 2 -
> + subelement));
> + }
Perhaps that could be used here too, though I'm not really sure exactly
what you're doing here.
Can you add a comment pointing to where this logic is mentioned in the
spec?
> + /* check other ie in subelement */
> + new_ie = cfg80211_find_ie(WLAN_EID_NON_TX_BSSID_CAP, subelement,
> + subie_len);
> + while (new_ie && (new_ie - subelement < subie_len)) {
> + if (new_ie[0] == WLAN_EID_NON_TX_BSSID_CAP ||
> + new_ie[0] == WLAN_EID_SSID ||
> + new_ie[0] == WLAN_EID_FMS_DESCRIPTOR ||
> + new_ie[0] == WLAN_EID_MULTI_BSSID_IDX) {
> + if ((new_ie + 2 + new_ie[1] - subelement) ==
> + subie_len) {
> + /* new_ie is the last ie */
> +
