[Qemu-devel] [PATCH 0/2] vmxnet3: Fix RX TCP/UDP checksum of partially summed packets

[Qemu-devel] [PATCH 0/2] vmxnet3: Fix RX TCP/UDP checksum of partially summed packets

[Shmulik Ladkani]

In case csum offloaded packet, vmxnet3 implementation always passes an
RxCompDesc with the "Checksum calculated and found correct" notification
to the OS. This emulates the observed ESXi behavior.

Therefore, if packet has the NEEDS_CSUM bit set, we must calculate and
place a fully computed checksum into the tcp/udp header. Otherwise, the
OS driver will receive a checksum-correct indication but with the actual
tcp/udp checksum field having just the pseudo header csum value.

Fix, by converting partially summed packets to be fully checksummed.

Shmulik Ladkani:
  net/vmxnet3: Refactor 'vmxnet_rx_pkt_attach_data'

Dana Rubin:
  net/vmxnet3: Fix RX TCP/UDP checksum on partially summed packets

 hw/net/vmxnet3.c       | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/net/vmxnet_rx_pkt.c | 12 +++++++---
 hw/net/vmxnet_rx_pkt.h | 11 ++++++++++
 3 files changed, 79 insertions(+), 3 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/2] net/vmxnet3: Refactor 'vmxnet_rx_pkt_attach_data'

[Shmulik Ladkani]

Separate RX packet protocol parsing out of 'vmxnet_rx_pkt_attach_data'.

Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
---
 hw/net/vmxnet3.c       |  1 +
 hw/net/vmxnet_rx_pkt.c | 12 +++++++++---
 hw/net/vmxnet_rx_pkt.h | 11 +++++++++++
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 706e060..dd22a0a 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1897,6 +1897,7 @@ vmxnet3_receive(NetClientState *nc, const uint8_t *buf, size_t size)
         get_eth_packet_type(PKT_GET_ETH_HDR(buf)));
 
     if (vmxnet3_rx_filter_may_indicate(s, buf, size)) {
+        vmxnet_rx_pkt_set_protocols(s->rx_pkt, buf, size);
         vmxnet_rx_pkt_attach_data(s->rx_pkt, buf, size, s->rx_vlan_stripping);
         bytes_indicated = vmxnet3_indicate_packet(s) ? size : -1;
         if (bytes_indicated < size) {
diff --git a/hw/net/vmxnet_rx_pkt.c b/hw/net/vmxnet_rx_pkt.c
index acbca6a..aa54629 100644
--- a/hw/net/vmxnet_rx_pkt.c
+++ b/hw/net/vmxnet_rx_pkt.c
@@ -92,9 +92,6 @@ void vmxnet_rx_pkt_attach_data(struct VmxnetRxPkt *pkt, const void *data,
     }
 
     pkt->tci = tci;
-
-    eth_get_protocols(data, len, &pkt->isip4, &pkt->isip6,
-        &pkt->isudp, &pkt->istcp);
 }
 
 void vmxnet_rx_pkt_dump(struct VmxnetRxPkt *pkt)
@@ -131,6 +128,15 @@ size_t vmxnet_rx_pkt_get_total_len(struct VmxnetRxPkt *pkt)
     return pkt->tot_len;
 }
 
+void vmxnet_rx_pkt_set_protocols(struct VmxnetRxPkt *pkt, const void *data,
+                                 size_t len)
+{
+    assert(pkt);
+
+    eth_get_protocols(data, len, &pkt->isip4, &pkt->isip6,
+        &pkt->isudp, &pkt->istcp);
+}
+
 void vmxnet_rx_pkt_get_protocols(struct VmxnetRxPkt *pkt,
                                  bool *isip4, bool *isip6,
                                  bool *isudp, bool *istcp)
