On 8/9/2018 3:33 PM, William Tu wrote:
On Thu, Aug 9, 2018 at 2:37 PM, Gregory Rose <[email protected]
<mailto:[email protected]>> 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
<[email protected] <mailto:[email protected]>> 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
<[email protected] <mailto:[email protected]>> 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
<[email protected]
<mailto:[email protected]>> 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
<[email protected]
<mailto:[email protected]>>
---
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
[email protected] <mailto:[email protected]>
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
<https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
