On 2021/03/12 14:44, Tetsuo Handa wrote:
> And what you are missing in your [PATCH 4,5,6/6] is
>
> diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
> index c4457026d5ad..3c64bd06ab53 100644
> --- a/drivers/usb/usbip/vhci_sysfs.c
> +++ b/drivers/usb/usbip/vhci_sysfs.c
> @@ -423,6 +423,7 @@ static ssize_t attach_store(struct device *dev, struct
> device_attribute *attr,
> /* end the lock */
>
> wake_up_process(vdev->ud.tcp_rx);
> + schedule_timeout_uninterruptible(HZ); // Consider being preempted
> here.
> wake_up_process(vdev->ud.tcp_tx);
>
> rh_port_connect(vdev, speed);
>
> . wake_up_process(tcp_tx) can call vhci_shutdown_connection() before
> wake_up_process(tcp_tx) is called.
wake_up_process(tcp_rx) can call vhci_shutdown_connection() before
wake_up_process(tcp_tx) is called.
> Since vhci_shutdown_connection() destroys tcp_tx thread and releases tcp_tx
> memory via kthread_stop_put(tcp_tx),
> wake_up_process(tcp_tx) will access already freed memory. Your patch
> converted "NULL pointer dereference caused by
> failing to call kthread_stop_put(tcp_tx)" into "use after free caused by
> succeeding to call kthread_stop_put(tcp_tx)".
>
And note that
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index c4457026d5ad..0e1a81d4632c 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -422,11 +422,11 @@ static ssize_t attach_store(struct device *dev, struct
device_attribute *attr,
spin_unlock_irqrestore(&vhci->lock, flags);
/* end the lock */
- wake_up_process(vdev->ud.tcp_rx);
- wake_up_process(vdev->ud.tcp_tx);
-
rh_port_connect(vdev, speed);
+ wake_up_process(vdev->ud.tcp_tx);
+ wake_up_process(vdev->ud.tcp_rx);
+
return count;
}
static DEVICE_ATTR_WO(attach);
is as well not sufficient, for you are still missing
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index c4457026d5ad..c958f89a9196 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -422,11 +422,13 @@ static ssize_t attach_store(struct device *dev, struct
device_attribute *attr,
spin_unlock_irqrestore(&vhci->lock, flags);
/* end the lock */
- wake_up_process(vdev->ud.tcp_rx);
- wake_up_process(vdev->ud.tcp_tx);
+ schedule_timeout_uninterruptible(HZ); // Consider being preempted
here.
rh_port_connect(vdev, speed);
+ wake_up_process(vdev->ud.tcp_tx);
+ wake_up_process(vdev->ud.tcp_rx);
+
return count;
}
static DEVICE_ATTR_WO(attach);
because vhci_port_disconnect() from detach_store() can call
usbip_event_add(&vdev->ud, VDEV_EVENT_DOWN)
(same use after free bug regarding tcp_tx and tcp_rx) as soon as all shared
states are set up and
spinlocks are released.
What you had better consider first is how to protect
event_handler()/usbip_sockfd_store()/attach_store()/detach_store() functions
from concurrent calls. Please respond to
https://lkml.kernel.org/r/[email protected]
before you try to make further changes.
