* [PATCH 0/2] Bluetooth: Fix race condition with rfkill handling
@ 2013-09-11 11:33 johan.hedberg
2013-09-11 11:33 ` [PATCH 1/2] Bluetooth: Introduce a new HCI_RFKILLED flag johan.hedberg
2013-09-11 11:33 ` [PATCH 2/2] Bluetooth: Fix rfkill functionality during the HCI setup stage johan.hedberg
0 siblings, 2 replies; 6+ messages in thread
From: johan.hedberg @ 2013-09-11 11:33 UTC (permalink / raw)
To: linux-bluetooth
Hi,
These two patches fix a pretty severe (hence Cc: stable) race condition
with rfkill handling. If a device gets rfkilled before the HCI setup
stage completes it stays unaccessible through the mgmt interface even it
eventually gets unblocked through rfkill.
The situation is easy to reproduce by running "rfkill block all" before
inserting a dongle, "rfkill unblock all" a bit later and then trying
to do e.g. "btmgmt info". The device will not show up there.
Johan
----------------------------------------------------------------
Johan Hedberg (2):
Bluetooth: Introduce a new HCI_RFKILLED flag
Bluetooth: Fix rfkill functionality during the HCI setup stage
include/net/bluetooth/hci.h | 1 +
net/bluetooth/hci_core.c | 22 +++++++++++++++++-----
2 files changed, 18 insertions(+), 5 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] Bluetooth: Introduce a new HCI_RFKILLED flag
2013-09-11 11:33 [PATCH 0/2] Bluetooth: Fix race condition with rfkill handling johan.hedberg
@ 2013-09-11 11:33 ` johan.hedberg
2013-09-11 20:30 ` Marcel Holtmann
2013-09-11 11:33 ` [PATCH 2/2] Bluetooth: Fix rfkill functionality during the HCI setup stage johan.hedberg
1 sibling, 1 reply; 6+ messages in thread
From: johan.hedberg @ 2013-09-11 11:33 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
This makes it more convenient to check for rfkill (no need to check for
dev->rfkill before calling rfkill_blocked()) and also avoids potential
races if the RFKILL state needs to be checked from within the rfkill
callback.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Cc: stable@vger.kernel.org
---
include/net/bluetooth/hci.h | 1 +
net/bluetooth/hci_core.c | 12 +++++++-----
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 30c88b5..ba008d5 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -104,6 +104,7 @@ enum {
enum {
HCI_SETUP,
HCI_AUTO_OFF,
+ HCI_RFKILLED,
HCI_MGMT,
HCI_PAIRABLE,
HCI_SERVICE_CACHE,
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 4dbb6cb..70efa16 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1148,7 +1148,7 @@ int hci_dev_open(__u16 dev)
goto done;
}
- if (hdev->rfkill && rfkill_blocked(hdev->rfkill)) {
+ if (test_bit(HCI_RFKILLED, &hdev->dev_flags)) {
ret = -ERFKILL;
goto done;
}
@@ -1597,10 +1597,12 @@ static int hci_rfkill_set_block(void *data, bool blocked)
if (test_bit(HCI_USER_CHANNEL, &hdev->dev_flags))
return -EBUSY;
- if (!blocked)
- return 0;
-
- hci_dev_do_close(hdev);
+ if (blocked) {
+ set_bit(HCI_RFKILLED, &hdev->dev_flags);
+ hci_dev_do_close(hdev);
+ } else {
+ clear_bit(HCI_RFKILLED, &hdev->dev_flags);
+ }
return 0;
}
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] Bluetooth: Fix rfkill functionality during the HCI setup stage
2013-09-11 11:33 [PATCH 0/2] Bluetooth: Fix race condition with rfkill handling johan.hedberg
2013-09-11 11:33 ` [PATCH 1/2] Bluetooth: Introduce a new HCI_RFKILLED flag johan.hedberg
@ 2013-09-11 11:33 ` johan.hedberg
2013-09-11 20:33 ` Marcel Holtmann
1 sibling, 1 reply; 6+ messages in thread
From: johan.hedberg @ 2013-09-11 11:33 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
We need to let the setup stage complete cleanly even when the HCI device
is rfkilled. Otherwise the HCI device will stay in an undefined state
and never get notified to user space through mgmt (even when it gets
unblocked through rfkill).
This patch makes sure that hci_dev_open() can be called in the HCI_SETUP
stage, that blocking the device doesn't abort the setup stage, and that
the device gets proper powered down as soon as the setup stage completes
in case it was blocked meanwhile.
The bug that this patch fixed can be very easily reproduced using e.g.
the rfkill command line too. By running "rfkill block all" before
inserting a Bluetooth dongle the resulting HCI device goes into a state
where it is never announced over mgmt, not even when "rfkill unblock all"
is run.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Cc: stable@vger.kernel.org
---
net/bluetooth/hci_core.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 70efa16..3ff99f2 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1148,7 +1148,11 @@ int hci_dev_open(__u16 dev)
goto done;
}
- if (test_bit(HCI_RFKILLED, &hdev->dev_flags)) {
+ /* Check for rfkill but allow the HCI setup stage to proceed
+ * (which in itself doesn't cause any RF activity).
+ */
+ if (test_bit(HCI_RFKILLED, &hdev->dev_flags) &&
+ !test_bit(HCI_SETUP, &hdev->dev_flags)) {
ret = -ERFKILL;
goto done;
}
@@ -1599,7 +1603,8 @@ static int hci_rfkill_set_block(void *data, bool blocked)
if (blocked) {
set_bit(HCI_RFKILLED, &hdev->dev_flags);
- hci_dev_do_close(hdev);
+ if (!test_bit(HCI_SETUP, &hdev->dev_flags))
+ hci_dev_do_close(hdev);
} else {
clear_bit(HCI_RFKILLED, &hdev->dev_flags);
}
@@ -1624,6 +1629,11 @@ static void hci_power_on(struct work_struct *work)
return;
}
+ if (test_bit(HCI_RFKILLED, &hdev->dev_flags)) {
+ clear_bit(HCI_AUTO_OFF, &hdev->dev_flags);
+ hci_dev_do_close(hdev);
+ }
+
if (test_bit(HCI_AUTO_OFF, &hdev->dev_flags))
queue_delayed_work(hdev->req_workqueue, &hdev->power_off,
HCI_AUTO_OFF_TIMEOUT);
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Introduce a new HCI_RFKILLED flag
2013-09-11 11:33 ` [PATCH 1/2] Bluetooth: Introduce a new HCI_RFKILLED flag johan.hedberg
@ 2013-09-11 20:30 ` Marcel Holtmann
0 siblings, 0 replies; 6+ messages in thread
From: Marcel Holtmann @ 2013-09-11 20:30 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
Hi Johan,
> This makes it more convenient to check for rfkill (no need to check for
> dev->rfkill before calling rfkill_blocked()) and also avoids potential
> races if the RFKILL state needs to be checked from within the rfkill
> callback.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> Cc: stable@vger.kernel.org
> ---
> include/net/bluetooth/hci.h | 1 +
> net/bluetooth/hci_core.c | 12 +++++++-----
> 2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 30c88b5..ba008d5 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -104,6 +104,7 @@ enum {
> enum {
> HCI_SETUP,
> HCI_AUTO_OFF,
> + HCI_RFKILLED,
> HCI_MGMT,
> HCI_PAIRABLE,
> HCI_SERVICE_CACHE,
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 4dbb6cb..70efa16 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1148,7 +1148,7 @@ int hci_dev_open(__u16 dev)
> goto done;
> }
>
> - if (hdev->rfkill && rfkill_blocked(hdev->rfkill)) {
> + if (test_bit(HCI_RFKILLED, &hdev->dev_flags)) {
> ret = -ERFKILL;
> goto done;
> }
the problem here is that we never set the initial value of HCI_RFKILLED properly. To make the patch fully equivalent, we should do that.
> @@ -1597,10 +1597,12 @@ static int hci_rfkill_set_block(void *data, bool blocked)
> if (test_bit(HCI_USER_CHANNEL, &hdev->dev_flags))
> return -EBUSY;
>
> - if (!blocked)
> - return 0;
> -
> - hci_dev_do_close(hdev);
> + if (blocked) {
> + set_bit(HCI_RFKILLED, &hdev->dev_flags);
> + hci_dev_do_close(hdev);
> + } else {
> + clear_bit(HCI_RFKILLED, &hdev->dev_flags);
> + }
>
> return 0;
Regards
Marcel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Fix rfkill functionality during the HCI setup stage
2013-09-11 11:33 ` [PATCH 2/2] Bluetooth: Fix rfkill functionality during the HCI setup stage johan.hedberg
@ 2013-09-11 20:33 ` Marcel Holtmann
2013-09-12 6:28 ` Johan Hedberg
0 siblings, 1 reply; 6+ messages in thread
From: Marcel Holtmann @ 2013-09-11 20:33 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
Hi Johan,
> We need to let the setup stage complete cleanly even when the HCI device
> is rfkilled. Otherwise the HCI device will stay in an undefined state
> and never get notified to user space through mgmt (even when it gets
> unblocked through rfkill).
>
> This patch makes sure that hci_dev_open() can be called in the HCI_SETUP
> stage, that blocking the device doesn't abort the setup stage, and that
> the device gets proper powered down as soon as the setup stage completes
> in case it was blocked meanwhile.
>
> The bug that this patch fixed can be very easily reproduced using e.g.
> the rfkill command line too. By running "rfkill block all" before
> inserting a Bluetooth dongle the resulting HCI device goes into a state
> where it is never announced over mgmt, not even when "rfkill unblock all"
> is run.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> Cc: stable@vger.kernel.org
> ---
> net/bluetooth/hci_core.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 70efa16..3ff99f2 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1148,7 +1148,11 @@ int hci_dev_open(__u16 dev)
> goto done;
> }
>
> - if (test_bit(HCI_RFKILLED, &hdev->dev_flags)) {
> + /* Check for rfkill but allow the HCI setup stage to proceed
> + * (which in itself doesn't cause any RF activity).
> + */
> + if (test_bit(HCI_RFKILLED, &hdev->dev_flags) &&
> + !test_bit(HCI_SETUP, &hdev->dev_flags)) {
> ret = -ERFKILL;
> goto done;
> }
so explain to me how this actually works. If we add a HCI controller when RFKILL is currently blocking all radios. Which part is setting HCI_RFKILLED initially? Or does the RFKILL subsystem is calling into set_blocked by itself during registration?
Regards
Marcel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Fix rfkill functionality during the HCI setup stage
2013-09-11 20:33 ` Marcel Holtmann
@ 2013-09-12 6:28 ` Johan Hedberg
0 siblings, 0 replies; 6+ messages in thread
From: Johan Hedberg @ 2013-09-12 6:28 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Wed, Sep 11, 2013, Marcel Holtmann wrote:
> > We need to let the setup stage complete cleanly even when the HCI device
> > is rfkilled. Otherwise the HCI device will stay in an undefined state
> > and never get notified to user space through mgmt (even when it gets
> > unblocked through rfkill).
> >
> > This patch makes sure that hci_dev_open() can be called in the HCI_SETUP
> > stage, that blocking the device doesn't abort the setup stage, and that
> > the device gets proper powered down as soon as the setup stage completes
> > in case it was blocked meanwhile.
> >
> > The bug that this patch fixed can be very easily reproduced using e.g.
> > the rfkill command line too. By running "rfkill block all" before
> > inserting a Bluetooth dongle the resulting HCI device goes into a state
> > where it is never announced over mgmt, not even when "rfkill unblock all"
> > is run.
> >
> > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> > net/bluetooth/hci_core.c | 14 ++++++++++++--
> > 1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 70efa16..3ff99f2 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -1148,7 +1148,11 @@ int hci_dev_open(__u16 dev)
> > goto done;
> > }
> >
> > - if (test_bit(HCI_RFKILLED, &hdev->dev_flags)) {
> > + /* Check for rfkill but allow the HCI setup stage to proceed
> > + * (which in itself doesn't cause any RF activity).
> > + */
> > + if (test_bit(HCI_RFKILLED, &hdev->dev_flags) &&
> > + !test_bit(HCI_SETUP, &hdev->dev_flags)) {
> > ret = -ERFKILL;
> > goto done;
> > }
>
> so explain to me how this actually works. If we add a HCI controller
> when RFKILL is currently blocking all radios. Which part is setting
> HCI_RFKILLED initially? Or does the RFKILL subsystem is calling into
> set_blocked by itself during registration?
Yes, the rfkill subsystem almost immediately calls our
hci_rfkill_set_block() function after we've registered the hdev but
before the hci_power_on() work callback that eventually calls
hci_dev_open() gets called. That's why I didn't think to initialize
HCI_RFKILLED after calling rfkill_register() (instead it defaults to
being unset). I will add this extra initialization in v2.
Considering that the situation is really simple to reproduce I'd
recommend just trying yourself and you'll see how the function calls go.
Johan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-09-12 6:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-11 11:33 [PATCH 0/2] Bluetooth: Fix race condition with rfkill handling johan.hedberg
2013-09-11 11:33 ` [PATCH 1/2] Bluetooth: Introduce a new HCI_RFKILLED flag johan.hedberg
2013-09-11 20:30 ` Marcel Holtmann
2013-09-11 11:33 ` [PATCH 2/2] Bluetooth: Fix rfkill functionality during the HCI setup stage johan.hedberg
2013-09-11 20:33 ` Marcel Holtmann
2013-09-12 6:28 ` Johan Hedberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).