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 16:27 ` David Howells
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 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.