[PATCH] rtl8180: Prevent using not initialized queues

[PATCH] rtl8180: Prevent using not initialized queues

[Alexander Wetzel]

Using not existing queues can panic the kernel with rtl8180/rtl8185
cards. Ignore the skb priority for those cards, they only have one
tx queue.

Cc: stable@vger.kernel.org
Reported-by: pa@panix.com
Tested-by: pa@panix.com
Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
---

Pierre Asselin (pa@panix.com) reported a kernel crash in the Gentoo forum:
https://forums.gentoo.org/viewtopic-t-1147832-postdays-0-postorder-asc-start-25.html
He also confirmed that this patch fixes the issue.

In summary this happened:
After updating wpa_supplicant from 2.9 to 2.10 the kernel crashed with a
"divide error: 0000" when connecting to an AP.
Control port tx now tries to use IEEE80211_AC_VO for the priority, which
wpa_supplicants starts to use in 2.10.

Since only the rtl8187se part of the driver supports QoS, the priority
of the skb is set to IEEE80211_AC_BE (2) by mac80211 for rtl8180/rtl8185
cards.

rtl8180 is then unconditionally reading out the priority and finally crashes on
drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c line 544 without this
patch:
	idx = (ring->idx + skb_queue_len(&ring->queue)) % ring->entries

"ring->entries" is zero for rtl8180/rtl8185 cards, tx_ring[2] never got
initialized.

 drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c b/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c
