All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jia-Ju Bai <baijiaju1990@163.com>
To: Larry Finger <Larry.Finger@lwfinger.net>
Cc: kvalo@codeaurora.org, linux-wireless@vger.kernel.org,
	b43-dev@lists.infradead.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_attr_interfmode_store
Date: Thu, 01 Jun 2017 09:05:07 +0800	[thread overview]
Message-ID: <592F6843.9000204@163.com> (raw)
In-Reply-To: <85905124-7167-aeb0-8aff-4ceec09e9542@lwfinger.net>

On 06/01/2017 01:33 AM, Larry Finger wrote:
> On 05/31/2017 05:29 AM, Jia-Ju Bai wrote:
>> The driver may sleep under a spin lock, and the function call path is:
>> b43legacy_attr_interfmode_store (acquire the lock by spin_lock_irqsave)
>>    b43legacy_radio_set_interference_mitigation
>>      b43legacy_radio_interference_mitigation_disable
>>        b43legacy_calc_nrssi_slope
>>          b43legacy_synth_pu_workaround
>>            might_sleep and msleep --> may sleep
>>
>> Fixing it may be complex, and a possible way is to remove
>> spin_lock_irqsave and spin_lock_irqrestore in
>> b43legacy_attr_interfmode_store, and the code has been protected by
>> mutex_lock and mutex_unlock.
>>
>> Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
>> ---
>>   drivers/net/wireless/broadcom/b43legacy/sysfs.c |    2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/b43legacy/sysfs.c 
>> b/drivers/net/wireless/broadcom/b43legacy/sysfs.c
>> index 2a1da15..9ede143 100644
>> --- a/drivers/net/wireless/broadcom/b43legacy/sysfs.c
>> +++ b/drivers/net/wireless/broadcom/b43legacy/sysfs.c
>> @@ -137,14 +137,12 @@ static ssize_t 
>> b43legacy_attr_interfmode_store(struct device *dev,
>>       }
>>         mutex_lock(&wldev->wl->mutex);
>> -    spin_lock_irqsave(&wldev->wl->irq_lock, flags);
>>         err = b43legacy_radio_set_interference_mitigation(wldev, mode);
>>       if (err)
>>           b43legacyerr(wldev->wl, "Interference Mitigation not "
>>                  "supported by device\n");
>>       mmiowb();
>> -    spin_unlock_irqrestore(&wldev->wl->irq_lock, flags);
>>       mutex_unlock(&wldev->wl->mutex);
>>         return err ? err : count;
>>
>
> Jia-Ju,
>
> Did you actually observe the attempt to sleep under the spin lock, or 
> did you discover this using some tool? In other words, have either of 
> your patches been tested?
>
> Larry
>
Hi,

In fact, my reported bugs are found by a static analysis tool written by 
me, and they are checked by my review of the driver code.
I admit my patches are not well tested, and they may not well fix the bugs.
I am looking forward to opinions and suggestions :)

Thanks,
Jia-Ju Bai

WARNING: multiple messages have this Message-ID (diff)
From: Jia-Ju Bai <baijiaju1990@163.com>
To: Larry Finger <Larry.Finger@lwfinger.net>
Cc: kvalo@codeaurora.org, linux-wireless@vger.kernel.org,
	b43-dev@lists.infradead.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_attr_interfmode_store
Date: Thu, 01 Jun 2017 09:05:07 +0800	[thread overview]
Message-ID: <592F6843.9000204@163.com> (raw)
In-Reply-To: <85905124-7167-aeb0-8aff-4ceec09e9542@lwfinger.net>

On 06/01/2017 01:33 AM, Larry Finger wrote:
> On 05/31/2017 05:29 AM, Jia-Ju Bai wrote:
>> The driver may sleep under a spin lock, and the function call path is:
>> b43legacy_attr_interfmode_store (acquire the lock by spin_lock_irqsave)
>>    b43legacy_radio_set_interference_mitigation
>>      b43legacy_radio_interference_mitigation_disable
>>        b43legacy_calc_nrssi_slope
>>          b43legacy_synth_pu_workaround
>>            might_sleep and msleep --> may sleep
>>
>> Fixing it may be complex, and a possible way is to remove
>> spin_lock_irqsave and spin_lock_irqrestore in
>> b43legacy_attr_interfmode_store, and the code has been protected by
>> mutex_lock and mutex_unlock.
>>
>> Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
>> ---
>>   drivers/net/wireless/broadcom/b43legacy/sysfs.c |    2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/b43legacy/sysfs.c 
>> b/drivers/net/wireless/broadcom/b43legacy/sysfs.c
>> index 2a1da15..9ede143 100644
>> --- a/drivers/net/wireless/broadcom/b43legacy/sysfs.c
>> +++ b/drivers/net/wireless/broadcom/b43legacy/sysfs.c
>> @@ -137,14 +137,12 @@ static ssize_t 
>> b43legacy_attr_interfmode_store(struct device *dev,
>>       }
>>         mutex_lock(&wldev->wl->mutex);
>> -    spin_lock_irqsave(&wldev->wl->irq_lock, flags);
>>         err = b43legacy_radio_set_interference_mitigation(wldev, mode);
>>       if (err)
>>           b43legacyerr(wldev->wl, "Interference Mitigation not "
>>                  "supported by device\n");
>>       mmiowb();
>> -    spin_unlock_irqrestore(&wldev->wl->irq_lock, flags);
>>       mutex_unlock(&wldev->wl->mutex);
>>         return err ? err : count;
>>
>
> Jia-Ju,
>
> Did you actually observe the attempt to sleep under the spin lock, or 
> did you discover this using some tool? In other words, have either of 
> your patches been tested?
>
> Larry
>
Hi,

