usbip: Fix infinite loop in vhci rx
https://lore.kernel.org/linux-usb/20191206032406.GE1208@mail-itl/T/#u In this mail thread, it shows system hang when there is receive error in vhci. There are two different causes in this bug.
[1] Wrong receive logic in vhci when using scatter-gather [2] Wrong error path of vhci_recv_ret_submit()
[1] considers normal reception to be an error condition and closes connection. And when [1] error situation occurs, wrong error path[2] causes the system freeze. So each patch fixes this bugs.
Suwan Kim (2): usbip: Fix receive error in vhci-hcd when using scatter-gather usbip: Fix error path of vhci_recv_ret_submit()
drivers/usb/usbip/usbip_common.c | 3 +++ drivers/usb/usbip/vhci_rx.c | 13 +++++++++---- 2 files changed, 12 insertions(+), 4 deletions(-)
When vhci uses SG and receives data whose size is smaller than SG buffer size, it tries to receive more data even if it acutally receives all the data from the server. If then, it erroneously adds error event and triggers connection shutdown.
vhci-hcd should check if it received all the data even if there are more SG entries left. So, check if it receivces all the data from the server in for_each_sg() loop.
Fixes: ea44d190764b ("usbip: Implement SG support to vhci-hcd and stub driver") Reported-by: Marek Marczykowski-Górecki marmarek@invisiblethingslab.com Signed-off-by: Suwan Kim suwan.kim027@gmail.com --- drivers/usb/usbip/usbip_common.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c index 6532d68e8808..e4b96674c405 100644 --- a/drivers/usb/usbip/usbip_common.c +++ b/drivers/usb/usbip/usbip_common.c @@ -727,6 +727,9 @@ int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb)
copy -= recv; ret += recv; + + if (!copy) + break; }
if (ret != size)
On Thu, Dec 12, 2019 at 02:28:40PM +0900, Suwan Kim wrote:
When vhci uses SG and receives data whose size is smaller than SG buffer size, it tries to receive more data even if it acutally receives all the data from the server. If then, it erroneously adds error event and triggers connection shutdown.
vhci-hcd should check if it received all the data even if there are more SG entries left. So, check if it receivces all the data from the server in for_each_sg() loop.
Fixes: ea44d190764b ("usbip: Implement SG support to vhci-hcd and stub driver") Reported-by: Marek Marczykowski-Górecki marmarek@invisiblethingslab.com Signed-off-by: Suwan Kim suwan.kim027@gmail.com
Tested-by: Marek Marczykowski-Górecki marmarek@invisiblethingslab.com
drivers/usb/usbip/usbip_common.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c index 6532d68e8808..e4b96674c405 100644 --- a/drivers/usb/usbip/usbip_common.c +++ b/drivers/usb/usbip/usbip_common.c @@ -727,6 +727,9 @@ int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb) copy -= recv; ret += recv;
if (!copy)break;if (ret != size)
If a transaction error happens in vhci_recv_ret_submit(), event handler closes connection and changes port status to kick hub_event. Then hub tries to flush the endpoint URBs, but that causes infinite loop between usb_hub_flush_endpoint() and vhci_urb_dequeue() because "vhci_priv" in vhci_urb_dequeue() was already released by vhci_recv_ret_submit() before a transmission error occurred. Thus, vhci_urb_dequeue() terminates early and usb_hub_flush_endpoint() continuously calls vhci_urb_dequeue().
The root cause of this issue is that vhci_recv_ret_submit() terminates early without giving back URB when transaction error occurs in vhci_recv_ret_submit(). That causes the error URB to still be linked at endpoint list without “vhci_priv".
So, in the case of trasnaction error in vhci_recv_ret_submit(), unlink URB from the endpoint, insert proper error code in urb->status and give back URB.
Reported-by: Marek Marczykowski-Górecki marmarek@invisiblethingslab.com Signed-off-by: Suwan Kim suwan.kim027@gmail.com --- drivers/usb/usbip/vhci_rx.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c index 33f8972ba842..dc26acad6baf 100644 --- a/drivers/usb/usbip/vhci_rx.c +++ b/drivers/usb/usbip/vhci_rx.c @@ -77,16 +77,21 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev, usbip_pack_pdu(pdu, urb, USBIP_RET_SUBMIT, 0);
/* recv transfer buffer */ - if (usbip_recv_xbuff(ud, urb) < 0) - return; + if (usbip_recv_xbuff(ud, urb) < 0) { + urb->status = -EPIPE; + goto error; + }
/* recv iso_packet_descriptor */ - if (usbip_recv_iso(ud, urb) < 0) - return; + if (usbip_recv_iso(ud, urb) < 0) { + urb->status = -EPIPE; + goto error; + }
/* restore the padding in iso packets */ usbip_pad_iso(ud, urb);
+error: if (usbip_dbg_flag_vhci_rx) usbip_dump_urb(urb);
On Thu, Dec 12, 2019 at 02:28:41PM +0900, Suwan Kim wrote:
If a transaction error happens in vhci_recv_ret_submit(), event handler closes connection and changes port status to kick hub_event. Then hub tries to flush the endpoint URBs, but that causes infinite loop between usb_hub_flush_endpoint() and vhci_urb_dequeue() because "vhci_priv" in vhci_urb_dequeue() was already released by vhci_recv_ret_submit() before a transmission error occurred. Thus, vhci_urb_dequeue() terminates early and usb_hub_flush_endpoint() continuously calls vhci_urb_dequeue().
The root cause of this issue is that vhci_recv_ret_submit() terminates early without giving back URB when transaction error occurs in vhci_recv_ret_submit(). That causes the error URB to still be linked at endpoint list without “vhci_priv".
So, in the case of trasnaction error in vhci_recv_ret_submit(),
^^^ typo
unlink URB from the endpoint, insert proper error code in urb->status and give back URB.
Reported-by: Marek Marczykowski-Górecki marmarek@invisiblethingslab.com Signed-off-by: Suwan Kim suwan.kim027@gmail.com
Tested-by: Marek Marczykowski-Górecki marmarek@invisiblethingslab.com
drivers/usb/usbip/vhci_rx.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c index 33f8972ba842..dc26acad6baf 100644 --- a/drivers/usb/usbip/vhci_rx.c +++ b/drivers/usb/usbip/vhci_rx.c @@ -77,16 +77,21 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev, usbip_pack_pdu(pdu, urb, USBIP_RET_SUBMIT, 0); /* recv transfer buffer */
- if (usbip_recv_xbuff(ud, urb) < 0)
return;
- if (usbip_recv_xbuff(ud, urb) < 0) {
urb->status = -EPIPE;goto error;- }
/* recv iso_packet_descriptor */
- if (usbip_recv_iso(ud, urb) < 0)
return;
- if (usbip_recv_iso(ud, urb) < 0) {
urb->status = -EPIPE;goto error;- }
/* restore the padding in iso packets */ usbip_pad_iso(ud, urb); +error: if (usbip_dbg_flag_vhci_rx) usbip_dump_urb(urb);
On Thu, 12 Dec 2019, Suwan Kim wrote:
If a transaction error happens in vhci_recv_ret_submit(), event handler closes connection and changes port status to kick hub_event. Then hub tries to flush the endpoint URBs, but that causes infinite loop between usb_hub_flush_endpoint() and vhci_urb_dequeue() because "vhci_priv" in vhci_urb_dequeue() was already released by vhci_recv_ret_submit() before a transmission error occurred. Thus, vhci_urb_dequeue() terminates early and usb_hub_flush_endpoint() continuously calls vhci_urb_dequeue().
The root cause of this issue is that vhci_recv_ret_submit() terminates early without giving back URB when transaction error occurs in vhci_recv_ret_submit(). That causes the error URB to still be linked at endpoint list without “vhci_priv".
So, in the case of trasnaction error in vhci_recv_ret_submit(), unlink URB from the endpoint, insert proper error code in urb->status and give back URB.
Reported-by: Marek Marczykowski-Górecki marmarek@invisiblethingslab.com Signed-off-by: Suwan Kim suwan.kim027@gmail.com
drivers/usb/usbip/vhci_rx.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c index 33f8972ba842..dc26acad6baf 100644 --- a/drivers/usb/usbip/vhci_rx.c +++ b/drivers/usb/usbip/vhci_rx.c @@ -77,16 +77,21 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev, usbip_pack_pdu(pdu, urb, USBIP_RET_SUBMIT, 0); /* recv transfer buffer */
- if (usbip_recv_xbuff(ud, urb) < 0)
return;
- if (usbip_recv_xbuff(ud, urb) < 0) {
urb->status = -EPIPE;goto error;- }
/* recv iso_packet_descriptor */
- if (usbip_recv_iso(ud, urb) < 0)
return;
- if (usbip_recv_iso(ud, urb) < 0) {
urb->status = -EPIPE;goto error;- }
-EPIPE is used for STALL. The appropriate error code for transaction error would be -EPROTO (or -EILSEQ or -ETIME, but people seem to be settling on -EPROTO).
Alan Stern
On Thu, Dec 12, 2019 at 10:54:08AM -0500, Alan Stern wrote:
On Thu, 12 Dec 2019, Suwan Kim wrote:
If a transaction error happens in vhci_recv_ret_submit(), event handler closes connection and changes port status to kick hub_event. Then hub tries to flush the endpoint URBs, but that causes infinite loop between usb_hub_flush_endpoint() and vhci_urb_dequeue() because "vhci_priv" in vhci_urb_dequeue() was already released by vhci_recv_ret_submit() before a transmission error occurred. Thus, vhci_urb_dequeue() terminates early and usb_hub_flush_endpoint() continuously calls vhci_urb_dequeue().
The root cause of this issue is that vhci_recv_ret_submit() terminates early without giving back URB when transaction error occurs in vhci_recv_ret_submit(). That causes the error URB to still be linked at endpoint list without “vhci_priv".
So, in the case of trasnaction error in vhci_recv_ret_submit(), unlink URB from the endpoint, insert proper error code in urb->status and give back URB.
Reported-by: Marek Marczykowski-Górecki marmarek@invisiblethingslab.com Signed-off-by: Suwan Kim suwan.kim027@gmail.com
drivers/usb/usbip/vhci_rx.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c index 33f8972ba842..dc26acad6baf 100644 --- a/drivers/usb/usbip/vhci_rx.c +++ b/drivers/usb/usbip/vhci_rx.c @@ -77,16 +77,21 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev, usbip_pack_pdu(pdu, urb, USBIP_RET_SUBMIT, 0); /* recv transfer buffer */
- if (usbip_recv_xbuff(ud, urb) < 0)
return;
- if (usbip_recv_xbuff(ud, urb) < 0) {
urb->status = -EPIPE;goto error;- }
/* recv iso_packet_descriptor */
- if (usbip_recv_iso(ud, urb) < 0)
return;
- if (usbip_recv_iso(ud, urb) < 0) {
urb->status = -EPIPE;goto error;- }
-EPIPE is used for STALL. The appropriate error code for transaction error would be -EPROTO (or -EILSEQ or -ETIME, but people seem to be settling on -EPROTO).
Thanks for the feedback. I will fix it :)
Regards, Suwan Kim
linux-stable-mirror@lists.linaro.org
-
Alan Stern -
Marek Marczykowski-Górecki -
Suwan Kim