net: fix vnet_hdr bustage with slirp

PATCH Applied Mark McLoughlin 3 participants 5 messages Aug 7, 2009 – Aug 9, 2009 KVM

Share & tools

Mark McLoughlin

author

2009-08-07 08:54 UTC #0 lore raw

PATCH 1/1 net: fix vnet_hdr bustage with slirp

slirp has started using VLANClientState::opaque and this has caused the kvm specific tap_has_vnet_hdr() hack to break because we blindly use this opaque pointer even if it is not a tap client.

Add yet another hack to check that we're actually getting called with a tap client.

[Needed on stable-0.11 too]

Signed-off-by Mark McLoughlin <[email protected]>

---
 net.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

net.c +6 -0

index c7702f8..2428f63 100644
@@ -1521,6 +1521,9 @@ int tap_has_vnet_hdr(void *opaque)
     VLANClientState *vc = opaque;
     TAPState *s = vc->opaque;
 
+    if (vc->receive != tap_receive)
+        return 0;
+
     return s ? s->has_vnet_hdr : 0;
 }

@@ -1529,6 +1532,9 @@ void tap_using_vnet_hdr(void *opaque, int using_vnet_hdr)
     VLANClientState *vc = opaque;
     TAPState *s = vc->opaque;
 
+    if (vc->receive != tap_receive)
+        return;
+
     if (!s || !s->has_vnet_hdr)
         return;

Signature · 1 line

1.6.2.5

Arnd Bergmann

2009-08-07 11:51 UTC #1 lore raw

Quoted 10 lines · Mark McLoughlin

slirp has started using VLANClientState::opaque and this has caused the kvm specific tap_has_vnet_hdr() hack to break because we blindly use this opaque pointer even if it is not a tap client.
Add yet another hack to check that we're actually getting called with a tap client.
[Needed on stable-0.11 too]
Signed-off-by Mark McLoughlin <[email protected]>

Jens and I discovered the same bug before, but then we forgot about sending a fix (sorry). Your patch should work fine as a workaround, but I wonder if it is the right solution.

The abstraction of struct VLANClientState is otherwise done through function pointers taking the VLANClientState pointer as their first argument. IMHO a cleaner abstraction would be to do the same for tap_has_vnet_hdr(), like the patch below, and similar for other functions passing 'opaque' pointers.

Signed-off-by Arnd Bergmann <[email protected]>

hw/virtio-net.c +8 -6

index 6dfe758..6b34e82 100644
@@ -123,7 +123,7 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev)
     VirtIONet *n = to_virtio_net(vdev);
     VLANClientState *host = n->vc->vlan->first_client;
 
-    if (tap_has_vnet_hdr(host)) {
+    if (host->has_vnet_hdr && host->has_vnet_hdr(host)) {
         tap_using_vnet_hdr(host, 1);
         features |= (1 << VIRTIO_NET_F_CSUM);
         features |= (1 << VIRTIO_NET_F_GUEST_CSUM);

@@ -166,7 +166,7 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
     n->mergeable_rx_bufs = !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF));
 
 #ifdef TAP_VNET_HDR
