All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexander Wetzel <alexander@wetzel-home.de>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: linux-wireless@vger.kernel.org,
	Alexander Wetzel <alexander@wetzel-home.de>,
	Pierre Asselin <pa@panix.com>
Subject: [PATCH] mac80211: Use full queue selection code for control port tx
Date: Sat,  7 May 2022 10:37:06 +0200	[thread overview]
Message-ID: <20220507083706.384513-1-alexander@wetzel-home.de> (raw)

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 <pa@panix.com>
Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
---

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


             reply	other threads:[~2022-05-07  8:37 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-07  8:37 Alexander Wetzel [this message]
2022-05-07 11:26 ` [PATCH] mac80211: Use full queue selection code for control port tx Toke Høiland-Jørgensen
2022-05-08  5:44   ` Felix Fietkau
2022-05-08 19:10     ` Toke Høiland-Jørgensen
2022-05-08 21:29       ` 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=20220507083706.384513-1-alexander@wetzel-home.de \
    --to=alexander@wetzel-home.de \
    --cc=johannes@sipsolutions.net \
    --cc=linux-wireless@vger.kernel.org \
    --cc=pa@panix.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.