On Mon, Mar 28, 2016 at 8:27 PM, Alexander Duyck <alexander.du...@gmail.com> wrote: > On Mon, Mar 28, 2016 at 8:17 PM, Tom Herbert <t...@herbertland.com> wrote: >> On Mon, Mar 28, 2016 at 6:54 PM, Jesse Gross <je...@kernel.org> wrote: >>> On Mon, Mar 28, 2016 at 6:24 PM, Tom Herbert <t...@herbertland.com> wrote: >>>> On Mon, Mar 28, 2016 at 4:58 PM, Alexander Duyck <adu...@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