[net PATCH] gro: Allow tunnel stacking in the case of FOU/GUE

[net PATCH] gro: Allow tunnel stacking in the case of FOU/GUE

[Alexander Duyck]

This patch should fix the issues seen with a recent fix to prevent
tunnel-in-tunnel frames from being generated with GRO.  The fix itself is
correct for now as long as we do not add any devices that support
NETIF_F_GSO_GRE_CSUM.  When such a device is added it could have the
potential to mess things up due to the fact that the outer transport header
points to the outer UDP header and not the GRE header as would be expected.

Fixes: fac8e0f579695 ("tunnels: Don't apply GRO to multiple layers of encapsulation.")
Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---

This should allow us to keep the fix that Jesse added without breaking the
3 cases that Tom called out in terms of FOU/GUE.

Additional work will be needed in net-next as we probably need to make it
so that offloads work correctly when we get around to supporting
NETIF_F_GSO_GRE_CSUM.

 net/ipv4/fou.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/net/ipv4/fou.c b/net/ipv4/fou.c
index 4136da9275b2..2c30256ee959 100644
--- a/net/ipv4/fou.c
+++ b/net/ipv4/fou.c
@@ -195,6 +195,22 @@ static struct sk_buff **fou_gro_receive(struct sk_buff **head,
 	u8 proto = NAPI_GRO_CB(skb)->proto;
 	const struct net_offload **offloads;
 
+	switch (proto) {
+	case IPPROTO_IPIP:
+	case IPPROTO_IPV6:
+	case IPPROTO_GRE:
+		/* We can clear the encap_mark for these 3 protocols as
+		 * we are either adding an L4 tunnel header to the outer
+		 * L3 tunnel header, or we are are simply treating the
+		 * GRE tunnel header as though it is a UDP protocol
+		 * specific header such as VXLAN or GENEVE.
+		 */
+		NAPI_GRO_CB(skb)->encap_mark = 0;
+		/* fall-through */
+	default:
+		break;
+	}
+
 	rcu_read_lock();
 	offloads = NAPI_GRO_CB(skb)->is_ipv6 ? inet6_offloads : inet_offloads;
 	ops = rcu_dereference(offloads[proto]);
@@ -359,6 +375,22 @@ static struct sk_buff **gue_gro_receive(struct sk_buff **head,
 		NAPI_GRO_CB(p)->flush |= NAPI_GRO_CB(p)->flush_id;
 	}
 
+	switch (guehdr->proto_ctype) {
+	case IPPROTO_IPIP:
+	case IPPROTO_IPV6:
+	case IPPROTO_GRE:
+		/* We can clear the encap_mark for these 3 protocols as
+		 * we are either adding an L4 tunnel header to the outer
+		 * L3 tunnel header, or we are are simply treating the
+		 * GRE tunnel header as though it is a UDP protocol
+		 * specific header such as VXLAN or GENEVE.
+		 */
+		NAPI_GRO_CB(skb)->encap_mark = 0;
+		/* fall-through */
+	default:
+		break;
+	}
+
 	rcu_read_lock();
 	offloads = NAPI_GRO_CB(skb)->is_ipv6 ? inet6_offloads : inet_offloads;
 	ops = rcu_dereference(offloads[guehdr->proto_ctype]);

Re: [net PATCH] gro: Allow tunnel stacking in the case of FOU/GUE

[Tom Herbert]

On Mon, Mar 28, 2016 at 4:58 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
> This patch should fix the issues seen with a recent fix to prevent
> tunnel-in-tunnel frames from being generated with GRO.  The fix itself is
> correct for now as long as we do not add any devices that support
> NETIF_F_GSO_GRE_CSUM.  When such a device is added it could have the
> potential to mess things up due to the fact that the outer transport header
> points to the outer UDP header and not the GRE header as would be expected.
>
Did you test this?

> Fixes: fac8e0f579695 ("tunnels: Don't apply GRO to multiple layers of encapsulation.")
> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
> ---
>
> This should allow us to keep the fix that Jesse added without breaking the
> 3 cases that Tom called out in terms of FOU/GUE.
>
> Additional work will be needed in net-next as we probably need to make it
> so that offloads work correctly when we get around to supporting
> NETIF_F_GSO_GRE_CSUM.
>
>  net/ipv4/fou.c |   32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/net/ipv4/fou.c b/net/ipv4/fou.c
> index 4136da9275b2..2c30256ee959 100644
> --- a/net/ipv4/fou.c
> +++ b/net/ipv4/fou.c
> @@ -195,6 +195,22 @@ static struct sk_buff **fou_gro_receive(struct sk_buff **head,
>         u8 proto = NAPI_GRO_CB(skb)->proto;
>         const struct net_offload **offloads;
>
> +       switch (proto) {
> +       case IPPROTO_IPIP:
> +       case IPPROTO_IPV6:
> +       case IPPROTO_GRE:
> +               /* We can clear the encap_mark for these 3 protocols as
> +                * we are either adding an L4 tunnel header to the outer
> +                * L3 tunnel header, or we are are simply treating the
> +                * GRE tunnel header as though it is a UDP protocol
> +                * specific header such as VXLAN or GENEVE.
> +                */
> +               NAPI_GRO_CB(skb)->encap_mark = 0;
> +               /* fall-through */
> +       default:
> +               break;
> +       }
> +
>         rcu_read_lock();
>         offloads = NAPI_GRO_CB(skb)->is_ipv6 ? inet6_offloads : inet_offloads;
>         ops = rcu_dereference(offloads[proto]);
> @@ -359,6 +375,22 @@ static struct sk_buff **gue_gro_receive(struct sk_buff **head,
>                 NAPI_GRO_CB(p)->flush |= NAPI_GRO_CB(p)->flush_id;
>         }
>
> +       switch (guehdr->proto_ctype) {
> +       case IPPROTO_IPIP:
> +       case IPPROTO_IPV6:
> +       case IPPROTO_GRE:
> +               /* We can clear the encap_mark for these 3 protocols as
> +                * we are either adding an L4 tunnel header to the outer
> +                * L3 tunnel header, or we are are simply treating the
> +                * GRE tunnel header as though it is a UDP protocol
> +                * specific header such as VXLAN or GENEVE.
> +                */
> +               NAPI_GRO_CB(skb)->encap_mark = 0;
> +               /* fall-through */
> +       default:
> +               break;
> +       }
> +
>         rcu_read_lock();
>         offloads = NAPI_GRO_CB(skb)->is_ipv6 ? inet6_offloads : inet_offloads;
>         ops = rcu_dereference(offloads[guehdr->proto_ctype]);
>

Re: [net PATCH] gro: Allow tunnel stacking in the case of FOU/GUE

[Tom Herbert]

On Mon, Mar 28, 2016 at 4:58 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
> This patch should fix the issues seen with a recent fix to prevent
> tunnel-in-tunnel frames from being generated with GRO.  The fix itself is
> correct for now as long as we do not add any devices that support
> NETIF_F_GSO_GRE_CSUM.  When such a device is added it could have the
> potential to mess things up due to the fact that the outer transport header
> points to the outer UDP header and not the GRE header as would be expected.
>
> Fixes: fac8e0f579695 ("tunnels: Don't apply GRO to multiple layers of encapsulation.")

This could only fix FOU/GUE. It is very possible someone else could
happily be doing some other layered encapsulation and never had a
problem before, so the decision to start enforcing only a single layer
of encapsulation for GRO would still break them. I still think we
should revert the patch, and for next version fixes things to that any
combination/nesting of encapsulation is supported, and if there are
exceptions to that support they need be clearly documented.

> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
> ---
>
> This should allow us to keep the fix that Jesse added without breaking the
> 3 cases that Tom called out in terms of FOU/GUE.
>
> Additional work will be needed in net-next as we probably need to make it
> so that offloads work correctly when we get around to supporting
> NETIF_F_GSO_GRE_CSUM.
>
>  net/ipv4/fou.c |   32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/net/ipv4/fou.c b/net/ipv4/fou.c
> index 4136da9275b2..2c30256ee959 100644
> --- a/net/ipv4/fou.c
> +++ b/net/ipv4/fou.c
> @@ -195,6 +195,22 @@ static struct sk_buff **fou_gro_receive(struct sk_buff **head,
>         u8 proto = NAPI_GRO_CB(skb)->proto;
>         const struct net_offload **offloads;
>
> +       switch (proto) {
> +       case IPPROTO_IPIP:
> +       case IPPROTO_IPV6:
> +       case IPPROTO_GRE:
> +               /* We can clear the encap_mark for these 3 protocols as
> +                * we are either adding an L4 tunnel header to the outer
> +                * L3 tunnel header, or we are are simply treating the
> +                * GRE tunnel header as though it is a UDP protocol
> +                * specific header such as VXLAN or GENEVE.
> +                */
> +               NAPI_GRO_CB(skb)->encap_mark = 0;
> +               /* fall-through */
> +       default:
> +               break;
> +       }
> +
>         rcu_read_lock();
>         offloads = NAPI_GRO_CB(skb)->is_ipv6 ? inet6_offloads : inet_offloads;
>         ops = rcu_dereference(offloads[proto]);
> @@ -359,6 +375,22 @@ static struct sk_buff **gue_gro_receive(struct sk_buff **head,
>                 NAPI_GRO_CB(p)->flush |= NAPI_GRO_CB(p)->flush_id;
>         }
>
> +       switch (guehdr->proto_ctype) {
> +       case IPPROTO_IPIP:
> +       case IPPROTO_IPV6:
> +       case IPPROTO_GRE:
> +               /* We can clear the encap_mark for these 3 protocols as
> +                * we are either adding an L4 tunnel header to the outer
> +                * L3 tunnel header, or we are are simply treating the
> +                * GRE tunnel header as though it is a UDP protocol
> +                * specific header such as VXLAN or GENEVE.
> +                */
> +               NAPI_GRO_CB(skb)->encap_mark = 0;
> +               /* fall-through */
> +       default:
> +               break;
> +       }
> +
>         rcu_read_lock();
>         offloads = NAPI_GRO_CB(skb)->is_ipv6 ? inet6_offloads : inet_offloads;
>         ops = rcu_dereference(offloads[guehdr->proto_ctype]);
>

Re: [net PATCH] gro: Allow tunnel stacking in the case of FOU/GUE

[Jesse Gross]

On Mon, Mar 28, 2016 at 6:24 PM, Tom Herbert <tom@herbertland.com> wrote:
> On Mon, Mar 28, 2016 at 4:58 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
>> This patch should fix the issues seen with a recent fix to prevent
>> tunnel-in-tunnel frames from being generated with GRO.  The fix itself is
>> correct for now as long as we do not add any devices that support
>> NETIF_F_GSO_GRE_CSUM.  When such a device is added it could have the
>> potential to mess things up due to the fact that the outer transport header
>> points to the outer UDP header and not the GRE header as would be expected.
>>
>> Fixes: fac8e0f579695 ("tunnels: Don't apply GRO to multiple layers of encapsulation.")
>
> This could only fix FOU/GUE. It is very possible someone else could
> happily be doing some other layered encapsulation and never had a
> problem before, so the decision to start enforcing only a single layer
> of encapsulation for GRO would still break them. I still think we
> should revert the patch, and for next version fixes things to that any
> combination/nesting of encapsulation is supported, and if there are
> exceptions to that support they need be clearly documented.

It was pointed out to me that prior to my patch, it was also possible
to remotely cause a stack overflow by filling up a packet with tunnel
headers and letting GRO descend through them over and over again.

Tom, I'm sorry that you don't like how I fixed this issue but there
really, truly is a bug here. I gave you a specific example to be clear
but that doesn't mean that is the only case. I am aware that the bug
is not encountered in all situations and that the fix removes an
optimization in some of those but I think that ensuring correct
behavior must come first.

Re: [net PATCH] gro: Allow tunnel stacking in the case of FOU/GUE

[Jesse Gross]

On Mon, Mar 28, 2016 at 4:58 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
> This patch should fix the issues seen with a recent fix to prevent
> tunnel-in-tunnel frames from being generated with GRO.  The fix itself is
> correct for now as long as we do not add any devices that support
> NETIF_F_GSO_GRE_CSUM.  When such a device is added it could have the
> potential to mess things up due to the fact that the outer transport header
> points to the outer UDP header and not the GRE header as would be expected.
>
> Fixes: fac8e0f579695 ("tunnels: Don't apply GRO to multiple layers of encapsulation.")
> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
> ---
>
> This should allow us to keep the fix that Jesse added without breaking the
> 3 cases that Tom called out in terms of FOU/GUE.
>
> Additional work will be needed in net-next as we probably need to make it
> so that offloads work correctly when we get around to supporting
> NETIF_F_GSO_GRE_CSUM.

Thanks, this looks like a reasonable fix to me. I agree that there is
more that can be done in the future to improve things but this should
restore GRO while still avoiding possible issues.

Re: [net PATCH] gro: Allow tunnel stacking in the case of FOU/GUE

[Tom Herbert]

On Mon, Mar 28, 2016 at 6:54 PM, Jesse Gross <jesse@kernel.org> wrote:
> On Mon, Mar 28, 2016 at 6:24 PM, Tom Herbert <tom@herbertland.com> wrote:
>> On Mon, Mar 28, 2016 at 4:58 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
>>> This patch should fix the issues seen with a recent fix to prevent
>>> tunnel-in-tunnel frames from being generated with GRO.  The fix itself is
>>> correct for now as long as we do not add any devices that support
>>> NETIF_F_GSO_GRE_CSUM.  When such a device is added it could have the
>>> potential to mess things up due to the fact that the outer transport header
>>> points to the outer UDP header and not the GRE header as would be expected.
>>>
>>> Fixes: fac8e0f579695 ("tunnels: Don't apply GRO to multiple layers of encapsulation.")
>>
>> This could only fix FOU/GUE. It is very possible someone else could
>> happily be doing some other layered encapsulation and never had a
>> problem before, so the decision to start enforcing only a single layer
>> of encapsulation for GRO would still break them. I still think we
>> should revert the patch, and for next version fixes things to that any
>> combination/nesting of encapsulation is supported, and if there are
>> exceptions to that support they need be clearly documented.
>
> It was pointed out to me that prior to my patch, it was also possible
> to remotely cause a stack overflow by filling up a packet with tunnel
> headers and letting GRO descend through them over and over again.
>
Then the fix would be set set a reasonable limit on the number of
encapsulation levels.

> Tom, I'm sorry that you don't like how I fixed this issue but there
> really, truly is a bug here. I gave you a specific example to be clear
> but that doesn't mean that is the only case. I am aware that the bug
> is not encountered in all situations and that the fix removes an
> optimization in some of those but I think that ensuring correct
> behavior must come first.

The example you gave results in packet loss, this is not
incorrectness. Actually reproduce a real issue that leads to
incorrectness and then we can talk about a solution.

Tom

Re: [net PATCH] gro: Allow tunnel stacking in the case of FOU/GUE

[Alexander Duyck]

On Mon, Mar 28, 2016 at 8:17 PM, Tom Herbert <tom@herbertland.com> wrote:
> On Mon, Mar 28, 2016 at 6:54 PM, Jesse Gross <jesse@kernel.org> wrote:
>> On Mon, Mar 28, 2016 at 6:24 PM, Tom Herbert <tom@herbertland.com> wrote:
>>> On Mon, Mar 28, 2016 at 4:58 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
>>>> This patch should fix the issues seen with a recent fix to prevent
>>>> tunnel-in-tunnel frames from being generated with GRO.  The fix itself is
>>>> correct for now as long as we do not add any devices that support
>>>> NETIF_F_GSO_GRE_CSUM.  When such a device is added it could have the
>>>> potential to mess things up due to the fact that the outer transport header
>>>> points to the outer UDP header and not the GRE header as would be expected.
>>>>
>>>> Fixes: fac8e0f579695 ("tunnels: Don't apply GRO to multiple layers of encapsulation.")
>>>
>>> This could only fix FOU/GUE. It is very possible someone else could
>>> happily be doing some other layered encapsulation and never had a
>>> problem before, so the decision to start enforcing only a single layer
>>> of encapsulation for GRO would still break them. I still think we
>>> should revert the patch, and for next version fixes things to that any
>>> combination/nesting of encapsulation is supported, and if there are
>>> exceptions to that support they need be clearly documented.
>>
>> It was pointed out to me that prior to my patch, it was also possible
>> to remotely cause a stack overflow by filling up a packet with tunnel
>> headers and letting GRO descend through them over and over again.
>>
> Then the fix would be set set a reasonable limit on the number of
> encapsulation levels.
>
>> Tom, I'm sorry that you don't like how I fixed this issue but there
>> really, truly is a bug here. I gave you a specific example to be clear
>> but that doesn't mean that is the only case. I am aware that the bug
>> is not encountered in all situations and that the fix removes an
>> optimization in some of those but I think that ensuring correct
>> behavior must come first.
>
> The example you gave results in packet loss, this is not
> incorrectness. Actually reproduce a real issue that leads to
> incorrectness and then we can talk about a solution.

Tom,

Just take a look in the __skb_udp_tunnel_segment or gre_gso_segment
code.  Then tell me how we are supposed to deal with the fact that the
GSO code expects skb_inner_network_offset() to be valid.  If you have
more than an inner and an outer network header we cannot.  So we
cannot put GRE in UDP, or UDP in GRE if there is a network header
between them.  The FOU/GUE code gets around this because in the IPIP
and SIT cases you are adding an L4 header between two L3 headers.  The
GRE case works because you essentially convert the GRE header into a
tunnel header like VXLAN or GENEVE and we just overwrite the outer
transport header offset.

What it comes down to is that we can only support 2 network headers
per frame.  One for the inner and one for the outer.  That is why we
can have an exception for GUE as it only has 2 network headers.  If we
had multiple levels of UDP, or GRE, or 2 levels of network headers
either before or after either UDP or GRE we cannot support
segmentation because the code will blow up and generate a malformed
frame.

- Alex

Re: [net PATCH] gro: Allow tunnel stacking in the case of FOU/GUE

[Tom Herbert]

On Mon, Mar 28, 2016 at 8:27 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Mon, Mar 28, 2016 at 8:17 PM, Tom Herbert <tom@herbertland.com> wrote:
>> On Mon, Mar 28, 2016 at 6:54 PM, Jesse Gross <jesse@kernel.org> wrote:
>>> On Mon, Mar 28, 2016 at 6:24 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>> On Mon, Mar 28, 2016 at 4:58 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
>>>>> This patch should fix the issues seen with a recent fix to prevent
>>>>> tunnel-in-tunnel frames from being generated with GRO.  The fix itself is
>>>>> correct for now as long as we do not add any devices that support
>>>>> NETIF_F_GSO_GRE_CSUM.  When such a device is added it could have the
>>>>> potential to mess things up due to the fact that the outer transport header
>>>>> points to the outer UDP header and not the GRE header as would be expected.
>>>>>
>>>>> Fixes: fac8e0f579695 ("tunnels: Don't apply GRO to multiple layers of encapsulation.")
>>>>
>>>> This could only fix FOU/GUE. It is very possible someone else could
>>>> happily be doing some other layered encapsulation and never had a
>>>> problem before, so the decision to start enforcing only a single layer
>>>> of encapsulation for GRO would still break them. I still think we
>>>> should revert the patch, and for next version fixes things to that any
>>>> combination/nesting of encapsulation is supported, and if there are
>>>> exceptions to that support they need be clearly documented.
>>>
>>> It was pointed out to me that prior to my patch, it was also possible
>>> to remotely cause a stack overflow by filling up a packet with tunnel
>>> headers and letting GRO descend through them over and over again.
>>>
>> Then the fix would be set set a reasonable limit on the number of
>> encapsulation levels.
>>
>>> Tom, I'm sorry that you don't like how I fixed this issue but there
>>> really, truly is a bug here. I gave you a specific example to be clear
>>> but that doesn't mean that is the only case. I am aware that the bug
>>> is not encountered in all situations and that the fix removes an
>>> optimization in some of those but I think that ensuring correct
>>> behavior must come first.
>>
>> The example you gave results in packet loss, this is not
>> incorrectness. Actually reproduce a real issue that leads to
>> incorrectness and then we can talk about a solution.
>
> Tom,
>
> Just take a look in the __skb_udp_tunnel_segment or gre_gso_segment
> code.  Then tell me how we are supposed to deal with the fact that the
> GSO code expects skb_inner_network_offset() to be valid.  If you have
> more than an inner and an outer network header we cannot.  So we
> cannot put GRE in UDP, or UDP in GRE if there is a network header
> between them.  The FOU/GUE code gets around this because in the IPIP
> and SIT cases you are adding an L4 header between two L3 headers.  The
> GRE case works because you essentially convert the GRE header into a
> tunnel header like VXLAN or GENEVE and we just overwrite the outer
> transport header offset.
>
> What it comes down to is that we can only support 2 network headers
> per frame.  One for the inner and one for the outer.  That is why we
> can have an exception for GUE as it only has 2 network headers.  If we
> had multiple levels of UDP, or GRE, or 2 levels of network headers
> either before or after either UDP or GRE we cannot support
> segmentation because the code will blow up and generate a malformed
> frame.
>
If you apply Edward's jumbo L2 header concept then
Eth|IP|UDP|VXLAN|Eth|IP|UDP|GUE|GRE|IPIP|IPv6|TCP|payload becomes
Eth|IP|UDP|encapsulation-hdrs|IPv6|TCP|Payload. One set of outer
headers, one set of inner headers. The rules that encapsulation_hdrs
don't contain fields that need to be modified for every segment need
to be supported in GRO and the stack when it generates such a
configuration.

> - Alex

Re: [net PATCH] gro: Allow tunnel stacking in the case of FOU/GUE

[Alex Duyck]

On Mon, Mar 28, 2016 at 9:01 PM, Tom Herbert <tom@herbertland.com> wrote:
> On Mon, Mar 28, 2016 at 8:27 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Mon, Mar 28, 2016 at 8:17 PM, Tom Herbert <tom@herbertland.com> wrote:
>>> On Mon, Mar 28, 2016 at 6:54 PM, Jesse Gross <jesse@kernel.org> wrote:
>>>> On Mon, Mar 28, 2016 at 6:24 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>>> On Mon, Mar 28, 2016 at 4:58 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
>>>>>> This patch should fix the issues seen with a recent fix to prevent
>>>>>> tunnel-in-tunnel frames from being generated with GRO.  The fix itself is
>>>>>> correct for now as long as we do not add any devices that support
>>>>>> NETIF_F_GSO_GRE_CSUM.  When such a device is added it could have the
>>>>>> potential to mess things up due to the fact that the outer transport header
>>>>>> points to the outer UDP header and not the GRE header as would be expected.
>>>>>>
>>>>>> Fixes: fac8e0f579695 ("tunnels: Don't apply GRO to multiple layers of encapsulation.")
>>>>>
>>>>> This could only fix FOU/GUE. It is very possible someone else could
>>>>> happily be doing some other layered encapsulation and never had a
>>>>> problem before, so the decision to start enforcing only a single layer
>>>>> of encapsulation for GRO would still break them. I still think we
>>>>> should revert the patch, and for next version fixes things to that any
>>>>> combination/nesting of encapsulation is supported, and if there are
>>>>> exceptions to that support they need be clearly documented.
>>>>
>>>> It was pointed out to me that prior to my patch, it was also possible
>>>> to remotely cause a stack overflow by filling up a packet with tunnel
>>>> headers and letting GRO descend through them over and over again.
>>>>
>>> Then the fix would be set set a reasonable limit on the number of
>>> encapsulation levels.
>>>
>>>> Tom, I'm sorry that you don't like how I fixed this issue but there
>>>> really, truly is a bug here. I gave you a specific example to be clear
>>>> but that doesn't mean that is the only case. I am aware that the bug
>>>> is not encountered in all situations and that the fix removes an
>>>> optimization in some of those but I think that ensuring correct
>>>> behavior must come first.
>>>
>>> The example you gave results in packet loss, this is not
>>> incorrectness. Actually reproduce a real issue that leads to
>>> incorrectness and then we can talk about a solution.
>>
>> Tom,
>>
>> Just take a look in the __skb_udp_tunnel_segment or gre_gso_segment
>> code.  Then tell me how we are supposed to deal with the fact that the
>> GSO code expects skb_inner_network_offset() to be valid.  If you have
>> more than an inner and an outer network header we cannot.  So we
>> cannot put GRE in UDP, or UDP in GRE if there is a network header
>> between them.  The FOU/GUE code gets around this because in the IPIP
>> and SIT cases you are adding an L4 header between two L3 headers.  The
>> GRE case works because you essentially convert the GRE header into a
>> tunnel header like VXLAN or GENEVE and we just overwrite the outer
>> transport header offset.
>>
>> What it comes down to is that we can only support 2 network headers
>> per frame.  One for the inner and one for the outer.  That is why we
>> can have an exception for GUE as it only has 2 network headers.  If we
>> had multiple levels of UDP, or GRE, or 2 levels of network headers
>> either before or after either UDP or GRE we cannot support
>> segmentation because the code will blow up and generate a malformed
>> frame.
>>
> If you apply Edward's jumbo L2 header concept then
> Eth|IP|UDP|VXLAN|Eth|IP|UDP|GUE|GRE|IPIP|IPv6|TCP|payload becomes
> Eth|IP|UDP|encapsulation-hdrs|IPv6|TCP|Payload. One set of outer
> headers, one set of inner headers. The rules that encapsulation_hdrs
> don't contain fields that need to be modified for every segment need
> to be supported in GRO and the stack when it generates such a
> configuration.

Thats all well and good but nothing like that exists now.  So you
cannot expect us to fix the kernel to support code that isn't there.
In addition there were a number of issues with the jumbo L2 header
approach.  That was one of the reasons why I went with the jumbo L3
header approach as it is much easier to be compliant with all the
RFCs.

We might be able to get some of that supported for net-next but things
are going to be limited.  We need to have the UDP tunnels actually
setting the DF bit which as far as I know none of them do now.  In
addition we will have to add rules for all the encapsulated types so
that we can enforce the outer IP header incrementing in the event that
DF is not set.  Then we will also have to go through and make certain
that we have the DF bit set in all headers between the transport and
the outer network header in order to allow support for GSO partial.

What you are describing is no small task.  There are bugs that need to
be fixed now in net.  We can try to get the features you want pushed
for net-next but they don't exist now so locking down GRO so that it
matches the feature set provided by GSO is not a regression.

- Alex

Re: [net PATCH] gro: Allow tunnel stacking in the case of FOU/GUE

[Tom Herbert]

On Mon, Mar 28, 2016 at 9:15 PM, Alex Duyck <aduyck@mirantis.com> wrote:
> On Mon, Mar 28, 2016 at 9:01 PM, Tom Herbert <tom@herbertland.com> wrote:
>> On Mon, Mar 28, 2016 at 8:27 PM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>> On Mon, Mar 28, 2016 at 8:17 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>> On Mon, Mar 28, 2016 at 6:54 PM, Jesse Gross <jesse@kernel.org> wrote:
>>>>> On Mon, Mar 28, 2016 at 6:24 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>>>> On Mon, Mar 28, 2016 at 4:58 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
>>>>>>> This patch should fix the issues seen with a recent fix to prevent
>>>>>>> tunnel-in-tunnel frames from being generated with GRO.  The fix itself is
>>>>>>> correct for now as long as we do not add any devices that support
>>>>>>> NETIF_F_GSO_GRE_CSUM.  When such a device is added it could have the
>>>>>>> potential to mess things up due to the fact that the outer transport header
>>>>>>> points to the outer UDP header and not the GRE header as would be expected.
>>>>>>>
>>>>>>> Fixes: fac8e0f579695 ("tunnels: Don't apply GRO to multiple layers of encapsulation.")
>>>>>>
>>>>>> This could only fix FOU/GUE. It is very possible someone else could
>>>>>> happily be doing some other layered encapsulation and never had a
>>>>>> problem before, so the decision to start enforcing only a single layer
>>>>>> of encapsulation for GRO would still break them. I still think we
>>>>>> should revert the patch, and for next version fixes things to that any
>>>>>> combination/nesting of encapsulation is supported, and if there are
>>>>>> exceptions to that support they need be clearly documented.
>>>>>
>>>>> It was pointed out to me that prior to my patch, it was also possible
>>>>> to remotely cause a stack overflow by filling up a packet with tunnel
>>>>> headers and letting GRO descend through them over and over again.
>>>>>
>>>> Then the fix would be set set a reasonable limit on the number of
>>>> encapsulation levels.
>>>>
>>>>> Tom, I'm sorry that you don't like how I fixed this issue but there
>>>>> really, truly is a bug here. I gave you a specific example to be clear
>>>>> but that doesn't mean that is the only case. I am aware that the bug
>>>>> is not encountered in all situations and that the fix removes an
>>>>> optimization in some of those but I think that ensuring correct
>>>>> behavior must come first.
>>>>
>>>> The example you gave results in packet loss, this is not
>>>> incorrectness. Actually reproduce a real issue that leads to
>>>> incorrectness and then we can talk about a solution.
>>>
>>> Tom,
>>>
>>> Just take a look in the __skb_udp_tunnel_segment or gre_gso_segment
>>> code.  Then tell me how we are supposed to deal with the fact that the
>>> GSO code expects skb_inner_network_offset() to be valid.  If you have
>>> more than an inner and an outer network header we cannot.  So we
>>> cannot put GRE in UDP, or UDP in GRE if there is a network header
>>> between them.  The FOU/GUE code gets around this because in the IPIP
>>> and SIT cases you are adding an L4 header between two L3 headers.  The
>>> GRE case works because you essentially convert the GRE header into a
>>> tunnel header like VXLAN or GENEVE and we just overwrite the outer
>>> transport header offset.
>>>
>>> What it comes down to is that we can only support 2 network headers
>>> per frame.  One for the inner and one for the outer.  That is why we
>>> can have an exception for GUE as it only has 2 network headers.  If we
>>> had multiple levels of UDP, or GRE, or 2 levels of network headers
>>> either before or after either UDP or GRE we cannot support
>>> segmentation because the code will blow up and generate a malformed
>>> frame.
>>>
>> If you apply Edward's jumbo L2 header concept then
>> Eth|IP|UDP|VXLAN|Eth|IP|UDP|GUE|GRE|IPIP|IPv6|TCP|payload becomes
>> Eth|IP|UDP|encapsulation-hdrs|IPv6|TCP|Payload. One set of outer
>> headers, one set of inner headers. The rules that encapsulation_hdrs
>> don't contain fields that need to be modified for every segment need
>> to be supported in GRO and the stack when it generates such a
>> configuration.
>
> Thats all well and good but nothing like that exists now.  So you
> cannot expect us to fix the kernel to support code that isn't there.

No, but I do expect that you support code that is already there. There
was apparently zero testing done on the original patch and it caused
one very obvious regression. So how can we have any confidence
whatsoever that this patch doesn't break other things? Furthermore,
with all these claims of bugs I still don't see that _anyone_ has
taken the time to reproduce any issue and show that this patch
materially fixes any thing. I seriously don't understand how basic
testing could be such a challenge.

Anyway, what I expect is moot. It's up to davem to decide what to do
with this...

Tom

> In addition there were a number of issues with the jumbo L2 header
> approach.  That was one of the reasons why I went with the jumbo L3
> header approach as it is much easier to be compliant with all the
> RFCs.
>
> We might be able to get some of that supported for net-next but things
> are going to be limited.  We need to have the UDP tunnels actually
> setting the DF bit which as far as I know none of them do now.  In
> addition we will have to add rules for all the encapsulated types so
> that we can enforce the outer IP header incrementing in the event that
> DF is not set.  Then we will also have to go through and make certain
> that we have the DF bit set in all headers between the transport and
> the outer network header in order to allow support for GSO partial.
>
> What you are describing is no small task.  There are bugs that need to
> be fixed now in net.  We can try to get the features you want pushed
> for net-next but they don't exist now so locking down GRO so that it
> matches the feature set provided by GSO is not a regression.
>
> - Alex

Re: [net PATCH] gro: Allow tunnel stacking in the case of FOU/GUE

[David Miller]

From: Tom Herbert <tom@herbertland.com>
Date: Mon, 28 Mar 2016 21:51:17 -0700

> No, but I do expect that you support code that is already there. There
> was apparently zero testing done on the original patch and it caused
> one very obvious regression. So how can we have any confidence
> whatsoever that this patch doesn't break other things? Furthermore,
> with all these claims of bugs I still don't see that _anyone_ has
> taken the time to reproduce any issue and show that this patch
> materially fixes any thing. I seriously don't understand how basic
> testing could be such a challenge.
> 
> Anyway, what I expect is moot. It's up to davem to decide what to do
> with this...

You being upset with a lack of testing is one issue, and is
legitimate.

But the fact that we can't support, and never could support, more than
one network header at a time except in a very special case for GRO
is very real.  And you must acknowledge that this was a very shaky
foundation upon which to erect the kinds of things you expect to work.

Re: [net PATCH] gro: Allow tunnel stacking in the case of FOU/GUE

[Tom Herbert]

On Mon, Mar 28, 2016 at 4:58 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
> This patch should fix the issues seen with a recent fix to prevent
> tunnel-in-tunnel frames from being generated with GRO.  The fix itself is
> correct for now as long as we do not add any devices that support
> NETIF_F_GSO_GRE_CSUM.  When such a device is added it could have the
> potential to mess things up due to the fact that the outer transport header
> points to the outer UDP header and not the GRE header as would be expected.
>
> Fixes: fac8e0f579695 ("tunnels: Don't apply GRO to multiple layers of encapsulation.")
> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
> ---
>
> This should allow us to keep the fix that Jesse added without breaking the
> 3 cases that Tom called out in terms of FOU/GUE.
>
> Additional work will be needed in net-next as we probably need to make it
> so that offloads work correctly when we get around to supporting
> NETIF_F_GSO_GRE_CSUM.
>
>  net/ipv4/fou.c |   32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/net/ipv4/fou.c b/net/ipv4/fou.c
> index 4136da9275b2..2c30256ee959 100644
> --- a/net/ipv4/fou.c
> +++ b/net/ipv4/fou.c
> @@ -195,6 +195,22 @@ static struct sk_buff **fou_gro_receive(struct sk_buff **head,
>         u8 proto = NAPI_GRO_CB(skb)->proto;
>         const struct net_offload **offloads;
>
> +       switch (proto) {
> +       case IPPROTO_IPIP:
> +       case IPPROTO_IPV6:
> +       case IPPROTO_GRE:
> +               /* We can clear the encap_mark for these 3 protocols as
> +                * we are either adding an L4 tunnel header to the outer
> +                * L3 tunnel header, or we are are simply treating the
> +                * GRE tunnel header as though it is a UDP protocol
> +                * specific header such as VXLAN or GENEVE.
> +                */
> +               NAPI_GRO_CB(skb)->encap_mark = 0;
> +               /* fall-through */
> +       default:
> +               break;
> +       }

Switch statement is not needed, just do NAPI_GRO_CB(skb)->encap_mark = 0;

> +
>         rcu_read_lock();
>         offloads = NAPI_GRO_CB(skb)->is_ipv6 ? inet6_offloads : inet_offloads;
>         ops = rcu_dereference(offloads[proto]);
> @@ -359,6 +375,22 @@ static struct sk_buff **gue_gro_receive(struct sk_buff **head,
>                 NAPI_GRO_CB(p)->flush |= NAPI_GRO_CB(p)->flush_id;
>         }
>
> +       switch (guehdr->proto_ctype) {
> +       case IPPROTO_IPIP:
> +       case IPPROTO_IPV6:
> +       case IPPROTO_GRE:
> +               /* We can clear the encap_mark for these 3 protocols as
> +                * we are either adding an L4 tunnel header to the outer
> +                * L3 tunnel header, or we are are simply treating the
> +                * GRE tunnel header as though it is a UDP protocol
> +                * specific header such as VXLAN or GENEVE.
> +                */
> +               NAPI_GRO_CB(skb)->encap_mark = 0;
> +               /* fall-through */
> +       default:
> +               break;
> +       }
> +
Here also.

>         rcu_read_lock();
>         offloads = NAPI_GRO_CB(skb)->is_ipv6 ? inet6_offloads : inet_offloads;
>         ops = rcu_dereference(offloads[guehdr->proto_ctype]);
>

Re: [net PATCH] gro: Allow tunnel stacking in the case of FOU/GUE

[Alexander Duyck]

On Tue, Mar 29, 2016 at 2:13 PM, Tom Herbert <tom@herbertland.com> wrote:
> On Mon, Mar 28, 2016 at 4:58 PM, Alexander Duyck <aduyck@mirantis.com> wrote:
>> This patch should fix the issues seen with a recent fix to prevent
>> tunnel-in-tunnel frames from being generated with GRO.  The fix itself is
>> correct for now as long as we do not add any devices that support
>> NETIF_F_GSO_GRE_CSUM.  When such a device is added it could have the
>> potential to mess things up due to the fact that the outer transport header
>> points to the outer UDP header and not the GRE header as would be expected.
>>
>> Fixes: fac8e0f579695 ("tunnels: Don't apply GRO to multiple layers of encapsulation.")
>> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
>> ---
>>
>> This should allow us to keep the fix that Jesse added without breaking the
>> 3 cases that Tom called out in terms of FOU/GUE.
>>
>> Additional work will be needed in net-next as we probably need to make it
>> so that offloads work correctly when we get around to supporting
>> NETIF_F_GSO_GRE_CSUM.
>>
>>  net/ipv4/fou.c |   32 ++++++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>
>> diff --git a/net/ipv4/fou.c b/net/ipv4/fou.c
>> index 4136da9275b2..2c30256ee959 100644
>> --- a/net/ipv4/fou.c
>> +++ b/net/ipv4/fou.c
>> @@ -195,6 +195,22 @@ static struct sk_buff **fou_gro_receive(struct sk_buff **head,
>>         u8 proto = NAPI_GRO_CB(skb)->proto;
>>         const struct net_offload **offloads;
>>
>> +       switch (proto) {
>> +       case IPPROTO_IPIP:
>> +       case IPPROTO_IPV6:
>> +       case IPPROTO_GRE:
>> +               /* We can clear the encap_mark for these 3 protocols as
>> +                * we are either adding an L4 tunnel header to the outer
>> +                * L3 tunnel header, or we are are simply treating the
>> +                * GRE tunnel header as though it is a UDP protocol
>> +                * specific header such as VXLAN or GENEVE.
>> +                */
>> +               NAPI_GRO_CB(skb)->encap_mark = 0;
>> +               /* fall-through */
>> +       default:
>> +               break;
>> +       }
>
> Switch statement is not needed, just do NAPI_GRO_CB(skb)->encap_mark = 0;
>
>> +
>>         rcu_read_lock();
>>         offloads = NAPI_GRO_CB(skb)->is_ipv6 ? inet6_offloads : inet_offloads;
>>         ops = rcu_dereference(offloads[proto]);
>> @@ -359,6 +375,22 @@ static struct sk_buff **gue_gro_receive(struct sk_buff **head,
>>                 NAPI_GRO_CB(p)->flush |= NAPI_GRO_CB(p)->flush_id;
>>         }
>>
>> +       switch (guehdr->proto_ctype) {
>> +       case IPPROTO_IPIP:
>> +       case IPPROTO_IPV6:
>> +       case IPPROTO_GRE:
>> +               /* We can clear the encap_mark for these 3 protocols as
>> +                * we are either adding an L4 tunnel header to the outer
>> +                * L3 tunnel header, or we are are simply treating the
>> +                * GRE tunnel header as though it is a UDP protocol
>> +                * specific header such as VXLAN or GENEVE.
>> +                */
>> +               NAPI_GRO_CB(skb)->encap_mark = 0;
>> +               /* fall-through */
>> +       default:
>> +               break;
>> +       }
>> +
> Here also.
>
>>         rcu_read_lock();
>>         offloads = NAPI_GRO_CB(skb)->is_ipv6 ? inet6_offloads : inet_offloads;
>>         ops = rcu_dereference(offloads[guehdr->proto_ctype]);
>>

Okay, I can update that and submit a v2.  The only real reason why I
had the switch statements was out of an abundance of caution since
those were the only 3 cases where I knew we would run into issues.

- Alex