[net-next PATCH 1/4] gre: Enforce IP ID verification on outer headers

[net-next PATCH 1/4] gre: Enforce IP ID verification on outer headers

[Alexander Duyck]

This change enforces the IP ID verification on outer headers.  As a result
if the DF flag is not set on the outer header we will force the flow to be
flushed in the event that the IP ID is out of sequence with the existing
flow.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 net/ipv4/gre_offload.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
index 47f4c544c916..632733f18019 100644
--- a/net/ipv4/gre_offload.c
+++ b/net/ipv4/gre_offload.c
@@ -175,8 +175,6 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
 					     null_compute_pseudo);
 	}
 
-	flush = 0;
-
 	for (p = *head; p; p = p->next) {
 		const struct gre_base_hdr *greh2;
 
@@ -205,6 +203,9 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
 				continue;
 			}
 		}
+
+		/* Include the IP ID check from the outer IP hdr */
+		NAPI_GRO_CB(p)->flush |= NAPI_GRO_CB(p)->flush_id;
 	}
 
 	skb_gro_pull(skb, grehlen);
@@ -213,6 +214,7 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
 	skb_gro_postpull_rcsum(skb, greh, grehlen);
 
 	pp = ptype->callbacks.gro_receive(head, skb);
+	flush = 0;
 
 out_unlock:
 	rcu_read_unlock();

[net-next PATCH 2/4] geneve: Enforce IP ID verification on outer headers

[Alexander Duyck]

This change enforces the IP ID verification on outer headers.  As a result
if the DF flag is not set on the outer header we will force the flow to be
flushed in the event that the IP ID is out of sequence with the existing
flow.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 drivers/net/geneve.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index bc5da357e16d..f4b89b73517e 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -462,8 +462,6 @@ static struct sk_buff **geneve_gro_receive(struct sk_buff **head,
 			goto out;
 	}
 
-	flush = 0;
-
 	for (p = *head; p; p = p->next) {
 		if (!NAPI_GRO_CB(p)->same_flow)
 			continue;
@@ -474,20 +472,23 @@ static struct sk_buff **geneve_gro_receive(struct sk_buff **head,
 			NAPI_GRO_CB(p)->same_flow = 0;
 			continue;
 		}
+
+		/* Include the IP ID check from the outer IP hdr */
+		NAPI_GRO_CB(p)->flush |= NAPI_GRO_CB(p)->flush_id;
 	}
 
 	type = gh->proto_type;
 
 	rcu_read_lock();
 	ptype = gro_find_receive_by_type(type);
-	if (!ptype) {
-		flush = 1;
+	if (!ptype)
 		goto out_unlock;
-	}
 
 	skb_gro_pull(skb, gh_len);
 	skb_gro_postpull_rcsum(skb, gh, gh_len);
+
 	pp = ptype->callbacks.gro_receive(head, skb);
+	flush = 0;
 
 out_unlock:
 	rcu_read_unlock();

[net-next PATCH 3/4] vxlan: Enforce IP ID verification on outer headers

[Alexander Duyck]

