[PATCH] mt76: use rcu_read_lock_bh in mt76_dma_rx_poll

[PATCH] mt76: use rcu_read_lock_bh in mt76_dma_rx_poll

[Felix Fietkau]

Fixes potential RCU issues and avoids calling ieee80211_rx_napi with softirq
enabled.

Reported-by: Markus Theil <markus.theil@tu-ilmenau.de>
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 drivers/net/wireless/mediatek/mt76/dma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireless/mediatek/mt76/dma.c
index 6173c80189ba..ed0ee2d4a452 100644
--- a/drivers/net/wireless/mediatek/mt76/dma.c
+++ b/drivers/net/wireless/mediatek/mt76/dma.c
@@ -528,7 +528,7 @@ mt76_dma_rx_poll(struct napi_struct *napi, int budget)
 	dev = container_of(napi->dev, struct mt76_dev, napi_dev);
 	qid = napi - dev->napi;
 
-	rcu_read_lock();
+	rcu_read_lock_bh();
 
 	do {
 		cur = mt76_dma_rx_process(dev, &dev->q_rx[qid], budget - done);
@@ -536,7 +536,7 @@ mt76_dma_rx_poll(struct napi_struct *napi, int budget)
 		done += cur;
 	} while (cur && done < budget);
 
-	rcu_read_unlock();
+	rcu_read_unlock_bh();
 
 	if (done < budget && napi_complete(napi))
 		dev->drv->rx_poll_complete(dev, qid);
-- 
2.24.0

Re: [PATCH] mt76: use rcu_read_lock_bh in mt76_dma_rx_poll

[Markus Theil]

On 11/20/19 12:46 PM, Felix Fietkau wrote:
> Fixes potential RCU issues and avoids calling ieee80211_rx_napi with softirq
> enabled.
>
> Reported-by: Markus Theil <markus.theil@tu-ilmenau.de>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>  drivers/net/wireless/mediatek/mt76/dma.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireless/mediatek/mt76/dma.c
> index 6173c80189ba..ed0ee2d4a452 100644
> --- a/drivers/net/wireless/mediatek/mt76/dma.c
> +++ b/drivers/net/wireless/mediatek/mt76/dma.c
> @@ -528,7 +528,7 @@ mt76_dma_rx_poll(struct napi_struct *napi, int budget)
>  	dev = container_of(napi->dev, struct mt76_dev, napi_dev);
>  	qid = napi - dev->napi;
>  
> -	rcu_read_lock();
> +	rcu_read_lock_bh();
>  
>  	do {
>  		cur = mt76_dma_rx_process(dev, &dev->q_rx[qid], budget - done);
> @@ -536,7 +536,7 @@ mt76_dma_rx_poll(struct napi_struct *napi, int budget)
>  		done += cur;
>  	} while (cur && done < budget);
>  
> -	rcu_read_unlock();
> +	rcu_read_unlock_bh();
>  
>  	if (done < budget && napi_complete(napi))
>  		dev->drv->rx_poll_complete(dev, qid);

This patch is incomplete. The same problem exists for usb. I'll send a fix for that.

Regards,
Markus

Re: [PATCH] mt76: use rcu_read_lock_bh in mt76_dma_rx_poll

[Felix Fietkau]

On 2019-11-20 12:59, Markus Theil wrote:
> On 11/20/19 12:46 PM, Felix Fietkau wrote:
>> Fixes potential RCU issues and avoids calling ieee80211_rx_napi with softirq
>> enabled.
>>
>> Reported-by: Markus Theil <markus.theil@tu-ilmenau.de>
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> ---
>>  drivers/net/wireless/mediatek/mt76/dma.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireless/mediatek/mt76/dma.c
>> index 6173c80189ba..ed0ee2d4a452 100644
>> --- a/drivers/net/wireless/mediatek/mt76/dma.c
>> +++ b/drivers/net/wireless/mediatek/mt76/dma.c
>> @@ -528,7 +528,7 @@ mt76_dma_rx_poll(struct napi_struct *napi, int budget)
>>  	dev = container_of(napi->dev, struct mt76_dev, napi_dev);
>>  	qid = napi - dev->napi;
>>  
>> -	rcu_read_lock();
>> +	rcu_read_lock_bh();
>>  
>>  	do {
>>  		cur = mt76_dma_rx_process(dev, &dev->q_rx[qid], budget - done);
>> @@ -536,7 +536,7 @@ mt76_dma_rx_poll(struct napi_struct *napi, int budget)
>>  		done += cur;
>>  	} while (cur && done < budget);
>>  
>> -	rcu_read_unlock();
>> +	rcu_read_unlock_bh();
>>  
>>  	if (done < budget && napi_complete(napi))
>>  		dev->drv->rx_poll_complete(dev, qid);
> 
> This patch is incomplete. The same problem exists for usb. I'll send a fix for that.
How so? On usb, it is called from a tasklet already.

- Felix

Re: [PATCH] mt76: use rcu_read_lock_bh in mt76_dma_rx_poll

[Markus Theil]

On 11/20/19 1:03 PM, Felix Fietkau wrote:
> On 2019-11-20 12:59, Markus Theil wrote:
>> On 11/20/19 12:46 PM, Felix Fietkau wrote:
>>> Fixes potential RCU issues and avoids calling ieee80211_rx_napi with softirq
>>> enabled.
>>>
>>> Reported-by: Markus Theil <markus.theil@tu-ilmenau.de>
>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>> ---
>>>  drivers/net/wireless/mediatek/mt76/dma.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireless/mediatek/mt76/dma.c
>>> index 6173c80189ba..ed0ee2d4a452 100644
>>> --- a/drivers/net/wireless/mediatek/mt76/dma.c
>>> +++ b/drivers/net/wireless/mediatek/mt76/dma.c
>>> @@ -528,7 +528,7 @@ mt76_dma_rx_poll(struct napi_struct *napi, int budget)
>>>  	dev = container_of(napi->dev, struct mt76_dev, napi_dev);
>>>  	qid = napi - dev->napi;
>>>  
>>> -	rcu_read_lock();
>>> +	rcu_read_lock_bh();
>>>  
>>>  	do {
>>>  		cur = mt76_dma_rx_process(dev, &dev->q_rx[qid], budget - done);
>>> @@ -536,7 +536,7 @@ mt76_dma_rx_poll(struct napi_struct *napi, int budget)
>>>  		done += cur;
>>>  	} while (cur && done < budget);
>>>  
>>> -	rcu_read_unlock();
>>> +	rcu_read_unlock_bh();
>>>  
>>>  	if (done < budget && napi_complete(napi))
>>>  		dev->drv->rx_poll_complete(dev, qid);
>> This patch is incomplete. The same problem exists for usb. I'll send a fix for that.
> How so? On usb, it is called from a tasklet already.
>
> - Felix

I've noticed the bug I reported on USB devices. I've only seen that mt76_rx_aggr_reorder_work also accesses mt76_rx_complete.
Maybe it only worked for me by accident. Of course, the tasklet should not run in parallel with multiple instances.

Markus