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.
--- Change log
Patch [1] - Add Tested-by tag Patch [2] - Add Tested-by tag - Fix typo - Fix error code in urb->status (-EPIPE->-EPROTO)
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 Tested-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 12/12/19 7:30 PM, 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 Tested-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)
Thanks Marek and Suwan for taking care of this.
Acked-by: Shuah Khan skhan@linuxfoundation.org
thanks, -- Shuah
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 transaction 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 Tested-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..00fc98741c5d 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 = -EPROTO; + goto error; + }
/* recv iso_packet_descriptor */ - if (usbip_recv_iso(ud, urb) < 0) - return; + if (usbip_recv_iso(ud, urb) < 0) { + urb->status = -EPROTO; + 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 12/12/19 7:30 PM, 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 transaction 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 Tested-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..00fc98741c5d 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 = -EPROTO;goto error;- }
/* recv iso_packet_descriptor */
- if (usbip_recv_iso(ud, urb) < 0)
return;
- if (usbip_recv_iso(ud, urb) < 0) {
urb->status = -EPROTO;goto error;- }
/* restore the padding in iso packets */ usbip_pad_iso(ud, urb); +error: if (usbip_dbg_flag_vhci_rx) usbip_dump_urb(urb);
Acked-by: Shuah Khan skhan@linuxfoundation.org
thanks, -- Shuah
linux-stable-mirror@lists.linaro.org
-
shuah -
Suwan Kim