veth regression with "don’t modify ip_summed; doing so treats packets with bad checksums as good."

Hmm that's troubling. We tested various UDP configurations but I don't think we tested this setup. Do you have code or more specific instructions we can use to replicate this bug?

I have an application that creates two pairs of veth devices.

a <-> b c <-> d

b and c have a raw packet socket opened on them and I 'bridge' frames between b and c to provide network emulation (ie, configurable delay).

I put IP 1.1.1.1/24 <http://1.1.1.1/24> on a, 1.1.1.2/24 <http://1.1.1.2/24> on d, and then create a UDP connection (using policy based routing to ensure frames are sent on the appropriate
interfaces).

This is user-space only app, and kernel in this case is completely unmodified.

The commit below breaks this feature: UDP frames are sniffed on both a and d ports (in both directions), but the UDP socket does not receive frames.

Using normal ethernet ports, this network emulation feature works fine, so it is specific to VETH.

A similar test with just sending UDP between a single veth pair: e <-> f works fine. Maybe it has something to do with raw packets?

The patch below is the culprit:

[greearb@ben-dt3 linux-2.6]$ git bisect bad ce8c839b74e3017996fad4e1b7ba2e2625ede82f is the first bad commit commit ce8c839b74e3017996fad4e1b7ba2e2625ede82f
Author: Vijay Pandurangan <[email protected] <mailto:[email protected]>>
Date: Fri Dec 18 14:34:59 2015 -0500

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

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.

...

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);

Any suggestions for how to fix this so that I get the old working behaviour and the bug this patch was trying to fix is also resolved?

Thanks,
Ben

--
Ben Greear <[email protected] <mailto:[email protected]>> Candela Technologies Inc http://www.candelatech.com

I have an application that creates two pairs of veth devices.

a <-> b c <-> d

b and c have a raw packet socket opened on them and I 'bridge' frames between b and c to provide network emulation (ie, configurable delay).

I put IP 1.1.1.1/24 on a, 1.1.1.2/24 on d, and then create a UDP connection (using policy based routing to ensure frames are sent on the appropriate interfaces).

This is user-space only app, and kernel in this case is completely unmodified.

The commit below breaks this feature: UDP frames are sniffed on both a and d ports
(in both directions), but the UDP socket does not receive frames.

Using normal ethernet ports, this network emulation feature works fine, so it is
specific to VETH.

A similar test with just sending UDP between a single veth pair: e <-> f works fine. Maybe it has something to do with raw packets?

Quoted 7 lines · Ben Greear

I have an application that creates two pairs of veth devices.

a <-> b c <-> d

b and c have a raw packet socket opened on them and I 'bridge' frames between b and c to provide network emulation (ie, configurable delay).

IIUC, you create two raw sockets in order to bridge these two veth pairs? That is, to receive packets on one socket and deliver packets on the other?

Quoted 15 lines

I put IP 1.1.1.1/24 on a, 1.1.1.2/24 on d, and then create a UDP connection (using policy based routing to ensure frames are sent on the appropriate interfaces).

This is user-space only app, and kernel in this case is completely unmodified.

The commit below breaks this feature: UDP frames are sniffed on both a and d ports
(in both directions), but the UDP socket does not receive frames.

Using normal ethernet ports, this network emulation feature works fine, so it is
specific to VETH.

Quoted 3 lines

A similar test with just sending UDP between a single veth pair: e <-> f works fine. Maybe it has something to do with raw packets?

Yeah, I have the same feeling. Could you trace kfree_skb() to see where these packets are dropped? At UDP layer?

Quoted 11 lines · Cong Wang

Quoted 7 lines · Ben Greear

I have an application that creates two pairs of veth devices.

a <-> b c <-> d

b and c have a raw packet socket opened on them and I 'bridge' frames between b and c to provide network emulation (ie, configurable delay).

IIUC, you create two raw sockets in order to bridge these two veth pairs? That is, to receive packets on one socket and deliver packets on the other?

Yes.

Quoted 15 lines

Quoted 15 lines

