[Qemu-devel] [PATCH 0/3] net: fix hub control flow (again)

[Qemu-devel] [PATCH 0/3] net: fix hub control flow (again)

[Stefan Hajnoczi]

Two networking issues have been observed:

1. When the USB network interface and dump net client are on the same "VLAN",
   the USB network interface drops packets.  This was assumed to be because the
   dump net client can receive packets all the time, whereas the USB network
   interface has a single buffer that the guest must empty.

The (incorrect) fix for #1 was to only broadcast "VLAN" packets when all net
clients can receive.  Once this code change was merged a new issue appeared:

2. When two NICs are on a "VLAN" they must both be up.  If one NIC is not up
   then no traffic can flow on the "VLAN".

This patch series fixes the root cause for #1, namely, that the USB network
interface clobbers its input buffer when usbnet_receive() is called before the guest has a chance to empty the buffer.

Then we can revert the "VLAN" flow control change which caused #2.  The result
is that both #1 and #2 are now fixed.

Stefan Hajnoczi (3):
  net: clean up usbnet_receive()
  net: fix usbnet_receive() packet drops
  net: broadcast hub packets if at least one port can receive

 hw/usb/dev-network.c |   49 +++++++++++++++++++++++++++++++++----------------
 net/hub.c            |    6 +++---
 2 files changed, 36 insertions(+), 19 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/3] net: clean up usbnet_receive()

[Stefan Hajnoczi]

The USB network interface has two code paths depending on whether or not
RNDIS mode is enabled.  Refactor usbnet_receive() so that there is a
common path throughout the function instead of duplicating everything
across if (is_rndis(s)) ... else ... code paths.

Clean up coding style and 80 character line wrap along the way.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/usb/dev-network.c |   30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index c84892c..0b5cb71 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -1250,20 +1250,27 @@ static int usb_net_handle_data(USBDevice *dev, USBPacket *p)
 static ssize_t usbnet_receive(NetClientState *nc, const uint8_t *buf, size_t size)
 {
     USBNetState *s = DO_UPCAST(NICState, nc, nc)->opaque;
-    struct rndis_packet_msg_type *msg;
+    uint8_t *in_buf = s->in_buf;
+    size_t total_size = size;
 
     if (is_rndis(s)) {
-        msg = (struct rndis_packet_msg_type *) s->in_buf;
         if (s->rndis_state != RNDIS_DATA_INITIALIZED) {
             return -1;
         }
-        if (size + sizeof(struct rndis_packet_msg_type) > sizeof(s->in_buf))
-            return -1;
+        total_size += sizeof(struct rndis_packet_msg_type);
+    }
+    if (total_size > sizeof(s->in_buf)) {
+        return -1;
+    }
 
+    if (is_rndis(s)) {
+        struct rndis_packet_msg_type *msg;
+
+        msg = (struct rndis_packet_msg_type *)in_buf;
         memset(msg, 0, sizeof(struct rndis_packet_msg_type));
         msg->MessageType = cpu_to_le32(RNDIS_PACKET_MSG);
-        msg->MessageLength = cpu_to_le32(size + sizeof(struct rndis_packet_msg_type));
-        msg->DataOffset = cpu_to_le32(sizeof(struct rndis_packet_msg_type) - 8);
+        msg->MessageLength = cpu_to_le32(size + sizeof(*msg));
+        msg->DataOffset = cpu_to_le32(sizeof(*msg) - 8);
         msg->DataLength = cpu_to_le32(size);
         /* msg->OOBDataOffset;
          * msg->OOBDataLength;
@@ -1273,14 +1280,11 @@ static ssize_t usbnet_receive(NetClientState *nc, const uint8_t *buf, size_t siz
          * msg->VcHandle;
          * msg->Reserved;
          */
-        memcpy(msg + 1, buf, size);
-        s->in_len = size + sizeof(struct rndis_packet_msg_type);
-    } else {
-        if (size > sizeof(s->in_buf))
-            return -1;
-        memcpy(s->in_buf, buf, size);
-        s->in_len = size;
+        in_buf += sizeof(*msg);
     }
+
+    memcpy(in_buf, buf, size);
+    s->in_len = total_size;
     s->in_ptr = 0;
     return size;
 }
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/3] net: fix usbnet_receive() packet drops

[Stefan Hajnoczi]

The USB network interface has a single buffer which the guest reads
from.  This patch prevents multiple calls to usbnet_receive() from
clobbering the input buffer.  Instead we queue packets until buffer
space becomes available again.

This is inspired by virtio-net and e1000 rxbuf handling.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/usb/dev-network.c |   19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 0b5cb71..e4a4359 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -1001,6 +1001,13 @@ static int rndis_keepalive_response(USBNetState *s,
     return 0;
 }
 
