[PATCH] wifi: mt76: do not increase mcu skb refcount if retry is not supported

[PATCH] wifi: mt76: do not increase mcu skb refcount if retry is not supported

[Felix Fietkau]

If mcu_skb_prepare_msg is not implemented, incrementing skb refcount does not
work for mcu message retry. In some cases (e.g. on SDIO), shared skbs can trigger
a BUG_ON, crashing the system.
Fix this by only incrementing refcount if retry is actually supported.

Fixes: 3688c18b65ae ("wifi: mt76: mt7915: retry mcu messages")
Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> #KernelCI
Tested-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 drivers/net/wireless/mediatek/mt76/mcu.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mcu.c b/drivers/net/wireless/mediatek/mt76/mcu.c
index 98da82b74094..3353012e8542 100644
--- a/drivers/net/wireless/mediatek/mt76/mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mcu.c
@@ -84,13 +84,16 @@ int mt76_mcu_skb_send_and_get_msg(struct mt76_dev *dev, struct sk_buff *skb,
 	mutex_lock(&dev->mcu.mutex);
 
 	if (dev->mcu_ops->mcu_skb_prepare_msg) {
+		orig_skb = skb;
 		ret = dev->mcu_ops->mcu_skb_prepare_msg(dev, skb, cmd, &seq);
 		if (ret < 0)
 			goto out;
 	}
 
 retry:
-	orig_skb = skb_get(skb);
+	/* orig skb might be needed for retry, mcu_skb_send_msg consumes it */
+	if (orig_skb)
+		skb_get(orig_skb);
 	ret = dev->mcu_ops->mcu_skb_send_msg(dev, skb, cmd, &seq);
 	if (ret < 0)
 		goto out;
@@ -105,7 +108,7 @@ int mt76_mcu_skb_send_and_get_msg(struct mt76_dev *dev, struct sk_buff *skb,
 	do {
 		skb = mt76_mcu_get_response(dev, expires);
 		if (!skb && !test_bit(MT76_MCU_RESET, &dev->phy.state) &&
-		    retry++ < dev->mcu_ops->max_retry) {
+		    orig_skb && retry++ < dev->mcu_ops->max_retry) {
 			dev_err(dev->dev, "Retry message %08x (seq %d)\n",
 				cmd, seq);
 			skb = orig_skb;
-- 
2.46.0

Re: [PATCH] wifi: mt76: do not increase mcu skb refcount if retry is not supported

[Kalle Valo]

Felix Fietkau <nbd@nbd.name> writes:

> If mcu_skb_prepare_msg is not implemented, incrementing skb refcount does not
> work for mcu message retry. In some cases (e.g. on SDIO), shared skbs can trigger
> a BUG_ON, crashing the system.
> Fix this by only incrementing refcount if retry is actually supported.
>
> Fixes: 3688c18b65ae ("wifi: mt76: mt7915: retry mcu messages")
> Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> #KernelCI
> Tested-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>

I'll queue this to wireless tree and add:

Closes: https://lore.kernel.org/r/d907b13a-f8be-4cb8-a0bb-560a21278041@notapiano/

Ok?

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

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

Re: [PATCH] wifi: mt76: do not increase mcu skb refcount if retry is not supported

[Felix Fietkau]

On 17.09.24 13:18, Kalle Valo wrote:
> Felix Fietkau <nbd@nbd.name> writes:
> 
>> If mcu_skb_prepare_msg is not implemented, incrementing skb refcount does not
>> work for mcu message retry. In some cases (e.g. on SDIO), shared skbs can trigger
>> a BUG_ON, crashing the system.
>> Fix this by only incrementing refcount if retry is actually supported.
>>
>> Fixes: 3688c18b65ae ("wifi: mt76: mt7915: retry mcu messages")
>> Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> #KernelCI
>> Tested-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> 
> I'll queue this to wireless tree and add:
> 
> Closes: https://lore.kernel.org/r/d907b13a-f8be-4cb8-a0bb-560a21278041@notapiano/
> 
> Ok?

Sure, thanks!

- Felix

Re: wifi: mt76: do not increase mcu skb refcount if retry is not supported

[Kalle Valo]

Felix Fietkau <nbd@nbd.name> wrote:

> If mcu_skb_prepare_msg is not implemented, incrementing skb refcount does not
> work for mcu message retry. In some cases (e.g. on SDIO), shared skbs can trigger
> a BUG_ON, crashing the system.
> Fix this by only incrementing refcount if retry is actually supported.
> 
> Fixes: 3688c18b65ae ("wifi: mt76: mt7915: retry mcu messages")
> Closes: https://lore.kernel.org/r/d907b13a-f8be-4cb8-a0bb-560a21278041@notapiano/
> Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> #KernelCI
> Tested-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>

Patch applied to wireless.git, thanks.

34b695481084 wifi: mt76: do not increase mcu skb refcount if retry is not supported

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20240917110942.22077-1-nbd@nbd.name/

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

Re: [PATCH] wifi: mt76: do not increase mcu skb refcount if retry is not supported

[Muhammad Usama Anjum]

On 9/17/24 4:09 PM, Felix Fietkau wrote:
> If mcu_skb_prepare_msg is not implemented, incrementing skb refcount does not
> work for mcu message retry. In some cases (e.g. on SDIO), shared skbs can trigger
> a BUG_ON, crashing the system.
> Fix this by only incrementing refcount if retry is actually supported.
> 
> Fixes: 3688c18b65ae ("wifi: mt76: mt7915: retry mcu messages")
> Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> #KernelCI
> Tested-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>  drivers/net/wireless/mediatek/mt76/mcu.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mcu.c b/drivers/net/wireless/mediatek/mt76/mcu.c
> index 98da82b74094..3353012e8542 100644
> --- a/drivers/net/wireless/mediatek/mt76/mcu.c
> +++ b/drivers/net/wireless/mediatek/mt76/mcu.c
> @@ -84,13 +84,16 @@ int mt76_mcu_skb_send_and_get_msg(struct mt76_dev *dev, struct sk_buff *skb,
>  	mutex_lock(&dev->mcu.mutex);
>  
>  	if (dev->mcu_ops->mcu_skb_prepare_msg) {
> +		orig_skb = skb;
>  		ret = dev->mcu_ops->mcu_skb_prepare_msg(dev, skb, cmd, &seq);
>  		if (ret < 0)
>  			goto out;
>  	}
>  
>  retry:
> -	orig_skb = skb_get(skb);
> +	/* orig skb might be needed for retry, mcu_skb_send_msg consumes it */
> +	if (orig_skb)
> +		skb_get(orig_skb);
>  	ret = dev->mcu_ops->mcu_skb_send_msg(dev, skb, cmd, &seq);
>  	if (ret < 0)
>  		goto out;
> @@ -105,7 +108,7 @@ int mt76_mcu_skb_send_and_get_msg(struct mt76_dev *dev, struct sk_buff *skb,
>  	do {
>  		skb = mt76_mcu_get_response(dev, expires);
>  		if (!skb && !test_bit(MT76_MCU_RESET, &dev->phy.state) &&
> -		    retry++ < dev->mcu_ops->max_retry) {
> +		    orig_skb && retry++ < dev->mcu_ops->max_retry) {
>  			dev_err(dev->dev, "Retry message %08x (seq %d)\n",
>  				cmd, seq);
>  			skb = orig_skb;
This patch is in next from 5 weeks. As 3688c18b65ae ("wifi: mt76: mt7915: retry mcu messages") is
already in the mainline, why this fix hasn't been included in mainline? I thought fixes are included
as soon as possible in mainline RCs. Am I missing something? 

Are we planning to include this fix in next release instead of current one?


-- 
BR,
Muhammad Usama Anjum

Re: [PATCH] wifi: mt76: do not increase mcu skb refcount if retry is not supported

[Kalle Valo]

Muhammad Usama Anjum <Usama.Anjum@collabora.com> writes:

> On 9/17/24 4:09 PM, Felix Fietkau wrote:
>
>> If mcu_skb_prepare_msg is not implemented, incrementing skb refcount does not
>> work for mcu message retry. In some cases (e.g. on SDIO), shared skbs can trigger
>> a BUG_ON, crashing the system.
>> Fix this by only incrementing refcount if retry is actually supported.
>> 
>> Fixes: 3688c18b65ae ("wifi: mt76: mt7915: retry mcu messages")
>> Reported-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> #KernelCI
>> Tested-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> ---
>>  drivers/net/wireless/mediatek/mt76/mcu.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/wireless/mediatek/mt76/mcu.c b/drivers/net/wireless/mediatek/mt76/mcu.c
>> index 98da82b74094..3353012e8542 100644
>> --- a/drivers/net/wireless/mediatek/mt76/mcu.c
>> +++ b/drivers/net/wireless/mediatek/mt76/mcu.c
>> @@ -84,13 +84,16 @@ int mt76_mcu_skb_send_and_get_msg(struct mt76_dev *dev, struct sk_buff *skb,
>>  	mutex_lock(&dev->mcu.mutex);
>>  
>>  	if (dev->mcu_ops->mcu_skb_prepare_msg) {
>> +		orig_skb = skb;
>>  		ret = dev->mcu_ops->mcu_skb_prepare_msg(dev, skb, cmd, &seq);
>>  		if (ret < 0)
>>  			goto out;
>>  	}
>>  
>>  retry:
>> -	orig_skb = skb_get(skb);
>> +	/* orig skb might be needed for retry, mcu_skb_send_msg consumes it */
>> +	if (orig_skb)
>> +		skb_get(orig_skb);
>>  	ret = dev->mcu_ops->mcu_skb_send_msg(dev, skb, cmd, &seq);
>>  	if (ret < 0)
>>  		goto out;
>> @@ -105,7 +108,7 @@ int mt76_mcu_skb_send_and_get_msg(struct mt76_dev *dev, struct sk_buff *skb,
>>  	do {
>>  		skb = mt76_mcu_get_response(dev, expires);
>>  		if (!skb && !test_bit(MT76_MCU_RESET, &dev->phy.state) &&
>> -		    retry++ < dev->mcu_ops->max_retry) {
>> +		    orig_skb && retry++ < dev->mcu_ops->max_retry) {
>>  			dev_err(dev->dev, "Retry message %08x (seq %d)\n",
>>  				cmd, seq);
>>  			skb = orig_skb;
> This patch is in next from 5 weeks. As 3688c18b65ae ("wifi: mt76: mt7915: retry mcu messages") is
> already in the mainline, why this fix hasn't been included in mainline? I thought fixes are included
> as soon as possible in mainline RCs. Am I missing something? 
>
> Are we planning to include this fix in next release instead of current one?

The commit is now in wireless tree:

34b695481084 wifi: mt76: do not increase mcu skb refcount if retry is not supported

With good luck it will be in v6.12-rc5 but no guarantees.

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

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