On Thu, Aug 9, 2018 at 2:37 PM, Gregory Rose <gvrose8...@gmail.com> wrote:
> On 8/9/2018 11:36 AM, Yifeng Sun wrote: > > Yes, I agree. It should be the way to go. > > Thanks, > Yifeng > > > If I'm reading the code correctly the OVS packet cmd will set the skb->cb > using the OVS_CB macro and then > pass that packet down to the ip6erspan_tunnel_xmit function. The > ip6erspan_tunnel_xmit function will > then use the IPCB macro to access the skb->cb area to set the flag to > zero. That works fine for packets > generated by the system that have used the IPCB macro accessor to set the > skb->cb data. However, it > is catastrophic when an skb was prepared with the OVS_CB macro because, as > you might imagine, it > is not pointing to an inet_skb_parm structure but an ovs_skb_cb structure > instead. > > on thing I don't understand is that: when packet/skb is sent to ip6erspan_tunnel_xmit, it is already out of OVS code. ip6erspan_tunnel_xmit clears the inner skb->cb, encap skb with outer erspan+gre header, then lookup the next netdev using outer ip address. The skb again arrives at another netdev. Until this point, there is no OVS code involves and so OVS_CB macro shouldn't cause issue. Yifeng, can you share the crash dump? -- William This code was *never* correct. Somehow or other it didn't cause a problem > until 4.15. We'll have to > figure out some way to check if the packet is ours or is system generated > to know if we should be using > the IPCB accessor. > > In instances of packets generated by the system or any other entity > transmitting ipv6 packets over the > tunnel we'll need to keep the code that clears the flags. > > Thanks, > > - Greg > > > On Thu, Aug 9, 2018 at 11:17 AM, Gregory Rose <gvrose8...@gmail.com> > wrote: > >> On 8/9/2018 11:03 AM, Yifeng Sun wrote: >> >> There is a difference regarding how skb_tunnel_info is stored in skb >> between ovs and upstream kernel. In ovs's compatible gre module, >> skb_tunnel_info is stored in skb->cb while in upstream kernel, it is >> referenced by skb->_skb_refdst. >> >> The upstream netdev code should be okay. >> >> To fix this issue, my guess is that either we comply to the kernel's way >> by using skb->_skb_refdst to store skb_tunnel_info, or we don't use >> IPCB at all. >> >> >> Ah, I see... >> >> We must comply with the kernel method for any given kernel. We can't be >> sure that we'll only handle >> or own packets. >> >> Can't this be fixed by a compatibility layer #define in acinclude.m4 so >> that kernels that store it in >> skb->cb vs. kernels that store it in skb->__skb_refdst will both work? >> >> Thanks, >> >> - Greg >> >> >> >> Thanks, >> Yifeng >> >> >> >> On Thu, Aug 9, 2018 at 10:28 AM, Gregory Rose <gvrose8...@gmail.com> >> wrote: >> >>> >>> On 8/8/2018 6:50 PM, William Tu wrote: >>> >>>> thanks for the fix. >>>> >>>> On Wed, Aug 8, 2018 at 11:32 AM, Yifeng Sun <pkusunyif...@gmail.com> >>>> wrote: >>>> >>>> In compatable gre module, skb->cb is used as ovs_gso_cb. >>>>> This bug clears the 16-23 bit in the address of ovs_gso_cb.tun_dst. >>>>> >>>>> can you explain more about ovs_gso_cb? >>>> >>>> Signed-off-by: Yifeng Sun <pkusunyif...@gmail.com> >>>>> --- >>>>> datapath/linux/compat/ip6_gre.c | 1 - >>>>> 1 file changed, 1 deletion(-) >>>>> >>>>> diff --git a/datapath/linux/compat/ip6_gre.c >>>>> b/datapath/linux/compat/ip6_ >>>>> gre.c >>>>> index 54a76ab..16c1f72 100644 >>>>> --- a/datapath/linux/compat/ip6_gre.c >>>>> +++ b/datapath/linux/compat/ip6_gre.c >>>>> @@ -1146,7 +1146,6 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct >>>>> sk_buff *skb, >>>>> goto tx_err; >>>>> >>>>> t->parms.o_flags &= ~TUNNEL_KEY; >>>>> - IPCB(skb)->flags = 0; >>>>> >>>>> The upstream linux kernel has the above code. >>>> Do we need to fix the upstream kernel then backport? >>>> >>> >>> That would be the normal work flow. >>> >>> Yifeng, >>> >>> Can you please post a patch with this fix to netdev? Taking William's >>> comments into account as well. >>> >>> Good catch and thanks for the fix! >>> >>> - Greg >>> >>> >>>> Thanks, >>>> William >>>> _______________________________________________ >>>> dev mailing list >>>> d...@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >>> >>> >> >> > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