+/* Prepare to receive the next packet */
+static void usb_net_reset_in_buf(USBNetState *s)
+{
+    s->in_ptr = s->in_len = 0;
+    qemu_flush_queued_packets(&s->nic->nc);
+}
+
 static int rndis_parse(USBNetState *s, uint8_t *data, int length)
 {
     uint32_t msg_type;
@@ -1025,7 +1032,8 @@ static int rndis_parse(USBNetState *s, uint8_t *data, int length)
 
     case RNDIS_RESET_MSG:
         rndis_clear_responsequeue(s);
-        s->out_ptr = s->in_ptr = s->in_len = 0;
+        s->out_ptr = 0;
+        usb_net_reset_in_buf(s);
         return rndis_reset_response(s, (rndis_reset_msg_type *) data);
 
     case RNDIS_KEEPALIVE_MSG:
@@ -1135,7 +1143,7 @@ static int usb_net_handle_datain(USBNetState *s, USBPacket *p)
     int ret = USB_RET_NAK;
 
     if (s->in_ptr > s->in_len) {
-        s->in_ptr = s->in_len = 0;
+        usb_net_reset_in_buf(s);
         ret = USB_RET_NAK;
         return ret;
     }
@@ -1152,7 +1160,7 @@ static int usb_net_handle_datain(USBNetState *s, USBPacket *p)
     if (s->in_ptr >= s->in_len &&
                     (is_rndis(s) || (s->in_len & (64 - 1)) || !ret)) {
         /* no short packet necessary */
-        s->in_ptr = s->in_len = 0;
+        usb_net_reset_in_buf(s);
     }
 
 #ifdef TRAFFIC_DEBUG
@@ -1263,6 +1271,11 @@ static ssize_t usbnet_receive(NetClientState *nc, const uint8_t *buf, size_t siz
         return -1;
     }
 
+    /* Only accept packet if input buffer is empty */
+    if (s->in_len > 0) {
+        return 0;
+    }
+
     if (is_rndis(s)) {
         struct rndis_packet_msg_type *msg;
 
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/3] net: broadcast hub packets if at least one port can receive

[Stefan Hajnoczi]

In commit 60c07d933c66c4b30a83b7ccbc8a0cb3df1b2d0e ("net: fix
qemu_can_send_packet logic") the "VLAN" broadcast behavior was changed
to queue packets if any net client cannot receive.  It turns out that
this was not actually the right fix and just hides the real bug that
hw/usb/dev-network.c:usbnet_receive() clobbers its receive buffer when
called multiple times in a row.  The commit also introduced a new bug
that "VLAN" packets would not be sent if one of multiple net clients was
down.

The hw/usb/dev-network.c bug has since been fixed, so this patch reverts
broadcast behavior to send packets as long as one net client can
receive.  Packets simply get queued for the net clients that are
temporarily unable to receive.

Reported-by: Roy.Li <rongqing.li@windriver.com>
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 net/hub.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/hub.c b/net/hub.c
index ac157e3..650a8b4 100644
--- a/net/hub.c
+++ b/net/hub.c
@@ -97,12 +97,12 @@ static int net_hub_port_can_receive(NetClientState *nc)
             continue;
         }
 
-        if (!qemu_can_send_packet(&port->nc)) {
-            return 0;
+        if (qemu_can_send_packet(&port->nc)) {
+            return 1;
         }
     }
 
-    return 1;
+    return 0;
 }
 
 static ssize_t net_hub_port_receive(NetClientState *nc,
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 0/3] net: fix hub control flow (again)

[Stefan Hajnoczi]

On Fri, Aug 24, 2012 at 02:39:50PM +0100, Stefan Hajnoczi wrote:
> Two networking issues have been observed:
> 
> 1. When the USB network interface and dump net client are on the same "VLAN",
>    the USB network interface drops packets.  This was assumed to be because the
>    dump net client can receive packets all the time, whereas the USB network
>    interface has a single buffer that the guest must empty.
> 
> The (incorrect) fix for #1 was to only broadcast "VLAN" packets when all net
> clients can receive.  Once this code change was merged a new issue appeared:
> 
> 2. When two NICs are on a "VLAN" they must both be up.  If one NIC is not up
>    then no traffic can flow on the "VLAN".
> 
> This patch series fixes the root cause for #1, namely, that the USB network
> interface clobbers its input buffer when usbnet_receive() is called before the guest has a chance to empty the buffer.
> 
> Then we can revert the "VLAN" flow control change which caused #2.  The result
> is that both #1 and #2 are now fixed.
> 
> Stefan Hajnoczi (3):
>   net: clean up usbnet_receive()
>   net: fix usbnet_receive() packet drops
>   net: broadcast hub packets if at least one port can receive
> 
>  hw/usb/dev-network.c |   49 +++++++++++++++++++++++++++++++++----------------
>  net/hub.c            |    6 +++---
>  2 files changed, 36 insertions(+), 19 deletions(-)
> 
> -- 
> 1.7.10.4
> 
> 

Applied to the net tree:
https://github.com/stefanha/qemu/commits/net

Stefan