* [PATCH] Bluetooth: hidp: Fix module reference cleanup
@ 2011-10-29 20:34 David Herrmann
2011-10-30 14:26 ` Anderson Lizardo
0 siblings, 1 reply; 6+ messages in thread
From: David Herrmann @ 2011-10-29 20:34 UTC (permalink / raw)
To: linux-bluetooth; +Cc: marcel, padovan, David Herrmann
Calling module_put(THIS_MODULE) is *never* safe when we cannot go sure that we
own at least two references. This is because the call may unload our module
before it returns and then the "return" will jump into invalid memory.
Gladly, module.h provides a wrapper for kthread-users: module_put_and_exit().
This puts our module and then exits the kthread without returning to the module.
This patch fixes the hidp kthread to use this wrapper instead of manually
freeing its own reference. See nfsd or lockd for other kthreads using this.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
net/bluetooth/hidp/core.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index fb68f34..5a470fc 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -95,7 +95,6 @@ static struct hidp_session *__hidp_get_session(bdaddr_t *bdaddr)
static void __hidp_link_session(struct hidp_session *session)
{
- __module_get(THIS_MODULE);
list_add(&session->list, &hidp_session_list);
hci_conn_hold_device(session->conn);
@@ -106,7 +105,6 @@ static void __hidp_unlink_session(struct hidp_session *session)
hci_conn_put_device(session->conn);
list_del(&session->list);
- module_put(THIS_MODULE);
}
static void __hidp_copy_session(struct hidp_session *session, struct hidp_conninfo *ci)
@@ -766,6 +764,7 @@ static int hidp_session(void *arg)
kfree(session->rd_data);
kfree(session);
+ module_put_and_exit(0);
return 0;
}
@@ -1033,6 +1032,7 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
product = 0x0000;
}
+ __module_get(THIS_MODULE);
session->task = kthread_run(hidp_session, session, "khidpd_%04x%04x",
vendor, product);
if (IS_ERR(session->task)) {
--
1.7.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] Bluetooth: hidp: Fix module reference cleanup
2011-10-29 20:34 [PATCH] Bluetooth: hidp: Fix module reference cleanup David Herrmann
@ 2011-10-30 14:26 ` Anderson Lizardo
2011-10-30 14:33 ` [PATCH v2] " David Herrmann
0 siblings, 1 reply; 6+ messages in thread
From: Anderson Lizardo @ 2011-10-30 14:26 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-bluetooth, marcel, padovan
Hi David,
On Sat, Oct 29, 2011 at 4:34 PM, David Herrmann
<dh.herrmann@googlemail.com> wrote:
> @@ -1033,6 +1032,7 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
> product = 0x0000;
> }
>
> + __module_get(THIS_MODULE);
> session->task = kthread_run(hidp_session, session, "khidpd_%04x%04x",
> vendor, product);
> if (IS_ERR(session->task)) {
Can this leak if kthread_run() fails?
Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2] Bluetooth: hidp: Fix module reference cleanup
2011-10-30 14:26 ` Anderson Lizardo
@ 2011-10-30 14:33 ` David Herrmann
2011-11-01 16:12 ` Gustavo Padovan
0 siblings, 1 reply; 6+ messages in thread
From: David Herrmann @ 2011-10-30 14:33 UTC (permalink / raw)
To: linux-bluetooth; +Cc: padovan, marcel, anderson.lizardo, David Herrmann
Calling module_put(THIS_MODULE) is *never* safe when we cannot go sure that we
own at least two references. This is because the call may unload our module
before it returns and then the "return" will jump into invalid memory.
Gladly, module.h provides a wrapper for kthread-users: module_put_and_exit().
This puts our module and then exits the kthread without returning to the module.
This patch fixes the hidp kthread to use this wrapper instead of manually
freeing its own reference. See nfsd or lockd for other kthreads using this.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
V2: Fix error path of kthread_run(). We now take the module reference inside the
kthread. This is safe because the caller will wait for us until we set
"waiting_for_startup" to 0. Thanks to Anderson Lizardo for pointing this out.
net/bluetooth/hidp/core.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index fb68f34..e3a6340 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -95,7 +95,6 @@ static struct hidp_session *__hidp_get_session(bdaddr_t *bdaddr)
static void __hidp_link_session(struct hidp_session *session)
{
- __module_get(THIS_MODULE);
list_add(&session->list, &hidp_session_list);
hci_conn_hold_device(session->conn);
@@ -106,7 +105,6 @@ static void __hidp_unlink_session(struct hidp_session *session)
hci_conn_put_device(session->conn);
list_del(&session->list);
- module_put(THIS_MODULE);
}
static void __hidp_copy_session(struct hidp_session *session, struct hidp_conninfo *ci)
@@ -700,6 +698,7 @@ static int hidp_session(void *arg)
BT_DBG("session %p", session);
+ __module_get(THIS_MODULE);
set_user_nice(current, -15);
init_waitqueue_entry(&ctrl_wait, current);
@@ -766,6 +765,7 @@ static int hidp_session(void *arg)
kfree(session->rd_data);
kfree(session);
+ module_put_and_exit(0);
return 0;
}
--
1.7.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] Bluetooth: hidp: Fix module reference cleanup
2011-10-30 14:33 ` [PATCH v2] " David Herrmann
@ 2011-11-01 16:12 ` Gustavo Padovan
2011-11-01 16:27 ` [PATCH] " David Herrmann
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo Padovan @ 2011-11-01 16:12 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-bluetooth, marcel, anderson.lizardo
Hi David,
* David Herrmann <dh.herrmann@googlemail.com> [2011-10-30 15:33:43 +0100]:
> Calling module_put(THIS_MODULE) is *never* safe when we cannot go sure that we
> own at least two references. This is because the call may unload our module
> before it returns and then the "return" will jump into invalid memory.
>
> Gladly, module.h provides a wrapper for kthread-users: module_put_and_exit().
> This puts our module and then exits the kthread without returning to the module.
>
> This patch fixes the hidp kthread to use this wrapper instead of manually
> freeing its own reference. See nfsd or lockd for other kthreads using this.
>
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> ---
> V2: Fix error path of kthread_run(). We now take the module reference inside the
> kthread. This is safe because the caller will wait for us until we set
> "waiting_for_startup" to 0. Thanks to Anderson Lizardo for pointing this out.
>
> net/bluetooth/hidp/core.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
This one doesn't apply anymore, please rebase. Thanks.
Gustavo
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Bluetooth: hidp: Fix module reference cleanup
2011-11-01 16:12 ` Gustavo Padovan
@ 2011-11-01 16:27 ` David Herrmann
2011-11-01 17:25 ` Gustavo Padovan
0 siblings, 1 reply; 6+ messages in thread
From: David Herrmann @ 2011-11-01 16:27 UTC (permalink / raw)
To: linux-bluetooth; +Cc: padovan, David Herrmann
Calling module_put(THIS_MODULE) is *never* safe when we cannot go sure that we
own at least two references. This is because the call may unload our module
before it returns and then the "return" will jump into invalid memory.
Gladly, module.h provides a wrapper for kthread-users: module_put_and_exit().
This puts our module and then exits the kthread without returning to the module.
This patch fixes the hidp kthread to use this wrapper instead of manually
freeing its own reference. See nfsd or lockd for other kthreads using this.
Calling __module_get() inside the kthread is safe as the hidp module will always
wait until the kthread sets "waiting_for_startup" to 0.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
V3: I rebased on top of bluetooth-next plus the patches already pending on the
ML. It should apply cleanly now. Otherwise, just tell me what tree to use for
the rebase ;) Or try "git am -3 ..."
net/bluetooth/hidp/core.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 217ef47..ff42c20 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -93,7 +93,6 @@ static struct hidp_session *__hidp_get_session(bdaddr_t *bdaddr)
static void __hidp_link_session(struct hidp_session *session)
{
- __module_get(THIS_MODULE);
list_add(&session->list, &hidp_session_list);
}
@@ -102,7 +101,6 @@ static void __hidp_unlink_session(struct hidp_session *session)
hci_conn_put_device(session->conn);
list_del(&session->list);
- module_put(THIS_MODULE);
}
static void __hidp_copy_session(struct hidp_session *session, struct hidp_conninfo *ci)
@@ -702,6 +700,7 @@ static int hidp_session(void *arg)
BT_DBG("session %p", session);
+ __module_get(THIS_MODULE);
set_user_nice(current, -15);
init_waitqueue_entry(&ctrl_wait, current);
@@ -780,6 +779,7 @@ static int hidp_session(void *arg)
kfree(session->rd_data);
kfree(session);
+ module_put_and_exit(0);
return 0;
}
--
1.7.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] Bluetooth: hidp: Fix module reference cleanup
2011-11-01 16:27 ` [PATCH] " David Herrmann
@ 2011-11-01 17:25 ` Gustavo Padovan
0 siblings, 0 replies; 6+ messages in thread
From: Gustavo Padovan @ 2011-11-01 17:25 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-bluetooth
Hi David,
* David Herrmann <dh.herrmann@googlemail.com> [2011-11-01 17:27:50 +0100]:
> Calling module_put(THIS_MODULE) is *never* safe when we cannot go sure that we
> own at least two references. This is because the call may unload our module
> before it returns and then the "return" will jump into invalid memory.
>
> Gladly, module.h provides a wrapper for kthread-users: module_put_and_exit().
> This puts our module and then exits the kthread without returning to the module.
>
> This patch fixes the hidp kthread to use this wrapper instead of manually
> freeing its own reference. See nfsd or lockd for other kthreads using this.
>
> Calling __module_get() inside the kthread is safe as the hidp module will always
> wait until the kthread sets "waiting_for_startup" to 0.
>
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> ---
> V3: I rebased on top of bluetooth-next plus the patches already pending on the
> ML. It should apply cleanly now. Otherwise, just tell me what tree to use for
> the rebase ;) Or try "git am -3 ..."
>
> net/bluetooth/hidp/core.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
Applied, thanks.
Gustavo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-11-01 17:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-29 20:34 [PATCH] Bluetooth: hidp: Fix module reference cleanup David Herrmann
2011-10-30 14:26 ` Anderson Lizardo
2011-10-30 14:33 ` [PATCH v2] " David Herrmann
2011-11-01 16:12 ` Gustavo Padovan
2011-11-01 16:27 ` [PATCH] " David Herrmann
2011-11-01 17:25 ` 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).