I put IP 1.1.1.1/24 on a, 1.1.1.2/24 on d, and then create a UDP connection (using policy based routing to ensure frames are sent on the appropriate interfaces).

This is user-space only app, and kernel in this case is completely unmodified.

The commit below breaks this feature: UDP frames are sniffed on both a and d ports
(in both directions), but the UDP socket does not receive frames.

Using normal ethernet ports, this network emulation feature works fine, so it is
specific to VETH.

Quoted 6 lines

Quoted 3 lines

A similar test with just sending UDP between a single veth pair: e <-> f works fine. Maybe it has something to do with raw packets?

Yeah, I have the same feeling. Could you trace kfree_skb() to see where these packets are dropped? At UDP layer?

Since reverting the patch fixes this, it almost certainly has to be due to some checksum checking logic. Since UDP sockets (between single veth pair) works, it would appear to be related to my packet bridge, so maybe it is specific to raw packets and/or sendmmsg api.

I'll investigate it better tomorrow.

Quoted 42 lines · Ben Greear

Quoted 11 lines · Cong Wang

Quoted 7 lines · Ben Greear

I have an application that creates two pairs of veth devices.

a <-> b c <-> d

b and c have a raw packet socket opened on them and I 'bridge' frames between b and c to provide network emulation (ie, configurable delay).

IIUC, you create two raw sockets in order to bridge these two veth pairs? That is, to receive packets on one socket and deliver packets on the other?

Yes.

Quoted 15 lines

Quoted 15 lines

I put IP 1.1.1.1/24 on a, 1.1.1.2/24 on d, and then create a UDP connection (using policy based routing to ensure frames are sent on the appropriate interfaces).

This is user-space only app, and kernel in this case is completely unmodified.

The commit below breaks this feature: UDP frames are sniffed on both a and d ports
(in both directions), but the UDP socket does not receive frames.

Using normal ethernet ports, this network emulation feature works fine, so it is
specific to VETH.

Quoted 6 lines

Quoted 3 lines

A similar test with just sending UDP between a single veth pair: e <-> f works fine. Maybe it has something to do with raw packets?

Yeah, I have the same feeling. Could you trace kfree_skb() to see where these packets are dropped? At UDP layer?

Since reverting the patch fixes this, it almost certainly has to be due to some checksum checking logic. Since UDP sockets (between single veth pair) works, it would appear to be related to my packet bridge, so maybe it is specific to raw packets and/or sendmmsg api.

Quoted 1 line

I'll investigate it better tomorrow.

So, I found time to poke at it this evening:

Sending between two veth pairs, no packet bridge involved.

UDP: ip_summed is 3 (CHECKSUM_PARTIAL) # Works fine.
raw packet frames, custom ether protocol (0x1111 type): ip_summed is 0 (NONE) # Works fine.

When I try to send UDP through the veth pairs & pkt bridge, I see this:

Quoted 58 lines · Ben Greear

Quoted 56 lines · Ben Greear

Quoted 8 lines · Cong Wang

Quoted 5 lines · Ben Greear

I have an application that creates two pairs of veth devices.

a <-> b c <-> d

Quoted 8 lines

Quoted 3 lines

b and c have a raw packet socket opened on them and I 'bridge' frames between b and c to provide network emulation (ie, configurable delay).

IIUC, you create two raw sockets in order to bridge these two veth pairs?
That is, to receive packets on one socket and deliver packets on the other?

Yes.

Quoted 19 lines

Quoted 19 lines

I put IP 1.1.1.1/24 on a, 1.1.1.2/24 on d, and then create a UDP connection
(using policy based routing to ensure frames are sent on the appropriate
interfaces).

This is user-space only app, and kernel in this case is completely unmodified.

The commit below breaks this feature: UDP frames are sniffed on both a and
d ports
(in both directions), but the UDP socket does not receive frames.

Using normal ethernet ports, this network emulation feature works fine, so
it is
specific to VETH.

Quoted 7 lines

Quoted 4 lines

