Re: [RFC PATCH v3 02/12] nl80211/cfg80211: Extended Key ID support

[Alexander Wetzel <alexander@wetzel-home.de>]

Am 22.02.19 um 09:30 schrieb Johannes Berg:

>> With Extended Key ID we stick to a similar approach: NL80211_CMD_NEW_KEY
>> is called to install the new key and nl80211_key_install_mode allows to
>> select between a "legacy" or "extended" key install: NL80211_KEY_RX_TX
>> for "legacy" or NL80211_KEY_RX_ONLY for "extended".
> 
> Ah. That's where the "EXT" came from in the mac80211 names :-) FWIW, I'm
> not sure that makes sense. Yes, we think of it as an "extension" or
> being "extended" now while we work on it, but ultimately it'll be a
> simple part of the API. But I digress.

"EXT" is only intended to be the short form for "Extended Key ID for 
Individually Addressed Frames" from IEEE 802.11 - 2016 and not as 
"extension for nl80211/mac80211".

>> NL80211_KEY_SWITCH_TX is only allowed with NL80211_CMD_SET_KEY and is
>> the only Extended Key ID action allowed for that function.
> 
> Yes, but what does it *do*? Your documentation said:
> 
> 	Switch Tx to a Rx only, referenced by sta mac and idx.
> 
> but that seems backwards to me based on the above requirement:
> 	Allow a key to be used for decryption first and switch it to a
> 	"normal" Rx/Tx key with a second call.
> 
Ah, now I see your point!

I'm viewing a Rx only key as something different, a new "key group" like 
the existing broadcast or unicast key groups. (Which btw. also why I 
have a problems to not have a key marked as such.)

"Switch TX" was intended to bring across, that the Tx status would 
switch from the "other" key to the referenced one. (That's also the 
reason I did not want to use ENABLE_TX.)

Or in other words:
Make a potential existing RX/TX key to a RX only key and the current Rx 
only key to the RX/Tx key. The key referenced by sta and mac must be 
flagged Rx only or the command will have no effect/fail. (The last 
requirement would vanish when we drop the Rx_Only key flag.)

> IOW, you said we need to have the ability to first install RX-only, and
> then switch the key to RX & TX. I'd have called this "ENABLE_TX" or
> something like that. Or perhaps SWITCH_(ON_)TX if you're so inclined,
> but the documentation should then say
> 
> 	Switch key from RX-only to TX/RX, referenced by ...
> 
> no? That's what I meant by "the other way around".

What do you think about:
	Switch key referenced by referenced by sta mac and idx from RX-
	only to RX/TX and make any other RX/TX key for the sta a RX-only
	key.

> 
>> Any non-pairwise keys can only use NL80211_KEY_RX_TX which is of course
>> always the default and also allowed for "legacy" pairwise keys.
> 
> Right.
> 
>> A Extended Key Install will:
>> 1) call NL80211_CMD_NEW_KEY with all the usual parameters of a new key
>> install plus the flag NL80211_KEY_RX_ONLY set.
> 
> So far makes sense.
> 
>> 2) Once ready will call NL80211_CMD_SET_KEY with flag
>> NL80211_KEY_SWITCH_TX to activate a specific key.
> 
> Also makes sense, but the documentation doesn't.
> 
> I think we should rename these perhaps
> 
> 	NL80211_KEY_RX_TX - install key and enable RX/TX (default)
> 	NL80211_KEY_RX_ONLY - install unicast key for RX only
> 	NL80211_KEY_ENABLE_TX - enable TX for a previously installed
> 				RX_ONLY key
> 
> I think?
> 
> About the ENABLE_TX vs. SWITCH_TX - we don't allow to switch TX *off*
> again, only *on*, so I think "enable" makes more sense than "switch".
>
See above. NL80211_KEY_ENABLE_TX would also implicit disable Tx for the 
"other" pairwise key (if present). Mac80211 e.g. evaluates delay 
tailromm for both keys again and always reduces it for the Rx only key 
when it was set and even sets the RX_only flag again.

That said I'm happy with any of these:
	NL80211_KEY_ENABLE_TX
	NL80211_KEY_SWITCH_TX
	NL80211_KEY_MOVE_TX

After that discussion I'm now wondering if we maybe should change the 
naming more fundamentally, similar to what I proposed for mac80211:

	NL80211_KEY_SET - install key and enable RX/TX (default)
	NL80211_KEY_EXT_SET add unicast key for Extended Key ID
	NL80211_KEY_EXT_ENABLE - Set the key referenced by
		sta mac the to the preferred TX key for sta

Here it would be nice to be able to use "NL80211_EXT_KEY_", but that 
would break the naming schema quite badly. And using 
NL80211_KEY_EXT_KEY_ looks also confusing...

Any preferences or other options? I think you have now all details about 
that and whatever looks best for you is it now.

> Anyway, my whole comment was only about the documentation text which
> seemed backward or at least unclear to me.
>
I did not see that when coming from understanding the code to writing 
comments/documentation. Chances are other readers will interpret that 
like  you did and not as intended... And the area is complex enough 
without those misunderstandings.

>> ok, I'll remove all the drive-by comment "cleanups".
>> It (often) looks kind of untidy and not really worth separate patches
>> and I'm kind of responsible for (most) of them.. I see why you don't
>> want to see those fixed drive-by...
>>
>> My understanding is now that we prefer to not change those and I'll
>> leave them alone.
> 
> I have no objection to even the most trivial cleanup patches going in
> separately, and if you like multiple in a bigger "clean up this area"
> patch, but here I think it detracts from the patch.
> 
I understand.
Honestly I would feel silly to put most of the drive-by changes into a 
dedicated patch and not worth the time for a review.
So I guess I'll only do separate patches if I really care about the 
format/comment in future:-)


Alexander