From: Alexander Wetzel <alexander@wetzel-home.de>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: linux-wireless@vger.kernel.org
Subject: Re: [RFC PATCH v3 03/12] mac80211: IEEE 802.11 Extended Key ID support
Date: Sat, 23 Feb 2019 22:02:50 +0100 [thread overview]
Message-ID: <440ae537-52d9-aa36-8b6b-17c2616fb4f9@wetzel-home.de> (raw)
In-Reply-To: <767ada740d984e180d0ac799487722293a50367c.camel@sipsolutions.net>
>>>>> + key->flags &= ~KEY_FLAG_RX_ONLY;
> [snip]
>>>>> + if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
>>>>> + ret = drv_set_key(local, EXT_KEY_RX_TX, sdata,
>>>>> + &sta->sta, &key->conf);
>>>>> + if (ret) {
>>>>> + sdata_err(sdata,
>>>>> + "failed to activate key for Tx (%d, %pM)\n",
>>>>> + key->conf.keyidx, sta->sta.addr);
>>>>> + return ret;
>>>>
>>>> You've already cleared the RX_ONLY flag, which gets you inconsistent
>>>> data now.
>>>>
>>>
>>> I don't think so, it looks ok for me. But the delay_tailroom logic took
>>> a surprisingly large chunk of time and I should explain how the updated
>>> logic is intended to work. Maybe I've messed it up somehow and just do
>>> not see it:
>
> My comment wasn't related to tailroom accounting at all. I've snipped
> more code above to hopefully make it clearer.
>
> If you fail to activate the key in the driver, then the key is not
> actually enabled for TX, and thus you should not have cleared the
> KEY_FLAG_RX_ONLY? You'll be returning this error all the way to
> userspace, I assume.
>
> (Maybe, btw, the driver should be allowed to return something like
> "oops, I now want to use TX software crypto" like it can do for other
> operations?)
Yes, an error here will get passed through to the userspace.
I think I mentioned already that tailroom update really got tricky with
Rx only keys and I'll also add some comments to help understanding better:-)
Here a more verbose version to verify the logic:
An error here will abort the key install and at the end that will
disconnect the sta. But don't forget that the key at that moment was
already installed successfully to the driver as a Rx only key and in
most cases the driver is already "ready" to use Tx with that key.
Mac80211 is just not handing over any MPDUs for that key.
A "normal" key install would already have increased the tailroom needed
count at that time, we just skipped that since a Rx only key never can
have use for a tailroom and it's now time to update the count if needed
for Tx.
If that call to the driver here fails mac80211 may have a different
understanding of the key RX/TX status than the driver for cards similar
to ath10k we do not care about that. While we still may be able to
transport some packets if the driver still can handle them with the old
Tx key the userspace will have to tear down at least the encryption and
probably the complete association to start over. The connection is
basically already dead and we only care about our mac80211 housekeeping,
so in that case our delay tailroom needed count.
Allowing SW crypto fallback makes no sense, there is no way mac80211 can
rescue the situation: Only drivers like ath10k handling the key
selection with PN assignment themselves should need this call, all
others just have to return 0. So for any driver needing this call
mac80211 can't fix whatever went wrong in the driver.
Here the comments I've now add to the code for the next revision of the
pacth: (The code itself is unchanged)
+ key->flags &= ~KEY_FLAG_RX_ONLY;
+
+ /* Dropping the KEY_FLAG_RX_ONLY flag forces us to update tailroom,
+ * do that first.
+ */
+ if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) ||
+ key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
+ IEEE80211_KEY_FLAG_PUT_MIC_SPACE |
+ IEEE80211_KEY_FLAG_RESERVE_TAILROOM))
+ increment_tailroom_need_count(sdata);
+
+ if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
+ ret = drv_set_key(local, EXT_KEY_RX_TX, sdata,
+ &sta->sta, &key->conf);
+ if (ret) {
+ /* This is a not recoverable error, so we don't set
+ * KEY_FLAG_RX_ONLY again or fix the tailroom needed
+ * count here. By skipping both
ieee80211_remove_key()
+ * will do that for us.
+ */
+ sdata_err(sdata,
+ "failed to activate key for Tx (%d,
%pM)\n",
+ key->conf.keyidx, sta->sta.addr);
+ return ret;
+ }
+ }
Alexander
next prev parent reply other threads:[~2019-02-23 21:12 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-10 21:06 [RFC PATCH v3 00/12] Draft for Extended Key ID support Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 01/12] mac80211: Optimize tailroom_needed update checks Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 02/12] nl80211/cfg80211: Extended Key ID support Alexander Wetzel
2019-02-15 10:52 ` Johannes Berg
2019-02-17 19:19 ` Alexander Wetzel
2019-02-22 8:30 ` Johannes Berg
2019-02-22 23:04 ` Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 03/12] mac80211: IEEE 802.11 " Alexander Wetzel
2019-02-15 11:06 ` Johannes Berg
2019-02-19 20:58 ` Alexander Wetzel
2019-02-21 19:47 ` Alexander Wetzel
2019-02-22 8:41 ` Johannes Berg
2019-02-23 21:02 ` Alexander Wetzel [this message]
2019-03-01 20:43 ` Alexander Wetzel
2019-02-23 17:26 ` Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 04/12] mac80211: Compatibility " Alexander Wetzel
2019-02-15 11:09 ` Johannes Berg
2019-02-21 20:07 ` Alexander Wetzel
2019-02-22 8:53 ` Johannes Berg
2019-02-23 22:50 ` Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 05/12] mac80211: Mark A-MPDU keyid borders for drivers Alexander Wetzel
2019-02-15 11:50 ` Johannes Berg
2019-02-21 21:20 ` Alexander Wetzel
2019-02-22 8:51 ` Johannes Berg
2019-02-23 21:47 ` Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 06/12] mac80211_hwsim: Ext Key ID support (NATIVE) Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 07/12] iwlwifi: Extended " Alexander Wetzel
2019-02-15 11:52 ` Johannes Berg
2019-02-22 20:50 ` Alexander Wetzel
2019-02-22 21:06 ` Johannes Berg
2019-02-24 13:04 ` Alexander Wetzel
2019-04-08 20:10 ` Johannes Berg
2019-04-10 20:46 ` Alexander Wetzel
2019-04-12 9:51 ` Johannes Berg
2019-04-14 16:12 ` Alexander Wetzel
2019-04-15 8:44 ` Johannes Berg
2019-04-15 20:09 ` Alexander Wetzel
2019-04-16 9:31 ` Johannes Berg
2019-04-16 18:28 ` Alexander Wetzel
2019-04-16 19:11 ` Johannes Berg
2019-04-16 21:32 ` Alexander Wetzel
2019-04-12 11:19 ` Johannes Berg
2019-02-10 21:06 ` [RFC PATCH v3 08/12] iwlwifi: dvm - EXT_KEY_ID A-MPDU API update Alexander Wetzel
2019-02-15 11:54 ` Johannes Berg
2019-02-22 21:15 ` Alexander Wetzel
2019-02-22 21:20 ` Johannes Berg
2019-02-10 21:06 ` [RFC PATCH v3 09/12] ath: Basic Extended Key ID support (COMPAT+NATIVE) Alexander Wetzel
2019-02-13 11:05 ` Kalle Valo
2019-02-13 23:15 ` Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 10/12] ath5k: ath_key_config() API compatibility update Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 11/12] ath9k: Extended Key ID support (COMPAT) Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 12/12] ath9k: EXT_KEY_ID A-MPDU API update Alexander Wetzel
2019-02-15 11:10 ` [RFC PATCH v3 00/12] Draft for Extended Key ID support Johannes Berg
2019-02-21 20:44 ` Alexander Wetzel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=440ae537-52d9-aa36-8b6b-17c2616fb4f9@wetzel-home.de \
--to=alexander@wetzel-home.de \
--cc=johannes@sipsolutions.net \
--cc=linux-wireless@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).