Calling only __ieee80211_select_queue() for control port TX exposes
drivers which do not support QoS to non-zero values in
skb->queue_mapping and even can assign not available queues to
info->hw_queue.
This can cause issues for drivers like we did e.g. see in
'746285cf81dc ("rtl818x: Prevent using not initialized queues")'.
This also prevents a redundant call to __ieee80211_select_queue() when
using control port TX with iTXQ (pull path).
And it starts to prioritize 802.11 preauthentication frames
(ETH_P_PREAUTH) on all TX paths.
Pierre Asselin confirmed that this patch indeed prevents crashing his
system without '746285cf81dc ("rtl818x: Prevent using not initialized
queues")'.
Tested-by: Pierre Asselin <[email protected]>
Signed-off-by: Alexander Wetzel <[email protected]>
---
Starting to prioritize ETH_P_PREAUTH was just added since I noticed that
contradictory to at least my expectations control port does accept
ETH_P_PREAUTH but handles these like a normal frame for the priority.
That can be broken out or even drop, when needed.
While looking at the code I also tripped over multiple other questions
and I'll probably propose a much more invasive change how to handle
the queue assignment. (End2end we seem to do some quite stupid things.)
Additionally I really don't get why we call skb_get_hash() on queue
assignment:
I found the commit '180ac48ee62f ("mac80211: calculate skb hash early
when using itxq")' but don't see why calculating the hash early is
useful. Any hints here are appreciated. fq_flow_idx() seems to do that
when needed and I can't find any other usage of the hash...
net/mac80211/tx.c | 18 +++---------------
net/mac80211/wme.c | 3 ++-
2 files changed, 5 insertions(+), 16 deletions(-)
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 0e4efc08c762..2fabf6c4547c 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -5727,7 +5727,6 @@ int ieee80211_tx_control_port(struct wiphy *wiphy, struct net_device *dev,
{
struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
struct ieee80211_local *local = sdata->local;
- struct sta_info *sta;
struct sk_buff *skb;
struct ethhdr *ehdr;
u32 ctrl_flags = 0;
@@ -5771,20 +5770,9 @@ int ieee80211_tx_control_port(struct wiphy *wiphy, struct net_device *dev,
skb_reset_network_header(skb);
skb_reset_mac_header(skb);
- /* update QoS header to prioritize control port frames if possible,
- * priorization also happens for control port frames send over
- * AF_PACKET
- */
- rcu_read_lock();
-
- if (ieee80211_lookup_ra_sta(sdata, skb, &sta) == 0 && !IS_ERR(sta)) {
- u16 queue = __ieee80211_select_queue(sdata, sta, skb);
-
- skb_set_queue_mapping(skb, queue);
- skb_get_hash(skb);
- }
-
- rcu_read_unlock();
+ /* skb bypassed queue selection in net/core/dev.c, do it now */
+ skb_set_queue_mapping(skb, ieee80211_select_queue(sdata, skb));
+ skb_get_hash(skb);
/* mutex lock is only needed for incrementing the cookie counter */
mutex_lock(&local->mtx);
diff --git a/net/mac80211/wme.c b/net/mac80211/wme.c
index 62c6733e0792..774afefbe0b0 100644
--- a/net/mac80211/wme.c
+++ b/net/mac80211/wme.c
@@ -160,7 +160,8 @@ u16 __ieee80211_select_queue(struct ieee80211_sub_if_data *sdata,
return IEEE80211_AC_BE;
}
- if (skb->protocol == sdata->control_port_protocol) {
+ if (skb->protocol == sdata->control_port_protocol ||
+ skb->protocol == cpu_to_be16(ETH_P_PREAUTH)) {
skb->priority = 7;
goto downgrade;
}
--
2.35.1
Felix Fietkau <[email protected]> writes:
> On 07.05.22 13:26, Toke Høiland-Jørgensen wrote:
>> Alexander Wetzel <[email protected]> writes:
>>
>>> Calling only __ieee80211_select_queue() for control port TX exposes
>>> drivers which do not support QoS to non-zero values in
>>> skb->queue_mapping and even can assign not available queues to
>>> info->hw_queue.
>>> This can cause issues for drivers like we did e.g. see in
>>> '746285cf81dc ("rtl818x: Prevent using not initialized queues")'.
>>>
>>> This also prevents a redundant call to __ieee80211_select_queue() when
>>> using control port TX with iTXQ (pull path).
>>> And it starts to prioritize 802.11 preauthentication frames
>>> (ETH_P_PREAUTH) on all TX paths.
>>>
>>> Pierre Asselin confirmed that this patch indeed prevents crashing his
>>> system without '746285cf81dc ("rtl818x: Prevent using not initialized
>>> queues")'.
>>>
>>> Tested-by: Pierre Asselin <[email protected]>
>>> Signed-off-by: Alexander Wetzel <[email protected]>
>>> ---
>>>
>>> Starting to prioritize ETH_P_PREAUTH was just added since I noticed that
>>> contradictory to at least my expectations control port does accept
>>> ETH_P_PREAUTH but handles these like a normal frame for the priority.
>>> That can be broken out or even drop, when needed.
>>>
>>> While looking at the code I also tripped over multiple other questions
>>> and I'll probably propose a much more invasive change how to handle
>>> the queue assignment. (End2end we seem to do some quite stupid things.)
>>>
>>> Additionally I really don't get why we call skb_get_hash() on queue
>>> assignment:
>>> I found the commit '180ac48ee62f ("mac80211: calculate skb hash early
>>> when using itxq")' but don't see why calculating the hash early is
>>> useful. Any hints here are appreciated. fq_flow_idx() seems to do that
>>> when needed and I can't find any other usage of the hash...
>>
>> The commit message of that commit has a hint:
>>
>> This avoids flow separation issues when using software encryption.
>>
>> The idea being that the packet contents can change on encryption, but
>> skb->hash is preserved, so you want it to run before encryption happens
>> to keep flows in the same queue.
>>
>> However, AFAICT ieee80211_tx_h_encrypt() is called after frames are
>> dequeued from the TXQs, so not actually sure this is needed. Adding
>> Felix, in the hope that he can explain the reasoning behind that
>> commit :)
> Sorry, I don't remember the details on that one. One advantage that I
> can think of (which isn't mentioned in the commit msg) is that it is
> likely better for performance to calculate the hash early since there
> is a good chance that more of the skb data is still in the CPU cache.
Right, that could be, I suppose. I don't think it'll hurt, at least;
Alexander, did you have any particular reason for wanting to get rid of
it?
-Toke
Alexander Wetzel <[email protected]> writes:
> Calling only __ieee80211_select_queue() for control port TX exposes
> drivers which do not support QoS to non-zero values in
> skb->queue_mapping and even can assign not available queues to
> info->hw_queue.
> This can cause issues for drivers like we did e.g. see in
> '746285cf81dc ("rtl818x: Prevent using not initialized queues")'.
>
> This also prevents a redundant call to __ieee80211_select_queue() when
> using control port TX with iTXQ (pull path).
> And it starts to prioritize 802.11 preauthentication frames
> (ETH_P_PREAUTH) on all TX paths.
>
> Pierre Asselin confirmed that this patch indeed prevents crashing his
> system without '746285cf81dc ("rtl818x: Prevent using not initialized
> queues")'.
>
> Tested-by: Pierre Asselin <[email protected]>
> Signed-off-by: Alexander Wetzel <[email protected]>
> ---
>
> Starting to prioritize ETH_P_PREAUTH was just added since I noticed that
> contradictory to at least my expectations control port does accept
> ETH_P_PREAUTH but handles these like a normal frame for the priority.
> That can be broken out or even drop, when needed.
>
> While looking at the code I also tripped over multiple other questions
> and I'll probably propose a much more invasive change how to handle
> the queue assignment. (End2end we seem to do some quite stupid things.)
>
> Additionally I really don't get why we call skb_get_hash() on queue
> assignment:
> I found the commit '180ac48ee62f ("mac80211: calculate skb hash early
> when using itxq")' but don't see why calculating the hash early is
> useful. Any hints here are appreciated. fq_flow_idx() seems to do that
> when needed and I can't find any other usage of the hash...
The commit message of that commit has a hint:
This avoids flow separation issues when using software encryption.
The idea being that the packet contents can change on encryption, but
skb->hash is preserved, so you want it to run before encryption happens
to keep flows in the same queue.
However, AFAICT ieee80211_tx_h_encrypt() is called after frames are
dequeued from the TXQs, so not actually sure this is needed. Adding
Felix, in the hope that he can explain the reasoning behind that commit :)
-Toke
>>>> Additionally I really don't get why we call skb_get_hash() on queue
>>>> assignment:
>>>> I found the commit '180ac48ee62f ("mac80211: calculate skb hash early
>>>> when using itxq")' but don't see why calculating the hash early is
>>>> useful. Any hints here are appreciated. fq_flow_idx() seems to do that
>>>> when needed and I can't find any other usage of the hash...
>>>
>>> The commit message of that commit has a hint:
>>>
>>> This avoids flow separation issues when using software encryption.
>>>
>>> The idea being that the packet contents can change on encryption, but
>>> skb->hash is preserved, so you want it to run before encryption happens
>>> to keep flows in the same queue.
>>>
>>> However, AFAICT ieee80211_tx_h_encrypt() is called after frames are
>>> dequeued from the TXQs, so not actually sure this is needed. Adding
>>> Felix, in the hope that he can explain the reasoning behind that
>>> commit :)
>> Sorry, I don't remember the details on that one. One advantage that I
>> can think of (which isn't mentioned in the commit msg) is that it is
>> likely better for performance to calculate the hash early since there
>> is a good chance that more of the skb data is still in the CPU cache.
>
> Right, that could be, I suppose. I don't think it'll hurt, at least;
> Alexander, did you have any particular reason for wanting to get rid of
> it?
No, not really. I just do not want to move code around I do not understand.
I'm looking into how mac80211 assigns the queues and ended up with the
impression, that this could be simplified.
Now I'm pretty sure that the distinction between iTXQ and other drivers
for queue selection is (nowadays?) pointless. (I'll argue the case
hopefully soon in another patch.)
My problem was only, how to handle the calls to skb_get_hash() in my
upcoming patch:
I could not figure out how this call helps to "avoids flow separation
issues when using software encryption", indicating that I still may have
a critical knowledge gap.
With the understanding that we try to get the hash calculated while the
skb may still be in the CPU buffers that's sorted out.
In fact I've now a first draft for the "queue simplification patch" and
will share that here after testing it with a card which actually
supports wake_tx_queue().
Alexander
On 07.05.22 13:26, Toke Høiland-Jørgensen wrote:
> Alexander Wetzel <[email protected]> writes:
>
>> Calling only __ieee80211_select_queue() for control port TX exposes
>> drivers which do not support QoS to non-zero values in
>> skb->queue_mapping and even can assign not available queues to
>> info->hw_queue.
>> This can cause issues for drivers like we did e.g. see in
>> '746285cf81dc ("rtl818x: Prevent using not initialized queues")'.
>>
>> This also prevents a redundant call to __ieee80211_select_queue() when
>> using control port TX with iTXQ (pull path).
>> And it starts to prioritize 802.11 preauthentication frames
>> (ETH_P_PREAUTH) on all TX paths.
>>
>> Pierre Asselin confirmed that this patch indeed prevents crashing his
>> system without '746285cf81dc ("rtl818x: Prevent using not initialized
>> queues")'.
>>
>> Tested-by: Pierre Asselin <[email protected]>
>> Signed-off-by: Alexander Wetzel <[email protected]>
>> ---
>>
>> Starting to prioritize ETH_P_PREAUTH was just added since I noticed that
>> contradictory to at least my expectations control port does accept
>> ETH_P_PREAUTH but handles these like a normal frame for the priority.
>> That can be broken out or even drop, when needed.
>>
>> While looking at the code I also tripped over multiple other questions
>> and I'll probably propose a much more invasive change how to handle
>> the queue assignment. (End2end we seem to do some quite stupid things.)
>>
>> Additionally I really don't get why we call skb_get_hash() on queue
>> assignment:
>> I found the commit '180ac48ee62f ("mac80211: calculate skb hash early
>> when using itxq")' but don't see why calculating the hash early is
>> useful. Any hints here are appreciated. fq_flow_idx() seems to do that
>> when needed and I can't find any other usage of the hash...
>
> The commit message of that commit has a hint:
>
> This avoids flow separation issues when using software encryption.
>
> The idea being that the packet contents can change on encryption, but
> skb->hash is preserved, so you want it to run before encryption happens
> to keep flows in the same queue.
>
> However, AFAICT ieee80211_tx_h_encrypt() is called after frames are
> dequeued from the TXQs, so not actually sure this is needed. Adding
> Felix, in the hope that he can explain the reasoning behind that commit :)Sorry, I don't remember the details on that one. One advantage that I
can think of (which isn't mentioned in the commit msg) is that it is
likely better for performance to calculate the hash early since there is
a good chance that more of the skb data is still in the CPU cache.
- Felix