lkml.org 
[lkml]   [2017]   [May]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_op_bss_info_changed
On 06/01/2017 08:07 AM, Larry Finger wrote:
> On 05/31/2017 10:32 AM, Michael Büsch wrote:
>> On Wed, 31 May 2017 13:26:43 +0300
>> Kalle Valo <kvalo@codeaurora.org> wrote:
>>
>>> Jia-Ju Bai <baijiaju1990@163.com> writes:
>>>
>>>> The driver may sleep under a spin lock, and the function call path is:
>>>> b43legacy_op_bss_info_changed (acquire the lock by spin_lock_irqsave)
>>>> b43legacy_synchronize_irq
>>>> synchronize_irq --> may sleep
>>>>
>>>> To fix it, the lock is released before b43legacy_synchronize_irq,
>>>> and the
>>>> lock is acquired again after this function.
>>>>
>>>> Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
>>>> ---
>>>> drivers/net/wireless/broadcom/b43legacy/main.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/net/wireless/broadcom/b43legacy/main.c
>>>> b/drivers/net/wireless/broadcom/b43legacy/main.c
>>>> index f1e3dad..31ead21 100644
>>>> --- a/drivers/net/wireless/broadcom/b43legacy/main.c
>>>> +++ b/drivers/net/wireless/broadcom/b43legacy/main.c
>>>> @@ -2859,7 +2859,9 @@ static void
>>>> b43legacy_op_bss_info_changed(struct ieee80211_hw *hw,
>>>> b43legacy_write32(dev, B43legacy_MMIO_GEN_IRQ_MASK, 0);
>>>> if (changed & BSS_CHANGED_BSSID) {
>>>> + spin_unlock_irqrestore(&wl->irq_lock, flags);
>>>> b43legacy_synchronize_irq(dev);
>>>> + spin_lock_irqsave(&wl->irq_lock, flags);
>>>
>>> To me this looks like a fragile workaround and not a real fix. You can
>>> easily add new race conditions with releasing the lock like this.
>>>
>>
>>
>> I think releasing the lock possibly is fine. It certainly is better than
>> sleeping with a lock held.
>> We disabled the device interrupts just before this line.
>>
>> However I think the synchronize_irq should be outside of the
>> conditional right after the write to B43legacy_MMIO_GEN_IRQ_MASK. (So
>> two lines above)
>> I don't think it makes sense to only synchronize if BSS_CHANGED_BSSID
>> is set.
>>
>>
>> On the other hand b43 does not have this irq-disabling foobar anymore.
>> So somebody must have removed it. Maybe you can find the commit that
>> removed this stuff from b43 and port it to b43legacy?
>>
>>
>> So I would vote for moving the synchronize_irq up outside of the
>> conditional and put the unlock/lock sequence around it.
>> And as a second patch on top of that try to remove this stuff
>> altogether like b43 did.
>
> The patch that removed it in b43 is
>
> commit 36dbd9548e92268127b0c31b0e121e63e9207108
> Author: Michael Buesch <mb@bu3sch.de>
> Date: Fri Sep 4 22:51:29 2009 +0200
>
> b43: Use a threaded IRQ handler
>
> Use a threaded IRQ handler to allow locking the mutex and
> sleeping while executing an interrupt.
> This removes usage of the irq_lock spinlock, but introduces
> a new hardirq_lock, which is _only_ used for the PCI/SSB lowlevel
> hard-irq handler. Sleeping busses (SDIO) will use mutex instead.
>
> Signed-off-by: Michael Buesch <mb@bu3sch.de>
> Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
>
> I vaguely remember this patch. Although it is roughly a 1000-line fix,
> I will try to port it to b43legacy. I still have an old BCM4306 PCMCIA
> card that I can test in a PowerBook G4.
>
> I agree with Michael that this is the way to go. Both of Jia-Ju's
> patches should be rejected.
>
> Larry
>
>

It is fine to me to fix the bug by porting this former patch.

Thanks,
Jia-Ju Bai

\
 
 \ /
  Last update: 2017-06-01 03:07    [W:0.105 / U:0.360 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site