[PATCH v2] Bluetooth: Fix error handling for l2cap_init()

[PATCH v2] Bluetooth: Fix error handling for l2cap_init()

[Anderson Lizardo]

create_singlethread_workqueue() may fail with errors such as -ENOMEM. If
this happens, the return value is not set to a negative value and the
module load will succeed. It will then crash on module unload because of
a destroy_workqueue() call on a NULL pointer.

Additionally, the _busy_wq workqueue is not being destroyed if any
errors happen on l2cap_init().

Signed-off-by: Anderson Lizardo <anderson.lizardo@openbossa.org>
---
 net/bluetooth/l2cap.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 18a802c..7980e24 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -4875,8 +4875,10 @@ static int __init l2cap_init(void)
 		return err;
 
 	_busy_wq = create_singlethread_workqueue("l2cap");
-	if (!_busy_wq)
-		goto error;
+	if (!_busy_wq) {
+		err = -ENOMEM;
+		goto error_busy_wq;
+	}
 
 	err = bt_sock_register(BTPROTO_L2CAP, &l2cap_sock_family_ops);
 	if (err < 0) {
@@ -4904,6 +4906,8 @@ static int __init l2cap_init(void)
 	return 0;
 
 error:
+	destroy_workqueue(_busy_wq);
+error_busy_wq:
 	proto_unregister(&l2cap_proto);
 	return err;
 }
-- 
1.7.0.4

Re: [PATCH v2] Bluetooth: Fix error handling for l2cap_init()

[Gustavo F. Padovan]

Hi Anderson,

* Anderson Lizardo <anderson.lizardo@openbossa.org> [2010-11-22 06:57:14 -0400]:

> create_singlethread_workqueue() may fail with errors such as -ENOMEM. If
> this happens, the return value is not set to a negative value and the
> module load will succeed. It will then crash on module unload because of
> a destroy_workqueue() call on a NULL pointer.
> 
> Additionally, the _busy_wq workqueue is not being destroyed if any
> errors happen on l2cap_init().
> 
> Signed-off-by: Anderson Lizardo <anderson.lizardo@openbossa.org>
> ---
>  net/bluetooth/l2cap.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 18a802c..7980e24 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -4875,8 +4875,10 @@ static int __init l2cap_init(void)
>  		return err;
>  
>  	_busy_wq = create_singlethread_workqueue("l2cap");
> -	if (!_busy_wq)
> -		goto error;
> +	if (!_busy_wq) {
> +		err = -ENOMEM;
> +		goto error_busy_wq;
> +	}

I prefer if you move the workqueue creation to after the
hci_register_proto block. That will make things easier.

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [PATCH v2] Bluetooth: Fix error handling for l2cap_init()

[Anderson Lizardo]

Hi Gustavo,

On Mon, Nov 22, 2010 at 1:34 PM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> * Anderson Lizardo <anderson.lizardo@openbossa.org> [2010-11-22 06:57:14 -0400]:
>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>> index 18a802c..7980e24 100644
>> --- a/net/bluetooth/l2cap.c
>> +++ b/net/bluetooth/l2cap.c
>> @@ -4875,8 +4875,10 @@ static int __init l2cap_init(void)
>>               return err;
>>
>>       _busy_wq = create_singlethread_workqueue("l2cap");
>> -     if (!_busy_wq)
>> -             goto error;
>> +     if (!_busy_wq) {
>> +             err = -ENOMEM;
>> +             goto error_busy_wq;
>> +     }
>
> I prefer if you move the workqueue creation to after the
> hci_register_proto block. That will make things easier.

I wonder if that might not introduce a race condition, because after
hci_register_proto() new connections may already arrive?

Or is it guaranteed to only happen after l2cap_init() has finished?

If not, I can make this change without problem.

Regards,
-- 
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil

Re: [PATCH v2] Bluetooth: Fix error handling for l2cap_init()

[Marcel Holtmann]

Hi Anderson,

