Aditya Kumar Singh <quic_adisi@xxxxxxxxxxx> writes:
> On 10/29/24 21:08, Kalle Valo wrote:
>> + aditya
>> Jeff Johnson <quic_jjohnson@xxxxxxxxxxx> writes:
>>
>>>> +static int ath12k_mac_station_unauthorize(struct ath12k *ar,
>>>> + struct ath12k_link_vif *arvif,
>>>> + struct ath12k_link_sta *arsta)
>>>> +{
>>>> + struct ath12k_peer *peer;
>>>> + int ret;
>>>> +
>>>> + lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
>>>> +
>>>> + spin_lock_bh(&ar->ab->base_lock);
>>>> +
>>>> + peer = ath12k_peer_find(ar->ab, arvif->vdev_id, arsta->addr);
>>>> + if (peer)
>>>> + peer->is_authorized = false;
>>>> +
>>>> + spin_unlock_bh(&ar->ab->base_lock);
>>>> +
>>>> + /* Driver should clear the peer keys during mac80211's ref ptr
>>>> + * gets cleared in __sta_info_destroy_part2 (trans from
>>>> + * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC)
>>>
>>> I'm unable to understand this comment
>> Indeed, that's weird. Aditya, do you have any idea what the comment
>> is
>> trying to say?
>>
>
> At present, ath12k clear the keys in ath12k_station_disassoc() which
> gets executed in state change from IEEE80211_STA_ASSOC to
> IEEE80211_STA_AUTH.
>
> However, in mac80211, once the station moves from
> IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC itself, the keys are
> deleted. Please see - __sta_info_destroy_part2() ->
> ieee80211_free_sta_keys().
>
> Now, ath12k peer object (struct ath12k_peer) holds the key reference
> from mac80211 (see ath12k_peer::keys[]). Hence, once mac80211 deletes
> the key, driver should not keep a reference to it or else it could
> lead to issues.
>
> Therefore, it is important that the driver should clear the peer keys
> during transition from IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC
> it self since we know that once we return from here, mac80211 is going
> to remove the keys.
>
> ath12k_mac_station_unauthorize() gets called when station moves from
> state IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC hence call to
> ath12k_clear_peer_keys() is moved from ath12k_station_disassoc() to
> ath12k_mac_station_unauthorize().
>
> Is this clear now?
Super clear :) Thanks!
> May be the comment in the code could be re-written as below?
>
> /* Driver must clear the keys during the state change from
> * IEEE80211_STA_AUTHORIZED to IEEE80211_STA_ASSOC, since after
> * returning from here, mac80211 is going to delete the keys
> * in __sta_info_destroy_part2(). This will ensure that the driver does
> * not retain stale key references after mac80211 deletes the keys.
> */
Looks good to me, I'll add that if it's ok for Jeff as well.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
