Quantcast

[PATCH 0/3][Yakkety][SRU] virtio: fix xmit packet flag settings

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 0/3][Yakkety][SRU] virtio: fix xmit packet flag settings

Jay Vosburgh
BugLink: https://bugs.launchpad.net/bugs/1683947

        Beginning with fd2a0437dc33 ("virtio_net: introduce
virtio_net_hdr_{from,to}_skb"), virtio_net incorrectly sets
VIRTIO_NET_HDR_F_DATA_VALID on CHECKSUM_UNNECESSARY packets.  If these
packets are modified prior to transmission (e.g., by NAT), they are
dropped by the receving virtio.  I suspect (but have not confirmed
experimentally) that this is because the checksum is incorrect after NAT
modifies the packet, but the checksum is not recomputed.

        Patch 1/3, 3e9e40e74753 ("virtio_net: Simplify call sites for
virtio_net_hdr_{from, to}_skb()."), is a prerequisite to permit patch 3/3,
6391a4481ba0, to apply more easily and end up with the final code closer
to mainline.

        Patch 2/3, 501db511397f ("virtio: don't set
VIRTIO_NET_HDR_F_DATA_VALID on xmit") was found by bisect to initially
resolve the issue.

        Patch 3/3, 6391a4481ba0 ("virtio-net: restore
VIRTIO_HDR_F_DATA_VALID on receiving") is a follow-on to 501db511397f for
the complete fix.

        As the issue was introduced during 4.8-rc and resolved before 4.10
release, this bug exists only in the Ubuntu Yakkety 4.8 kernel series.

        -J

---
        -Jay Vosburgh, [hidden email]

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 1/3][Yakkety][SRU] virtio_net: Simplify call sites for virtio_net_hdr_{from, to}_skb().

Jay Vosburgh
From: Jarno Rajahalme <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1683947

No point storing the return value of virtio_net_hdr_to_skb() or
virtio_net_hdr_from_skb() to a variable when the value is used only
once as a boolean in an immediately following if statement.

Signed-off-by: Jarno Rajahalme <[hidden email]>
Signed-off-by: David S. Miller <[hidden email]>
(cherry picked from 3e9e40e74753346218e8285cf1ecff9ef3a624c6)
Signed-off-by: Jay Vosburgh <[hidden email]>
---
 drivers/net/macvtap.c | 5 ++---
 drivers/net/tun.c     | 8 +++-----
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 070e3290aa6e..5da9861ad79c 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -821,9 +821,8 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
  if (iov_iter_count(iter) < vnet_hdr_len)
  return -EINVAL;
 
- ret = virtio_net_hdr_from_skb(skb, &vnet_hdr,
-      macvtap_is_little_endian(q));
- if (ret)
+ if (virtio_net_hdr_from_skb(skb, &vnet_hdr,
+    macvtap_is_little_endian(q)))
  BUG();
 
  if (copy_to_iter(&vnet_hdr, sizeof(vnet_hdr), iter) !=
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 6f9df375c5d4..57f9e0ffa6ff 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1266,8 +1266,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
  return -EFAULT;
  }
 
- err = virtio_net_hdr_to_skb(skb, &gso, tun_is_little_endian(tun));
- if (err) {
+ if (virtio_net_hdr_to_skb(skb, &gso, tun_is_little_endian(tun))) {
  this_cpu_inc(tun->pcpu_stats->rx_frame_errors);
  kfree_skb(skb);
  return -EINVAL;
@@ -1381,9 +1380,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
  if (iov_iter_count(iter) < vnet_hdr_sz)
  return -EINVAL;
 
- ret = virtio_net_hdr_from_skb(skb, &gso,
-      tun_is_little_endian(tun));
- if (ret) {
+ if (virtio_net_hdr_from_skb(skb, &gso,
+    tun_is_little_endian(tun))) {
  struct skb_shared_info *sinfo = skb_shinfo(skb);
  pr_err("unexpected GSO type: "
        "0x%x, gso_size %d, hdr_len %d\n",
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 2/3][Yakkety][SRU] virtio: don't set VIRTIO_NET_HDR_F_DATA_VALID on xmit

Jay Vosburgh
In reply to this post by Jay Vosburgh
From: Rolf Neugebauer <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1683947

This patch part reverts fd2a0437dc33 and e858fae2b0b8 which introduced a
subtle change in how the virtio_net flags are derived from the SKBs
ip_summed field.

With the above commits, the flags are set to VIRTIO_NET_HDR_F_DATA_VALID
when ip_summed == CHECKSUM_UNNECESSARY, thus treating it differently to
ip_summed == CHECKSUM_NONE, which should be the same.

Further, the virtio spec 1.0 / CS04 explicitly says that
VIRTIO_NET_HDR_F_DATA_VALID must not be set by the driver.

Fixes: fd2a0437dc33 ("virtio_net: introduce virtio_net_hdr_{from,to}_skb")
Fixes: e858fae2b0b8 (" virtio_net: use common code for virtio_net_hdr and skb GSO conversion")
Signed-off-by: Rolf Neugebauer <[hidden email]>
Acked-by: Michael S. Tsirkin <[hidden email]>
Signed-off-by: David S. Miller <[hidden email]>
(cherry picked from 3e9e40e74753346218e8285cf1ecff9ef3a624c6)
Signed-off-by: Jay Vosburgh <[hidden email]>
---
 include/linux/virtio_net.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 1c912f85e041..40914bb396e7 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -91,8 +91,6 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
  skb_checksum_start_offset(skb));
  hdr->csum_offset = __cpu_to_virtio16(little_endian,
  skb->csum_offset);
- } else if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
- hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
  } /* else everything is zero */
 
  return 0;
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 3/3][Yakkety][SRU] virtio-net: restore VIRTIO_HDR_F_DATA_VALID on receiving

Jay Vosburgh
In reply to this post by Jay Vosburgh
From: Jason Wang <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1683947

Commit 501db511397f ("virtio: don't set VIRTIO_NET_HDR_F_DATA_VALID on
xmit") in fact disables VIRTIO_HDR_F_DATA_VALID on receiving path too,
fixing this by adding a hint (has_data_valid) and set it only on the
receiving path.

Cc: Rolf Neugebauer <[hidden email]>
Signed-off-by: Jason Wang <[hidden email]>
Acked-by: Rolf Neugebauer <[hidden email]>
Signed-off-by: David S. Miller <[hidden email]>
(backported from 6391a4481ba0796805d6581e42f9f0418c099e34)
Signed-off-by: Jay Vosburgh <[hidden email]>
---
 drivers/net/macvtap.c      | 2 +-
 drivers/net/tun.c          | 2 +-
 drivers/net/virtio_net.c   | 2 +-
 include/linux/virtio_net.h | 6 +++++-
 net/packet/af_packet.c     | 2 +-
 5 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 5da9861ad79c..72142a0140e9 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -822,7 +822,7 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
  return -EINVAL;
 
  if (virtio_net_hdr_from_skb(skb, &vnet_hdr,
-    macvtap_is_little_endian(q)))
+    macvtap_is_little_endian(q), true))
  BUG();
 
  if (copy_to_iter(&vnet_hdr, sizeof(vnet_hdr), iter) !=
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 57f9e0ffa6ff..69de9dde4e28 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1381,7 +1381,7 @@ static ssize_t tun_put_user(struct tun_struct *tun,
  return -EINVAL;
 
  if (virtio_net_hdr_from_skb(skb, &gso,
-    tun_is_little_endian(tun))) {
+    tun_is_little_endian(tun), true)) {
  struct skb_shared_info *sinfo = skb_shinfo(skb);
  pr_err("unexpected GSO type: "
        "0x%x, gso_size %d, hdr_len %d\n",
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d8072092e5f0..d540f0d714af 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -839,7 +839,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
  hdr = skb_vnet_hdr(skb);
 
  if (virtio_net_hdr_from_skb(skb, &hdr->hdr,
-    virtio_is_little_endian(vi->vdev)))
+    virtio_is_little_endian(vi->vdev), false))
  BUG();
 
  if (vi->mergeable_rx_bufs)
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 40914bb396e7..f211c348e592 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -56,7 +56,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 
 static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
   struct virtio_net_hdr *hdr,
-  bool little_endian)
+  bool little_endian,
+  bool has_data_valid)
 {
  memset(hdr, 0, sizeof(*hdr));
 
@@ -91,6 +92,9 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
  skb_checksum_start_offset(skb));
  hdr->csum_offset = __cpu_to_virtio16(little_endian,
  skb->csum_offset);
+ } else if (has_data_valid &&
+   skb->ip_summed == CHECKSUM_UNNECESSARY) {
+ hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
  } /* else everything is zero */
 
  return 0;
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index e518c29e547b..d9f28d320b40 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1972,7 +1972,7 @@ static int __packet_rcv_vnet(const struct sk_buff *skb,
 {
  *vnet_hdr = (const struct virtio_net_hdr) { 0 };
 
- if (virtio_net_hdr_from_skb(skb, vnet_hdr, vio_le()))
+ if (virtio_net_hdr_from_skb(skb, vnet_hdr, vio_le(), true))
  BUG();
 
  return 0;
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

ACK: [PATCH 1/3][Yakkety][SRU] virtio_net: Simplify call sites for virtio_net_hdr_{from, to}_skb().

Colin Ian King-2
In reply to this post by Jay Vosburgh
On 20/04/17 00:41, Jay Vosburgh wrote:

> From: Jarno Rajahalme <[hidden email]>
>
> BugLink: https://bugs.launchpad.net/bugs/1683947
>
> No point storing the return value of virtio_net_hdr_to_skb() or
> virtio_net_hdr_from_skb() to a variable when the value is used only
> once as a boolean in an immediately following if statement.
>
> Signed-off-by: Jarno Rajahalme <[hidden email]>
> Signed-off-by: David S. Miller <[hidden email]>
> (cherry picked from 3e9e40e74753346218e8285cf1ecff9ef3a624c6)
> Signed-off-by: Jay Vosburgh <[hidden email]>
> ---
>  drivers/net/macvtap.c | 5 ++---
>  drivers/net/tun.c     | 8 +++-----
>  2 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index 070e3290aa6e..5da9861ad79c 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -821,9 +821,8 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
>   if (iov_iter_count(iter) < vnet_hdr_len)
>   return -EINVAL;
>  
> - ret = virtio_net_hdr_from_skb(skb, &vnet_hdr,
> -      macvtap_is_little_endian(q));
> - if (ret)
> + if (virtio_net_hdr_from_skb(skb, &vnet_hdr,
> +    macvtap_is_little_endian(q)))
>   BUG();
>  
>   if (copy_to_iter(&vnet_hdr, sizeof(vnet_hdr), iter) !=
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 6f9df375c5d4..57f9e0ffa6ff 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1266,8 +1266,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>   return -EFAULT;
>   }
>  
> - err = virtio_net_hdr_to_skb(skb, &gso, tun_is_little_endian(tun));
> - if (err) {
> + if (virtio_net_hdr_to_skb(skb, &gso, tun_is_little_endian(tun))) {
>   this_cpu_inc(tun->pcpu_stats->rx_frame_errors);
>   kfree_skb(skb);
>   return -EINVAL;
> @@ -1381,9 +1380,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>   if (iov_iter_count(iter) < vnet_hdr_sz)
>   return -EINVAL;
>  
> - ret = virtio_net_hdr_from_skb(skb, &gso,
> -      tun_is_little_endian(tun));
> - if (ret) {
> + if (virtio_net_hdr_from_skb(skb, &gso,
> +    tun_is_little_endian(tun))) {
>   struct skb_shared_info *sinfo = skb_shinfo(skb);
>   pr_err("unexpected GSO type: "
>         "0x%x, gso_size %d, hdr_len %d\n",
>

Clean cherrypick and is just a code simplification, small change as a
prerequisite for next patches.

Acked-by: Colin Ian King <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

CMT: [PATCH 2/3][Yakkety][SRU] virtio: don't set VIRTIO_NET_HDR_F_DATA_VALID on xmit

Colin Ian King-2
In reply to this post by Jay Vosburgh
On 20/04/17 00:41, Jay Vosburgh wrote:

> From: Rolf Neugebauer <[hidden email]>
>
> BugLink: https://bugs.launchpad.net/bugs/1683947
>
> This patch part reverts fd2a0437dc33 and e858fae2b0b8 which introduced a
> subtle change in how the virtio_net flags are derived from the SKBs
> ip_summed field.
>
> With the above commits, the flags are set to VIRTIO_NET_HDR_F_DATA_VALID
> when ip_summed == CHECKSUM_UNNECESSARY, thus treating it differently to
> ip_summed == CHECKSUM_NONE, which should be the same.
>
> Further, the virtio spec 1.0 / CS04 explicitly says that
> VIRTIO_NET_HDR_F_DATA_VALID must not be set by the driver.
>
> Fixes: fd2a0437dc33 ("virtio_net: introduce virtio_net_hdr_{from,to}_skb")
> Fixes: e858fae2b0b8 (" virtio_net: use common code for virtio_net_hdr and skb GSO conversion")
> Signed-off-by: Rolf Neugebauer <[hidden email]>
> Acked-by: Michael S. Tsirkin <[hidden email]>
> Signed-off-by: David S. Miller <[hidden email]>
> (cherry picked from 3e9e40e74753346218e8285cf1ecff9ef3a624c6)
> Signed-off-by: Jay Vosburgh <[hidden email]>
> ---
>  include/linux/virtio_net.h | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 1c912f85e041..40914bb396e7 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -91,8 +91,6 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
>   skb_checksum_start_offset(skb));
>   hdr->csum_offset = __cpu_to_virtio16(little_endian,
>   skb->csum_offset);
> - } else if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
> - hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
>   } /* else everything is zero */
>  
>   return 0;
>

This is a cherry pick of 501db511397fd6efff3aa5b4e8de415b55559550 and
not 3e9e40e74753346218e8285cf1ecff9ef3a624c6 as reported in the patch.

Apart from that detail, this fix is an upstream fix that looks OK to me.





--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

ACK [PATCH 3/3][Yakkety][SRU] virtio-net: restore VIRTIO_HDR_F_DATA_VALID on receiving

Colin Ian King-2
In reply to this post by Jay Vosburgh
On 20/04/17 00:41, Jay Vosburgh wrote:

> From: Jason Wang <[hidden email]>
>
> BugLink: https://bugs.launchpad.net/bugs/1683947
>
> Commit 501db511397f ("virtio: don't set VIRTIO_NET_HDR_F_DATA_VALID on
> xmit") in fact disables VIRTIO_HDR_F_DATA_VALID on receiving path too,
> fixing this by adding a hint (has_data_valid) and set it only on the
> receiving path.
>
> Cc: Rolf Neugebauer <[hidden email]>
> Signed-off-by: Jason Wang <[hidden email]>
> Acked-by: Rolf Neugebauer <[hidden email]>
> Signed-off-by: David S. Miller <[hidden email]>
> (backported from 6391a4481ba0796805d6581e42f9f0418c099e34)
> Signed-off-by: Jay Vosburgh <[hidden email]>
> ---
>  drivers/net/macvtap.c      | 2 +-
>  drivers/net/tun.c          | 2 +-
>  drivers/net/virtio_net.c   | 2 +-
>  include/linux/virtio_net.h | 6 +++++-
>  net/packet/af_packet.c     | 2 +-
>  5 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index 5da9861ad79c..72142a0140e9 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -822,7 +822,7 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
>   return -EINVAL;
>  
>   if (virtio_net_hdr_from_skb(skb, &vnet_hdr,
> -    macvtap_is_little_endian(q)))
> +    macvtap_is_little_endian(q), true))
>   BUG();
>  
>   if (copy_to_iter(&vnet_hdr, sizeof(vnet_hdr), iter) !=
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 57f9e0ffa6ff..69de9dde4e28 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1381,7 +1381,7 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>   return -EINVAL;
>  
>   if (virtio_net_hdr_from_skb(skb, &gso,
> -    tun_is_little_endian(tun))) {
> +    tun_is_little_endian(tun), true)) {
>   struct skb_shared_info *sinfo = skb_shinfo(skb);
>   pr_err("unexpected GSO type: "
>         "0x%x, gso_size %d, hdr_len %d\n",
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index d8072092e5f0..d540f0d714af 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -839,7 +839,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
>   hdr = skb_vnet_hdr(skb);
>  
>   if (virtio_net_hdr_from_skb(skb, &hdr->hdr,
> -    virtio_is_little_endian(vi->vdev)))
> +    virtio_is_little_endian(vi->vdev), false))
>   BUG();
>  
>   if (vi->mergeable_rx_bufs)
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 40914bb396e7..f211c348e592 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -56,7 +56,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
>  
>  static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
>    struct virtio_net_hdr *hdr,
> -  bool little_endian)
> +  bool little_endian,
> +  bool has_data_valid)
>  {
>   memset(hdr, 0, sizeof(*hdr));
>  
> @@ -91,6 +92,9 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
>   skb_checksum_start_offset(skb));
>   hdr->csum_offset = __cpu_to_virtio16(little_endian,
>   skb->csum_offset);
> + } else if (has_data_valid &&
> +   skb->ip_summed == CHECKSUM_UNNECESSARY) {
> + hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
>   } /* else everything is zero */
>  
>   return 0;
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index e518c29e547b..d9f28d320b40 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1972,7 +1972,7 @@ static int __packet_rcv_vnet(const struct sk_buff *skb,
>  {
>   *vnet_hdr = (const struct virtio_net_hdr) { 0 };
>  
> - if (virtio_net_hdr_from_skb(skb, vnet_hdr, vio_le()))
> + if (virtio_net_hdr_from_skb(skb, vnet_hdr, vio_le(), true))
>   BUG();
>  
>   return 0;
>

Backport looks good. With all the 3 patches the fix has a good set of
tests that give positive results, the fixes are upstream, so if the sha
of patch 2 can be fixed up, I'm OK with these.

Acked-by: Colin Ian King <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

ACK: [PATCH 0/3][Yakkety][SRU] virtio: fix xmit packet flag settings

Seth Forshee
In reply to this post by Jay Vosburgh
On Wed, Apr 19, 2017 at 04:41:44PM -0700, Jay Vosburgh wrote:

> BugLink: https://bugs.launchpad.net/bugs/1683947
>
> Beginning with fd2a0437dc33 ("virtio_net: introduce
> virtio_net_hdr_{from,to}_skb"), virtio_net incorrectly sets
> VIRTIO_NET_HDR_F_DATA_VALID on CHECKSUM_UNNECESSARY packets.  If these
> packets are modified prior to transmission (e.g., by NAT), they are
> dropped by the receving virtio.  I suspect (but have not confirmed
> experimentally) that this is because the checksum is incorrect after NAT
> modifies the packet, but the checksum is not recomputed.
>
> Patch 1/3, 3e9e40e74753 ("virtio_net: Simplify call sites for
> virtio_net_hdr_{from, to}_skb()."), is a prerequisite to permit patch 3/3,
> 6391a4481ba0, to apply more easily and end up with the final code closer
> to mainline.
>
> Patch 2/3, 501db511397f ("virtio: don't set
> VIRTIO_NET_HDR_F_DATA_VALID on xmit") was found by bisect to initially
> resolve the issue.
>
> Patch 3/3, 6391a4481ba0 ("virtio-net: restore
> VIRTIO_HDR_F_DATA_VALID on receiving") is a follow-on to 501db511397f for
> the complete fix.
>
> As the issue was introduced during 4.8-rc and resolved before 4.10
> release, this bug exists only in the Ubuntu Yakkety 4.8 kernel series.

Patches are clean cherry picks with positive tests results. Assuming the
upstream sha-1's are fixed up as Colin pointed out:

Acked-by: Seth Forshee <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: ACK: [PATCH 0/3][Yakkety][SRU] virtio: fix xmit packet flag settings

Jay Vosburgh
Seth Forshee <[hidden email]> wrote:

>On Wed, Apr 19, 2017 at 04:41:44PM -0700, Jay Vosburgh wrote:
>> BugLink: https://bugs.launchpad.net/bugs/1683947
>>
>> Beginning with fd2a0437dc33 ("virtio_net: introduce
>> virtio_net_hdr_{from,to}_skb"), virtio_net incorrectly sets
>> VIRTIO_NET_HDR_F_DATA_VALID on CHECKSUM_UNNECESSARY packets.  If these
>> packets are modified prior to transmission (e.g., by NAT), they are
>> dropped by the receving virtio.  I suspect (but have not confirmed
>> experimentally) that this is because the checksum is incorrect after NAT
>> modifies the packet, but the checksum is not recomputed.
>>
>> Patch 1/3, 3e9e40e74753 ("virtio_net: Simplify call sites for
>> virtio_net_hdr_{from, to}_skb()."), is a prerequisite to permit patch 3/3,
>> 6391a4481ba0, to apply more easily and end up with the final code closer
>> to mainline.
>>
>> Patch 2/3, 501db511397f ("virtio: don't set
>> VIRTIO_NET_HDR_F_DATA_VALID on xmit") was found by bisect to initially
>> resolve the issue.
>>
>> Patch 3/3, 6391a4481ba0 ("virtio-net: restore
>> VIRTIO_HDR_F_DATA_VALID on receiving") is a follow-on to 501db511397f for
>> the complete fix.
>>
>> As the issue was introduced during 4.8-rc and resolved before 4.10
>> release, this bug exists only in the Ubuntu Yakkety 4.8 kernel series.
>
>Patches are clean cherry picks with positive tests results. Assuming the
>upstream sha-1's are fixed up as Colin pointed out:

        Will fix patch 2/3 and repost the series.

        -J

---
        -Jay Vosburgh, [hidden email]

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Loading...