> create_singlethread_workqueue() may fail with errors such as -ENOMEM. If
> this happens, the return value is not set to a negative value and the
> module load will succeed. It will then crash on module unload because of
> a destroy_workqueue() call on a NULL pointer.
> 
> Additionally, the _busy_wq workqueue is not being destroyed if any
> errors happen on l2cap_init().
> 
> Signed-off-by: Anderson Lizardo <anderson.lizardo@openbossa.org>
> ---
>  net/bluetooth/l2cap.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 18a802c..7980e24 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -4875,8 +4875,10 @@ static int __init l2cap_init(void)
>  		return err;
>  
>  	_busy_wq = create_singlethread_workqueue("l2cap");
> -	if (!_busy_wq)
> -		goto error;
> +	if (!_busy_wq) {
> +		err = -ENOMEM;
> +		goto error_busy_wq;
> +	}

aren't these returning PTR_ERR etc.?

Regards

Marcel

Re: [PATCH v2] Bluetooth: Fix error handling for l2cap_init()

[Anderson Lizardo]

Hi Marcel,

On Wed, Nov 24, 2010 at 8:42 AM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>> index 18a802c..7980e24 100644
>> --- a/net/bluetooth/l2cap.c
>> +++ b/net/bluetooth/l2cap.c
>> @@ -4875,8 +4875,10 @@ static int __init l2cap_init(void)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 return err;
>>
>> =A0 =A0 =A0 _busy_wq =3D create_singlethread_workqueue("l2cap");
>> - =A0 =A0 if (!_busy_wq)
>> - =A0 =A0 =A0 =A0 =A0 =A0 goto error;
>> + =A0 =A0 if (!_busy_wq) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 err =3D -ENOMEM;
>> + =A0 =A0 =A0 =A0 =A0 =A0 goto error_busy_wq;
>> + =A0 =A0 }
>
> aren't these returning PTR_ERR etc.?

No, create_singlethread_workqueue() is just a wrapper around
__alloc_workqueue_key(), which returns eiter a kzalloc()'ed pointer,
or NULL on error. There is no way to get the actual reason of the
error, but by taking a look at the function we can see most (if not
all) errors are -ENOMEM. Thus why I used it here.

Padovan: so how to proceed here: keep the patch as is and keep
semantics, of make your proposed changes (with a slightly risk of a
race condition and having _busy_wq NULL) ?

Regards,
--=20
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil

Re: [PATCH v2] Bluetooth: Fix error handling for l2cap_init()

[Gustavo F. Padovan]

Hi Anderson,

* Anderson Lizardo <anderson.lizardo@openbossa.org> [2010-11-24 11:13:30 -0=
400]:

> Hi Marcel,
>=20
> On Wed, Nov 24, 2010 at 8:42 AM, Marcel Holtmann <marcel@holtmann.org> wr=
ote:
> >> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> >> index 18a802c..7980e24 100644
> >> --- a/net/bluetooth/l2cap.c
> >> +++ b/net/bluetooth/l2cap.c
> >> @@ -4875,8 +4875,10 @@ static int __init l2cap_init(void)
> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 return err;
> >>
> >> =A0 =A0 =A0 _busy_wq =3D create_singlethread_workqueue("l2cap");
> >> - =A0 =A0 if (!_busy_wq)
> >> - =A0 =A0 =A0 =A0 =A0 =A0 goto error;
> >> + =A0 =A0 if (!_busy_wq) {
> >> + =A0 =A0 =A0 =A0 =A0 =A0 err =3D -ENOMEM;
> >> + =A0 =A0 =A0 =A0 =A0 =A0 goto error_busy_wq;
> >> + =A0 =A0 }
> >
> > aren't these returning PTR_ERR etc.?
>=20
> No, create_singlethread_workqueue() is just a wrapper around
> __alloc_workqueue_key(), which returns eiter a kzalloc()'ed pointer,
> or NULL on error. There is no way to get the actual reason of the
> error, but by taking a look at the function we can see most (if not
> all) errors are -ENOMEM. Thus why I used it here.
>=20
> Padovan: so how to proceed here: keep the patch as is and keep
> semantics, of make your proposed changes (with a slightly risk of a
> race condition and having _busy_wq NULL) ?

I'm not sure that my idea is right, so I have another option here. On
create_singlethread_workqueue error, just call proto_unregister() and
then return -ENOMEM, and destroy your workqueue under the label error.
This way we avoid create a new label and also have a simple error
handling there.

--=20
Gustavo F. Padovan
http://profusion.mobi