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(-)
--
2.20.1
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 <[email protected]>
Signed-off-by: Suwan Kim <[email protected]>
---
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)
--
2.20.1
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 <[email protected]>
Signed-off-by: Suwan Kim <[email protected]>
---
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);
--
2.20.1
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 <[email protected]>
> Signed-off-by: Suwan Kim <[email protected]>
Tested-by: Marek Marczykowski-Górecki <[email protected]>
> ---
> 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);
>
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
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 <[email protected]>
> Signed-off-by: Suwan Kim <[email protected]>
Tested-by: Marek Marczykowski-Górecki <[email protected]>
> ---
> 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)
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
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 <[email protected]>
> Signed-off-by: Suwan Kim <[email protected]>
> ---
> 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 <[email protected]>
> > Signed-off-by: Suwan Kim <[email protected]>
> > ---
> > 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