diff --git a/hw/net/vmxnet_rx_pkt.h b/hw/net/vmxnet_rx_pkt.h
index 5f8352a..a425846 100644
--- a/hw/net/vmxnet_rx_pkt.h
+++ b/hw/net/vmxnet_rx_pkt.h
@@ -55,6 +55,17 @@ void vmxnet_rx_pkt_init(struct VmxnetRxPkt **pkt, bool has_virt_hdr);
 size_t vmxnet_rx_pkt_get_total_len(struct VmxnetRxPkt *pkt);
 
 /**
+ * parse and set packet analysis results
+ *
+ * @pkt:            packet
+ * @data:           pointer to the data buffer to be parsed
+ * @len:            data length
+ *
+ */
+void vmxnet_rx_pkt_set_protocols(struct VmxnetRxPkt *pkt, const void *data,
+                                 size_t len);
+
+/**
  * fetches packet analysis results
  *
  * @pkt:            packet
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/2] net/vmxnet3: Refactor 'vmxnet_rx_pkt_attach_data'

[Dmitry Fleytman]

> On Jul 14, 2015, at 11:55, Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> wrote:
> 
> Separate RX packet protocol parsing out of 'vmxnet_rx_pkt_attach_data'.
> 

Ack

> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
> ---
> hw/net/vmxnet3.c       |  1 +
> hw/net/vmxnet_rx_pkt.c | 12 +++++++++---
> hw/net/vmxnet_rx_pkt.h | 11 +++++++++++
> 3 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 706e060..dd22a0a 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -1897,6 +1897,7 @@ vmxnet3_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>         get_eth_packet_type(PKT_GET_ETH_HDR(buf)));
> 
>     if (vmxnet3_rx_filter_may_indicate(s, buf, size)) {
> +        vmxnet_rx_pkt_set_protocols(s->rx_pkt, buf, size);
>         vmxnet_rx_pkt_attach_data(s->rx_pkt, buf, size, s->rx_vlan_stripping);
>         bytes_indicated = vmxnet3_indicate_packet(s) ? size : -1;
>         if (bytes_indicated < size) {
> diff --git a/hw/net/vmxnet_rx_pkt.c b/hw/net/vmxnet_rx_pkt.c
> index acbca6a..aa54629 100644
> --- a/hw/net/vmxnet_rx_pkt.c
> +++ b/hw/net/vmxnet_rx_pkt.c
> @@ -92,9 +92,6 @@ void vmxnet_rx_pkt_attach_data(struct VmxnetRxPkt *pkt, const void *data,
>     }
> 
>     pkt->tci = tci;
> -
> -    eth_get_protocols(data, len, &pkt->isip4, &pkt->isip6,
> -        &pkt->isudp, &pkt->istcp);
> }
> 
> void vmxnet_rx_pkt_dump(struct VmxnetRxPkt *pkt)
> @@ -131,6 +128,15 @@ size_t vmxnet_rx_pkt_get_total_len(struct VmxnetRxPkt *pkt)
>     return pkt->tot_len;
> }
> 
> +void vmxnet_rx_pkt_set_protocols(struct VmxnetRxPkt *pkt, const void *data,
> +                                 size_t len)
> +{
> +    assert(pkt);
> +
> +    eth_get_protocols(data, len, &pkt->isip4, &pkt->isip6,
> +        &pkt->isudp, &pkt->istcp);
> +}
> +
> void vmxnet_rx_pkt_get_protocols(struct VmxnetRxPkt *pkt,
>                                  bool *isip4, bool *isip6,
>                                  bool *isudp, bool *istcp)
> diff --git a/hw/net/vmxnet_rx_pkt.h b/hw/net/vmxnet_rx_pkt.h
> index 5f8352a..a425846 100644
> --- a/hw/net/vmxnet_rx_pkt.h
> +++ b/hw/net/vmxnet_rx_pkt.h
> @@ -55,6 +55,17 @@ void vmxnet_rx_pkt_init(struct VmxnetRxPkt **pkt, bool has_virt_hdr);
> size_t vmxnet_rx_pkt_get_total_len(struct VmxnetRxPkt *pkt);
> 
> /**
> + * parse and set packet analysis results
> + *
> + * @pkt:            packet
> + * @data:           pointer to the data buffer to be parsed
> + * @len:            data length
> + *
> + */
> +void vmxnet_rx_pkt_set_protocols(struct VmxnetRxPkt *pkt, const void *data,
> +                                 size_t len);
> +
> +/**
>  * fetches packet analysis results
>  *
>  * @pkt:            packet
> -- 
> 1.9.1
> 

[Qemu-devel] [PATCH 2/2] net/vmxnet3: Fix RX TCP/UDP checksum on partially summed packets

[Shmulik Ladkani]

From: Dana Rubin <dana.rubin@ravellosystems.com>

Convert partially summed packets to be fully checksummed.

In case csum offloaded packet, vmxnet3 implementation always passes an
RxCompDesc with the "Checksum calculated and found correct" notification
to the OS. This emulates the observed ESXi behavior.

Therefore, if packet has the NEEDS_CSUM bit set, we must calculate and
place a fully computed checksum into the tcp/udp header. Otherwise, the
OS driver will receive a checksum-correct indication but with the actual
tcp/udp checksum field having just the pseudo header csum value.

If host OS performs forwarding, it will forward an incorrectly
checksummed packet.

Signed-off-by: Dana Rubin <dana.rubin@ravellosystems.com>
Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
---
 hw/net/vmxnet3.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index dd22a0a..59b06b8 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -885,6 +885,63 @@ vmxnet3_get_next_rx_descr(VMXNET3State *s, bool is_head,
     }
 }
 
+/* In case packet was csum offloaded (either NEEDS_CSUM or DATA_VALID),
+ * the implementation always passes an RxCompDesc with a "Checksum
+ * calculated and found correct" to the OS (cnc=0 and tuc=1, see
+ * vmxnet3_rx_update_descr). This emulates the observed ESXi behavior.
+ *
+ * Therefore, if packet has the NEEDS_CSUM set, we must calculate
+ * and place a fully computed checksum into the tcp/udp header.
+ * Otherwise, the OS driver will receive a checksum-correct indication
+ * (CHECKSUM_UNNECESSARY), but with the actual tcp/udp checksum field
+ * having just the pseudo header csum value.
+ *
+ * While this is not a problem if packet is destined for local delivery,
+ * in the case the host OS performs forwarding, it will forward an
+ * incorrectly checksummed packet.
+ */
+static void vmxnet3_rx_need_csum_calculate(struct VmxnetRxPkt *pkt,
+                                           const void *pkt_data,
+                                           size_t pkt_len)
+{
+    struct virtio_net_hdr *vhdr;
+    bool isip4, isip6, istcp, isudp;
+    uint8_t *data;
+    int len;
+
+    if (!vmxnet_rx_pkt_has_virt_hdr(pkt)) {
+        return;
+    }
+
+    vhdr = vmxnet_rx_pkt_get_vhdr(pkt);
+    if (!VMXNET_FLAG_IS_SET(vhdr->flags, VIRTIO_NET_HDR_F_NEEDS_CSUM)) {
+        return;
+    }
+
+    vmxnet_rx_pkt_get_protocols(pkt, &isip4, &isip6, &isudp, &istcp);
+    if (!(isip4 || isip6) || !(istcp || isudp)) {
+        return;
+    }
+
+    vmxnet3_dump_virt_hdr(vhdr);
+
+    /* Validate packet len: csum_start + scum_offset + length of csum field */
+    if (pkt_len < (vhdr->csum_start + vhdr->csum_offset + 2)) {
+        VMW_PKPRN("packet len:%d < csum_start(%d) + csum_offset(%d) + 2, "
+                  "cannot calculate checksum",
+                  len, vhdr->csum_start, vhdr->csum_offset);
+        return;
+    }
+
+    data = (uint8_t *)pkt_data + vhdr->csum_start;
+    len = pkt_len - vhdr->csum_start;
+    /* Put the checksum obtained into the packet */
+    stw_be_p(data + vhdr->csum_offset, net_raw_checksum(data, len));
+
+    vhdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
+    vhdr->flags |= VIRTIO_NET_HDR_F_DATA_VALID;
+}
+
 static void vmxnet3_rx_update_descr(struct VmxnetRxPkt *pkt,
     struct Vmxnet3_RxCompDesc *rxcd)
 {
@@ -1898,6 +1955,7 @@ vmxnet3_receive(NetClientState *nc, const uint8_t *buf, size_t size)
 
     if (vmxnet3_rx_filter_may_indicate(s, buf, size)) {
         vmxnet_rx_pkt_set_protocols(s->rx_pkt, buf, size);
+        vmxnet3_rx_need_csum_calculate(s->rx_pkt, buf, size);
         vmxnet_rx_pkt_attach_data(s->rx_pkt, buf, size, s->rx_vlan_stripping);
         bytes_indicated = vmxnet3_indicate_packet(s) ? size : -1;
         if (bytes_indicated < size) {
-- 
1.9.1

Re: [Qemu-devel] [PATCH 2/2] net/vmxnet3: Fix RX TCP/UDP checksum on partially summed packets

[Dmitry Fleytman]

> On Jul 14, 2015, at 11:55, Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> wrote:
> 
> From: Dana Rubin <dana.rubin@ravellosystems.com>
> 
> Convert partially summed packets to be fully checksummed.
> 
> In case csum offloaded packet, vmxnet3 implementation always passes an
> RxCompDesc with the "Checksum calculated and found correct" notification
> to the OS. This emulates the observed ESXi behavior.
> 
> Therefore, if packet has the NEEDS_CSUM bit set, we must calculate and
> place a fully computed checksum into the tcp/udp header. Otherwise, the
> OS driver will receive a checksum-correct indication but with the actual
> tcp/udp checksum field having just the pseudo header csum value.
> 
> If host OS performs forwarding, it will forward an incorrectly
> checksummed packet.

Ack.

> 
> Signed-off-by: Dana Rubin <dana.rubin@ravellosystems.com>
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
> ---
> hw/net/vmxnet3.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
> 
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index dd22a0a..59b06b8 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -885,6 +885,63 @@ vmxnet3_get_next_rx_descr(VMXNET3State *s, bool is_head,
>     }
> }
> 
> +/* In case packet was csum offloaded (either NEEDS_CSUM or DATA_VALID),
> + * the implementation always passes an RxCompDesc with a "Checksum
> + * calculated and found correct" to the OS (cnc=0 and tuc=1, see
> + * vmxnet3_rx_update_descr). This emulates the observed ESXi behavior.
> + *
> + * Therefore, if packet has the NEEDS_CSUM set, we must calculate
> + * and place a fully computed checksum into the tcp/udp header.
> + * Otherwise, the OS driver will receive a checksum-correct indication
> + * (CHECKSUM_UNNECESSARY), but with the actual tcp/udp checksum field
> + * having just the pseudo header csum value.
> + *
> + * While this is not a problem if packet is destined for local delivery,
> + * in the case the host OS performs forwarding, it will forward an
> + * incorrectly checksummed packet.
> + */
> +static void vmxnet3_rx_need_csum_calculate(struct VmxnetRxPkt *pkt,
> +                                           const void *pkt_data,
> +                                           size_t pkt_len)
> +{
> +    struct virtio_net_hdr *vhdr;
> +    bool isip4, isip6, istcp, isudp;
> +    uint8_t *data;
> +    int len;
> +
> +    if (!vmxnet_rx_pkt_has_virt_hdr(pkt)) {
> +        return;
> +    }
> +
> +    vhdr = vmxnet_rx_pkt_get_vhdr(pkt);
> +    if (!VMXNET_FLAG_IS_SET(vhdr->flags, VIRTIO_NET_HDR_F_NEEDS_CSUM)) {
> +        return;
> +    }
> +
> +    vmxnet_rx_pkt_get_protocols(pkt, &isip4, &isip6, &isudp, &istcp);
> +    if (!(isip4 || isip6) || !(istcp || isudp)) {
> +        return;
> +    }
> +
> +    vmxnet3_dump_virt_hdr(vhdr);
> +
> +    /* Validate packet len: csum_start + scum_offset + length of csum field */
> +    if (pkt_len < (vhdr->csum_start + vhdr->csum_offset + 2)) {
> +        VMW_PKPRN("packet len:%d < csum_start(%d) + csum_offset(%d) + 2, "
> +                  "cannot calculate checksum",
> +                  len, vhdr->csum_start, vhdr->csum_offset);
> +        return;
> +    }
> +
> +    data = (uint8_t *)pkt_data + vhdr->csum_start;
> +    len = pkt_len - vhdr->csum_start;
> +    /* Put the checksum obtained into the packet */
> +    stw_be_p(data + vhdr->csum_offset, net_raw_checksum(data, len));
> +
> +    vhdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
> +    vhdr->flags |= VIRTIO_NET_HDR_F_DATA_VALID;
> +}
> +
> static void vmxnet3_rx_update_descr(struct VmxnetRxPkt *pkt,
>     struct Vmxnet3_RxCompDesc *rxcd)
> {
> @@ -1898,6 +1955,7 @@ vmxnet3_receive(NetClientState *nc, const uint8_t *buf, size_t size)
> 
>     if (vmxnet3_rx_filter_may_indicate(s, buf, size)) {
>         vmxnet_rx_pkt_set_protocols(s->rx_pkt, buf, size);
> +        vmxnet3_rx_need_csum_calculate(s->rx_pkt, buf, size);
>         vmxnet_rx_pkt_attach_data(s->rx_pkt, buf, size, s->rx_vlan_stripping);
>         bytes_indicated = vmxnet3_indicate_packet(s) ? size : -1;
>         if (bytes_indicated < size) {
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 0/2] vmxnet3: Fix RX TCP/UDP checksum of partially summed packets

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1159 bytes --]

On Tue, Jul 14, 2015 at 11:55:14AM +0300, Shmulik Ladkani wrote:
> In case csum offloaded packet, vmxnet3 implementation always passes an
> RxCompDesc with the "Checksum calculated and found correct" notification
> to the OS. This emulates the observed ESXi behavior.
> 
> Therefore, if packet has the NEEDS_CSUM bit set, we must calculate and
> place a fully computed checksum into the tcp/udp header. Otherwise, the
> OS driver will receive a checksum-correct indication but with the actual
> tcp/udp checksum field having just the pseudo header csum value.
> 
> Fix, by converting partially summed packets to be fully checksummed.
> 
> Shmulik Ladkani:
>   net/vmxnet3: Refactor 'vmxnet_rx_pkt_attach_data'
> 
> Dana Rubin:
>   net/vmxnet3: Fix RX TCP/UDP checksum on partially summed packets
> 
>  hw/net/vmxnet3.c       | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/net/vmxnet_rx_pkt.c | 12 +++++++---
>  hw/net/vmxnet_rx_pkt.h | 11 ++++++++++
>  3 files changed, 79 insertions(+), 3 deletions(-)
> 
> -- 
> 1.9.1
> 
> 

Thanks, applied to my net tree:
https://github.com/stefanha/qemu/commits/net

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]