Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1654810

Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_op_bss_info_changed

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

On Thu, 01 Jun 2017 07:27:20 +0300
Kalle Valo <kvalo@codeaurora.org> wrote:

> Michael Büsch <m@bues.ch> writes:
> 
> >> > --- 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.  
> 
> Sure, but IMHO in general I think the practise of releasing the lock
> like this in a middle of function is dangerous as one can easily miss
> that upper and lower halves of the function are not actually atomic
> anymore. And in this case that it's under a conditional makes it even
> worse.
> 


Yes in general I agree. Releasing and re-acquiring a lock is dangerous.
But I think in this special case here it might be harmless.
The irq_lock is used mostly (if not exclusively; I don't fully
remember) to protect against the IRQ top and bottom half.
But we disabled the device IRQs a line above and the purpose of this
synchronize is to make sure the handler will finish and thus make
dropping the lock save.
Of course it does not make sense to do this with the lock held :)

-- 
Michael

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_op_bss_info_changed Jia-Ju Bai <baijiaju1990@163.com> - 2017-05-31 12:10 +0200
  Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_op_bss_info_changed Kalle Valo <kvalo@codeaurora.org> - 2017-05-31 12:30 +0200
    Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in  b43legacy_op_bss_info_changed Arend van Spriel <arend.vanspriel@broadcom.com> - 2017-05-31 14:20 +0200
    Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in  b43legacy_op_bss_info_changed Michael Büsch <m@bues.ch> - 2017-05-31 17:40 +0200
      Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in  b43legacy_op_bss_info_changed Larry Finger <Larry.Finger@lwfinger.net> - 2017-06-01 02:10 +0200
        Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_op_bss_info_changed Jia-Ju Bai <baijiaju1990@163.com> - 2017-06-01 03:10 +0200
        Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in  b43legacy_op_bss_info_changed Michael Büsch <m@bues.ch> - 2017-06-01 07:40 +0200
      Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_op_bss_info_changed Kalle Valo <kvalo@codeaurora.org> - 2017-06-01 06:30 +0200
        Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in  b43legacy_op_bss_info_changed Michael Büsch <m@bues.ch> - 2017-06-01 07:30 +0200

csiph-web