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

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

[PATCH v2 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.

v2: fixed cherry pick commit id in patch 2/3

        -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 v2 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 v2 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 501db511397fd6efff3aa5b4e8de415b55559550)
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 v2 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 v2 0/3][Yakkety][SRU] virtio: fix xmit packet flag settings

Colin Ian King-2
In reply to this post by Jay Vosburgh
On 20/04/17 17:14, 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.
>
> v2: fixed cherry pick commit id in patch 2/3
>
> -J
>
> ---
> -Jay Vosburgh, [hidden email]
>
Thanks Jay for the re-submission.

Looks good!

#include my acks from the first round, all 3 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

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

Kamal Mostafa-2
In reply to this post by Jay Vosburgh
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Stefan Bader-2
In reply to this post by Jay Vosburgh
On 20.04.2017 18:14, 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.
>
> v2: fixed cherry pick commit id in patch 2/3
>
> -J
>
> ---
> -Jay Vosburgh, [hidden email]
>


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

signature.asc (836 bytes) Download Attachment
Loading...