A similar test with just sending UDP between a single veth pair: e <-> f
works fine. Maybe it has something to do with raw packets?

Yeah, I have the same feeling. Could you trace kfree_skb() to see where these packets are dropped? At UDP layer?

Since reverting the patch fixes this, it almost certainly has to be due to some
checksum checking logic. Since UDP sockets (between single veth pair) works, it would appear to be related to my packet bridge, so maybe it is specific to raw packets and/or sendmmsg api.

Quoted 6 lines

Quoted 1 line

I'll investigate it better tomorrow.

So, I found time to poke at it this evening:

Sending between two veth pairs, no packet bridge involved.

Errrr, to be clear: I mean sending between two ends of a single veth pair here.

Quoted 6 lines

UDP: ip_summed is 3 (CHECKSUM_PARTIAL) # Works fine.
raw packet frames, custom ether protocol (0x1111 type): ip_summed is 0 (NONE) # Works fine.

When I try to send UDP through the veth pairs & pkt bridge, I see this:

Thanks,
Ben

--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com

Oops, I think my last email didn't go through due to an inadvertent html attachment from my phone mail client.

Can you send us a copy of a packet you're sending and/or confirm that the IP and UDP4 checksums are set correctly in the packet?

If those are set right, I think we need to read through the networking code again to see why this is broken...

Quoted 9 lines · Vijay Pandurangan

Oops, I think my last email didn't go through due to an inadvertent html attachment from my phone mail client.

Can you send us a copy of a packet you're sending and/or confirm that the IP and UDP4 checksums are set correctly in the packet?

If those are set right, I think we need to read through the networking code again to see why this is broken...

Wireshark decodes the packet as having no checksum errors.

I think the contents of the packet is correct, but the 'ip_summed' field is set incorrectly to 'NONE' when transmitting on a raw packet socket.

Thanks,
Ben

--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com

agreed. It should maybe be set to CHECKSUM_UNNECESSARY. The comment seems to imply that it's treated the same as CHECKSUM_NONE but that's evidently not true. I think that would fix the checksumming issue but I'm fearful it may break something else:
http://lxr.free-electrons.com/source/include/linux/skbuff.h#L177

I'm really worried there are other equally subtle bugs hidden in this code. Do we have any kind of regression test, or any automated way to test all possible values on an skb to determine side effects to any change here? (I'm new to the kernel so sorry if there's an answer in an FAQ somewhere).

If not,
1. how should we ensure that our change doesn't break something else?
2. Should we audit / simplify the checksum code or come up with a list of test cases that covers all uses?

Quoted 27 lines · Ben Greear

Quoted 9 lines · Vijay Pandurangan

Oops, I think my last email didn't go through due to an inadvertent html attachment from my phone mail client.

Can you send us a copy of a packet you're sending and/or confirm that the IP and UDP4 checksums are set correctly in the packet?

If those are set right, I think we need to read through the networking code again to see why this is broken...

Wireshark decodes the packet as having no checksum errors.

I think the contents of the packet is correct, but the 'ip_summed' field is set incorrectly to 'NONE' when transmitting on a raw packet socket.

Thanks,
Ben

--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com

Actually, maybe they should be set to CHECKSUM_PARTIAL if we want veth to drop the packets if they have bad checksums before they hit the application level.

Quoted 4 lines · Vijay Pandurangan

Actually, maybe they should be set to CHECKSUM_PARTIAL if we want veth to drop the packets if they have bad checksums before they hit the application level.

VETH is pretty special in that when you transmit a frame on one device, it's pair receives it, and unless there is RAM corruption or bugs in the kernel, then it cannot be corrupted.

But, if you are routing frames from the network to veth devices, then the original packet could be corrupted, as described in your patch.

Probably the right behaviour is to keep the old logic for packets originating from sockets, at least. I am not sure of a good way to implement that.

As for testing, I am not sure. Probably you have to make a good effort and then just deal with fallout like what I found. I would guess that any of us who have ever written an interesting patch have also written an interesting bug!

Thanks,
Ben

--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com

Quoted 10 lines · Ben Greear

Quoted 4 lines · Vijay Pandurangan

Actually, maybe they should be set to CHECKSUM_PARTIAL if we want veth to drop the packets if they have bad checksums before they hit the application level.

VETH is pretty special in that when you transmit a frame on one device, it's pair receives it, and unless there is RAM corruption or bugs in the kernel, then it cannot be corrupted.

Yeah, you're right that that's an optimization. However, I think that we should first ensure that

a->veth->b

operates exactly like:

a->physical eth 1 -> physical eth 2->b

in all cases. Once we have that working everywhere we could think about optimizations.

If we're willing to refactor, we could implement the optimization by allowing veth devices to know whether their immediate peer is. If a veth knows it's talking to another veth, it could under some circumstances elide checksum calculation and verification. I'm not sure what abstractions that would break, though. What do you guys think?

Quoted 31 lines · Vijay Pandurangan

Quoted 10 lines · Ben Greear

Quoted 5 lines · Vijay Pandurangan

Actually, maybe they should be set to CHECKSUM_PARTIAL if we want veth to drop the packets if they have bad checksums before they hit the application level.

Quoted 3 lines

VETH is pretty special in that when you transmit a frame on one device, it's pair receives it, and unless there is RAM corruption or bugs in the kernel, then it cannot be corrupted.

Yeah, you're right that that's an optimization. However, I think that we should first ensure that

a->veth->b

operates exactly like:

a->physical eth 1 -> physical eth 2->b

in all cases. Once we have that working everywhere we could think about optimizations.

Quoted 6 lines

If we're willing to refactor, we could implement the optimization by allowing veth devices to know whether their immediate peer is. If a veth knows it's talking to another veth, it could under some circumstances elide checksum calculation and verification. I'm not sure what abstractions that would break, though. What do you guys think?

veth ALWAYS transmits to another VETH. The problem is that when veth is given a packet to transmit, it is difficult to know where that packet came from.

And, adding software checksumming to veth for every frame would be a huge performance hit.

Thanks,
Ben

--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com

Quoted 9 lines · Vijay Pandurangan

Oops, I think my last email didn't go through due to an inadvertent html attachment from my phone mail client.

Can you send us a copy of a packet you're sending and/or confirm that the IP and UDP4 checksums are set correctly in the packet?

If those are set right, I think we need to read through the networking code again to see why this is broken...

Wireshark decodes the packet as having no checksum errors.

I think the contents of the packet is correct, but the 'ip_summed' field is set incorrectly to 'NONE' when transmitting on a raw packet socket.

Quoted 14 lines · Ben Greear

Quoted 9 lines · Vijay Pandurangan

Oops, I think my last email didn't go through due to an inadvertent html attachment from my phone mail client.

Can you send us a copy of a packet you're sending and/or confirm that the IP and UDP4 checksums are set correctly in the packet?

If those are set right, I think we need to read through the networking code again to see why this is broken...

Quoted 5 lines

Wireshark decodes the packet as having no checksum errors.

I think the contents of the packet is correct, but the 'ip_summed' field is set incorrectly to 'NONE' when transmitting on a raw packet socket.

Yeah, these bugs are all due to the different interpretations of ip_summed on TX path and RX path. I think the following patch should work, if the comments don't mislead me. Could you give it a try?

For the long term, we need to unify the meaning of ip_summed on TX path and RX path, or at least translate it in skb_scrub_packet().

Quoted 32 lines · Cong Wang

Quoted 17 lines · Ben Greear

Quoted 10 lines · Vijay Pandurangan

Oops, I think my last email didn't go through due to an inadvertent html attachment from my phone mail client.

Can you send us a copy of a packet you're sending and/or confirm that the IP and UDP4 checksums are set correctly in the packet?

If those are set right, I think we need to read through the networking code again to see why this is broken...

Quoted 5 lines

Wireshark decodes the packet as having no checksum errors.

I think the contents of the packet is correct, but the 'ip_summed' field is set incorrectly to 'NONE' when transmitting on a raw packet socket.

Yeah, these bugs are all due to the different interpretations of ip_summed on TX path and RX path. I think the following patch should work, if the comments don't mislead me. Could you give it a try?

Quoted 2 lines

For the long term, we need to unify the meaning of ip_summed on TX path and RX path, or at least translate it in skb_scrub_packet().

I can test this tomorrow, but I think it will not work. I'm not sending raw IP frames, I'm sending full ethernet frames. Socket is PF_PACKET, SOCK_RAW.

Your patch may still be useful for others though?

Quoted 35 lines · Ben Greear

Quoted 32 lines · Cong Wang

Quoted 17 lines · Ben Greear

Quoted 10 lines · Vijay Pandurangan

Oops, I think my last email didn't go through due to an inadvertent html attachment from my phone mail client.

Can you send us a copy of a packet you're sending and/or confirm that the IP and UDP4 checksums are set correctly in the packet?

If those are set right, I think we need to read through the networking code again to see why this is broken...

Quoted 5 lines

Wireshark decodes the packet as having no checksum errors.

I think the contents of the packet is correct, but the 'ip_summed' field is set incorrectly to 'NONE' when transmitting on a raw packet socket.

Yeah, these bugs are all due to the different interpretations of ip_summed on TX path and RX path. I think the following patch should work, if the comments don't mislead me. Could you give it a try?

Quoted 8 lines

Quoted 2 lines

For the long term, we need to unify the meaning of ip_summed on TX path and RX path, or at least translate it in skb_scrub_packet().

I can test this tomorrow, but I think it will not work. I'm not sending raw IP frames, I'm sending full ethernet frames. Socket is PF_PACKET, SOCK_RAW.

Your patch may still be useful for others though?

Here we go:

net/packet/af_packet.c +1 -0

index 1ecfa71..ab66080 100644
@@ -1925,6 +1925,7 @@ static int packet_sendmsg_spkt(struct socket
 *sock, struct msghdr *msg,
                 goto out_unlock;
         }
 
+       skb->ip_summed = CHECKSUM_UNNECESSARY;
         skb->protocol = proto;
         skb->dev = dev;

         skb->priority = sk->sk_priority;

net/packet/af_packet.c +1 -0

@@ -2496,6 +2497,7 @@ static int tpacket_fill_skb(struct packet_sock
 *po, struct sk_buff *skb,
 
         ph.raw = frame;
 
+       skb->ip_summed = CHECKSUM_UNNECESSARY;
         skb->protocol = proto;
         skb->dev = dev;

         skb->priority = po->sk.sk_priority;

net/packet/af_packet.c +1 -0

@@ -2805,6 +2807,7 @@ static struct sk_buff *packet_alloc_skb(struct
 sock *sk, size_t prepad,
         skb_put(skb, linear);
         skb->data_len = len - linear;
         skb->len += len - linear;
+       skb->ip_summed = CHECKSUM_UNNECESSARY;
 
         return skb;

  }

I am suspicious that this will break at least some drivers. I grepped around
for ip_summed, and found this, for instance:

davicom/dm9000.c

        /* The DM9000 is not smart enough to leave fragmented packets alone.
*/
        if (dm->ip_summed != ip_summed) {
                if (ip_summed == CHECKSUM_NONE)
                        iow(dm, DM9000_TCCR, 0);
                else
                        iow(dm, DM9000_TCCR, TCCR_IP | TCCR_UDP | TCCR_TCP);
                dm->ip_summed = ip_summed;
        }

It is taking action based on ip_summed == CHECKSUM_NONE, and your change will probably break that.

I would suggest that we try to make any fix specific only to veth, at least for now. A tree-wide audit of drivers is probably required to safely make the kind of change you propose above.

So, unless you can explain why your change is safe, then I do not plan to test it.

So I believe dm9000 needs to fix.

I am suspicious that this will break at least some drivers. I grepped around for ip_summed, and found this, for instance:

davicom/dm9000.c

        /* The DM9000 is not smart enough to leave fragmented packets

From: Ben Greear <[email protected]>
Date: Fri, 25 Mar 2016 09:10:58 -0700

Quoted 5 lines

I am suspicious that this will break at least some drivers. I grepped around for ip_summed, and found this, for instance:

davicom/dm9000.c

Quoted 1 line

         /* The DM9000 is not smart enough to leave fragmented packets

An really old (circa 1997), not-oft-used, driver such as this is not the place to be looking for correct usage of skb->ip_summed semantics.

I would never use whatever a driver like this does influence whether I apply a bug fix or not.

Anyway, you know the stack and drivers better than me, so if you think Cong's patch is valid, then I'll test it and make sure it works in my setups at least.

Here we go:

net/packet/af_packet.c +1 -0

index 1ecfa71..ab66080 100644
@@ -1925,6 +1925,7 @@ static int packet_sendmsg_spkt(struct socket
 *sock, struct msghdr *msg,
                 goto out_unlock;
         }
 
+       skb->ip_summed = CHECKSUM_UNNECESSARY;
         skb->protocol = proto;
         skb->dev = dev;

         skb->priority = sk->sk_priority;

net/packet/af_packet.c +1 -0

@@ -2496,6 +2497,7 @@ static int tpacket_fill_skb(struct packet_sock
 *po, struct sk_buff *skb,
 
         ph.raw = frame;
 
+       skb->ip_summed = CHECKSUM_UNNECESSARY;
         skb->protocol = proto;
         skb->dev = dev;

         skb->priority = po->sk.sk_priority;

net/packet/af_packet.c +1 -0

@@ -2805,6 +2807,7 @@ static struct sk_buff *packet_alloc_skb(struct
 sock *sk, size_t prepad,
         skb_put(skb, linear);
         skb->data_len = len - linear;
         skb->len += len - linear;
+       skb->ip_summed = CHECKSUM_UNNECESSARY;
 
         return skb;

  }

Quoted 32 lines · Cong Wang

Here we go:

net/packet/af_packet.c +1 -0

index 1ecfa71..ab66080 100644
@@ -1925,6 +1925,7 @@ static int packet_sendmsg_spkt(struct socket
 *sock, struct msghdr *msg,
                 goto out_unlock;
         }
 
+       skb->ip_summed = CHECKSUM_UNNECESSARY;
         skb->protocol = proto;
         skb->dev = dev;

         skb->priority = sk->sk_priority;

net/packet/af_packet.c +1 -0

@@ -2496,6 +2497,7 @@ static int tpacket_fill_skb(struct packet_sock
 *po, struct sk_buff *skb,
 
         ph.raw = frame;
 
+       skb->ip_summed = CHECKSUM_UNNECESSARY;
         skb->protocol = proto;
         skb->dev = dev;

         skb->priority = po->sk.sk_priority;

net/packet/af_packet.c +1 -0

@@ -2805,6 +2807,7 @@ static struct sk_buff *packet_alloc_skb(struct
 sock *sk, size_t prepad,
         skb_put(skb, linear);
         skb->data_len = len - linear;
         skb->len += len - linear;
+       skb->ip_summed = CHECKSUM_UNNECESSARY;
 
         return skb;

Quoted 1 line

  }

I have tested UDP, TCP, TCPv6 and custom Ethernet frames across a veth pair.

And, UDP, TCP, and pktgen across a pair of veth pairs bridged by my user-space packet filter.

All of these tests work fine with your patch as far as I can tell.

So, you can add:

Tested-by Ben Greear <[email protected]>

That said, it could easily break some drivers and/or other scenarios that I have not tested, so at the least it should cook a while upstream before going into the
stable tree....

Thanks,
Ben

--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com

consider two scenarios, where process a sends raw ethernet frames containing UDP packets to b

I) process a --> veth --> process b

II) process a -> eth -> wire -> eth -> process b

I believe (I) is the simplest setup we can create that will replicate this bug.

If process a sends frames that contain UDP packets to process b, what is the behaviour we want if the UDP packet *has an incorrect checksum*?

It seems to me that I and II should have identical behaviour, and I would think that (II) would not deliver the packets to the application.

In (I) with Cong's patch would we be delivering corrupt UDP packets to process b despite an incorrect checksum in (I)?

If so, I would argue that this patch isn't right.

Quoted 15 lines · Vijay Pandurangan

consider two scenarios, where process a sends raw ethernet frames containing UDP packets to b

I) process a --> veth --> process b

II) process a -> eth -> wire -> eth -> process b

I believe (I) is the simplest setup we can create that will replicate this bug.

If process a sends frames that contain UDP packets to process b, what is the behaviour we want if the UDP packet *has an incorrect checksum*?

Quoted 8 lines

It seems to me that I and II should have identical behaviour, and I would think that (II) would not deliver the packets to the application.

In (I) with Cong's patch would we be delivering corrupt UDP packets to process b despite an incorrect checksum in (I)?

If so, I would argue that this patch isn't right.

Checksums are normally used to deal with flaky transport mechanisms, and once a machine receives the frame, we do not keep re-calculating checksums
as we move it through various drivers and subsystems.

In particular, checksums are NOT a security mechanism and can be easily faked.

Since packets sent on one veth never actually hit any unreliable transport before they are received on the peer veth, then there should be no need to checksum packets whose origin is known to be on the local machine.

Any frame sent from a socket can be considered to be a local packet in my opinion.

That is what Cong's patch does as far as I can tell.

Thanks,
Ben

--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com

Quoted 31 lines · Ben Greear

Quoted 15 lines · Vijay Pandurangan

consider two scenarios, where process a sends raw ethernet frames containing UDP packets to b

I) process a --> veth --> process b

II) process a -> eth -> wire -> eth -> process b

I believe (I) is the simplest setup we can create that will replicate this bug.

If process a sends frames that contain UDP packets to process b, what is the behaviour we want if the UDP packet *has an incorrect checksum*?

Quoted 8 lines

It seems to me that I and II should have identical behaviour, and I would think that (II) would not deliver the packets to the application.

In (I) with Cong's patch would we be delivering corrupt UDP packets to process b despite an incorrect checksum in (I)?

If so, I would argue that this patch isn't right.

Checksums are normally used to deal with flaky transport mechanisms, and once a machine receives the frame, we do not keep re-calculating checksums
as we move it through various drivers and subsystems.

Quoted 6 lines

In particular, checksums are NOT a security mechanism and can be easily faked.

Since packets sent on one veth never actually hit any unreliable transport before they are received on the peer veth, then there should be no need to checksum packets whose origin is known to be on the local machine.

That's a good argument. I'm trying to figure out how to reconcile your thoughts with the argument that virtual ethernet devices are an abstraction that should behave identically to perfectly-functional physical ethernet devices when connected with a wire.

In my view, the invariant must be identical functionality, and if I were writing a regression test for this system, that's what I would test. I think optimizations for eliding checksums should be implemented only if they don't alter this functionality.

There must be a way to structure / write this code so that we can optimize veths without causing different behaviour ...

Quoted 2 lines

Any frame sent from a socket can be considered to be a local packet in my opinion.

I'm not sure that's totally right. Your bridge is adding a delay to your packets; it could just as easily be simulating corruption by corrupting 5% of packets going through it. If this change allows corrupt packets to be delivered to an application when they could not be delivered if the packets were routed via physical eths, I think that is a bug.

A real NIC can either do hardware checksums, or it cannot. If it cannot, then the host must do it on the CPU for both transmit and receive.

Veth is not a real NIC, and it cannot do hardware checksum offloading.

So, we either lie and pretend it does, or we eat massive amounts of CPU usage to calculate and check checksums when sending across a veth pair.

Quoted 10 lines

Quoted 2 lines

Any frame sent from a socket can be considered to be a local packet in my opinion.

I'm not sure that's totally right. Your bridge is adding a delay to your packets; it could just as easily be simulating corruption by corrupting 5% of packets going through it. If this change allows corrupt packets to be delivered to an application when they could not be delivered if the packets were routed via physical eths, I think that is a bug.

I actually do support corrupting the frame, but what I normally do is corrupt the contents
of the packet, and then recalculate the IP checksum (and TCP if it applies) and send it on its way. The receiving NIC and stack will pass the frame up to
the application since the checksums match, and it would be up the application
to deal with it. So, I can easily cause an application to receive corrupted frames over physical eths.

I can also corrupt without updating the checksums in case you want to test another systems NIC and/or stack.

But, if I am purposely corrupting a frame destined for veth, then the only reason
I would want the stack to check the checksums is if I were testing my own stack's checksum logic, and that seems to be a pretty limited use.

Thanks,
Ben

--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com

Quoted 4 lines · Ben Greear

A real NIC can either do hardware checksums, or it cannot. If it cannot, then the host must do it on the CPU for both transmit and receive.

Quoted 6 lines

Veth is not a real NIC, and it cannot do hardware checksum offloading.

So, we either lie and pretend it does, or we eat massive amounts of CPU usage to calculate and check checksums when sending across a veth pair.

That's a good point. Does anyone know what the overhead actually is these days?

Quoted 4 lines

But, if I am purposely corrupting a frame destined for veth, then the only reason
I would want the stack to check the checksums is if I were testing my own stack's checksum logic, and that seems to be a pretty limited use.

In the common case you're 100% right. OTOH, there's something disconcerting about an abstraction layer lying and behaving unexpectedly. Most traffic that originates on a machine can have its checksums safely ignored. Whatever the reason is (maybe, as you say you're testing checksums – on the other hand maybe there's a bug in your code somewhere), I really feel like we should try to figure out a way to ensure that this optimization is at the very least opt-in…

consider two scenarios, where process a sends raw ethernet frames containing UDP packets to b

I) process a --> veth --> process b

II) process a -> eth -> wire -> eth -> process b

I believe (I) is the simplest setup we can create that will replicate this bug.

If process a sends frames that contain UDP packets to process b, what is the behaviour we want if the UDP packet *has an incorrect checksum*?

It seems to me that I and II should have identical behaviour, and I would think that (II) would not deliver the packets to the application.

In (I) with Cong's patch would we be delivering corrupt UDP packets to process b despite an incorrect checksum in (I)?

Quoted 32 lines · Cong Wang

Here we go:

net/packet/af_packet.c +1 -0

index 1ecfa71..ab66080 100644
@@ -1925,6 +1925,7 @@ static int packet_sendmsg_spkt(struct socket
 *sock, struct msghdr *msg,
                 goto out_unlock;
         }
 
+       skb->ip_summed = CHECKSUM_UNNECESSARY;
         skb->protocol = proto;
         skb->dev = dev;

         skb->priority = sk->sk_priority;

net/packet/af_packet.c +1 -0

@@ -2496,6 +2497,7 @@ static int tpacket_fill_skb(struct packet_sock
 *po, struct sk_buff *skb,
 
         ph.raw = frame;
 
+       skb->ip_summed = CHECKSUM_UNNECESSARY;
         skb->protocol = proto;
         skb->dev = dev;

         skb->priority = po->sk.sk_priority;

net/packet/af_packet.c +1 -0

@@ -2805,6 +2807,7 @@ static struct sk_buff *packet_alloc_skb(struct
 sock *sk, size_t prepad,
         skb_put(skb, linear);
         skb->data_len = len - linear;
         skb->len += len - linear;
+       skb->ip_summed = CHECKSUM_UNNECESSARY;
 
         return skb;

Quoted 1 line

  }

I have tested UDP, TCP, TCPv6 and custom Ethernet frames across a veth pair.

And, UDP, TCP, and pktgen across a pair of veth pairs bridged by my user-space packet filter.

All of these tests work fine with your patch as far as I can tell.

So, you can add:

Tested-by Ben Greear <[email protected]>

That said, it could easily break some drivers and/or other scenarios that I have not tested, so at the least it should cook a while upstream before going into the
stable tree....