* [PATCH 1/3] Bluetooth: Fix hci core device initialization
@ 2011-10-08 12:58 David Herrmann
2011-10-08 12:58 ` [PATCH 2/3] Bluetooth: Rename sysfs un/register to add/del David Herrmann
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: David Herrmann @ 2011-10-08 12:58 UTC (permalink / raw)
To: linux-bluetooth; +Cc: marcel, padovan, David Herrmann
We must not call device_del() if we didn't use device_add(). See module.c for
comments on that. Therefore, we need to call device_initialize() when allocating
the hci device and later device_add() instead of device_register().
This also fixes a bug when hci_register_dev() failed and we call hci_free_dev()
without a valid core device. hci_free_dev() segfaults while calling put_device()
on invalid memory.
We already do this with hci_conn connections (hci_conn_init_sysfs()) so they do
not need to be fixed.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
include/net/bluetooth/hci_core.h | 1 +
net/bluetooth/hci_core.c | 1 +
net/bluetooth/hci_sysfs.c | 18 ++++++++++++------
3 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 5b92442..81741ac 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -597,6 +597,7 @@ int hci_recv_frame(struct sk_buff *skb);
int hci_recv_fragment(struct hci_dev *hdev, int type, void *data, int count);
int hci_recv_stream_fragment(struct hci_dev *hdev, void *data, int count);
+void hci_init_sysfs(struct hci_dev *hdev);
int hci_register_sysfs(struct hci_dev *hdev);
void hci_unregister_sysfs(struct hci_dev *hdev);
void hci_conn_init_sysfs(struct hci_conn *conn);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index b84458d..d2445cb 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -912,6 +912,7 @@ struct hci_dev *hci_alloc_dev(void)
if (!hdev)
return NULL;
+ hci_init_sysfs(hdev);
skb_queue_head_init(&hdev->driver_init);
return hdev;
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 22f1a6c..a7d5de3 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -542,6 +542,17 @@ static int auto_accept_delay_get(void *data, u64 *val)
DEFINE_SIMPLE_ATTRIBUTE(auto_accept_delay_fops, auto_accept_delay_get,
auto_accept_delay_set, "%llu\n");
+void hci_init_sysfs(struct hci_dev *hdev)
+{
+ struct device *dev = &hdev->dev;
+
+ dev->type = &bt_host;
+ dev->class = bt_class;
+
+ dev_set_drvdata(dev, hdev);
+ device_initialize(dev);
+}
+
int hci_register_sysfs(struct hci_dev *hdev)
{
struct device *dev = &hdev->dev;
@@ -549,15 +560,10 @@ int hci_register_sysfs(struct hci_dev *hdev)
BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
- dev->type = &bt_host;
- dev->class = bt_class;
dev->parent = hdev->parent;
-
dev_set_name(dev, "%s", hdev->name);
- dev_set_drvdata(dev, hdev);
-
- err = device_register(dev);
+ err = device_add(dev);
if (err < 0)
return err;
--
1.7.7
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] Bluetooth: Rename sysfs un/register to add/del
2011-10-08 12:58 [PATCH 1/3] Bluetooth: Fix hci core device initialization David Herrmann
@ 2011-10-08 12:58 ` David Herrmann
2011-10-08 12:58 ` [PATCH 3/3] Bluetooth: Forward errors from hci_register_dev David Herrmann
2011-10-10 18:44 ` [PATCH 1/3] Bluetooth: Fix hci core device initialization Gustavo Padovan
2 siblings, 0 replies; 6+ messages in thread
From: David Herrmann @ 2011-10-08 12:58 UTC (permalink / raw)
To: linux-bluetooth; +Cc: marcel, padovan, David Herrmann
As we introduced hci_init_sysfs() we should also rename hci_register_sysfs() and
hci_unregister_sysfs() to hci_add_sysfs() and hci_del_sysfs() like we do with
hci_conn_add/del_sysfs(). It looks more consistent now.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
include/net/bluetooth/hci_core.h | 4 ++--
net/bluetooth/hci_core.c | 4 ++--
net/bluetooth/hci_sysfs.c | 4 ++--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 81741ac..2d03b0b 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -598,8 +598,8 @@ int hci_recv_fragment(struct hci_dev *hdev, int type, void *data, int count);
int hci_recv_stream_fragment(struct hci_dev *hdev, void *data, int count);
void hci_init_sysfs(struct hci_dev *hdev);
-int hci_register_sysfs(struct hci_dev *hdev);
-void hci_unregister_sysfs(struct hci_dev *hdev);
+int hci_add_sysfs(struct hci_dev *hdev);
+void hci_del_sysfs(struct hci_dev *hdev);
void hci_conn_init_sysfs(struct hci_conn *conn);
void hci_conn_add_sysfs(struct hci_conn *conn);
void hci_conn_del_sysfs(struct hci_conn *conn);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index d2445cb..4975578 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1506,7 +1506,7 @@ int hci_register_dev(struct hci_dev *hdev)
if (!hdev->workqueue)
goto nomem;
- hci_register_sysfs(hdev);
+ hci_add_sysfs(hdev);
hdev->rfkill = rfkill_alloc(hdev->name, &hdev->dev,
RFKILL_TYPE_BLUETOOTH, &hci_rfkill_ops, hdev);
@@ -1561,7 +1561,7 @@ int hci_unregister_dev(struct hci_dev *hdev)
rfkill_destroy(hdev->rfkill);
}
- hci_unregister_sysfs(hdev);
+ hci_del_sysfs(hdev);
hci_del_off_timer(hdev);
del_timer(&hdev->adv_timer);
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index a7d5de3..1f9f876 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -553,7 +553,7 @@ void hci_init_sysfs(struct hci_dev *hdev)
device_initialize(dev);
}
-int hci_register_sysfs(struct hci_dev *hdev)
+int hci_add_sysfs(struct hci_dev *hdev)
{
struct device *dev = &hdev->dev;
int err;
@@ -587,7 +587,7 @@ int hci_register_sysfs(struct hci_dev *hdev)
return 0;
}
-void hci_unregister_sysfs(struct hci_dev *hdev)
+void hci_del_sysfs(struct hci_dev *hdev)
{
BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
--
1.7.7
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] Bluetooth: Forward errors from hci_register_dev
2011-10-08 12:58 [PATCH 1/3] Bluetooth: Fix hci core device initialization David Herrmann
2011-10-08 12:58 ` [PATCH 2/3] Bluetooth: Rename sysfs un/register to add/del David Herrmann
@ 2011-10-08 12:58 ` David Herrmann
2011-10-10 18:44 ` [PATCH 1/3] Bluetooth: Fix hci core device initialization Gustavo Padovan
2 siblings, 0 replies; 6+ messages in thread
From: David Herrmann @ 2011-10-08 12:58 UTC (permalink / raw)
To: linux-bluetooth; +Cc: marcel, padovan, David Herrmann
We need to catch errors when calling hci_add_sysfs() and return them to the
caller to avoid kernel oopses on device_add() failure.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
net/bluetooth/hci_core.c | 18 ++++++++++++------
1 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 4975578..fdcbf8f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1426,7 +1426,7 @@ int hci_add_adv_entry(struct hci_dev *hdev,
int hci_register_dev(struct hci_dev *hdev)
{
struct list_head *head = &hci_dev_list, *p;
- int i, id = 0;
+ int i, id = 0, error;
BT_DBG("%p name %s bus %d owner %p", hdev, hdev->name,
hdev->bus, hdev->owner);
@@ -1503,10 +1503,14 @@ int hci_register_dev(struct hci_dev *hdev)
write_unlock_bh(&hci_dev_list_lock);
hdev->workqueue = create_singlethread_workqueue(hdev->name);
- if (!hdev->workqueue)
- goto nomem;
+ if (!hdev->workqueue) {
+ error = -ENOMEM;
+ goto err;
+ }
- hci_add_sysfs(hdev);
+ error = hci_add_sysfs(hdev);
+ if (error < 0)
+ goto err_wqueue;
hdev->rfkill = rfkill_alloc(hdev->name, &hdev->dev,
RFKILL_TYPE_BLUETOOTH, &hci_rfkill_ops, hdev);
@@ -1525,12 +1529,14 @@ int hci_register_dev(struct hci_dev *hdev)
return id;
-nomem:
+err_wqueue:
+ destroy_workqueue(hdev->workqueue);
+err:
write_lock_bh(&hci_dev_list_lock);
list_del(&hdev->list);
write_unlock_bh(&hci_dev_list_lock);
- return -ENOMEM;
+ return error;
}
EXPORT_SYMBOL(hci_register_dev);
--
1.7.7
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] Bluetooth: Fix hci core device initialization
2011-10-08 12:58 [PATCH 1/3] Bluetooth: Fix hci core device initialization David Herrmann
2011-10-08 12:58 ` [PATCH 2/3] Bluetooth: Rename sysfs un/register to add/del David Herrmann
2011-10-08 12:58 ` [PATCH 3/3] Bluetooth: Forward errors from hci_register_dev David Herrmann
@ 2011-10-10 18:44 ` Gustavo Padovan
2011-10-11 12:35 ` David Herrmann
2 siblings, 1 reply; 6+ messages in thread
From: Gustavo Padovan @ 2011-10-10 18:44 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-bluetooth, marcel
Hi David,
* David Herrmann <dh.herrmann@googlemail.com> [2011-10-08 14:58:47 +0200]:
> We must not call device_del() if we didn't use device_add(). See module.c for
> comments on that. Therefore, we need to call device_initialize() when allocating
> the hci device and later device_add() instead of device_register().
>
> This also fixes a bug when hci_register_dev() failed and we call hci_free_dev()
> without a valid core device. hci_free_dev() segfaults while calling put_device()
> on invalid memory.
Please let me know if the following diff also fixes this problem.
It seems to fixes other issues like failing in usb_driver_claim_interface().
Gustavo
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index b84458d..ac446a7 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -922,9 +922,6 @@ EXPORT_SYMBOL(hci_alloc_dev);
void hci_free_dev(struct hci_dev *hdev)
{
skb_queue_purge(&hdev->driver_init);
-
- /* will free via device release */
- put_device(&hdev->dev);
}
EXPORT_SYMBOL(hci_free_dev);
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 22f1a6c..1e5ccde 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -587,7 +587,7 @@ void hci_unregister_sysfs(struct hci_dev *hdev)
debugfs_remove_recursive(hdev->debugfs);
- device_del(&hdev->dev);
+ device_unregister(&hdev->dev);
}
int __init bt_sysfs_init(void)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] Bluetooth: Fix hci core device initialization
2011-10-10 18:44 ` [PATCH 1/3] Bluetooth: Fix hci core device initialization Gustavo Padovan
@ 2011-10-11 12:35 ` David Herrmann
2011-10-13 20:51 ` Gustavo Padovan
0 siblings, 1 reply; 6+ messages in thread
From: David Herrmann @ 2011-10-11 12:35 UTC (permalink / raw)
To: David Herrmann, linux-bluetooth, marcel
Hi Gustavo
On Mon, Oct 10, 2011 at 8:44 PM, Gustavo Padovan <padovan@profusion.mobi> w=
rote:
> Hi David,
>
> * David Herrmann <dh.herrmann@googlemail.com> [2011-10-08 14:58:47 +0200]=
:
>
>> We must not call device_del() if we didn't use device_add(). See module.=
c for
>> comments on that. Therefore, we need to call device_initialize() when al=
locating
>> the hci device and later device_add() instead of device_register().
>>
>> This also fixes a bug when hci_register_dev() failed and we call hci_fre=
e_dev()
>> without a valid core device. hci_free_dev() segfaults while calling put_=
device()
>> on invalid memory.
>
> Please let me know if the following diff also fixes this problem.
> It seems to fixes other issues like failing in usb_driver_claim_interface=
().
Could you elaborate more? I what way does may patch not fix this issue?
> =A0 =A0 =A0 =A0Gustavo
>
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index b84458d..ac446a7 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -922,9 +922,6 @@ EXPORT_SYMBOL(hci_alloc_dev);
> =A0void hci_free_dev(struct hci_dev *hdev)
> =A0{
> =A0 =A0 =A0 =A0skb_queue_purge(&hdev->driver_init);
> -
> - =A0 =A0 =A0 /* will free via device release */
> - =A0 =A0 =A0 put_device(&hdev->dev);
This does not work. We need to drop a reference here, otherwise this
function is totally useless except cleaning up the skb queue. In fact,
this function will be called on an HCI device without having a
reference.
> =A0}
> =A0EXPORT_SYMBOL(hci_free_dev);
>
> diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> index 22f1a6c..1e5ccde 100644
> --- a/net/bluetooth/hci_sysfs.c
> +++ b/net/bluetooth/hci_sysfs.c
> @@ -587,7 +587,7 @@ void hci_unregister_sysfs(struct hci_dev *hdev)
>
> =A0 =A0 =A0 =A0debugfs_remove_recursive(hdev->debugfs);
>
> - =A0 =A0 =A0 device_del(&hdev->dev);
> + =A0 =A0 =A0 device_unregister(&hdev->dev);
This drops the last reference in most cases and hence "hdev" will get
deleted here and the caller (hci_unregister_dev) will fail cleaning up
the device.
The problem is, we actually have two reference counts. The
device_get/device_put reference count (which actually cleans up the
hdev device with kfree() when dropping the last reference) and the
hdev->refcnt which is in charge of cleaning up the caller's data. We
need to take a reference with device_get() in hci_alloc_dev and drop
it in hci_free_dev, otherwise we have inconsistent behaviour.
> =A0}
>
> =A0int __init bt_sysfs_init(void)
>
>
Regards
David
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] Bluetooth: Fix hci core device initialization
2011-10-11 12:35 ` David Herrmann
@ 2011-10-13 20:51 ` Gustavo Padovan
0 siblings, 0 replies; 6+ messages in thread
From: Gustavo Padovan @ 2011-10-13 20:51 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-bluetooth, marcel
Hi David,
* David Herrmann <dh.herrmann@googlemail.com> [2011-10-11 14:35:01 +0200]:
> Hi Gustavo
>=20
> On Mon, Oct 10, 2011 at 8:44 PM, Gustavo Padovan <padovan@profusion.mobi>=
wrote:
> > Hi David,
> >
> > * David Herrmann <dh.herrmann@googlemail.com> [2011-10-08 14:58:47 +020=
0]:
> >
> >> We must not call device_del() if we didn't use device_add(). See modul=
e.c for
> >> comments on that. Therefore, we need to call device_initialize() when =
allocating
> >> the hci device and later device_add() instead of device_register().
> >>
> >> This also fixes a bug when hci_register_dev() failed and we call hci_f=
ree_dev()
> >> without a valid core device. hci_free_dev() segfaults while calling pu=
t_device()
> >> on invalid memory.
> >
> > Please let me know if the following diff also fixes this problem.
> > It seems to fixes other issues like failing in usb_driver_claim_interfa=
ce().
>=20
> Could you elaborate more? I what way does may patch not fix this issue?
>=20
> > =A0 =A0 =A0 =A0Gustavo
> >
> >
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index b84458d..ac446a7 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -922,9 +922,6 @@ EXPORT_SYMBOL(hci_alloc_dev);
> > =A0void hci_free_dev(struct hci_dev *hdev)
> > =A0{
> > =A0 =A0 =A0 =A0skb_queue_purge(&hdev->driver_init);
> > -
> > - =A0 =A0 =A0 /* will free via device release */
> > - =A0 =A0 =A0 put_device(&hdev->dev);
>=20
> This does not work. We need to drop a reference here, otherwise this
> function is totally useless except cleaning up the skb queue. In fact,
> this function will be called on an HCI device without having a
> reference.
Yes, I forgot about this put_device magic.
I reconsidered your patches and applied them all. Thanks.
Gustavo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-10-13 20:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-08 12:58 [PATCH 1/3] Bluetooth: Fix hci core device initialization David Herrmann
2011-10-08 12:58 ` [PATCH 2/3] Bluetooth: Rename sysfs un/register to add/del David Herrmann
2011-10-08 12:58 ` [PATCH 3/3] Bluetooth: Forward errors from hci_register_dev David Herrmann
2011-10-10 18:44 ` [PATCH 1/3] Bluetooth: Fix hci core device initialization Gustavo Padovan
2011-10-11 12:35 ` David Herrmann
2011-10-13 20:51 ` Gustavo Padovan
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).