-    if (!tap_has_vnet_hdr(host) || !host->set_offload)
+    if (!(host->has_vnet_hdr && host->has_vnet_hdr(host)) || !host->set_offload)
         return;
 
     host->set_offload(host,

@@ -398,7 +398,7 @@ static int receive_header(VirtIONet *n, struct iovec *iov, int iovcnt,
     hdr->gso_type = VIRTIO_NET_HDR_GSO_NONE;
 
 #ifdef TAP_VNET_HDR
-    if (tap_has_vnet_hdr(n->vc->vlan->first_client)) {
+    if ((host->has_vnet_hdr && host->has_vnet_hdr(n->vc->vlan->first_client))) {
         memcpy(hdr, buf, sizeof(*hdr));
         offset = sizeof(*hdr);
         work_around_broken_dhclient(hdr, buf + offset, size - offset);

@@ -425,7 +425,8 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
         return 1;
 
 #ifdef TAP_VNET_HDR
-    if (tap_has_vnet_hdr(n->vc->vlan->first_client))
+    if ((host->has_vnet_hdr &&
+	    host->has_vnet_hdr(n->vc->vlan->first_client)))
         ptr += sizeof(struct virtio_net_hdr);
 #endif

@@ -529,7 +530,8 @@ static void virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
 {
     VirtQueueElement elem;
 #ifdef TAP_VNET_HDR
-    int has_vnet_hdr = tap_has_vnet_hdr(n->vc->vlan->first_client);
+    int has_vnet_hdr = (host->has_vnet_hdr &&
+		    host->has_vnet_hdr(n->vc->vlan->first_client));
 #else
     int has_vnet_hdr = 0;
 #endif

@@ -620,7 +622,7 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
     qemu_put_buffer(f, (uint8_t *)n->vlans, MAX_VLAN >> 3);
 
 #ifdef TAP_VNET_HDR
-    qemu_put_be32(f, tap_has_vnet_hdr(n->vc->vlan->first_client));
+    qemu_put_be32(f, (host->has_vnet_hdr && host->has_vnet_hdr(n->vc->vlan->first_client)));
 #else
     qemu_put_be32(f, 0);
 #endif

net.c +3 -3

index 931def1..b56ae78 100644
@@ -754,7 +754,7 @@ static void vmchannel_read(void *opaque, const uint8_t *buf, int size)
 
 #ifdef _WIN32
 
-int tap_has_vnet_hdr(void *opaque)
+static int tap_has_vnet_hdr(struct VLANClientState *vc)
 {
     return 0;
 }

@@ -906,9 +906,8 @@ static void tap_send(void *opaque)
     } while (s->size > 0);
 }
 
-int tap_has_vnet_hdr(void *opaque)
+static int tap_has_vnet_hdr(struct VLANClientState *vc)
 {
-    VLANClientState *vc = opaque;
     TAPState *s = vc->opaque;
 
     return s ? s->has_vnet_hdr : 0;

@@ -991,6 +990,7 @@ static TAPState *net_tap_fd_init(VLANState *vlan,
     s->has_vnet_hdr = vnet_hdr != 0;
     s->vc = qemu_new_vlan_client(vlan, model, name, tap_receive,
                                  NULL, tap_cleanup, s);
+    s->vc->has_vnet_hdr = tap_has_vnet_hdr;
     s->vc->fd_readv = tap_receive_iov;
 #ifdef TUNSETOFFLOAD
     s->vc->set_offload = tap_set_offload;

net.h +1 -1

index bc42428..7c79734 100644
@@ -21,6 +21,7 @@ struct VLANClientState {
     IOCanRWHandler *fd_can_read;
     NetCleanup *cleanup;
     LinkStatusChanged *link_status_changed;
+    int (*has_vnet_hdr)(struct VLANClientState *);
     int link_down;
     SetOffload *set_offload;
     void *opaque;

@@ -72,7 +73,6 @@ void qemu_handler_true(void *opaque);
 void do_info_network(Monitor *mon);
 int do_set_link(Monitor *mon, const char *name, const char *up_or_down);
 
-int tap_has_vnet_hdr(void *opaque);
 void tap_using_vnet_hdr(void *opaque, int using_vnet_hdr);
 
 /* NIC info */

Mark McLoughlin

author

2009-08-07 12:20 UTC #2 lore raw

Quoted 16 lines · Arnd Bergmann

Quoted 10 lines · Mark McLoughlin
slirp has started using VLANClientState::opaque and this has caused the kvm specific tap_has_vnet_hdr() hack to break because we blindly use this opaque pointer even if it is not a tap client.
Add yet another hack to check that we're actually getting called with a tap client.
[Needed on stable-0.11 too]
Signed-off-by Mark McLoughlin <[email protected]>
Jens and I discovered the same bug before, but then we forgot about sending a fix (sorry). Your patch should work fine as a workaround, but I wonder if it is the right solution.

Quoted 5 lines

The abstraction of struct VLANClientState is otherwise done through function pointers taking the VLANClientState pointer as their first argument. IMHO a cleaner abstraction would be to do the same for tap_has_vnet_hdr(), like the patch below, and similar for other functions passing 'opaque' pointers.

Indeed, but using vc->vlan->first_client is a great big hole in the abstraction as it is.

The vnet_hdr code in qemu-kvm.git is a hack which we plan to (eventually) replace by allowing a nic to be paired directly with a backend.

Your patch is fine, but I'd suggest since both are a hack we stick with mine since it'll reduce merge conflicts. Both hacks will go away eventually, anyway.

Thanks,
Mark.

Arnd Bergmann

2009-08-07 13:20 UTC #3 lore raw

Quoted 7 lines · Mark McLoughlin

The vnet_hdr code in qemu-kvm.git is a hack which we plan to (eventually) replace by allowing a nic to be paired directly with a backend.
Your patch is fine, but I'd suggest since both are a hack we stick with mine since it'll reduce merge conflicts. Both hacks will go away eventually, anyway.

Ok, sounds good.

Thanks,

	Arnd <><

Avi Kivity

Applied

2009-08-09 09:47 UTC #4 lore raw

In reply to Mark McLoughlin

Quoted 8 lines · Mark McLoughlin

slirp has started using VLANClientState::opaque and this has caused the kvm specific tap_has_vnet_hdr() hack to break because we blindly use this opaque pointer even if it is not a tap client.
Add yet another hack to check that we're actually getting called with a tap client.

Applied, thanks.

Quoted 2 lines

[Needed on stable-0.11 too]

There as well.

Signature · 1 line

error compiling committee.c: too many arguments to function