| Message ID | 1467274002-61390-19-git-send-email-pshelar@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Jun 30, 2016 at 1:06 AM, Pravin B Shelar <pshelar@ovn.org> wrote: > Upstream commit: > commit 229740c63169462a838a8b8e16391ed000934631 > Author: Jarno Rajahalme <jarno@ovn.org> > > udp_offload: Set encapsulation before inner completes. > > UDP tunnel segmentation code relies on the inner offsets being set for > an UDP tunnel GSO packet, but the inner *_complete() functions will > set the inner offsets only if 'encapsulation' is set before calling > them. Currently, udp_gro_complete() sets 'encapsulation' only after > the inner *_complete() functions are done. This causes the inner > offsets having invalid values after udp_gro_complete() returns, which > in turn will make it impossible to properly segment the packet in case > it needs to be forwarded, which would be visible to the user either as > invalid packets being sent or as packet loss. > > This patch fixes this by setting skb's 'encapsulation' in > udp_gro_complete() before calling into the inner complete functions, > and by making each possible UDP tunnel gro_complete() callback set the > inner_mac_header to the beginning of the tunnel payload. > > Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > Reviewed-by: Alexander Duyck <aduyck@mirantis.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > > Signed-off-by: Pravin B Shelar <pshelar@ovn.org> > --- > datapath/linux/compat/geneve.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/datapath/linux/compat/geneve.c b/datapath/linux/compat/geneve.c > index e049221..cc8740b 100644 > --- a/datapath/linux/compat/geneve.c > +++ b/datapath/linux/compat/geneve.c > @@ -566,6 +566,8 @@ static int geneve_gro_complete(struct sk_buff *skb, int nhoff, > err = ptype->callbacks.gro_complete(skb, nhoff + gh_len); > > rcu_read_unlock(); > + > + skb_set_inner_mac_header(skb, nhoff + gh_len); > return err; > } > #endif This commit also adds a comment to vxlan.c, should we also backport that for completeness? Acked-by: Jesse Gross <jesse@kernel.org>
I'm not really familiar with these backports, but is it so that the main change the upstream patch introduced to udp_gro_complete() is not applicable to the backports? Jarno > On Jun 30, 2016, at 1:06 AM, Pravin B Shelar <pshelar@ovn.org> wrote: > > Upstream commit: > commit 229740c63169462a838a8b8e16391ed000934631 > Author: Jarno Rajahalme <jarno@ovn.org> > > udp_offload: Set encapsulation before inner completes. > > UDP tunnel segmentation code relies on the inner offsets being set for > an UDP tunnel GSO packet, but the inner *_complete() functions will > set the inner offsets only if 'encapsulation' is set before calling > them. Currently, udp_gro_complete() sets 'encapsulation' only after > the inner *_complete() functions are done. This causes the inner > offsets having invalid values after udp_gro_complete() returns, which > in turn will make it impossible to properly segment the packet in case > it needs to be forwarded, which would be visible to the user either as > invalid packets being sent or as packet loss. > > This patch fixes this by setting skb's 'encapsulation' in > udp_gro_complete() before calling into the inner complete functions, > and by making each possible UDP tunnel gro_complete() callback set the > inner_mac_header to the beginning of the tunnel payload. > > Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > Reviewed-by: Alexander Duyck <aduyck@mirantis.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > > Signed-off-by: Pravin B Shelar <pshelar@ovn.org> > --- > datapath/linux/compat/geneve.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/datapath/linux/compat/geneve.c b/datapath/linux/compat/geneve.c > index e049221..cc8740b 100644 > --- a/datapath/linux/compat/geneve.c > +++ b/datapath/linux/compat/geneve.c > @@ -566,6 +566,8 @@ static int geneve_gro_complete(struct sk_buff *skb, int nhoff, > err = ptype->callbacks.gro_complete(skb, nhoff + gh_len); > > rcu_read_unlock(); > + > + skb_set_inner_mac_header(skb, nhoff + gh_len); > return err; > } > #endif > -- > 1.9.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
I guess it's not applicable in the sense that the fix is not really in OVS. Kernels with that bug will continue to have it even after this backport. I think it would be pretty challenging to fully backport this through the OVS tree - it would likely require pulling in the whole UDP offloads module. That would pretty messy and would silently prevent other non-OVS users from taking advantage of offloads. To the extent that this affects OVS users, I think probably needs to be picked up by distribution kernels. On Mon, Jul 4, 2016 at 4:02 AM, Jarno Rajahalme <jarno@ovn.org> wrote: > I'm not really familiar with these backports, but is it so that the main change the upstream patch introduced to udp_gro_complete() is not applicable to the backports? > > Jarno > >> On Jun 30, 2016, at 1:06 AM, Pravin B Shelar <pshelar@ovn.org> wrote: >> >> Upstream commit: >> commit 229740c63169462a838a8b8e16391ed000934631 >> Author: Jarno Rajahalme <jarno@ovn.org> >> >> udp_offload: Set encapsulation before inner completes. >> >> UDP tunnel segmentation code relies on the inner offsets being set for >> an UDP tunnel GSO packet, but the inner *_complete() functions will >> set the inner offsets only if 'encapsulation' is set before calling >> them. Currently, udp_gro_complete() sets 'encapsulation' only after >> the inner *_complete() functions are done. This causes the inner >> offsets having invalid values after udp_gro_complete() returns, which >> in turn will make it impossible to properly segment the packet in case >> it needs to be forwarded, which would be visible to the user either as >> invalid packets being sent or as packet loss. >> >> This patch fixes this by setting skb's 'encapsulation' in >> udp_gro_complete() before calling into the inner complete functions, >> and by making each possible UDP tunnel gro_complete() callback set the >> inner_mac_header to the beginning of the tunnel payload. >> >> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> >> Reviewed-by: Alexander Duyck <aduyck@mirantis.com> >> Signed-off-by: David S. Miller <davem@davemloft.net> >> >> Signed-off-by: Pravin B Shelar <pshelar@ovn.org> >> --- >> datapath/linux/compat/geneve.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/datapath/linux/compat/geneve.c b/datapath/linux/compat/geneve.c >> index e049221..cc8740b 100644 >> --- a/datapath/linux/compat/geneve.c >> +++ b/datapath/linux/compat/geneve.c >> @@ -566,6 +566,8 @@ static int geneve_gro_complete(struct sk_buff *skb, int nhoff, >> err = ptype->callbacks.gro_complete(skb, nhoff + gh_len); >> >> rcu_read_unlock(); >> + >> + skb_set_inner_mac_header(skb, nhoff + gh_len); >> return err; >> } >> #endif >> -- >> 1.9.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On Tue, Jul 5, 2016 at 12:07 PM, Jesse Gross <jesse@kernel.org> wrote: > I guess it's not applicable in the sense that the fix is not really in > OVS. Kernels with that bug will continue to have it even after this > backport. > > I think it would be pretty challenging to fully backport this through > the OVS tree - it would likely require pulling in the whole UDP > offloads module. That would pretty messy and would silently prevent > other non-OVS users from taking advantage of offloads. To the extent > that this affects OVS users, I think probably needs to be picked up by > distribution kernels. > Loadable module can not change kernel udp_gro_complete(), so I can not backport it into OVS. Anyways setting inner mac header is still improvement, if distributions back port whole commit. > On Mon, Jul 4, 2016 at 4:02 AM, Jarno Rajahalme <jarno@ovn.org> wrote: >> I'm not really familiar with these backports, but is it so that the main change the upstream patch introduced to udp_gro_complete() is not applicable to the backports? >> >> Jarno >> >>> On Jun 30, 2016, at 1:06 AM, Pravin B Shelar <pshelar@ovn.org> wrote: >>> >>> Upstream commit: >>> commit 229740c63169462a838a8b8e16391ed000934631 >>> Author: Jarno Rajahalme <jarno@ovn.org> >>> >>> udp_offload: Set encapsulation before inner completes. >>> >>> UDP tunnel segmentation code relies on the inner offsets being set for >>> an UDP tunnel GSO packet, but the inner *_complete() functions will >>> set the inner offsets only if 'encapsulation' is set before calling >>> them. Currently, udp_gro_complete() sets 'encapsulation' only after >>> the inner *_complete() functions are done. This causes the inner >>> offsets having invalid values after udp_gro_complete() returns, which >>> in turn will make it impossible to properly segment the packet in case >>> it needs to be forwarded, which would be visible to the user either as >>> invalid packets being sent or as packet loss. >>> >>> This patch fixes this by setting skb's 'encapsulation' in >>> udp_gro_complete() before calling into the inner complete functions, >>> and by making each possible UDP tunnel gro_complete() callback set the >>> inner_mac_header to the beginning of the tunnel payload. >>> >>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> >>> Reviewed-by: Alexander Duyck <aduyck@mirantis.com> >>> Signed-off-by: David S. Miller <davem@davemloft.net> >>> >>> Signed-off-by: Pravin B Shelar <pshelar@ovn.org> >>> --- >>> datapath/linux/compat/geneve.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/datapath/linux/compat/geneve.c b/datapath/linux/compat/geneve.c >>> index e049221..cc8740b 100644 >>> --- a/datapath/linux/compat/geneve.c >>> +++ b/datapath/linux/compat/geneve.c >>> @@ -566,6 +566,8 @@ static int geneve_gro_complete(struct sk_buff *skb, int nhoff, >>> err = ptype->callbacks.gro_complete(skb, nhoff + gh_len); >>> >>> rcu_read_unlock(); >>> + >>> + skb_set_inner_mac_header(skb, nhoff + gh_len); >>> return err; >>> } >>> #endif >>> -- >>> 1.9.1 >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> http://openvswitch.org/mailman/listinfo/dev >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev
diff --git a/datapath/linux/compat/geneve.c b/datapath/linux/compat/geneve.c index e049221..cc8740b 100644 --- a/datapath/linux/compat/geneve.c +++ b/datapath/linux/compat/geneve.c @@ -566,6 +566,8 @@ static int geneve_gro_complete(struct sk_buff *skb, int nhoff, err = ptype->callbacks.gro_complete(skb, nhoff + gh_len); rcu_read_unlock(); + + skb_set_inner_mac_header(skb, nhoff + gh_len); return err; } #endif
Upstream commit: commit 229740c63169462a838a8b8e16391ed000934631 Author: Jarno Rajahalme <jarno@ovn.org> udp_offload: Set encapsulation before inner completes. UDP tunnel segmentation code relies on the inner offsets being set for an UDP tunnel GSO packet, but the inner *_complete() functions will set the inner offsets only if 'encapsulation' is set before calling them. Currently, udp_gro_complete() sets 'encapsulation' only after the inner *_complete() functions are done. This causes the inner offsets having invalid values after udp_gro_complete() returns, which in turn will make it impossible to properly segment the packet in case it needs to be forwarded, which would be visible to the user either as invalid packets being sent or as packet loss. This patch fixes this by setting skb's 'encapsulation' in udp_gro_complete() before calling into the inner complete functions, and by making each possible UDP tunnel gro_complete() callback set the inner_mac_header to the beginning of the tunnel payload. Signed-off-by: Jarno Rajahalme <jarno@ovn.org> Reviewed-by: Alexander Duyck <aduyck@mirantis.com> Signed-off-by: David S. Miller <davem@davemloft.net> Signed-off-by: Pravin B Shelar <pshelar@ovn.org> --- datapath/linux/compat/geneve.c | 2 ++ 1 file changed, 2 insertions(+)