In fact, my reported bugs are found by a static analysis tool written by 
me, and they are checked by my review of the driver code.
I admit my patches are not well tested, and they may not well fix the bugs.
I am looking forward to opinions and suggestions :)

Thanks,
Jia-Ju Bai

WARNING: multiple messages have this Message-ID (diff)
From: Jia-Ju Bai <baijiaju1990-9Onoh4P/yGk@public.gmane.org>
To: Larry Finger <Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	b43-dev-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_attr_interfmode_store
Date: Thu, 01 Jun 2017 09:05:07 +0800	[thread overview]
Message-ID: <592F6843.9000204@163.com> (raw)
In-Reply-To: <85905124-7167-aeb0-8aff-4ceec09e9542-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>

On 06/01/2017 01:33 AM, Larry Finger wrote:
> On 05/31/2017 05:29 AM, Jia-Ju Bai wrote:
>> The driver may sleep under a spin lock, and the function call path is:
>> b43legacy_attr_interfmode_store (acquire the lock by spin_lock_irqsave)
>>    b43legacy_radio_set_interference_mitigation
>>      b43legacy_radio_interference_mitigation_disable
>>        b43legacy_calc_nrssi_slope
>>          b43legacy_synth_pu_workaround
>>            might_sleep and msleep --> may sleep
>>
>> Fixing it may be complex, and a possible way is to remove
>> spin_lock_irqsave and spin_lock_irqrestore in
>> b43legacy_attr_interfmode_store, and the code has been protected by
>> mutex_lock and mutex_unlock.
>>
>> Signed-off-by: Jia-Ju Bai <baijiaju1990-9Onoh4P/yGk@public.gmane.org>
>> ---
>>   drivers/net/wireless/broadcom/b43legacy/sysfs.c |    2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/b43legacy/sysfs.c 
>> b/drivers/net/wireless/broadcom/b43legacy/sysfs.c
>> index 2a1da15..9ede143 100644
>> --- a/drivers/net/wireless/broadcom/b43legacy/sysfs.c
>> +++ b/drivers/net/wireless/broadcom/b43legacy/sysfs.c
>> @@ -137,14 +137,12 @@ static ssize_t 
>> b43legacy_attr_interfmode_store(struct device *dev,
>>       }
>>         mutex_lock(&wldev->wl->mutex);
>> -    spin_lock_irqsave(&wldev->wl->irq_lock, flags);
>>         err = b43legacy_radio_set_interference_mitigation(wldev, mode);
>>       if (err)
>>           b43legacyerr(wldev->wl, "Interference Mitigation not "
>>                  "supported by device\n");
>>       mmiowb();
>> -    spin_unlock_irqrestore(&wldev->wl->irq_lock, flags);
>>       mutex_unlock(&wldev->wl->mutex);
>>         return err ? err : count;
>>
>
> Jia-Ju,
>
> Did you actually observe the attempt to sleep under the spin lock, or 
> did you discover this using some tool? In other words, have either of 
> your patches been tested?
>
> Larry
>
Hi,

In fact, my reported bugs are found by a static analysis tool written by 
me, and they are checked by my review of the driver code.
I admit my patches are not well tested, and they may not well fix the bugs.
I am looking forward to opinions and suggestions :)

Thanks,
Jia-Ju Bai

  reply	other threads:[~2017-06-01  1:05 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-31 10:29 [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_attr_interfmode_store Jia-Ju Bai
2017-05-31 10:29 ` Jia-Ju Bai
2017-05-31 15:17 ` Michael Büsch
2017-05-31 15:17   ` Michael Büsch
2017-05-31 17:33 ` Larry Finger
2017-05-31 17:33   ` Larry Finger
2017-06-01  1:05   ` Jia-Ju Bai [this message]
2017-06-01  1:05     ` Jia-Ju Bai
2017-06-01  1:05     ` Jia-Ju Bai
2017-06-01  4:15     ` Kalle Valo
2017-06-01  4:15       ` Kalle Valo
2017-06-01  4:15       ` Kalle Valo
2017-06-01 16:11     ` Jonathan Corbet
2017-06-01 16:11       ` Jonathan Corbet
2017-06-01 17:43       ` Larry Finger
2017-06-01 17:43         ` Larry Finger
2017-06-02  1:18       ` Jia-Ju Bai
2017-06-02  1:18         ` Jia-Ju Bai
2017-06-02  1:18         ` Jia-Ju Bai
2017-07-30 10:24         ` Michael Büsch
2017-07-30 10:24           ` Michael Büsch
2017-07-30 10:24           ` Michael Büsch
2017-06-01 23:24 ` kbuild test robot
2017-06-01 23:24   ` kbuild test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=592F6843.9000204@163.com \
    --to=baijiaju1990@163.com \
    --cc=Larry.Finger@lwfinger.net \
    --cc=b43-dev@lists.infradead.org \
    --cc=kvalo@codeaurora.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.