Re: [PATCH] veth: don’t modify ip_summed; doing so treats packets with bad checksums as good.

[Vijay Pandurangan]

Just as a followup, I wrote a short blog detailing the bug and our
resolution: (https://twitter.com/vijayp/status/697837808417779716)

Thanks again for your help in guiding us through our first kernel
patch. This was a great experience!

direct link: 
https://medium.com/vijay-pandurangan/linux-kernel-bug-delivers-corrupt-tcp-ip-data-to-mesos-kubernetes-docker-containers-4986f88f7a19#.aymvnbaa8

On Wed, Dec 23, 2015 at 9:57 AM, Cong Wang  wrote:
> On Tue, Dec 22, 2015 at 11:37 PM, Vijay Pandurangan  wrote:
>> Cool, thanks! I see it in the -stable queue. Is there anything else I need
>> to do to help with getting this into main or backporting?  Happy to pitch in
>> if I can be helpful.
>>
>
> DaveM usually just backports it to a few recent stable tree, if you want
> to backport further, for example 3.14, you probably need to send
> the commit ID to Greg KH.
>
> Thanks.

Re: [PATCH] veth: don’t modify ip_summed; doing so treats packets with bad checksums as good.

[Cong Wang]

On Tue, Dec 22, 2015 at 11:37 PM, Vijay Pandurangan  wrote:
> Cool, thanks! I see it in the -stable queue. Is there anything else I need
> to do to help with getting this into main or backporting?  Happy to pitch in
> if I can be helpful.
>

DaveM usually just backports it to a few recent stable tree, if you want
to backport further, for example 3.14, you probably need to send
the commit ID to Greg KH.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] veth: don’t modify ip_summed; doing so treats packets with bad checksums as good.

[David Miller]

From: Vijay Pandurangan 
Date: Fri, 18 Dec 2015 14:34:59 -0500

> Packets that arrive from real hardware devices have ip_summed ==
> CHECKSUM_UNNECESSARY if the hardware verified the checksums, or
> CHECKSUM_NONE if the packet is bad or it was unable to verify it. The
> current version of veth will replace CHECKSUM_NONE with
> CHECKSUM_UNNECESSARY, which causes corrupt packets routed from hardware to
> a veth device to be delivered to the application. This caused applications
> at Twitter to receive corrupt data when network hardware was corrupting
> packets.
> 
> We believe this was added as an optimization to skip computing and
> verifying checksums for communication between containers. However, locally
> generated packets have ip_summed == CHECKSUM_PARTIAL, so the code as
> written does nothing for them. As far as we can tell, after removing this
> code, these packets are transmitted from one stack to another unmodified
> (tcpdump shows invalid checksums on both sides, as expected), and they are
> delivered correctly to applications. We didn’t test every possible network
> configuration, but we tried a few common ones such as bridging containers,
> using NAT between the host and a container, and routing from hardware
> devices to containers. We have effectively deployed this in production at
> Twitter (by disabling RX checksum offloading on veth devices).
> 
> This code dates back to the first version of the driver, commit
>  ("[NET]: Virtual ethernet device driver"), so I
> suspect this bug occurred mostly because the driver API has evolved
> significantly since then. Commit <0b7967503dc97864f283a> ("net/veth: Fix
> packet checksumming") (in December 2010) fixed this for packets that get
> created locally and sent to hardware devices, by not changing
> CHECKSUM_PARTIAL. However, the same issue still occurs for packets coming
> in from hardware devices.
> 
> Co-authored-by: Evan Jones 
> Signed-off-by: Evan Jones 
> Cc: Nicolas Dichtel 
> Cc: Phil Sutter 
> Cc: Toshiaki Makita 
> Cc: netdev@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Vijay Pandurangan 

Applied and queued up for -stable, thanks.
N‹§²ζμrΈ›yϊθšΨb²X¬ΆΗ§vΨ^–)ήΊ{.nΗ+‰·§zΧ^Ύ)ν…
ζθw*jg¬±¨Ά‰šŽŠέ’j/κδzΉή–Šΰ2Šή™¨θ­Ϊ&’)ί‘«aΆΪώψ�G«ιh�ζj:+v‰¨Šwθ†Ω₯

Re: [PATCH] veth: don’t modify ip_summed; doing so treats packets with bad checksums as good.

[Cong Wang]

