From: Oleg Nesterov <oleg@redhat.com>
To: Tobias Klauser <tklauser@distanz.ch>
Cc: Matt Mooney <mfm@muteddisk.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-kernel@vger.kernel.org
Subject: [PATCH] staging: usbip: fix the usage of kthread_stop()
Date: Tue, 13 Mar 2012 19:07:18 +0100 [thread overview]
Message-ID: <20120313180718.GA9224@redhat.com> (raw)
In-Reply-To: <20120313114512.GM21503@distanz.ch>
On 03/13, Tobias Klauser wrote:
>
> Looks OK to me at a first glance. I'll try to dig up my test system
> where I was able to produce that NULL pointer dereference and test your
> patch there and will then let you know my results.
Oh, great. Please find the patch below.
Of course it wasn't tested at all ;)
-----------------------------------------------------------------------
>From 8bec01c69c2b523e93432d36f9e211b6ec4a6771 Mon Sep 17 00:00:00 2001
From: Oleg Nesterov <oleg@redhat.com>
Date: Tue, 13 Mar 2012 18:55:20 +0100
Subject: [PATCH] staging: usbip: fix the usage of kthread_stop()
stub_shutdown_connection() and vhci_shutdown_connection() use
task_is_dead() before kthread_stop(). This buys nothing and wrong.
kthread_stop() is fine even if this thread is dead. However, if it
is dead nothing protects this task_struct, we shouldn't touch this
memory.
Change the code to do the necessary get_task_struct/put_task_struct.
This patch assumes that
- xxx_shutdown_connection() is always called, so we can't
leak the task_struct.
- kthread_stop_put() can't be called twice on the same task.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
drivers/staging/usbip/stub_dev.c | 12 ++++++------
drivers/staging/usbip/usbip_common.h | 17 +++++++++++++++++
drivers/staging/usbip/vhci_hcd.c | 8 ++++----
drivers/staging/usbip/vhci_sysfs.c | 4 ++--
4 files changed, 29 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/usbip/stub_dev.c b/drivers/staging/usbip/stub_dev.c
index 03420e2..fc6ceca 100644
--- a/drivers/staging/usbip/stub_dev.c
+++ b/drivers/staging/usbip/stub_dev.c
@@ -113,8 +113,8 @@ static ssize_t store_sockfd(struct device *dev, struct device_attribute *attr,
spin_unlock(&sdev->ud.lock);
- sdev->ud.tcp_rx = kthread_run(stub_rx_loop, &sdev->ud, "stub_rx");
- sdev->ud.tcp_tx = kthread_run(stub_tx_loop, &sdev->ud, "stub_tx");
+ sdev->ud.tcp_rx = kthread_get_run(stub_rx_loop, &sdev->ud, "stub_rx");
+ sdev->ud.tcp_tx = kthread_get_run(stub_tx_loop, &sdev->ud, "stub_tx");
spin_lock(&sdev->ud.lock);
sdev->ud.status = SDEV_ST_USED;
@@ -187,10 +187,10 @@ static void stub_shutdown_connection(struct usbip_device *ud)
}
/* 1. stop threads */
- if (ud->tcp_rx && !task_is_dead(ud->tcp_rx))
- kthread_stop(ud->tcp_rx);
- if (ud->tcp_tx && !task_is_dead(ud->tcp_tx))
- kthread_stop(ud->tcp_tx);
+ if (ud->tcp_rx)
+ kthread_stop_put(ud->tcp_rx);
+ if (ud->tcp_tx)
+ kthread_stop_put(ud->tcp_tx);
/*
* 2. close the socket
diff --git a/drivers/staging/usbip/usbip_common.h b/drivers/staging/usbip/usbip_common.h
index b8f8c48..0d52234 100644
--- a/drivers/staging/usbip/usbip_common.h
+++ b/drivers/staging/usbip/usbip_common.h
@@ -292,6 +292,23 @@ struct usbip_device {
} eh_ops;
};
+#define kthread_get_run(threadfn, data, namefmt, ...) \
+({ \
+ struct task_struct *__k \
+ = kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \
+ if (!IS_ERR(__k)) { \
+ get_task_struct(__k); \
+ wake_up_process(__k); \
+ } \
+ __k; \
+})
+
+#define kthread_stop_put(k) \
+ do { \
+ kthread_stop(k); \
+ put_task_struct(k); \
+ } while (0)
+
/* usbip_common.c */
void usbip_dump_urb(struct urb *purb);
void usbip_dump_header(struct usbip_header *pdu);
diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
index 2ee97e2..fb50fdc 100644
--- a/drivers/staging/usbip/vhci_hcd.c
+++ b/drivers/staging/usbip/vhci_hcd.c
@@ -860,10 +860,10 @@ static void vhci_shutdown_connection(struct usbip_device *ud)
}
/* kill threads related to this sdev, if v.c. exists */
- if (vdev->ud.tcp_rx && !task_is_dead(vdev->ud.tcp_rx))
- kthread_stop(vdev->ud.tcp_rx);
- if (vdev->ud.tcp_tx && !task_is_dead(vdev->ud.tcp_tx))
- kthread_stop(vdev->ud.tcp_tx);
+ if (vdev->ud.tcp_rx)
+ kthread_stop_put(vdev->ud.tcp_rx);
+ if (vdev->ud.tcp_tx)
+ kthread_stop_put(vdev->ud.tcp_tx);
pr_info("stop threads\n");
diff --git a/drivers/staging/usbip/vhci_sysfs.c b/drivers/staging/usbip/vhci_sysfs.c
index 0cd039b..7ce9c2f 100644
--- a/drivers/staging/usbip/vhci_sysfs.c
+++ b/drivers/staging/usbip/vhci_sysfs.c
@@ -222,8 +222,8 @@ static ssize_t store_attach(struct device *dev, struct device_attribute *attr,
spin_unlock(&the_controller->lock);
/* end the lock */
- vdev->ud.tcp_rx = kthread_run(vhci_rx_loop, &vdev->ud, "vhci_rx");
- vdev->ud.tcp_tx = kthread_run(vhci_tx_loop, &vdev->ud, "vhci_tx");
+ vdev->ud.tcp_rx = kthread_get_run(vhci_rx_loop, &vdev->ud, "vhci_rx");
+ vdev->ud.tcp_tx = kthread_get_run(vhci_tx_loop, &vdev->ud, "vhci_tx");
rh_port_connect(rhport, speed);
--
1.5.5.1
next prev parent reply other threads:[~2012-03-13 18:14 UTC|newest]
Thread overview: 63+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-09-19 21:45 [PATCH] user namespace: make signal.c respect user namespaces Serge E. Hallyn
2011-09-19 21:47 ` [PATCH] user namespace: usb: make usb urbs user namespace aware Serge E. Hallyn
2011-09-20 13:17 ` Oleg Nesterov
2011-09-20 13:33 ` Serge E. Hallyn
2011-09-21 5:01 ` [PATCH] user namespace: usb: make usb urbs user namespace aware (v2) Serge E. Hallyn
2011-09-21 18:31 ` Oleg Nesterov
2011-09-21 19:12 ` Serge E. Hallyn
2011-09-21 19:18 ` Greg KH
2011-09-23 1:27 ` [PATCH resend] " Serge E. Hallyn
2011-09-23 15:48 ` Alan Stern
2011-09-23 16:06 ` Serge E. Hallyn
2011-09-23 16:21 ` Alan Stern
2011-09-23 17:22 ` Serge E. Hallyn
2011-09-23 18:35 ` Alan Stern
2011-09-20 12:22 ` [PATCH] user namespace: make signal.c respect user namespaces Oleg Nesterov
2011-09-20 12:44 ` Serge E. Hallyn
2011-09-20 13:41 ` Oleg Nesterov
2011-09-20 14:39 ` [PATCH 0/2] (Was: user namespace: make signal.c respect user namespaces) Oleg Nesterov
2011-09-20 14:39 ` [PATCH 1/2] creds: kill __task_cred()->task_is_dead() check Oleg Nesterov
2011-09-20 15:14 ` drivers/staging/usbip/ abuses task_is_dead/exit_state Oleg Nesterov
2011-09-20 18:38 ` Greg KH
2012-03-06 17:39 ` ping: " Oleg Nesterov
2012-03-06 19:30 ` Tobias Klauser
2012-03-08 18:57 ` Oleg Nesterov
2012-03-13 11:45 ` Tobias Klauser
2012-03-13 18:07 ` Oleg Nesterov [this message]
2012-04-01 23:17 ` [PATCH] staging: usbip: fix the usage of kthread_stop() Oleg Nesterov
2012-04-02 8:11 ` Tobias Klauser
2011-09-20 15:28 ` [PATCH 1/2] creds: kill __task_cred()->task_is_dead() check Paul E. McKenney
2011-09-20 15:40 ` Oleg Nesterov
2011-09-20 15:48 ` Paul E. McKenney
2011-09-20 14:39 ` [PATCH 2/2] creds: __task_cred(current) doesn't need rcu_read_lock_held() Oleg Nesterov
2011-09-20 15:07 ` Serge Hallyn
2011-09-20 15:35 ` Oleg Nesterov
2011-09-20 16:19 ` David Howells
2011-09-20 16:38 ` Oleg Nesterov
2011-09-20 16:50 ` David Howells
2011-09-20 17:13 ` Oleg Nesterov
2011-09-20 16:27 ` [PATCH 1/2] creds: kill __task_cred()->task_is_dead() check David Howells
2011-09-20 15:39 ` [PATCH] user namespace: make signal.c respect user namespaces Serge Hallyn
2011-09-20 16:24 ` Oleg Nesterov
2011-09-20 16:45 ` Serge E. Hallyn
2011-09-20 18:17 ` Oleg Nesterov
2011-09-21 5:00 ` [PATCH] user namespace: make signal.c respect user namespaces (v2) Serge E. Hallyn
2011-09-20 17:48 ` [PATCH] user namespace: make signal.c respect user namespaces Oleg Nesterov
2011-09-20 18:53 ` Serge E. Hallyn
2011-09-21 17:53 ` Oleg Nesterov
2011-09-22 15:23 ` Serge Hallyn
2011-09-23 16:31 ` Serge E. Hallyn
2011-09-23 17:36 ` Oleg Nesterov
2011-09-23 21:20 ` Serge E. Hallyn
2011-09-24 16:37 ` Oleg Nesterov
2011-09-25 20:17 ` Serge E. Hallyn
2011-09-26 16:06 ` Oleg Nesterov
2011-09-27 14:28 ` Serge Hallyn
2011-09-27 14:38 ` Oleg Nesterov
2011-09-27 15:27 ` Serge Hallyn
2011-09-27 17:12 ` Oleg Nesterov
2011-10-04 17:42 ` Serge E. Hallyn
2011-10-09 19:00 ` Oleg Nesterov
2011-10-11 13:08 ` Serge E. Hallyn
2011-10-08 20:02 ` Serge E. Hallyn
2011-10-09 19:03 ` Oleg Nesterov
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=20120313180718.GA9224@redhat.com \
--to=oleg@redhat.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mfm@muteddisk.com \
--cc=tklauser@distanz.ch \
/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 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).