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
