From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
To: Shuah Khan <skhan@linuxfoundation.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Valentina Manea <valentina.manea.m@gmail.com>,
Shuah Khan <shuah@kernel.org>
Cc: Hillf Danton <hdanton@sina.com>,
linux-usb@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>
Subject: Re: [PATCH] usb: usbip: fix error handling of kthread_get_run()
Date: Thu, 11 Feb 2021 12:01:57 +0900 [thread overview]
Message-ID: <16608833-02b7-7ba8-e9fc-7e45c87fc7f1@i-love.sakura.ne.jp> (raw)
In-Reply-To: <f8cae6b1-8f84-0e6a-7d9c-fc4aec68f07b@i-love.sakura.ne.jp>
On 2021/02/11 10:04, Tetsuo Handa wrote:
> On 2021/02/11 5:15, Shuah Khan wrote:
>> I you would like to fix this, please send me a complete fix.
>
> If you want to handle the unlikely "__kthread_create_on_node() fails without
> being killed" case, such change ( the drivers/usb/usbip/vhci_sysfs.c portion in
> https://syzkaller.appspot.com/x/patch.diff?x=16c3c090d00000 ) should be a separate
> patch. Since this patch declares "Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread")",
> this patch intends for the minimal change and does not want to do extra things.
>
If you want a complete fix, the (untested) diff will become large.
drivers/usb/usbip/stub_dev.c | 61 ++++++++++++++++++++++------------
drivers/usb/usbip/vhci_sysfs.c | 33 +++++++++++++++---
drivers/usb/usbip/vudc_sysfs.c | 35 +++++++++++++++----
3 files changed, 95 insertions(+), 34 deletions(-)
diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index 2305d425e6c9..72c561878df7 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -39,77 +39,94 @@ static DEVICE_ATTR_RO(usbip_status);
* is used to transfer usbip requests by kernel threads. -1 is a magic number
* by which usbip connection is finished.
*/
static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
struct stub_device *sdev = dev_get_drvdata(dev);
int sockfd = 0;
struct socket *socket;
int rv;
+ int err;
+ struct task_struct *tx = NULL;
+ struct task_struct *rx = NULL;
if (!sdev) {
dev_err(dev, "sdev is null\n");
return -ENODEV;
}
rv = sscanf(buf, "%d", &sockfd);
if (rv != 1)
return -EINVAL;
if (sockfd != -1) {
- int err;
+ socket = sockfd_lookup(sockfd, &err);
+ if (!socket)
+ return -EINVAL;
+
+ /* Create threads now. */
+ rx = kthread_create(stub_rx_loop, &sdev->ud, "stub_rx");
+ tx = kthread_create(stub_tx_loop, &sdev->ud, "stub_tx");
+ if (IS_ERR(rx) || IS_ERR(tx))
+ goto thread_error;
dev_info(dev, "stub up\n");
spin_lock_irq(&sdev->ud.lock);
if (sdev->ud.status != SDEV_ST_AVAILABLE) {
+ spin_unlock_irq(&sdev->ud.lock);
dev_err(dev, "not ready\n");
- goto err;
+ err = -EINVAL;
+ goto thread_error;
}
- socket = sockfd_lookup(sockfd, &err);
- if (!socket)
- goto err;
-
sdev->ud.tcp_socket = socket;
sdev->ud.sockfd = sockfd;
-
- spin_unlock_irq(&sdev->ud.lock);
-
- 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_irq(&sdev->ud.lock);
sdev->ud.status = SDEV_ST_USED;
spin_unlock_irq(&sdev->ud.lock);
+ /* Start the threads. */
+ get_task_struct(rx);
+ sdev->ud.tcp_rx = rx;
+ wake_up_process(rx);
+ get_task_struct(tx);
+ sdev->ud.tcp_tx = tx;
+ wake_up_process(tx);
+
} else {
dev_info(dev, "stub down\n");
spin_lock_irq(&sdev->ud.lock);
- if (sdev->ud.status != SDEV_ST_USED)
- goto err;
-
+ if (sdev->ud.status != SDEV_ST_USED) {
+ spin_unlock_irq(&sdev->ud.lock);
+ return -EINVAL;
+ }
spin_unlock_irq(&sdev->ud.lock);
+ /* Race warning: sdev->ud.status == SDEV_ST_USED may be no longer true. */
usbip_event_add(&sdev->ud, SDEV_EVENT_DOWN);
}
return count;
-
-err:
- spin_unlock_irq(&sdev->ud.lock);
- return -EINVAL;
+thread_error:
+ sockfd_put(socket);
+ if (IS_ERR(rx))
+ err = PTR_ERR(rx);
+ else if (rx)
+ kthread_stop(rx);
+ if (IS_ERR(tx))
+ err = PTR_ERR(tx);
+ else if (tx)
+ kthread_stop(tx);
+ return err;
}
static DEVICE_ATTR_WO(usbip_sockfd);
static struct attribute *usbip_attrs[] = {
&dev_attr_usbip_status.attr,
&dev_attr_usbip_sockfd.attr,
&dev_attr_usbip_debug.attr,
NULL,
};
ATTRIBUTE_GROUPS(usbip);
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index be37aec250c2..0d10021c4186 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -305,20 +305,22 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
{
struct socket *socket;
int sockfd = 0;
__u32 port = 0, pdev_nr = 0, rhport = 0, devid = 0, speed = 0;
struct usb_hcd *hcd;
struct vhci_hcd *vhci_hcd;
struct vhci_device *vdev;
struct vhci *vhci;
int err;
unsigned long flags;
+ struct task_struct *tx;
+ struct task_struct *rx;
/*
* @rhport: port number of vhci_hcd
* @sockfd: socket descriptor of an established TCP connection
* @devid: unique device identifier in a remote host
* @speed: usb device speed in a remote host
*/
if (sscanf(buf, "%u %u %u %u", &port, &sockfd, &devid, &speed) != 4)
return -EINVAL;
pdev_nr = port_to_pdev_nr(port);
@@ -345,62 +347,83 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
if (speed == USB_SPEED_SUPER)
vdev = &vhci->vhci_hcd_ss->vdev[rhport];
else
vdev = &vhci->vhci_hcd_hs->vdev[rhport];
/* Extract socket from fd. */
socket = sockfd_lookup(sockfd, &err);
if (!socket)
return -EINVAL;
+ /* Create threads now. */
+ rx = kthread_create(vhci_rx_loop, &vdev->ud, "vhci_rx");
+ tx = kthread_create(vhci_tx_loop, &vdev->ud, "vhci_tx");
+ if (IS_ERR(rx) || IS_ERR(tx))
+ goto thread_error;
+
/* now need lock until setting vdev status as used */
/* begin a lock */
spin_lock_irqsave(&vhci->lock, flags);
spin_lock(&vdev->ud.lock);
if (vdev->ud.status != VDEV_ST_NULL) {
/* end of the lock */
spin_unlock(&vdev->ud.lock);
spin_unlock_irqrestore(&vhci->lock, flags);
- sockfd_put(socket);
-
dev_err(dev, "port %d already used\n", rhport);
/*
* Will be retried from userspace
* if there's another free port.
*/
- return -EBUSY;
+ err = -EBUSY;
+ goto thread_error;
}
dev_info(dev, "pdev(%u) rhport(%u) sockfd(%d)\n",
pdev_nr, rhport, sockfd);
dev_info(dev, "devid(%u) speed(%u) speed_str(%s)\n",
devid, speed, usb_speed_string(speed));
vdev->devid = devid;
vdev->speed = speed;
vdev->ud.sockfd = sockfd;
vdev->ud.tcp_socket = socket;
vdev->ud.status = VDEV_ST_NOTASSIGNED;
spin_unlock(&vdev->ud.lock);
spin_unlock_irqrestore(&vhci->lock, flags);
/* end the lock */
- 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");
+ /* Start the threads. */
+ get_task_struct(rx);
+ vdev->ud.tcp_rx = rx;
+ wake_up_process(rx);
+ get_task_struct(tx);
+ vdev->ud.tcp_tx = tx;
+ wake_up_process(tx);
rh_port_connect(vdev, speed);
return count;
+thread_error:
+ sockfd_put(socket);
+ if (IS_ERR(rx))
+ err = PTR_ERR(rx);
+ else
+ kthread_stop(rx);
+ if (IS_ERR(tx))
+ err = PTR_ERR(tx);
+ else
+ kthread_stop(tx);
+ return err;
}
static DEVICE_ATTR_WO(attach);
#define MAX_STATUS_NAME 16
struct status_attr {
struct device_attribute attr;
char name[MAX_STATUS_NAME+1];
};
diff --git a/drivers/usb/usbip/vudc_sysfs.c b/drivers/usb/usbip/vudc_sysfs.c
index 100f680c572a..d01acb6d6b1c 100644
--- a/drivers/usb/usbip/vudc_sysfs.c
+++ b/drivers/usb/usbip/vudc_sysfs.c
@@ -93,29 +93,40 @@ static BIN_ATTR_RO(dev_desc, sizeof(struct usb_device_descriptor));
static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *attr,
const char *in, size_t count)
{
struct vudc *udc = (struct vudc *) dev_get_drvdata(dev);
int rv;
int sockfd = 0;
int err;
struct socket *socket;
unsigned long flags;
int ret;
+ struct task_struct *tx = NULL;
+ struct task_struct *rx = NULL;
rv = kstrtoint(in, 0, &sockfd);
if (rv != 0)
return -EINVAL;
if (!udc) {
dev_err(dev, "no device");
return -ENODEV;
}
+
+ /* Create threads now. */
+ if (sockfd != -1) {
+ rx = kthread_create(&v_rx_loop, &udc->ud, "vudc_rx");
+ tx = kthread_create(&v_tx_loop, &udc->ud, "vudc_tx");
+ if (IS_ERR(rx) || IS_ERR(tx))
+ goto thread_error;
+ }
+
spin_lock_irqsave(&udc->lock, flags);
/* Don't export what we don't have */
if (!udc->driver || !udc->pullup) {
dev_err(dev, "gadget not bound");
ret = -ENODEV;
goto unlock;
}
if (sockfd != -1) {
if (udc->connected) {
@@ -132,33 +143,34 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a
}
socket = sockfd_lookup(sockfd, &err);
if (!socket) {
dev_err(dev, "failed to lookup sock");
ret = -EINVAL;
goto unlock_ud;
}
udc->ud.tcp_socket = socket;
+ udc->ud.status = SDEV_ST_USED;
spin_unlock_irq(&udc->ud.lock);
spin_unlock_irqrestore(&udc->lock, flags);
- udc->ud.tcp_rx = kthread_get_run(&v_rx_loop,
- &udc->ud, "vudc_rx");
- udc->ud.tcp_tx = kthread_get_run(&v_tx_loop,
- &udc->ud, "vudc_tx");
+ /* Start the threads. */
+ get_task_struct(rx);
+ udc->ud.tcp_rx = rx;
+ wake_up_process(rx);
+ get_task_struct(tx);
+ udc->ud.tcp_tx = tx;
+ wake_up_process(tx);
spin_lock_irqsave(&udc->lock, flags);
- spin_lock_irq(&udc->ud.lock);
- udc->ud.status = SDEV_ST_USED;
- spin_unlock_irq(&udc->ud.lock);
ktime_get_ts64(&udc->start_time);
v_start_timer(udc);
udc->connected = 1;
} else {
if (!udc->connected) {
dev_err(dev, "Device not connected");
ret = -EINVAL;
goto unlock;
}
@@ -174,20 +186,29 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a
}
spin_unlock_irqrestore(&udc->lock, flags);
return count;
unlock_ud:
spin_unlock_irq(&udc->ud.lock);
unlock:
spin_unlock_irqrestore(&udc->lock, flags);
+thread_error:
+ if (IS_ERR(rx))
+ ret = PTR_ERR(rx);
+ else if (rx)
+ kthread_stop(rx);
+ if (IS_ERR(tx))
+ ret = PTR_ERR(tx);
+ else if (tx)
+ kthread_stop(tx);
return ret;
}
static DEVICE_ATTR_WO(usbip_sockfd);
static ssize_t usbip_status_show(struct device *dev,
struct device_attribute *attr, char *out)
{
struct vudc *udc = (struct vudc *) dev_get_drvdata(dev);
int status;
next prev parent reply other threads:[~2021-02-11 3:03 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-20 18:44 KASAN: null-ptr-deref Write in vhci_shutdown_connection syzbot
2021-02-05 13:57 ` [PATCH] usb: usbip: fix error handling of kthread_get_run() Tetsuo Handa
2021-02-05 16:27 ` Shuah Khan
2021-02-06 1:08 ` Tetsuo Handa
2021-02-10 18:11 ` Shuah Khan
2021-02-10 18:16 ` Tetsuo Handa
2021-02-10 18:20 ` Shuah Khan
2021-02-10 18:43 ` Tetsuo Handa
2021-02-10 20:15 ` Shuah Khan
2021-02-11 1:04 ` Tetsuo Handa
2021-02-11 3:01 ` Tetsuo Handa [this message]
2021-02-11 13:40 ` Tetsuo Handa
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=16608833-02b7-7ba8-e9fc-7e45c87fc7f1@i-love.sakura.ne.jp \
--to=penguin-kernel@i-love.sakura.ne.jp \
--cc=arnd@arndb.de \
--cc=gregkh@linuxfoundation.org \
--cc=hdanton@sina.com \
--cc=linux-usb@vger.kernel.org \
--cc=shuah@kernel.org \
--cc=skhan@linuxfoundation.org \
--cc=valentina.manea.m@gmail.com \
/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).