Message ID | alpine.DEB.2.02.1402110928070.7090@tomh.mtv.corp.google.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On 11/02/2014 19:43, Tom Herbert wrote: > The code to validate checksum in UDP gro_receive explictly checks > against driver having set CHECKSUM_COMPLETE. This does not perform > GRO on UDP packets with a checksum of zero (no checksum needed). > This patch adds the condition to allow UDP checksum to be zero. > Signed-off-by: Tom Herbert <therbert@google.com> > --- > net/ipv4/udp_offload.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c > index 25f5cee..4db7796 100644 > --- a/net/ipv4/udp_offload.c > +++ b/net/ipv4/udp_offload.c > @@ -156,13 +156,9 @@ static struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *s > unsigned int hlen, off; > int flush = 1; > > - if (NAPI_GRO_CB(skb)->udp_mark || > - (!skb->encapsulation && skb->ip_summed != CHECKSUM_COMPLETE)) > + if (NAPI_GRO_CB(skb)->udp_mark) > goto out; > > - /* mark that this skb passed once through the udp gro layer */ > - NAPI_GRO_CB(skb)->udp_mark = 1; > - > off = skb_gro_offset(skb); > hlen = off + sizeof(*uh); > uh = skb_gro_header_fast(skb, off); > @@ -172,6 +168,13 @@ static struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *s > goto out; > } > > + if (!skb->encapsulation && > + skb->ip_summed != CHECKSUM_COMPLETE && uh->check != 0) > + goto out; > + > + /* mark that this skb passed once through the udp gro layer */ > + NAPI_GRO_CB(skb)->udp_mark = 1; > + Hi Tom, Considering the patch just "as is" vs. the current code, its OK. However, as skbs have only one indicator for the status of the checksum checks done by the receiving hardware, the basic assertion I thought we needed here is to reject skb if either it has the udp mark set or the encapsulation field is false, this is according to the conventions set by these two commits 0afb166 vxlan: Add capability of Rx checksum offload for inner packet 6a674e9 net: Add support for hardware-offloaded encapsulation B/c after finalizing the GRO work and decapsulating, skb injected up into the TCP stack with ip_summed equals to CHECKSUM_NONE are rejected. If this assumption is wrong, maybe we can remove testing the ip_summed field here altogether? Or. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Hi Tom, > > Considering the patch just "as is" vs. the current code, its OK. > > However, as skbs have only one indicator for the status of the checksum > checks done by the receiving hardware, the basic assertion I thought we > needed here is to reject skb if either it has the udp mark set or the > encapsulation field is false, this is according to the conventions set by > these two commits > > 0afb166 vxlan: Add capability of Rx checksum offload for inner packet > 6a674e9 net: Add support for hardware-offloaded encapsulation > > B/c after finalizing the GRO work and decapsulating, skb injected up into > the TCP stack with ip_summed equals to CHECKSUM_NONE are rejected. If this > assumption is wrong, maybe we can remove testing the ip_summed field here > altogether? > If I'm interpreting the semantics correctly, when skb->encapsulation is set the ip_summed is valid for both the inner and outer header (e.g. CHECKSUM_COMPLETE is always assumed okay for both layers). If skb->encapsulation is not set then ip_summed is only valid for outer header. So then the patch is broken in the case that encap is not set, ip_summed is CHECKSUM_UNNECESSARY, csum == 0, and we need to validate the inner checksum. But even worse, is there a fundamental issue where udp4_csum_init is able to change ip_summed to be CHECKSUM_UNNECESSARY (either check == 0 or ip_summed == CHECKSUM_UNNECESSARY) regardless of skb->encapsulation, sending the packet into encap_rcv which could ultimately incorrectly apply ip_summed on the inner TCP/UDP packet? Tom > Or. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 14, 2014 at 12:27 AM, Tom Herbert <therbert@google.com> wrote: >> [...] this is according to the conventions set by >> 0afb166 vxlan: Add capability of Rx checksum offload for inner packet >> 6a674e9 net: Add support for hardware-offloaded encapsulation >> B/c after finalizing the GRO work and decapsulating, skb injected up into >> the TCP stack with ip_summed equals to CHECKSUM_NONE are rejected. If >> this assumption is wrong, maybe we can remove testing the ip_summed field >> here altogether? > If I'm interpreting the semantics correctly, when skb->encapsulation > is set the ip_summed is valid for both the inner and outer header > (e.g. CHECKSUM_COMPLETE is always assumed okay for both layers). If > skb->encapsulation is not set then ip_summed is only valid for outer header. Yep, I think this would be correct interpertation, Joseph, agree? > So then the patch is broken in the case that encap is not set, > ip_summed is CHECKSUM_UNNECESSARY, csum == 0, and we need to > validate the inner checksum. Just to make sure, by "the patch" you refer to your patch or the current code? > But even worse, is there a fundamental issue where udp4_csum_init is able > to change ip_summed to be CHECKSUM_UNNECESSARY (either check == 0 > or ip_summed == CHECKSUM_UNNECESSARY) regardless of > skb->encapsulation, sending the packet into encap_rcv which could > ultimately incorrectly apply ip_summed on the inner TCP/UDP packet? By fundamental you mean performance issue or functionality issue (bug) or both? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 13 Feb 2014, Or Gerlitz wrote: > On Fri, Feb 14, 2014 at 12:27 AM, Tom Herbert <therbert@google.com> wrote: > > >> [...] this is according to the conventions set by > >> 0afb166 vxlan: Add capability of Rx checksum offload for inner packet > >> 6a674e9 net: Add support for hardware-offloaded encapsulation > >> B/c after finalizing the GRO work and decapsulating, skb injected up into > >> the TCP stack with ip_summed equals to CHECKSUM_NONE are rejected. If >> this assumption is wrong, maybe we can remove testing the ip_summed field > >> here altogether? > > > If I'm interpreting the semantics correctly, when skb->encapsulation > > is set the ip_summed is valid for both the inner and outer header > > (e.g. CHECKSUM_COMPLETE is always assumed okay for both layers). If > > skb->encapsulation is not set then ip_summed is only valid for outer header. > > Yep, I think this would be correct interpertation, Joseph, agree? Agreed. > > > So then the patch is broken in the case that encap is not set, > > ip_summed is CHECKSUM_UNNECESSARY, csum == 0, and we need to > > validate the inner checksum. > > Just to make sure, by "the patch" you refer to your patch or the current code? > > > But even worse, is there a fundamental issue where udp4_csum_init is able > > to change ip_summed to be CHECKSUM_UNNECESSARY (either check == 0 > > or ip_summed == CHECKSUM_UNNECESSARY) regardless of > > skb->encapsulation, sending the packet into encap_rcv which could > > ultimately incorrectly apply ip_summed on the inner TCP/UDP packet? > > By fundamental you mean performance issue or functionality issue (bug) or both? > I would expect the check to be for ip_summed == CHECKSUM_UNNECESSARY. This was the original thought behind commit: 0afb166 vxlan: Add capability of Rx checksum offload for inner packet -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> > But even worse, is there a fundamental issue where udp4_csum_init is able >> > to change ip_summed to be CHECKSUM_UNNECESSARY (either check == 0 >> > or ip_summed == CHECKSUM_UNNECESSARY) regardless of >> > skb->encapsulation, sending the packet into encap_rcv which could >> > ultimately incorrectly apply ip_summed on the inner TCP/UDP packet? >> >> By fundamental you mean performance issue or functionality issue (bug) or both? >> > > I would expect the check to be for ip_summed == CHECKSUM_UNNECESSARY. This > was the original thought behind commit: > > 0afb166 vxlan: Add capability of Rx checksum offload for inner packet It looks like udp4_csum_init turns CHECKSUM_COMPLETE and check==0 into CHECKSUM_UNNECESSARY which could bypass the checksum validation for the encapsulated packet. This would be a significant functionality bug. Unfortunately udp4_csum_init writes ip_summed without regard to encapsulation. Seems like the logic in the UDP rcv path should be: if ip_summed == CHECKSUM_COMPLETE, ensure this is same value when calling encap_rcv if ip_summed == CHECKSUM_UNNECESSARY && !skb->encap change to CHECKSUM_NONE before calling encap_rcv if ip_summed == CHECKSUM_UNNECESSARY && skb->encap ip_summed value is okay In any case, we need to consider the orignal ip_summed value from the driver, not the one that udp4_csum_init (udp_gro or anywhere else in the path) would set. Also, udp_gro_receive should be able to handle the case where ip_summed == CHECKSUM_UNNECESSARY and !skb->encapsulation, that will be very common scenario. Probably CHECKSUM_NONE also. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 13 Feb 2014, Tom Herbert wrote: > >> > But even worse, is there a fundamental issue where udp4_csum_init is able > >> > to change ip_summed to be CHECKSUM_UNNECESSARY (either check == 0 > >> > or ip_summed == CHECKSUM_UNNECESSARY) regardless of > >> > skb->encapsulation, sending the packet into encap_rcv which could > >> > ultimately incorrectly apply ip_summed on the inner TCP/UDP packet? > >> > >> By fundamental you mean performance issue or functionality issue (bug) or both? > >> > > > > I would expect the check to be for ip_summed == CHECKSUM_UNNECESSARY. This > > was the original thought behind commit: > > > > 0afb166 vxlan: Add capability of Rx checksum offload for inner packet > > It looks like udp4_csum_init turns CHECKSUM_COMPLETE and check==0 into > CHECKSUM_UNNECESSARY which could bypass the checksum validation for > the encapsulated packet. This would be a significant functionality > bug. Unfortunately udp4_csum_init writes ip_summed without regard to > encapsulation. > > Seems like the logic in the UDP rcv path should be: > > if ip_summed == CHECKSUM_COMPLETE, ensure this is same value when > calling encap_rcv > if ip_summed == CHECKSUM_UNNECESSARY && !skb->encap change to > CHECKSUM_NONE before calling encap_rcv > if ip_summed == CHECKSUM_UNNECESSARY && skb->encap ip_summed value is okay > > In any case, we need to consider the orignal ip_summed value from the > driver, not the one that udp4_csum_init (udp_gro or anywhere else in > the path) would set. > > Also, udp_gro_receive should be able to handle the case where > ip_summed == CHECKSUM_UNNECESSARY and !skb->encapsulation, that will > be very common scenario. Probably CHECKSUM_NONE also. > Yes, I now see your point and totaly agree. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> In any case, we need to consider the orignal ip_summed value from the >> driver, not the one that udp4_csum_init (udp_gro or anywhere else in >> the path) would set. >> >> Also, udp_gro_receive should be able to handle the case where >> ip_summed == CHECKSUM_UNNECESSARY and !skb->encapsulation, that will >> be very common scenario. Probably CHECKSUM_NONE also. >> > > Yes, I now see your point and totaly agree. Thanks. Okay, I'll look at fixing this. I suspect we want to maintain CHECKSUM_COMPLETE as long as possible in UDP receive path (or any other encap path) and not be converting to CHECKSUM_UNNECESSARY. When crossing the encapsulation layer we'll need to deal with skb->encapsulation and CHECKSUM_UNNECESSARY. Note to HW vendors: can you please start providing the full packet checksum (CHECKSUM_COMPLETE) and stop perpetuating the extremely protocol specific, restrictive checksum validation! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c index 25f5cee..4db7796 100644 --- a/net/ipv4/udp_offload.c +++ b/net/ipv4/udp_offload.c @@ -156,13 +156,9 @@ static struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *s unsigned int hlen, off; int flush = 1; - if (NAPI_GRO_CB(skb)->udp_mark || - (!skb->encapsulation && skb->ip_summed != CHECKSUM_COMPLETE)) + if (NAPI_GRO_CB(skb)->udp_mark) goto out; - /* mark that this skb passed once through the udp gro layer */ - NAPI_GRO_CB(skb)->udp_mark = 1; - off = skb_gro_offset(skb); hlen = off + sizeof(*uh); uh = skb_gro_header_fast(skb, off); @@ -172,6 +168,13 @@ static struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *s goto out; } + if (!skb->encapsulation && + skb->ip_summed != CHECKSUM_COMPLETE && uh->check != 0) + goto out; + + /* mark that this skb passed once through the udp gro layer */ + NAPI_GRO_CB(skb)->udp_mark = 1; + rcu_read_lock(); uo_priv = rcu_dereference(udp_offload_base); for (; uo_priv != NULL; uo_priv = rcu_dereference(uo_priv->next)) {
The code to validate checksum in UDP gro_receive explictly checks against driver having set CHECKSUM_COMPLETE. This does not perform GRO on UDP packets with a checksum of zero (no checksum needed). This patch adds the condition to allow UDP checksum to be zero. Signed-off-by: Tom Herbert <therbert@google.com> --- net/ipv4/udp_offload.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)