Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1295079 > unrolled thread
| Started by | Vijay Pandurangan <vijayp@vijayp.ca> |
|---|---|
| First post | 2015-12-18 19:00 +0100 |
| Last post | 2015-12-19 22:40 +0100 |
| Articles | 9 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH] veth: don't modify ip-summed; doing so treats packets with bad checksums as good. Vijay Pandurangan <vijayp@vijayp.ca> - 2015-12-18 19:00 +0100
Re: [PATCH] veth: don't modify ip-summed; doing so treats packets with bad checksums as good. Cong Wang <xiyou.wangcong@gmail.com> - 2015-12-18 20:10 +0100
[PATCH] veth: don’t modify ip_summed; doing so treats packets with bad checksums as good. Vijay Pandurangan <vijayp@vijayp.ca> - 2015-12-18 20:40 +0100
Re: [PATCH] veth: don’t modify ip_summed; doing so treats packets with bad checksums as good. Cong Wang <xiyou.wangcong@gmail.com> - 2015-12-19 22:50 +0100
Re: [PATCH] veth: don’t modify ip_summed; doing so treats packets with bad checksums as good. David Miller <davem@davemloft.net> - 2015-12-22 21:20 +0100
Re: [PATCH] veth: don’t modify ip_summed; doing so treats packets with bad checksums as good. Cong Wang <xiyou.wangcong@gmail.com> - 2015-12-23 19:00 +0100
Re: [PATCH] veth: don't modify ip-summed; doing so treats packets with bad checksums as good. Vijay Pandurangan <vijayp@vijayp.ca> - 2015-12-18 20:50 +0100
Re: [PATCH] veth: don't modify ip-summed; doing so treats packets with bad checksums as good. Cong Wang <xiyou.wangcong@gmail.com> - 2015-12-19 22:10 +0100
Re: [PATCH] veth: don't modify ip-summed; doing so treats packets with bad checksums as good. Cong Wang <cwang@twopensource.com> - 2015-12-19 22:40 +0100
| From | Vijay Pandurangan <vijayp@vijayp.ca> |
|---|---|
| Date | 2015-12-18 19:00 +0100 |
| Subject | [PATCH] veth: don't modify ip-summed; doing so treats packets with bad checksums as good. |
| Message-ID | <qH7tU-iI-11@gated-at.bofh.it> |
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
<e314dbdc1c0dc6a548ecf> ("[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 <ej@evanjones.ca>
Signed-off-by: Evan Jones <ej@evanjones.ca>
Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Cc: Phil Sutter <phil@nwl.cc>
Cc: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Vijay Pandurangan <vijayp@vijayp.ca>
---
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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Cong Wang <xiyou.wangcong@gmail.com> |
|---|---|
| Date | 2015-12-18 20:10 +0100 |
| Subject | Re: [PATCH] veth: don't modify ip-summed; doing so treats packets with bad checksums as good. |
| Message-ID | <qH8zE-1cy-1@gated-at.bofh.it> |
| In reply to | #1295079 |
(Cc'ing Eric B and Tom)
On Fri, Dec 18, 2015 at 9:54 AM, Vijay Pandurangan <vijayp@vijayp.ca> 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.
Yeah, https://reviews.apache.org/r/41158/.
This is because normally packets to a veth device are _only_ from its pair
device, Mesos network isolator redirects packets from a hardware interface
to veth, which violates this expectation. This is also why no one else sees
this bug. ;)
>
> 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).
I am wondering if there is any other CHECKSUM_NONE case in the tx
path we could miss here. Mesos case is too special not only because
it redirects packets from hardware to veth, but also because it moves
packets from RX path to TX path.
Eric? Tom?
>
> This code dates back to the first version of the driver, commit
> <e314dbdc1c0dc6a548ecf> ("[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 <ej@evanjones.ca>
> Signed-off-by: Evan Jones <ej@evanjones.ca>
> Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Cc: Phil Sutter <phil@nwl.cc>
> Cc: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Vijay Pandurangan <vijayp@vijayp.ca>
Your patch looks good to me but your email client corrupts your patch,
so please resend.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Vijay Pandurangan <vijayp@vijayp.ca> |
|---|---|
| Date | 2015-12-18 20:40 +0100 |
| Subject | [PATCH] veth: don’t modify ip_summed; doing so treats packets with bad checksums as good. |
| Message-ID | <qH92F-1o5-1@gated-at.bofh.it> |
| In reply to | #1295101 |
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
<e314dbdc1c0dc6a548ecf> ("[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 <ej@evanjones.ca>
Signed-off-by: Evan Jones <ej@evanjones.ca>
Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Cc: Phil Sutter <phil@nwl.cc>
Cc: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Vijay Pandurangan <vijayp@vijayp.ca>
---
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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Cong Wang <xiyou.wangcong@gmail.com> |
|---|---|
| Date | 2015-12-19 22:50 +0100 |
| Subject | Re: [PATCH] veth: don’t modify ip_summed; doing so treats packets with bad checksums as good. |
| Message-ID | <qHxy2-83-1@gated-at.bofh.it> |
| In reply to | #1295108 |
On Fri, Dec 18, 2015 at 11:34 AM, Vijay Pandurangan <vijayp@vijayp.ca> 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
> <e314dbdc1c0dc6a548ecf> ("[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 <ej@evanjones.ca>
> Signed-off-by: Evan Jones <ej@evanjones.ca>
> Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Cc: Phil Sutter <phil@nwl.cc>
> Cc: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Vijay Pandurangan <vijayp@vijayp.ca>
Acked-by: Cong Wang <cwang@twopensource.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | David Miller <davem@davemloft.net> |
|---|---|
| Date | 2015-12-22 21:20 +0100 |
| Subject | Re: [PATCH] veth: don’t modify ip_summed; doing so treats packets with bad checksums as good. |
| Message-ID | <qIBzz-7r-3@gated-at.bofh.it> |
| In reply to | #1295108 |
RnJvbTogVmlqYXkgUGFuZHVyYW5nYW4gPHZpamF5cEB2aWpheXAuY2E+DQpEYXRlOiBGcmksIDE4 IERlYyAyMDE1IDE0OjM0OjU5IC0wNTAwDQoNCj4gUGFja2V0cyB0aGF0IGFycml2ZSBmcm9tIHJl YWwgaGFyZHdhcmUgZGV2aWNlcyBoYXZlIGlwX3N1bW1lZCA9PQ0KPiBDSEVDS1NVTV9VTk5FQ0VT U0FSWSBpZiB0aGUgaGFyZHdhcmUgdmVyaWZpZWQgdGhlIGNoZWNrc3Vtcywgb3INCj4gQ0hFQ0tT VU1fTk9ORSBpZiB0aGUgcGFja2V0IGlzIGJhZCBvciBpdCB3YXMgdW5hYmxlIHRvIHZlcmlmeSBp dC4gVGhlDQo+IGN1cnJlbnQgdmVyc2lvbiBvZiB2ZXRoIHdpbGwgcmVwbGFjZSBDSEVDS1NVTV9O T05FIHdpdGgNCj4gQ0hFQ0tTVU1fVU5ORUNFU1NBUlksIHdoaWNoIGNhdXNlcyBjb3JydXB0IHBh Y2tldHMgcm91dGVkIGZyb20gaGFyZHdhcmUgdG8NCj4gYSB2ZXRoIGRldmljZSB0byBiZSBkZWxp dmVyZWQgdG8gdGhlIGFwcGxpY2F0aW9uLiBUaGlzIGNhdXNlZCBhcHBsaWNhdGlvbnMNCj4gYXQg VHdpdHRlciB0byByZWNlaXZlIGNvcnJ1cHQgZGF0YSB3aGVuIG5ldHdvcmsgaGFyZHdhcmUgd2Fz IGNvcnJ1cHRpbmcNCj4gcGFja2V0cy4NCj4gDQo+IFdlIGJlbGlldmUgdGhpcyB3YXMgYWRkZWQg YXMgYW4gb3B0aW1pemF0aW9uIHRvIHNraXAgY29tcHV0aW5nIGFuZA0KPiB2ZXJpZnlpbmcgY2hl Y2tzdW1zIGZvciBjb21tdW5pY2F0aW9uIGJldHdlZW4gY29udGFpbmVycy4gSG93ZXZlciwgbG9j YWxseQ0KPiBnZW5lcmF0ZWQgcGFja2V0cyBoYXZlIGlwX3N1bW1lZCA9PSBDSEVDS1NVTV9QQVJU SUFMLCBzbyB0aGUgY29kZSBhcw0KPiB3cml0dGVuIGRvZXMgbm90aGluZyBmb3IgdGhlbS4gQXMg ZmFyIGFzIHdlIGNhbiB0ZWxsLCBhZnRlciByZW1vdmluZyB0aGlzDQo+IGNvZGUsIHRoZXNlIHBh Y2tldHMgYXJlIHRyYW5zbWl0dGVkIGZyb20gb25lIHN0YWNrIHRvIGFub3RoZXIgdW5tb2RpZmll ZA0KPiAodGNwZHVtcCBzaG93cyBpbnZhbGlkIGNoZWNrc3VtcyBvbiBib3RoIHNpZGVzLCBhcyBl eHBlY3RlZCksIGFuZCB0aGV5IGFyZQ0KPiBkZWxpdmVyZWQgY29ycmVjdGx5IHRvIGFwcGxpY2F0 aW9ucy4gV2UgZGlkbqJ0IHRlc3QgZXZlcnkgcG9zc2libGUgbmV0d29yaw0KPiBjb25maWd1cmF0 aW9uLCBidXQgd2UgdHJpZWQgYSBmZXcgY29tbW9uIG9uZXMgc3VjaCBhcyBicmlkZ2luZyBjb250 YWluZXJzLA0KPiB1c2luZyBOQVQgYmV0d2VlbiB0aGUgaG9zdCBhbmQgYSBjb250YWluZXIsIGFu ZCByb3V0aW5nIGZyb20gaGFyZHdhcmUNCj4gZGV2aWNlcyB0byBjb250YWluZXJzLiBXZSBoYXZl IGVmZmVjdGl2ZWx5IGRlcGxveWVkIHRoaXMgaW4gcHJvZHVjdGlvbiBhdA0KPiBUd2l0dGVyIChi eSBkaXNhYmxpbmcgUlggY2hlY2tzdW0gb2ZmbG9hZGluZyBvbiB2ZXRoIGRldmljZXMpLg0KPiAN Cj4gVGhpcyBjb2RlIGRhdGVzIGJhY2sgdG8gdGhlIGZpcnN0IHZlcnNpb24gb2YgdGhlIGRyaXZl ciwgY29tbWl0DQo+IDxlMzE0ZGJkYzFjMGRjNmE1NDhlY2Y+ICgiW05FVF06IFZpcnR1YWwgZXRo ZXJuZXQgZGV2aWNlIGRyaXZlciIpLCBzbyBJDQo+IHN1c3BlY3QgdGhpcyBidWcgb2NjdXJyZWQg bW9zdGx5IGJlY2F1c2UgdGhlIGRyaXZlciBBUEkgaGFzIGV2b2x2ZWQNCj4gc2lnbmlmaWNhbnRs eSBzaW5jZSB0aGVuLiBDb21taXQgPDBiNzk2NzUwM2RjOTc4NjRmMjgzYT4gKCJuZXQvdmV0aDog Rml4DQo+IHBhY2tldCBjaGVja3N1bW1pbmciKSAoaW4gRGVjZW1iZXIgMjAxMCkgZml4ZWQgdGhp cyBmb3IgcGFja2V0cyB0aGF0IGdldA0KPiBjcmVhdGVkIGxvY2FsbHkgYW5kIHNlbnQgdG8gaGFy ZHdhcmUgZGV2aWNlcywgYnkgbm90IGNoYW5naW5nDQo+IENIRUNLU1VNX1BBUlRJQUwuIEhvd2V2 ZXIsIHRoZSBzYW1lIGlzc3VlIHN0aWxsIG9jY3VycyBmb3IgcGFja2V0cyBjb21pbmcNCj4gaW4g ZnJvbSBoYXJkd2FyZSBkZXZpY2VzLg0KPiANCj4gQ28tYXV0aG9yZWQtYnk6IEV2YW4gSm9uZXMg PGVqQGV2YW5qb25lcy5jYT4NCj4gU2lnbmVkLW9mZi1ieTogRXZhbiBKb25lcyA8ZWpAZXZhbmpv bmVzLmNhPg0KPiBDYzogTmljb2xhcyBEaWNodGVsIDxuaWNvbGFzLmRpY2h0ZWxANndpbmQuY29t Pg0KPiBDYzogUGhpbCBTdXR0ZXIgPHBoaWxAbndsLmNjPg0KPiBDYzogVG9zaGlha2kgTWFraXRh IDxtYWtpdGEudG9zaGlha2lAbGFiLm50dC5jby5qcD4NCj4gQ2M6IG5ldGRldkB2Z2VyLmtlcm5l bC5vcmcNCj4gQ2M6IGxpbnV4LWtlcm5lbEB2Z2VyLmtlcm5lbC5vcmcNCj4gU2lnbmVkLW9mZi1i eTogVmlqYXkgUGFuZHVyYW5nYW4gPHZpamF5cEB2aWpheXAuY2E+DQoNCkFwcGxpZWQgYW5kIHF1 ZXVlZCB1cCBmb3IgLXN0YWJsZSwgdGhhbmtzLg0K -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Cong Wang <xiyou.wangcong@gmail.com> |
|---|---|
| Date | 2015-12-23 19:00 +0100 |
| Subject | Re: [PATCH] veth: don’t modify ip_summed; doing so treats packets with bad checksums as good. |
| Message-ID | <qIVRD-4gb-9@gated-at.bofh.it> |
| In reply to | #1297046 |
On Tue, Dec 22, 2015 at 11:37 PM, Vijay Pandurangan <vijayp@vijayp.ca> 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 linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Vijay Pandurangan <vijayp@vijayp.ca> |
|---|---|
| Date | 2015-12-18 20:50 +0100 |
| Subject | Re: [PATCH] veth: don't modify ip-summed; doing so treats packets with bad checksums as good. |
| Message-ID | <qH9cm-1rI-21@gated-at.bofh.it> |
| In reply to | #1295101 |
Evan and I have demonstrated this bug on Kubernetes as well, so it's
not just a problem in Mesos. (See
https://github.com/kubernetes/kubernetes/issues/18898)
Sorry about my email client, I've re-sent the patch in another thread
from git-email as I should have initially.
I'll read through the TX path again to see if we missed something, but
I'd love input from anyone else!
--
Vijay Pandurangan
https://www.twitter.com/vijayp
http://www.vijayp.ca
On Fri, Dec 18, 2015 at 2:00 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> (Cc'ing Eric B and Tom)
>
> On Fri, Dec 18, 2015 at 9:54 AM, Vijay Pandurangan <vijayp@vijayp.ca> 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.
>
> Yeah, https://reviews.apache.org/r/41158/.
>
> This is because normally packets to a veth device are _only_ from its pair
> device, Mesos network isolator redirects packets from a hardware interface
> to veth, which violates this expectation. This is also why no one else sees
> this bug. ;)
>
>>
>> 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).
>
>
> I am wondering if there is any other CHECKSUM_NONE case in the tx
> path we could miss here. Mesos case is too special not only because
> it redirects packets from hardware to veth, but also because it moves
> packets from RX path to TX path.
>
> Eric? Tom?
>
>>
>> This code dates back to the first version of the driver, commit
>> <e314dbdc1c0dc6a548ecf> ("[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 <ej@evanjones.ca>
>> Signed-off-by: Evan Jones <ej@evanjones.ca>
>> Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> Cc: Phil Sutter <phil@nwl.cc>
>> Cc: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>> Cc: netdev@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Vijay Pandurangan <vijayp@vijayp.ca>
>
> Your patch looks good to me but your email client corrupts your patch,
> so please resend.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Cong Wang <xiyou.wangcong@gmail.com> |
|---|---|
| Date | 2015-12-19 22:10 +0100 |
| Subject | Re: [PATCH] veth: don't modify ip-summed; doing so treats packets with bad checksums as good. |
| Message-ID | <qHwVk-8mU-9@gated-at.bofh.it> |
| In reply to | #1295114 |
On Fri, Dec 18, 2015 at 11:42 AM, Vijay Pandurangan <vijayp@vijayp.ca> wrote:
> Evan and I have demonstrated this bug on Kubernetes as well, so it's
> not just a problem in Mesos. (See
> https://github.com/kubernetes/kubernetes/issues/18898)
>
Interesting... then this problem is much more serious than I thought.
Looks like in RX path the bridge sets the checksum to CHECKSUM_NONE
too:
static inline void skb_forward_csum(struct sk_buff *skb)
{
/* Unfortunately we don't support this one. Any brave souls? */
if (skb->ip_summed == CHECKSUM_COMPLETE)
skb->ip_summed = CHECKSUM_NONE;
}
I guess this is probably why Docker/Kubernetes could be affected too.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Cong Wang <cwang@twopensource.com> |
|---|---|
| Date | 2015-12-19 22:40 +0100 |
| Subject | Re: [PATCH] veth: don't modify ip-summed; doing so treats packets with bad checksums as good. |
| Message-ID | <qHxol-8wo-5@gated-at.bofh.it> |
| In reply to | #1295470 |
On Sat, Dec 19, 2015 at 1:01 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Fri, Dec 18, 2015 at 11:42 AM, Vijay Pandurangan <vijayp@vijayp.ca> wrote:
>> Evan and I have demonstrated this bug on Kubernetes as well, so it's
>> not just a problem in Mesos. (See
>> https://github.com/kubernetes/kubernetes/issues/18898)
>>
>
> Interesting... then this problem is much more serious than I thought.
>
> Looks like in RX path the bridge sets the checksum to CHECKSUM_NONE
> too:
>
> static inline void skb_forward_csum(struct sk_buff *skb)
> {
> /* Unfortunately we don't support this one. Any brave souls? */
> if (skb->ip_summed == CHECKSUM_COMPLETE)
> skb->ip_summed = CHECKSUM_NONE;
> }
>
> I guess this is probably why Docker/Kubernetes could be affected too.
Hmm, no, actually this is due to netem does the software checksum
and sets it to CHECKSUM_NONE:
if (q->corrupt && q->corrupt >= get_crandom(&q->corrupt_cor)) {
if (!(skb = skb_unshare(skb, GFP_ATOMIC)) ||
(skb->ip_summed == CHECKSUM_PARTIAL &&
skb_checksum_help(skb)))
return qdisc_drop(skb, sch);
skb->data[prandom_u32() % skb_headlen(skb)] ^=
1<<(prandom_u32() % 8);
}
But anyway, your patch still looks correct to me.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web