> Aah.. That's a copy-paste from something old.. I think I removed the > IEEE80211_KEY_FLAG_GENERATE_IV flag from here for BIP.. I'm not > completely sure yet, how we should have this, i.e., whether that flag > should be used here or not. It is unclear what type of hwaccel, if any, > vendors are going to implement for BIP. Yeah, good point, I guess we should just not implement hwaccel until we find hardware that can handle it. > > I tried ensure pskb_expand_head is only called at most once when the > > frame is handed to master_start_xmit to avoid problems with skb truesize > > and such. Could you add the necessary space at that point already, > > possibly simply reserving max(mmic-len, mmie-len) or so instead of the > > current mmic-len (I think)? I'd hate to add back calls to > > pskb_expand_head at other places, and it's only 18 bytes so not really > > huge. > > I thought about that for a moment, but then decided that it would be > okay to just do this here since MMIE is larger than any other tailroom > we have and there is next to no real use for multicast/broadcast > management frames, so there is no need to optimize this. I don't see how > this would require any other place to use pskb_expand_head. Well the thing is that you can't just call pskb_expand_head without orphaning the SKB first and all that truesize adjusting, because of truesize accounting, because it might now or later belong to a userspace socket. > Anyway, I would assume it would be possible to do this by changing > IEEE80211_ENCRYPT_TAILROOM from 12 to 22 which would waste 10 bytes > extra for every frame (well, not waste for BIP-protected frames but > there are next to none of them). 22? Is there something else on the frame in addition to the MMIE? > I converted some of the places to use the new helpers, but did not go > through all places. I would assume there is still places that can be > made to use them, but as long as FC is available in host byte order, it > is easier and faster to use it. Sure, if rx->fc were to disappear, that > may make it more likely that these get changed ;-). Heh, ok, that's fine, I didn't realise that it was using the CPU order FC. > > Is that what the draft says? Because iterating the IEs would be > > different, this means you could potentially have something like a vendor > > IE last that encapsulates the MMIE including type/len fields, which > > should probably not be used? > > MMIE has to be the last "IE" in the frame. Sure, it would, in theory, be > possible to receive a frame that does not have MMIE, but has "MMIE like" > data in another IE. As long as we can make sure that this is reached > only for frames that are received from MFP enabled AP, the simple > solution of pointing to the end of the frame is enough. Otherwise, we > would need to parse all the IEs and know about the start of IEs in all > different types of Action frames.. That's not really something I would > like to do. Yeah, true, and we actually have that in another place too. If we then remove the MMIE, the IE sanity checks should catch the bad frame anyway, when/if it is parsed. Except we removed those because APs were sending bogus information. I'm fine with this, but we should be aware of the consequence. > > > + /* Remove MMIE */ > > > + skb_trim(skb, skb->len - sizeof(*mmie)); > > > > Is that actually necessary? Since it's an IE, it should be ignored by > > all other code, no? Not that it matters though. > > I don't think it is necessary in the sense that leaving the "IE" in > would break anything. However, I do think this is the correct thing to > do here and matches with what we do with TKIP/CCMP. MMIE is not really > an IE, it is just a frame component that is made to look like one. The > current IEEE 802.11w draft is bit confused about what MMIE is at places, > but anyway, I would compare it to TKIP ICV and CCMP MIC in the end of > the frames. Works for me. johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
