[PATCH 2/3] net: UDP gro_receive accept csum=0

[PATCH 2/3] net: UDP gro_receive accept csum=0

[Tom Herbert]

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;
+
 	rcu_read_lock();
 	uo_priv = rcu_dereference(udp_offload_base);
 	for (; uo_priv != NULL; uo_priv = rcu_dereference(uo_priv->next)) {
-- 
1.9.0.rc1.175.g0b1dcb5

Re: [PATCH 2/3] net: UDP gro_receive accept csum=0

[Or Gerlitz]

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.

Re: [PATCH 2/3] net: UDP gro_receive accept csum=0

[Tom Herbert]

> 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.
>

Re: [PATCH 2/3] net: UDP gro_receive accept csum=0

[Or Gerlitz]

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?

Re: [PATCH 2/3] net: UDP gro_receive accept csum=0

[Joseph Gasparakis]

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

Re: [PATCH 2/3] net: UDP gro_receive accept csum=0

[Tom Herbert]

>> > 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.

Re: [PATCH 2/3] net: UDP gro_receive accept csum=0

[Joseph Gasparakis]

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.

Re: [PATCH 2/3] net: UDP gro_receive accept csum=0

[Tom Herbert]

>> 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!