On Wednesday 23 June 2021 14:16:12 Johannes Berg wrote:

On Fri, 2021-06-11 at 12:10 +0200, Pali Rohár wrote:

quoted

@@ -341,8 +341,11 @@ static void ieee80211_key_replace(struct ieee80211_sub_if_data *sdata,

 	if (sta) {
 		if (pairwise) {
 			rcu_assign_pointer(sta->ptk[idx], new);
-			sta->ptk_idx = idx;
-			ieee80211_check_fast_xmit(sta);
+			if (new) {
+				set_sta_flag(new->sta, WLAN_STA_USES_ENCRYPTION);
+				new->sta->ptk_idx = new->conf.keyidx;

I'm not entirely sure moving that assignment under the guard is correct.

quoted

+				ieee80211_check_fast_xmit(new->sta);

and I'm pretty sure that moving call under the guard is incorrect,
although in the end it probably doesn't even matter if we will drop all
frames anyway (due to this patch).

So all you need under the assignment is the flag, but also only
theoretically, because the function cannot be called with old==NULL &&
new==NULL, the first time around it's called we must have old==NULL (no
key was ever installed), and so the first time it's called it must be
old==NULL && new!=NULL, and then the flag gets set and we never want to
clear it again, so I believe you don't need the "if (new)" condition at
all.

In the code as it was in (and before) my patch the condition is
necessary because we use 'new' to obtain the 'sta' and 'local' pointers,
but otherwise we don't really need it even in the current version.

johannes

Now I see, thank you for explanation. So the code should be like this:

 		if (pairwise) {
 			rcu_assign_pointer(sta->ptk[idx], new);
+			set_sta_flag(sta, WLAN_STA_USES_ENCRYPTION);
 			sta->ptk_idx = idx;
 			ieee80211_check_fast_xmit(sta);
 		} else {

Right?