This change enforces the IP ID verification on outer headers.  As a result
if the DF flag is not set on the outer header we will force the flow to be
flushed in the event that the IP ID is out of sequence with the existing
flow.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 drivers/net/vxlan.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 775ddb48388d..906587d1531a 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -591,8 +591,6 @@ static struct sk_buff **vxlan_gro_receive(struct sk_buff **head,
 
 	skb_gro_pull(skb, sizeof(struct vxlanhdr)); /* pull vxlan header */
 
-	flush = 0;
-
 	for (p = *head; p; p = p->next) {
 		if (!NAPI_GRO_CB(p)->same_flow)
 			continue;
@@ -603,10 +601,13 @@ static struct sk_buff **vxlan_gro_receive(struct sk_buff **head,
 			NAPI_GRO_CB(p)->same_flow = 0;
 			continue;
 		}
+
+		/* Include the IP ID check from the outer IP hdr */
+		NAPI_GRO_CB(p)->flush |= NAPI_GRO_CB(p)->flush_id;
 	}
 
 	pp = eth_gro_receive(head, skb);
-
+	flush = 0;
 out:
 	skb_gro_remcsum_cleanup(skb, &grc);
 	NAPI_GRO_CB(skb)->flush |= flush;

[net-next PATCH 4/4] gue: Enforce IP ID verification on outer headers

[Alexander Duyck]

This change enforces the IP ID verification on outer headers.  As a result
if the DF flag is not set on the outer header we will force the flow to be
flushed in the event that the IP ID is out of sequence with the existing
flow.

In addition I believe this fixes a bug in which it was possible for no GRO
handler to be defined for a given protocol but we were not setting the
flush bit as it had been cleared before the check.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 net/ipv4/fou.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/fou.c b/net/ipv4/fou.c
index 88dab0c1670c..5b93572e7434 100644
--- a/net/ipv4/fou.c
+++ b/net/ipv4/fou.c
@@ -319,8 +319,6 @@ static struct sk_buff **gue_gro_receive(struct sk_buff **head,
 
 	skb_gro_pull(skb, hdrlen);
 
-	flush = 0;
-
 	for (p = *head; p; p = p->next) {
 		const struct guehdr *guehdr2;
 
@@ -343,6 +341,9 @@ static struct sk_buff **gue_gro_receive(struct sk_buff **head,
 			NAPI_GRO_CB(p)->same_flow = 0;
 			continue;
 		}
+
+		/* Include the IP ID check from the outer IP hdr */
+		NAPI_GRO_CB(p)->flush |= NAPI_GRO_CB(p)->flush_id;
 	}
 
 	rcu_read_lock();
@@ -352,6 +353,7 @@ static struct sk_buff **gue_gro_receive(struct sk_buff **head,
 		goto out_unlock;
 
 	pp = ops->callbacks.gro_receive(head, skb);
+	flush = 0;
 
 out_unlock:
 	rcu_read_unlock();

Re: [net-next PATCH 3/4] vxlan: Enforce IP ID verification on outer headers

[Or Gerlitz]

On Mon, Mar 7, 2016 at 7:22 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
> This change enforces the IP ID verification on outer headers.  As a result
> if the DF flag is not set on the outer header we will force the flow to be
> flushed in the event that the IP ID is out of sequence with the existing
> flow.

Can you please state the precise requirement for aggregation w.r.t IP
IDs here? and point to where/how this is enforced, e.g for
non-tunneled TCP GRO-ing?

Or.

Re: [net-next PATCH 3/4] vxlan: Enforce IP ID verification on outer headers

[David Miller]

From: Or Gerlitz <gerlitz.or@gmail.com>
Date: Mon, 7 Mar 2016 20:05:20 +0200

> On Mon, Mar 7, 2016 at 7:22 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
>> This change enforces the IP ID verification on outer headers.  As a result
>> if the DF flag is not set on the outer header we will force the flow to be
>> flushed in the event that the IP ID is out of sequence with the existing
>> flow.
> 
> Can you please state the precise requirement for aggregation w.r.t IP
> IDs here? and point to where/how this is enforced, e.g for
> non-tunneled TCP GRO-ing?

I also didn't see a nice "PATCH 0/4" posting explaining this series and
I'd really like to see that.

Re: [net-next PATCH 3/4] vxlan: Enforce IP ID verification on outer headers

[Alex Duyck]

On Mon, Mar 7, 2016 at 11:09 AM, David Miller <davem@davemloft.net> wrote:
> From: Or Gerlitz <gerlitz.or@gmail.com>
> Date: Mon, 7 Mar 2016 20:05:20 +0200
>
>> On Mon, Mar 7, 2016 at 7:22 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
>>> This change enforces the IP ID verification on outer headers.  As a result
>>> if the DF flag is not set on the outer header we will force the flow to be
>>> flushed in the event that the IP ID is out of sequence with the existing
>>> flow.
>>
>> Can you please state the precise requirement for aggregation w.r.t IP
>> IDs here? and point to where/how this is enforced, e.g for
>> non-tunneled TCP GRO-ing?
>
> I also didn't see a nice "PATCH 0/4" posting explaining this series and
> I'd really like to see that.

Sorry about that.  I forgot to add the cover page when I sent this.

The enforcement is coming from the IP and TCP layers.  If you take a
look in inet_gro_receive we have the NAPI_GRO_CB(p)->flush_id value
being populated based on the difference between the expected ID and
the received one.  So for IPv4 we overwrite it, and for IPv6 we set it
to 0.  The only consumer currently using it is TCP in tcp_gro_receive.
The problem is with tunnels we lose the data for the outer when the
inner overwrites it, as a result we can put whatever we want currently
in the outer IP ID and it will be accepted.

The patch set is based off of a conversation several of us had on the
list about doing TSO for tunnels and the fact that the IP IDs for the
outer header have to advance.  It makes it easier for me to validate
that I am doing things properly if GRO doesn't destroy the IP ID data
for the outer headers.

- Alex

Re: [net-next PATCH 3/4] vxlan: Enforce IP ID verification on outer headers

[Jesse Gross]

On Mon, Mar 7, 2016 at 3:06 PM, Alex Duyck <aduyck@mirantis.com> wrote:
> On Mon, Mar 7, 2016 at 11:09 AM, David Miller <davem@davemloft.net> wrote:
>> From: Or Gerlitz <gerlitz.or@gmail.com>
>> Date: Mon, 7 Mar 2016 20:05:20 +0200
>>
>>> On Mon, Mar 7, 2016 at 7:22 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
>>>> This change enforces the IP ID verification on outer headers.  As a result
>>>> if the DF flag is not set on the outer header we will force the flow to be
>>>> flushed in the event that the IP ID is out of sequence with the existing
>>>> flow.
>>>
>>> Can you please state the precise requirement for aggregation w.r.t IP
>>> IDs here? and point to where/how this is enforced, e.g for
>>> non-tunneled TCP GRO-ing?
>>
>> I also didn't see a nice "PATCH 0/4" posting explaining this series and
>> I'd really like to see that.
>
> Sorry about that.  I forgot to add the cover page when I sent this.
>
> The enforcement is coming from the IP and TCP layers.  If you take a
> look in inet_gro_receive we have the NAPI_GRO_CB(p)->flush_id value
> being populated based on the difference between the expected ID and
> the received one.  So for IPv4 we overwrite it, and for IPv6 we set it
> to 0.  The only consumer currently using it is TCP in tcp_gro_receive.
> The problem is with tunnels we lose the data for the outer when the
> inner overwrites it, as a result we can put whatever we want currently
> in the outer IP ID and it will be accepted.
>
> The patch set is based off of a conversation several of us had on the
> list about doing TSO for tunnels and the fact that the IP IDs for the
> outer header have to advance.  It makes it easier for me to validate
> that I am doing things properly if GRO doesn't destroy the IP ID data
> for the outer headers.

In net/ipv4/af_inet.c:inet_gro_receive() there is the following
comment above where NAPI_GRO_CB(p)->flush_id is set:

/* Save the IP ID check to be included later when we get to
* the transport layer so only the inner most IP ID is checked.
* This is because some GSO/TSO implementations do not
* correctly increment the IP ID for the outer hdrs.
*/

There was a long discussion about this a couple years ago and the
conclusion was that it is the inner IP ID is really the important one
in the case of encapsulation. Obviously, things like TCP/IP header
compression don't apply to the outer encapsulation header.

Re: [net-next PATCH 3/4] vxlan: Enforce IP ID verification on outer headers

[David Miller]

From: Jesse Gross <jesse@kernel.org>
Date: Mon, 7 Mar 2016 15:42:59 -0800

> On Mon, Mar 7, 2016 at 3:06 PM, Alex Duyck <aduyck@mirantis.com> wrote:
>> On Mon, Mar 7, 2016 at 11:09 AM, David Miller <davem@davemloft.net> wrote:
>>> From: Or Gerlitz <gerlitz.or@gmail.com>
>>> Date: Mon, 7 Mar 2016 20:05:20 +0200
>>>
>>>> On Mon, Mar 7, 2016 at 7:22 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
>>>>> This change enforces the IP ID verification on outer headers.  As a result
>>>>> if the DF flag is not set on the outer header we will force the flow to be
>>>>> flushed in the event that the IP ID is out of sequence with the existing
>>>>> flow.
>>>>
>>>> Can you please state the precise requirement for aggregation w.r.t IP
>>>> IDs here? and point to where/how this is enforced, e.g for
>>>> non-tunneled TCP GRO-ing?
>>>
>>> I also didn't see a nice "PATCH 0/4" posting explaining this series and
>>> I'd really like to see that.
>>
>> Sorry about that.  I forgot to add the cover page when I sent this.
>>
>> The enforcement is coming from the IP and TCP layers.  If you take a
>> look in inet_gro_receive we have the NAPI_GRO_CB(p)->flush_id value
>> being populated based on the difference between the expected ID and
>> the received one.  So for IPv4 we overwrite it, and for IPv6 we set it
>> to 0.  The only consumer currently using it is TCP in tcp_gro_receive.
>> The problem is with tunnels we lose the data for the outer when the
>> inner overwrites it, as a result we can put whatever we want currently
>> in the outer IP ID and it will be accepted.
>>
>> The patch set is based off of a conversation several of us had on the
>> list about doing TSO for tunnels and the fact that the IP IDs for the
>> outer header have to advance.  It makes it easier for me to validate
>> that I am doing things properly if GRO doesn't destroy the IP ID data
>> for the outer headers.
> 
> In net/ipv4/af_inet.c:inet_gro_receive() there is the following
> comment above where NAPI_GRO_CB(p)->flush_id is set:
> 
> /* Save the IP ID check to be included later when we get to
> * the transport layer so only the inner most IP ID is checked.
> * This is because some GSO/TSO implementations do not
> * correctly increment the IP ID for the outer hdrs.
> */
> 
> There was a long discussion about this a couple years ago and the
> conclusion was that it is the inner IP ID is really the important one
> in the case of encapsulation. Obviously, things like TCP/IP header
> compression don't apply to the outer encapsulation header.

That's how I remember the conversation going as wel...

Re: [net-next PATCH 3/4] vxlan: Enforce IP ID verification on outer headers

[Alexander Duyck]

On Wed, Mar 9, 2016 at 1:02 PM, David Miller <davem@davemloft.net> wrote:
> From: Jesse Gross <jesse@kernel.org>
> Date: Mon, 7 Mar 2016 15:42:59 -0800
>
>> On Mon, Mar 7, 2016 at 3:06 PM, Alex Duyck <aduyck@mirantis.com> wrote:
>>> On Mon, Mar 7, 2016 at 11:09 AM, David Miller <davem@davemloft.net> wrote:
>>>> From: Or Gerlitz <gerlitz.or@gmail.com>
>>>> Date: Mon, 7 Mar 2016 20:05:20 +0200
>>>>
>>>>> On Mon, Mar 7, 2016 at 7:22 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
>>>>>> This change enforces the IP ID verification on outer headers.  As a result
>>>>>> if the DF flag is not set on the outer header we will force the flow to be
>>>>>> flushed in the event that the IP ID is out of sequence with the existing
>>>>>> flow.
>>>>>
>>>>> Can you please state the precise requirement for aggregation w.r.t IP
>>>>> IDs here? and point to where/how this is enforced, e.g for
>>>>> non-tunneled TCP GRO-ing?
>>>>
>>>> I also didn't see a nice "PATCH 0/4" posting explaining this series and
>>>> I'd really like to see that.
>>>
>>> Sorry about that.  I forgot to add the cover page when I sent this.
>>>
>>> The enforcement is coming from the IP and TCP layers.  If you take a
>>> look in inet_gro_receive we have the NAPI_GRO_CB(p)->flush_id value
>>> being populated based on the difference between the expected ID and
>>> the received one.  So for IPv4 we overwrite it, and for IPv6 we set it
>>> to 0.  The only consumer currently using it is TCP in tcp_gro_receive.
>>> The problem is with tunnels we lose the data for the outer when the
>>> inner overwrites it, as a result we can put whatever we want currently
>>> in the outer IP ID and it will be accepted.
>>>
>>> The patch set is based off of a conversation several of us had on the
>>> list about doing TSO for tunnels and the fact that the IP IDs for the
>>> outer header have to advance.  It makes it easier for me to validate
>>> that I am doing things properly if GRO doesn't destroy the IP ID data
>>> for the outer headers.
>>
>> In net/ipv4/af_inet.c:inet_gro_receive() there is the following
>> comment above where NAPI_GRO_CB(p)->flush_id is set:
>>
>> /* Save the IP ID check to be included later when we get to
>> * the transport layer so only the inner most IP ID is checked.
>> * This is because some GSO/TSO implementations do not
>> * correctly increment the IP ID for the outer hdrs.
>> */
>>
>> There was a long discussion about this a couple years ago and the
>> conclusion was that it is the inner IP ID is really the important one
>> in the case of encapsulation. Obviously, things like TCP/IP header
>> compression don't apply to the outer encapsulation header.
>
> That's how I remember the conversation going as wel...

Given that then, how would you feel about a software scheme to use
existing TSO support on devices to support UDP/GRE tunnel TSO by
pre-computing the values for the outer headers and then having those
duplicated on TSO frames?

This is kind of what got me started down the path of validating the
outer IP ID.  If we are okay with UDP or GRE encapsulated tunnels
using the same IP ID for the outer headers then I should be able to
code up something in software so that we can get more devices
supporting tunnel TSO offload for tunnels.

- Alex