Am 15.02.19 um 12:50 schrieb Johannes Berg:
The intent of the wording was probably written without considering
Extended Key IDs. At least it makes no sense for me to forbid mixing
MPDUs using keyid 0 and 1 in one A-MPDU.
I think it may make sense - reprogramming the hardware engines may take
some time, and doing that in the middle of the A-MPDU may not be
feasible? You don't just have to load the key (that you need to do
anyway) but also extract the status? I dunno, I'm more handwaving, but
it doesn't make sense to add such a requirement when only one key index
can be used to start with.
I'm pretty new to all that and I know I have still huge gaps everywhere.
But the card must be able to process MPDUs using both KeyIDs at that
moment already. When receiving a A-MPDU it should not be very hard to
check each MPDU and process it accordingly. (TX should even be simpler:
The driver simply can decide if he want's to have a key border in the
A-MPDU or not and switch to key when it wants.)
Forbidding to mix unicast with both keyIDs is adding an ugly overhead to
an otherwise quite simple solution and adds complexity, at least for us
here.
So while I would like to understand the reason for that rule it's still
a rule and breaking it seems to be a bad idea.
The code is assuming that the driver is not aggregating MPDUs more than
5s apart. We probably don't have wait nearly so long but I'm not sure
what is the minimum time.
OTOH, if you have a lot of BE/VI/VO traffic BK might be starved even
longer than that, technically indefinitely.
Hm, there is nothing preventing us to drop this "idle" switch as long as
we also drop the 10s Rx accel offload when idle fallback in the COMPAT
patch. (We have to drop the RX idle accel patch or get a small risk of
dropping packets in unlikely but possible scenarios. If that are eapol
packets things will become hairy, probably disconnecting the sta.)
It just feels strange to potentially still use the old key for one
packet more without time limit. It could be, that the first EAPOL packet
we send for the next rekey would still use the previous key, the second
eapol packet the current.
+static struct ieee80211_key debug_noinline
+*ieee80211_select_sta_key(struct ieee80211_tx_data *tx)
+{
+ struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
+ struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
+ struct sta_info *sta = tx->sta;
+ struct ieee80211_key *key;
+ struct ieee80211_key *next_key;
+
+ key = rcu_dereference(tx->sta->ptk[tx->sta->ptk_idx]);
+
+ if (likely(sta->ptk_idx_next == INVALID_PTK_KEYIDX))
+ return key;
+
+ /* Only when using Extended Key ID the code below can be executed */
+
+ if (!ieee80211_is_data_present(hdr->frame_control))
+ return key;
+
+ if (sta->ptk_idx_next == sta->ptk_idx) {
+ /* First packet using new key with A-MPDU active*/
+ sta->ptk_idx_next = INVALID_PTK_KEYIDX;
+ ieee80211_check_fast_xmit(tx->sta);
I'm not convinced you can call this from this context? It looks safe
though, but it's really strange in a way.
Well, it's seems to work fine, no warnings or problems so far:-)
But I also had doubts and only after finding out that
ieee80211_check_fast_xmit() is already being called from the same
context I dared to use it, see ieee80211_tx_prepare().
+ info->flags &= ~IEEE80211_TX_CTL_AMPDU;
Like you say above, I don't think this really makes a lot of sense. If
we don't have any free bits I guess we should try to find some ...
Well, all I can think of is quite invasive, so I hoped you would have a
better idea:
Move the flags out from CB and use the freed space for a pointer to the
new construct.
That will touch a ton of code and slow down things a bit.
The question here also would be, if we should use a struct able to
handle other extensions or just a long long for flags.
If it comes to that I would propose to merge the other patches up to
this one and then start looking into that...
Alexander
