Subject: Re: [RFC PATCH v3 03/12] mac80211: IEEE 802.11 Extended Key ID
 support
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org
References: <20190210210620.31181-1-alexander@wetzel-home.de>
 <20190210210620.31181-4-alexander@wetzel-home.de>
 <f4d8db23bc3804fdc98f2ce7a702215bd82fd4e3.camel@sipsolutions.net>
 <e27bffe8-4c75-34d9-b326-39ffb05b73eb@wetzel-home.de>
 <171d2db4-7639-4738-2a6d-c899f0247aee@wetzel-home.de>
 <767ada740d984e180d0ac799487722293a50367c.camel@sipsolutions.net>
From:   Alexander Wetzel <alexander@wetzel-home.de>
Message-ID: <440ae537-52d9-aa36-8b6b-17c2616fb4f9@wetzel-home.de>
Date:   Sat, 23 Feb 2019 22:02:50 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.1
MIME-Version: 1.0
In-Reply-To: <767ada740d984e180d0ac799487722293a50367c.camel@sipsolutions.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk


>>>>> +    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