On Fri, Dec 18, 2015 at 11:34 AM, Vijay Pandurangan  wrote:
> Packets that arrive from real hardware devices have ip_summed ==
> CHECKSUM_UNNECESSARY if the hardware verified the checksums, or
> CHECKSUM_NONE if the packet is bad or it was unable to verify it. The
> current version of veth will replace CHECKSUM_NONE with
> CHECKSUM_UNNECESSARY, which causes corrupt packets routed from hardware to
> a veth device to be delivered to the application. This caused applications
> at Twitter to receive corrupt data when network hardware was corrupting
> packets.
>
> We believe this was added as an optimization to skip computing and
> verifying checksums for communication between containers. However, locally
> generated packets have ip_summed == CHECKSUM_PARTIAL, so the code as
> written does nothing for them. As far as we can tell, after removing this
> code, these packets are transmitted from one stack to another unmodified
> (tcpdump shows invalid checksums on both sides, as expected), and they are
> delivered correctly to applications. We didn’t test every possible network
> configuration, but we tried a few common ones such as bridging containers,
> using NAT between the host and a container, and routing from hardware
> devices to containers. We have effectively deployed this in production at
> Twitter (by disabling RX checksum offloading on veth devices).
>
> This code dates back to the first version of the driver, commit
>  ("[NET]: Virtual ethernet device driver"), so I
> suspect this bug occurred mostly because the driver API has evolved
> significantly since then. Commit <0b7967503dc97864f283a> ("net/veth: Fix
> packet checksumming") (in December 2010) fixed this for packets that get
> created locally and sent to hardware devices, by not changing
> CHECKSUM_PARTIAL. However, the same issue still occurs for packets coming
> in from hardware devices.
>
> Co-authored-by: Evan Jones 
> Signed-off-by: Evan Jones 
> Cc: Nicolas Dichtel 
> Cc: Phil Sutter 
> Cc: Toshiaki Makita 
> Cc: netdev@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Vijay Pandurangan 


Acked-by: Cong Wang 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] veth: don’t modify ip_summed; doing so treats packets with bad checksums as good.

[Vijay Pandurangan]

Packets that arrive from real hardware devices have ip_summed ==
CHECKSUM_UNNECESSARY if the hardware verified the checksums, or
CHECKSUM_NONE if the packet is bad or it was unable to verify it. The
current version of veth will replace CHECKSUM_NONE with
CHECKSUM_UNNECESSARY, which causes corrupt packets routed from hardware to
a veth device to be delivered to the application. This caused applications
at Twitter to receive corrupt data when network hardware was corrupting
packets.

We believe this was added as an optimization to skip computing and
verifying checksums for communication between containers. However, locally
generated packets have ip_summed == CHECKSUM_PARTIAL, so the code as
written does nothing for them. As far as we can tell, after removing this
code, these packets are transmitted from one stack to another unmodified
(tcpdump shows invalid checksums on both sides, as expected), and they are
delivered correctly to applications. We didn’t test every possible network
configuration, but we tried a few common ones such as bridging containers,
using NAT between the host and a container, and routing from hardware
devices to containers. We have effectively deployed this in production at
Twitter (by disabling RX checksum offloading on veth devices).

This code dates back to the first version of the driver, commit
 ("[NET]: Virtual ethernet device driver"), so I
suspect this bug occurred mostly because the driver API has evolved
significantly since then. Commit <0b7967503dc97864f283a> ("net/veth: Fix
packet checksumming") (in December 2010) fixed this for packets that get
created locally and sent to hardware devices, by not changing
CHECKSUM_PARTIAL. However, the same issue still occurs for packets coming
in from hardware devices.

Co-authored-by: Evan Jones 
Signed-off-by: Evan Jones 
Cc: Nicolas Dichtel 
Cc: Phil Sutter 
Cc: Toshiaki Makita 
Cc: netdev@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Vijay Pandurangan 
---
 drivers/net/veth.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 0ef4a5a..ba21d07 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -117,12 +117,6 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct 
net_device *dev)
kfree_skb(skb);
goto drop;
}
-   /* don't change ip_summed == CHECKSUM_PARTIAL, as that
-* will cause bad checksum on forwarded packets
-*/
-   if (skb->ip_summed == CHECKSUM_NONE &&
-   rcv->features & NETIF_F_RXCSUM)
-   skb->ip_summed = CHECKSUM_UNNECESSARY;
 
if (likely(dev_forward_skb(rcv, skb) == NET_RX_SUCCESS)) {
struct pcpu_vstats *stats = this_cpu_ptr(dev->vstats);
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html