Re: [RFC PATCH v2 2/2] mac80211: Add support for Extended Key ID

[Johannes Berg <johannes@sipsolutions.net>]

> + * @IEEE80211_KEY_FLAG_RX_ONLY: Set by mac80211 to indicate that the key
> + *      must not be used for TX (yet).

I'm not sure that's relevant, since you have one key pointer for TX?

> + * @IEEE80211_KEY_FLAG_SET_TX: Set by mac80211 to indicate that a previously
> + *      installed key with IEEE80211_KEY_FLAG_RX_ONLY should take over TX also.

That also doesn't seem relevant ...

Oh, all of this is for HW offloads?

I _think_ I would prefer to have new key ops instead. Now you'd have 

SET_KEY / <empty flags>
SET_KEY / RX_ONLY
SET_KEY / SET_TX

but I think maybe

SET_KEY
SET_KEY_RX_ONLY
KEY_ENABLE_TX

would make more sense?

> +	if (pairwise && params->flag == NL80211_KEY_SET_TX) {
> +		mutex_lock(&local->sta_mtx);
> +		sta = sta_info_get_bss(sdata, mac_addr);
> +
> +		if (!sta ||
> +		   !(key = rcu_dereference(sta->ptk[key_idx])) ||

indentation here is off by one

> +		   !(key->conf.flags | IEEE80211_KEY_FLAG_RX_ONLY)) {

that makes no sense, should be & I guess

> -	/* PTK only using key ID 0 needs special handling on rekey */
> -	if (new_key && sta && ptk0rekey) {
> +	/* PTK rekey without Extended Key ID needs special handling */
> +	if (new_key && pairwise && sta &&
> +	    !test_sta_flag(sta, WLAN_STA_EXT_KEY_ID)) {
>  		local = old_key->local;
>  		sdata = old_key->sdata;

This seems wrong, even if you have ext key ID support and everything,
but you do 0 -> 0 rekeying, then you still need all the special handling
(in fact also then if you go 1->1!). So it seems you'd instead want to
see if you're going from a TX key to a TX key with the same key ID, and
then you don't need this flag at all.

> +++ b/net/mac80211/sta_info.c
> @@ -350,6 +350,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
>  	sta->sta.max_rx_aggregation_subframes =
>  		local->hw.max_rx_aggregation_subframes;
>  
> +	sta->ptk_idx = NUM_DEFAULT_KEYS - 1;

That makes no sense? Why should it be 3? That's invalid anyway?

johannes