+ 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
