| Message ID | 1533753171-11730-1-git-send-email-pkusunyifeng@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,1/8] ip6_gre: Fix a bug that clears address bits | expand |
thanks for the fix. On Wed, Aug 8, 2018 at 11:32 AM, Yifeng Sun <pkusunyifeng@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 <pkusunyifeng@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? Thanks, William
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 <pkusunyifeng@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 <pkusunyifeng@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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. Thanks, Yifeng On Thu, Aug 9, 2018 at 10:28 AM, Gregory Rose <gvrose8192@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 <pkusunyifeng@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 <pkusunyifeng@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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > >
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 <gvrose8192@gmail.com > <mailto:gvrose8192@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 > <pkusunyifeng@gmail.com <mailto:pkusunyifeng@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 <pkusunyifeng@gmail.com > <mailto:pkusunyifeng@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 > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > >
Yes, I agree. It should be the way to go. Thanks, Yifeng On Thu, Aug 9, 2018 at 11:17 AM, Gregory Rose <gvrose8192@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 <gvrose8192@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 <pkusunyifeng@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 <pkusunyifeng@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 >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >> >> > >
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. 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 <gvrose8192@gmail.com > <mailto:gvrose8192@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 >> <gvrose8192@gmail.com <mailto:gvrose8192@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 >> <pkusunyifeng@gmail.com <mailto:pkusunyifeng@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 <pkusunyifeng@gmail.com >> <mailto:pkusunyifeng@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 >> dev@openvswitch.org <mailto:dev@openvswitch.org> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >> >> >> > >
On Thu, Aug 9, 2018 at 2:37 PM, Gregory Rose <gvrose8192@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 <gvrose8192@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 <gvrose8192@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 <pkusunyifeng@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 <pkusunyifeng@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 >>>> dev@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >>> >>> >> >> > >
On 8/9/2018 3:33 PM, William Tu wrote: > > > On Thu, Aug 9, 2018 at 2:37 PM, Gregory Rose <gvrose8192@gmail.com > <mailto:gvrose8192@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 OK, I did not read the code correctly. It's an ovs_gso_cb structure and now I get where you were talking about bits 16-23 being over written - that being the tun_dst pointer in our case since USE_UPSTREAM_TUNNEL is not defined. That wasn't really clear to me before. Here's a before and after dump of the skb->cb data area: [17182.462235] Before --------- [17182.463006] 00 9c 6f 8b 21 9d ff ff 00 00 4c 00 00 00 00 00 [17182.463010]00 f2 22 b6 21 9d ff ff 00 00 00 00 00 00 00 00 [17182.463777] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [17182.464519] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [17182.465277] --------- [17182.466749] After --------- [17182.467462] 00 9c 6f 8b 21 9d ff ff 00 00 4c 00 00 00 00 00 [17182.467465] 00 f2 22 b6 00 00 ff ff 00 00 00 00 00 00 00 00 [17182.468221] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [17182.468968] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [17182.469734] --------- Note the bits 16-23 of the tun_dst address being overwritten... :O Guh - keeping track of all the CB accessors is a real tangled mess. - Greg > > 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 >> <gvrose8192@gmail.com <mailto:gvrose8192@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 >>> <gvrose8192@gmail.com <mailto:gvrose8192@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 >>> <pkusunyifeng@gmail.com >>> <mailto:pkusunyifeng@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 >>> <pkusunyifeng@gmail.com >>> <mailto:pkusunyifeng@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 >>> dev@openvswitch.org <mailto:dev@openvswitch.org> >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >>> >>> >>> >> >> > >
On 8/8/2018 11:32 AM, Yifeng Sun 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. > > Signed-off-by: Yifeng Sun <pkusunyifeng@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; > > tun_info = ovs_skb_tunnel_info(skb); > if (unlikely(!tun_info || Yifeng, First, I'm sorry but you actually did identify the problem correctly at first, I just didn't see it. My bad. Second, I'm no longer worried about removing the code. It shouldn't be used in the case where we are not using USE_UPSTREAM_TUNNEL. If USE_UPSTREAM_TUNNEL is defined then this entire module does not compile anyway. So I think removing the IPCB macro that sets the flags to zero is fine. The same needs to be done in _gre6_xmit(). Go ahead and resubmit the patch with the _gre6_xmit() case handled as well and I think that should be fine. Thanks for all your work!!! - Greg
Hi Greg, Thanks for clarifying this bug, you did a much better job than I did. Sorry I didn't describe it clearly. I will created another patch, as you suggested. William, below is the crash dump you requested. [ 645.832991] general protection fault: 0000 [#1] SMP PTI [ 645.833033] Modules linked in: veth vport_vxlan(OE) vport_stt(OE) vport_lisp(OE) vport_geneve(OE) openvswitch(OE) tunnel6 nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack udp_tunnel netconsole rpcsec_gss_krb5 auth_rpcgss nfsv4 nfs vmw_vsock_vmci_transport vsock lockd grace fscache vmw_balloon sb_edac joydev input_leds serio_raw coretemp vmw_vmci shpchp i2c_piix4 mac_hid ib_iser rdma_cm iw_cm ib_cm ib_core sunrpc configfs iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi autofs4 btrfs zstd_decompress zstd_compress xxhash raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq libcrc32c raid1 raid0 multipath linear hid_generic usbhid hid crct10dif_pclmul crc32_pclmul ghash_clmulni_intel vmwgfx pcbc ttm drm_kms_helper [ 645.836096] syscopyarea sysfillrect sysimgblt fb_sys_fops drm psmouse aesni_intel aes_x86_64 crypto_simd glue_helper cryptd mptspi mptscsih mptbase vmxnet3 ahci libahci scsi_transport_spi pata_acpi [ 645.837697] CPU: 0 PID: 17724 Comm: handler2 Tainted: G OE 4.15.18 #1 [ 645.838250] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 09/21/2015 [ 645.839419] RIP: 0010:ip6erspan_tunnel_xmit+0x1da/0x6f0 [openvswitch] [ 645.839996] RSP: 0018:ffffb36d008177c0 EFLAGS: 00010286 [ 645.840562] RAX: 00000000ffffffea RBX: ffff9be571e84000 RCX: 0000000000000040 [ 645.841126] RDX: 000000000000003e RSI: ffff000071cf7e00 RDI: ffff9be571cf7600 [ 645.841697] RBP: ffffb36d00817880 R08: 0000000000000018 R09: 0000000000000000 [ 645.842268] R10: ffffb36d00817a90 R11: 00000000fffffffe R12: ffff9be5610e4400 [ 645.842838] R13: 0000000000000000 R14: 0000000000000001 R15: 0000000000000000 [ 645.843422] FS: 00007eff6e146700(0000) GS:ffff9be5bfc00000(0000) knlGS:0000000000000000 [ 645.844015] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 645.844607] CR2: 00007febc8f859d0 CR3: 000000005de8a002 CR4: 00000000001606f0 [ 645.845250] Call Trace: [ 645.846070] do_execute_actions+0x1505/0x1900 [openvswitch] [ 645.846686] ? reserve_sfa_size+0x28/0x100 [openvswitch] [ 645.847319] ? __ovs_nla_copy_actions+0x15c/0x6d0 [openvswitch] [ 645.847958] ? __kmalloc_reserve.isra.39+0x2e/0x80 [ 645.848581] ? _cond_resched+0x16/0x40 [ 645.849196] ? __kmalloc+0x16d/0x1f0 [ 645.849791] ? ovs_execute_actions+0x48/0x120 [openvswitch] [ 645.850401] ovs_execute_actions+0x48/0x120 [openvswitch] [ 645.851012] ovs_packet_cmd_execute+0x267/0x2b0 [openvswitch] [ 645.851624] genl_family_rcv_msg+0x1e9/0x380 [ 645.852231] genl_rcv_msg+0x47/0x90 [ 645.852820] ? genl_family_rcv_msg+0x380/0x380 [ 645.853399] netlink_rcv_skb+0xde/0x110 [ 645.853962] genl_rcv+0x24/0x40 [ 645.854505] netlink_unicast+0x183/0x240 [ 645.855040] netlink_sendmsg+0x2c2/0x3b0 [ 645.855568] sock_sendmsg+0x36/0x40 [ 645.856074] ___sys_sendmsg+0x2bc/0x2d0 [ 645.856571] ? ep_item_poll.isra.10+0x3b/0xc0 [ 645.857045] ? ep_send_events_proc+0x87/0x1a0 [ 645.857501] ? ep_read_events_proc+0xd0/0xd0 [ 645.857940] ? ep_scan_ready_list+0x1f3/0x200 [ 645.858371] ? ep_poll+0x1fb/0x3b0 [ 645.858788] ? __sys_sendmsg+0x51/0x90 [ 645.859189] __sys_sendmsg+0x51/0x90 [ 645.859585] do_syscall_64+0x6e/0x120 [ 645.859964] entry_SYSCALL_64_after_hwframe+0x3d/0xa2 [ 645.860472] RIP: 0033:0x7eff6ec36a6d [ 645.860843] RSP: 002b:00007eff6e0e81a0 EFLAGS: 00000293 ORIG_RAX: 000000000000002e [ 645.861226] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007eff6ec36a6d [ 645.861622] RDX: 0000000000000000 RSI: 00007eff6e0e8200 RDI: 0000000000000011 [ 645.862009] RBP: 00007eff6e0e8fe0 R08: 0000000000000000 R09: 00007eff6e0e9a60 [ 645.862396] R10: 0000000000a5d340 R11: 0000000000000293 R12: 00000000009f7690 [ 645.862775] R13: 00007eff6e0e8200 R14: 0000000000000002 R15: 00007eff6e0e86a0 [ 645.863189] Code: 0f 85 a6 fe ff ff 45 31 c9 b8 ea ff ff ff 66 45 89 4c 24 3c 66 81 a3 1a 09 00 00 ff fb 49 8b 74 24 38 48 85 f6 0f 84 a2 fe ff ff <0f> b6 96 f1 00 00 00 b8 ea ff ff ff f6 c2 01 0f 84 8d fe ff ff [ 645.864513] RIP: ip6erspan_tunnel_xmit+0x1da/0x6f0 [openvswitch] RSP: ffffb36d008177c0 [ 645.865043] ---[ end trace 25bab82318677b6f ]--- [ 645.865452] Kernel panic - not syncing: Fatal exception in interrupt [ 645.865923] Kernel Offset: 0x3d000000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff) [ 645.866752] ---[ end Kernel panic - not syncing: Fatal exception in interrupt On Thu, Aug 9, 2018 at 3:59 PM, Gregory Rose <gvrose8192@gmail.com> wrote: > On 8/8/2018 11:32 AM, Yifeng Sun 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. >> >> Signed-off-by: Yifeng Sun <pkusunyifeng@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; >> tun_info = ovs_skb_tunnel_info(skb); >> if (unlikely(!tun_info || >> > > Yifeng, > > First, I'm sorry but you actually did identify the problem correctly at > first, I just didn't see it. My bad. > > Second, I'm no longer worried about removing the code. It shouldn't be > used in the case where > we are not using USE_UPSTREAM_TUNNEL. If USE_UPSTREAM_TUNNEL is defined > then this entire > module does not compile anyway. So I think removing the IPCB macro that > sets the flags to zero is fine. > > The same needs to be done in _gre6_xmit(). > > Go ahead and resubmit the patch with the _gre6_xmit() case handled as well > and I think that should be fine. > > Thanks for all your work!!! > > - Greg >
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; tun_info = ovs_skb_tunnel_info(skb); if (unlikely(!tun_info ||
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. Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> --- datapath/linux/compat/ip6_gre.c | 1 - 1 file changed, 1 deletion(-)