index 2477e18c7cae..025619cd14e8 100644
--- a/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c
+++ b/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c
@@ -460,8 +460,10 @@ static void rtl8180_tx(struct ieee80211_hw *dev,
 	struct rtl8180_priv *priv = dev->priv;
 	struct rtl8180_tx_ring *ring;
 	struct rtl8180_tx_desc *entry;
+	unsigned int prio = 0;
 	unsigned long flags;
-	unsigned int idx, prio, hw_prio;
+	unsigned int idx, hw_prio;
+
 	dma_addr_t mapping;
 	u32 tx_flags;
 	u8 rc_flags;
@@ -470,7 +472,9 @@ static void rtl8180_tx(struct ieee80211_hw *dev,
 	/* do arithmetic and then convert to le16 */
 	u16 frame_duration = 0;
 
-	prio = skb_get_queue_mapping(skb);
+	/* rtl8180/rtl8185 only has one useable tx queue */
+	if (dev->queues > IEEE80211_AC_BK)
+		prio = skb_get_queue_mapping(skb);
 	ring = &priv->tx_ring[prio];
 
 	mapping = dma_map_single(&priv->pdev->dev, skb->data, skb->len,
-- 
2.35.1

Re: [PATCH] rtl8180: Prevent using not initialized queues

[Kalle Valo]

Alexander Wetzel <alexander@wetzel-home.de> writes:

> Using not existing queues can panic the kernel with rtl8180/rtl8185
> cards. Ignore the skb priority for those cards, they only have one
> tx queue.
>
> Cc: stable@vger.kernel.org
> Reported-by: pa@panix.com
> Tested-by: pa@panix.com
> Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
> ---
>
> Pierre Asselin (pa@panix.com) reported a kernel crash in the Gentoo forum:
> https://forums.gentoo.org/viewtopic-t-1147832-postdays-0-postorder-asc-start-25.html
> He also confirmed that this patch fixes the issue.
>
> In summary this happened:
> After updating wpa_supplicant from 2.9 to 2.10 the kernel crashed with a
> "divide error: 0000" when connecting to an AP.
> Control port tx now tries to use IEEE80211_AC_VO for the priority, which
> wpa_supplicants starts to use in 2.10.
>
> Since only the rtl8187se part of the driver supports QoS, the priority
> of the skb is set to IEEE80211_AC_BE (2) by mac80211 for rtl8180/rtl8185
> cards.
>
> rtl8180 is then unconditionally reading out the priority and finally crashes on
> drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c line 544 without this
> patch:
> 	idx = (ring->idx + skb_queue_len(&ring->queue)) % ring->entries
>
> "ring->entries" is zero for rtl8180/rtl8185 cards, tx_ring[2] never got
> initialized.

All this after "---" line is very useful information but the actual
commit log is just two sentences. I would copy all to the commit log.
We don't need to limit the size of the commit log, on the contrary we
should include all the information in it.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] rtl8180: Prevent using not initialized queues

[Alexander Wetzel]

On 23.04.22 08:21, Kalle Valo wrote:
> Alexander Wetzel <alexander@wetzel-home.de> writes:
> 
>> Using not existing queues can panic the kernel with rtl8180/rtl8185
>> cards. Ignore the skb priority for those cards, they only have one
>> tx queue.
>>
>> Cc: stable@vger.kernel.org
>> Reported-by: pa@panix.com
>> Tested-by: pa@panix.com
>> Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
>> ---
>>
>> Pierre Asselin (pa@panix.com) reported a kernel crash in the Gentoo forum:
>> https://forums.gentoo.org/viewtopic-t-1147832-postdays-0-postorder-asc-start-25.html
>> He also confirmed that this patch fixes the issue.
>>
>> In summary this happened:
>> After updating wpa_supplicant from 2.9 to 2.10 the kernel crashed with a
>> "divide error: 0000" when connecting to an AP.
>> Control port tx now tries to use IEEE80211_AC_VO for the priority, which
>> wpa_supplicants starts to use in 2.10.
>>
>> Since only the rtl8187se part of the driver supports QoS, the priority
>> of the skb is set to IEEE80211_AC_BE (2) by mac80211 for rtl8180/rtl8185
>> cards.
>>
>> rtl8180 is then unconditionally reading out the priority and finally crashes on
>> drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c line 544 without this
>> patch:
>> 	idx = (ring->idx + skb_queue_len(&ring->queue)) % ring->entries
>>
>> "ring->entries" is zero for rtl8180/rtl8185 cards, tx_ring[2] never got
>> initialized.
> 
> All this after "---" line is very useful information but the actual
> commit log is just two sentences. I would copy all to the commit log.
> We don't need to limit the size of the commit log, on the contrary we
> should include all the information in it.
> 

I see what you mean, fine for me.
If you prefer I can also make an update but feel to handle that at your 
convenience. If you e.g. see a better way to do that drop the patch and 
simply submit your version.

While I spent some time figuring out how QoS is intended to work and I'm 
pretty sure I finally got the outline it I'm still wondering why we 
never set the priority for skb's on the normal transmit path.

Obviously the idea is to keep the queue from whoever set it prior to us 
and just overwriting it with good reason.

I plan to look a bit more into that, especially since Pierre's system 
was working when wpa_supplicant is not using control Port. Thus 
skb_get_queue_mapping() must return zero - or max one - on that path. 
That only makes sense when the network subsystem knows that QoS is not 
supported and is not bothering to set the queue. (Or if we would map 
zero to IEEE80211_AC_BE, but we are not handling it that way)

It basically drills down to the fact that we only call 
_ieee80211_select_queue() on the normal tx path for drivers supporting 
wake_tx_queue. I would have expected that call to be done for all 
drivers. (Or at least all drivers supporting QoS.)

So there is either a strange bug or - so far more likely - some serious 
gap in my still evolving understanding of QoS.

Re: [PATCH] rtl8180: Prevent using not initialized queues

[Kalle Valo]

Alexander Wetzel <alexander@wetzel-home.de> writes:

> On 23.04.22 08:21, Kalle Valo wrote:
>> Alexander Wetzel <alexander@wetzel-home.de> writes:
>>
>>> Using not existing queues can panic the kernel with rtl8180/rtl8185
>>> cards. Ignore the skb priority for those cards, they only have one
>>> tx queue.
>>>
>>> Cc: stable@vger.kernel.org
>>> Reported-by: pa@panix.com
>>> Tested-by: pa@panix.com
>>> Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
>>> ---
>>>
>>> Pierre Asselin (pa@panix.com) reported a kernel crash in the Gentoo forum:
>>> https://forums.gentoo.org/viewtopic-t-1147832-postdays-0-postorder-asc-start-25.html
>>> He also confirmed that this patch fixes the issue.
>>>
>>> In summary this happened:
>>> After updating wpa_supplicant from 2.9 to 2.10 the kernel crashed with a
>>> "divide error: 0000" when connecting to an AP.
>>> Control port tx now tries to use IEEE80211_AC_VO for the priority, which
>>> wpa_supplicants starts to use in 2.10.
>>>
>>> Since only the rtl8187se part of the driver supports QoS, the priority
>>> of the skb is set to IEEE80211_AC_BE (2) by mac80211 for rtl8180/rtl8185
>>> cards.
>>>
>>> rtl8180 is then unconditionally reading out the priority and finally crashes on
>>> drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c line 544 without this
>>> patch:
>>> 	idx = (ring->idx + skb_queue_len(&ring->queue)) % ring->entries
>>>
>>> "ring->entries" is zero for rtl8180/rtl8185 cards, tx_ring[2] never got
>>> initialized.
>>
>> All this after "---" line is very useful information but the actual
>> commit log is just two sentences. I would copy all to the commit log.
>> We don't need to limit the size of the commit log, on the contrary we
>> should include all the information in it.
>>
>
> I see what you mean, fine for me.
> If you prefer I can also make an update but feel to handle that at
> your convenience. If you e.g. see a better way to do that drop the
> patch and simply submit your version.

I can edit the commit log during commit, no need to resubmit because of
this.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: rtl818x: Prevent using not initialized queues

[Kalle Valo]

Alexander Wetzel <alexander@wetzel-home.de> wrote:

> Using not existing queues can panic the kernel with rtl8180/rtl8185 cards.
> Ignore the skb priority for those cards, they only have one tx queue. Pierre
> Asselin (pa@panix.com) reported the kernel crash in the Gentoo forum:
> 
> https://forums.gentoo.org/viewtopic-t-1147832-postdays-0-postorder-asc-start-25.html
> 
> He also confirmed that this patch fixes the issue. In summary this happened:
> 
> After updating wpa_supplicant from 2.9 to 2.10 the kernel crashed with a
> "divide error: 0000" when connecting to an AP. Control port tx now tries to
> use IEEE80211_AC_VO for the priority, which wpa_supplicants starts to use in
> 2.10.
> 
> Since only the rtl8187se part of the driver supports QoS, the priority
> of the skb is set to IEEE80211_AC_BE (2) by mac80211 for rtl8180/rtl8185
> cards.
> 
> rtl8180 is then unconditionally reading out the priority and finally crashes on
> drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c line 544 without this
> patch:
> 	idx = (ring->idx + skb_queue_len(&ring->queue)) % ring->entries
> 
> "ring->entries" is zero for rtl8180/rtl8185 cards, tx_ring[2] never got
> initialized.
> 
> Cc: stable@vger.kernel.org
> Reported-by: pa@panix.com
> Tested-by: pa@panix.com
> Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>

Patch applied to wireless-next.git, thanks.

746285cf81dc rtl818x: Prevent using not initialized queues

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20220422145228.7567-1-alexander@wetzel-home.de/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches