| Message ID | CAM_iQpWwch3+6u+X0DBGyVSWttd7Nz2COdmq2q=G9BxisW52bA@mail.gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 03/24/2016 10:33 PM, Cong Wang wrote: > On Thu, Mar 24, 2016 at 10:13 PM, Ben Greear <greearb@candelatech.com> wrote: >> >> >> On 03/24/2016 10:06 PM, Cong Wang wrote: >>> >>> On Thu, Mar 24, 2016 at 9:34 PM, Ben Greear <greearb@candelatech.com> >>> wrote: >>>> >>>> >>>> >>>> On 03/24/2016 06:44 PM, Vijay Pandurangan wrote: >>>>> >>>>> >>>>> 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. >>> >>> >>> 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(). >> >> >> 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: > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 1ecfa71..ab66080 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -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; > @@ -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; > @@ -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. Thanks, Ben
On Fri, Mar 25, 2016 at 9:10 AM, Ben Greear <greearb@candelatech.com> wrote: > 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. I just blindly trust the comments there: * CHECKSUM_UNNECESSARY: * * This has the same meaning on as CHECKSUM_NONE for checksum offload on * output. Let's Cc Tom who wrote this comment. On the other hand, hyperv got this correctly: if ((skb->ip_summed == CHECKSUM_NONE) || (skb->ip_summed == CHECKSUM_UNNECESSARY)) goto do_send; So I believe dm9000 needs to fix. Thanks.
From: Ben Greear <greearb@candelatech.com> Date: Fri, 25 Mar 2016 09:10:58 -0700 > 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 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.
From: Cong Wang <xiyou.wangcong@gmail.com> Date: Fri, 25 Mar 2016 09:32:23 -0700 > So I believe dm9000 needs to fix. +1
On 03/25/2016 09:44 AM, David Miller wrote: > From: Ben Greear <greearb@candelatech.com> > Date: Fri, 25 Mar 2016 09:10:58 -0700 > >> 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 > > 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. Point is, it took me 5 minutes to find that, and I did not look hard at many other drivers. e1000e and igb appear to be fine, and maybe the rest of them are too. Lord knows what other strange setups might be effected by the ip_summed change. 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. Thanks, Ben
From: Ben Greear <greearb@candelatech.com> Date: Fri, 25 Mar 2016 10:14:46 -0700 > 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. It probably is, I'm just waiting to see if Tom Herbert will give some feedback or not as this is an area he understands well.
On 03/24/2016 10:33 PM, Cong Wang wrote: > Here we go: > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 1ecfa71..ab66080 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -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; > @@ -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; > @@ -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 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 <greearb@candelatech.com> 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
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. On Fri, Mar 25, 2016 at 4:56 PM, Ben Greear <greearb@candelatech.com> wrote: > On 03/24/2016 10:33 PM, Cong Wang wrote: > >> Here we go: >> >> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c >> index 1ecfa71..ab66080 100644 >> --- a/net/packet/af_packet.c >> +++ b/net/packet/af_packet.c >> @@ -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; >> @@ -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; >> @@ -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 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 <greearb@candelatech.com> > > 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 <greearb@candelatech.com> > Candela Technologies Inc http://www.candelatech.com >
On Fri, Mar 25, 2016 at 1:56 PM, Ben Greear <greearb@candelatech.com> wrote: > On 03/24/2016 10:33 PM, Cong Wang wrote: > >> Here we go: >> >> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c >> index 1ecfa71..ab66080 100644 >> --- a/net/packet/af_packet.c >> +++ b/net/packet/af_packet.c >> @@ -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; >> @@ -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; >> @@ -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 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 <greearb@candelatech.com> Thanks for testing! I will send it out formally after I audit more drivers, also let's give Tom some time to response. > > 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.... Yeah. Thanks.
On 03/25/2016 02:59 PM, Vijay Pandurangan wrote: > 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. 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
On Fri, Mar 25, 2016 at 2:59 PM, Vijay Pandurangan <vijayp@vijayp.ca> wrote: > 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)? > Right, I thought packet socket does the checksum by itself, so the problem is: if user-space does the checksum like packet socket, its checksum could be wrong therefore we can not trust it on RX path once it loops back. Let me think about it again.
On Fri, Mar 25, 2016 at 6:23 PM, Ben Greear <greearb@candelatech.com> wrote: > On 03/25/2016 02:59 PM, Vijay Pandurangan wrote: >> >> 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. > > > 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. 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 ... > > 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. > > That is what Cong's patch does as far as I can tell. > > > Thanks, > Ben > > > -- > Ben Greear <greearb@candelatech.com> > Candela Technologies Inc http://www.candelatech.com >
On 03/25/2016 04:03 PM, Vijay Pandurangan wrote: > On Fri, Mar 25, 2016 at 6:23 PM, Ben Greear <greearb@candelatech.com> wrote: >> On 03/25/2016 02:59 PM, Vijay Pandurangan wrote: >>> >>> 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. >> >> >> 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. > > 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 ... 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. >> 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
On Fri, Mar 25, 2016 at 7:46 PM, Ben Greear <greearb@candelatech.com> wrote: > 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. > That's a good point. Does anyone know what the overhead actually is these days? >>> 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. 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… > > > Thanks, > Ben > > -- > Ben Greear <greearb@candelatech.com> > Candela Technologies Inc http://www.candelatech.com >
On 04/07/2016 08:11 AM, Vijay Pandurangan wrote: > On Fri, Mar 25, 2016 at 7:46 PM, Ben Greear <greearb@candelatech.com> wrote: >> 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. >> > > That's a good point. Does anyone know what the overhead actually is these days? You could try setting up a system with ixgbe or similar, and then manually disable csum offload using ethtool, and see how that performs in comparison to hardware offload? >> 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… I'm fine with allowing a user to force software-csum on veth devices if someone wants to code that up, but forcing sw-csum for local frames on veth devices should be disabled by default. Thanks, Ben
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 1ecfa71..ab66080 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -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;