Well, I guess ideally one would backport 2ce9e71bb960 with a
clarification to the commit message and then un-revert the fix. That
could be made easy for me by posting a 2-patch series, I guess.
On Wed, Feb 27, 2019 at 05:26:30PM +0300, Ilya Maximets wrote:
> Looks like I found the solution.
> Following patch on branch-2.10 makes tests immune to the change:
>
> 2ce9e71bb960 ("tests: Inject ARP replies for snoop tests on different port")
>
> It's the test change only that makes tunnel_push to be not the last action.
> If we'll backport it to 2.9, we could safely un-revert the fix.
> However, commit message of the 2ce9e71bb960 will not be fully correct because
> we're not going to backport the feature it was made for.
>
> Ben, what do you think?
>
> Best regards, Ilya Maximets.
>
> On 27.02.2019 16:28, Anju Thomas wrote:
> > So then should we refrain from adding this fix to 2.9 if it does not make
> > sense or should I remove the cases altogether?
> >
> > Regards
> > Anju
> >
> > -----Original Message-----
> > From: Ilya Maximets [mailto:[email protected]]
> > Sent: Wednesday, February 27, 2019 6:55 PM
> > To: Anju Thomas <[email protected]>; [email protected]
> > Cc: Ben Pfaff <[email protected]>
> > Subject: Re: [ovs-dev] Fix crash due to multiple tnl push action
> >
> > Hi.
> > Thanks for the patch.
> >
> > Few comments:
> >
> > 1. You may use --subject-prefix="PATCH branch-2.9". In this case ovsrobot
> > will be able to apply patch to the right branch for checking.
> >
> > 2. This patch fixes tests (I didn't check), but it effectively makes them
> > useless by replacing all the useful data by simple 'drop' action.
> > It's the same as just removing the broken test.
> >
> > Best regards, Ilya Maximets.
> >
> > On 27.02.2019 14:41, Anju Thomas wrote:
> >> During slow path packet processing, if the action is to output to a
> >> tunnel port, the slow path processing of the encapsulated packet
> >> continues on the underlay bridge and additional actions (e.g. optional
> >> VLAN encapsulation, bond link selection and finally output to port)
> >> are collected there.
> >>
> >> To prepare for a continuation of the processing of the original packet
> >> (e.g. output to other tunnel ports in a flooding scenario), the
> >> “tunnel_push” action and the actions of the underlay bridge are
> >> encapsulated in a clone() action to preserve the original packet.
> >>
> >> If the underlay bridge decides to drop the tunnel packet (for example
> >> if both bonded ports are down simultaneously), the clone(tunnel_push))
> >> actions previously generated as part of translation of the output to
> >> tunnel port are discarded and a stand-alone tunnel_push action is
> >> added instead. Thus the tunnel header is pushed on to the original packet.
> >> This is the bug.
> >>
> >> Consequences: If packet processing continues with sending to further
> >> tunnel ports, multiple tunnel header pushes will happen on the
> >> original packet as typically the tunnels all traverse the same
> >> underlay bond which is down. The packet may not have enough headroom
> >> to accommodate all the tunnel headers. OVS crashes if it runs out of
> >> space while trying to push the tunnel headers.
> >>
> >> Even in case there is enough headroom, the packet will not be freed
> >> since the accumulated action list contains only the tunnel header push
> >> action without any output port action. Thus, we either have a crash or
> >> a packet buffer leak.
> >>
> >> Signed-off-by: Anju Thomas <[email protected]>
> >> ---
> >> ofproto/ofproto-dpif-xlate.c | 7 -------
> >> tests/tunnel-push-pop-ipv6.at | 10 +++++-----
> >> tests/tunnel-push-pop.at | 18 +++++++++---------
> >> 3 files changed, 14 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/ofproto/ofproto-dpif-xlate.c
> >> b/ofproto/ofproto-dpif-xlate.c index 7777ed8..7e69469 100644
> >> --- a/ofproto/ofproto-dpif-xlate.c
> >> +++ b/ofproto/ofproto-dpif-xlate.c
> >> @@ -3510,13 +3510,6 @@ native_tunnel_output(struct xlate_ctx *ctx, const
> >> struct xport *xport,
> >> nl_msg_end_non_empty_nested(ctx->odp_actions, clone_ofs);
> >> } else {
> >> nl_msg_cancel_nested(ctx->odp_actions, clone_ofs);
> >> - /* XXX : There is no real use-case for a tunnel push without
> >> - * any post actions. However keeping it now
> >> - * as is to make the 'make check' happy. Should remove when
> >> all the
> >> - * make check tunnel test case does something meaningful on a
> >> - * tunnel encap packets.
> >> - */
> >> - odp_put_tnl_push_action(ctx->odp_actions, &tnl_push_data);
> >> }
> >>
> >> /* Restore context status. */ diff --git
> >> a/tests/tunnel-push-pop-ipv6.at b/tests/tunnel-push-pop-ipv6.at index
> >> 7ca522a..c9e4a2b 100644
> >> --- a/tests/tunnel-push-pop-ipv6.at
> >> +++ b/tests/tunnel-push-pop-ipv6.at
> >> @@ -93,28 +93,28 @@ dnl Check VXLAN tunnel push AT_CHECK([ovs-ofctl
> >> add-flow int-br action=2]) AT_CHECK([ovs-appctl ofproto/trace
> >> ovs-dummy
> >> 'in_port(2),eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:01),eth_type(
> >> 0x0800),ipv4(src=1.1.3.88,dst=1.1.3.112,proto=47,tos=0,ttl=64,frag=no)
> >> '], [0], [stdout]) AT_CHECK([tail -1 stdout], [0],
> >> - [Datapath actions:
> >> tnl_push(tnl_port(4789),header(size=70,type=4,eth(dst=f8:bc:12:44:34:b
> >> 6,src=aa:55:aa:55:00:00,dl_type=0x86dd),ipv6(src=2001:cafe::88,dst=200
> >> 1:cafe::92,label=0,proto=17,tclass=0x0,hlimit=64),udp(src=0,dst=4789,c
> >> sum=0xffff),vxlan(flags=0x8000000,vni=0x7b)),out_port(100))
> >> + [Datapath actions: drop
> >> ])
> >>
> >> dnl Check VXLAN tunnel push set tunnel id by flow and checksum
> >> AT_CHECK([ovs-ofctl add-flow int-br "actions=set_tunnel:124,4"])
> >> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> >> 'in_port(2),eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:01),eth_type(
> >> 0x0800),ipv4(src=1.1.3.88,dst=1.1.3.112,proto=47,tos=0,ttl=64,frag=no)
> >> '], [0], [stdout]) AT_CHECK([tail -1 stdout], [0],
> >> - [Datapath actions:
> >> tnl_push(tnl_port(4789),header(size=70,type=4,eth(dst=f8:bc:12:44:34:b
> >> 7,src=aa:55:aa:55:00:00,dl_type=0x86dd),ipv6(src=2001:cafe::88,dst=200
> >> 1:cafe::93,label=0,proto=17,tclass=0x0,hlimit=64),udp(src=0,dst=4789,c
> >> sum=0xffff),vxlan(flags=0x8000000,vni=0x7c)),out_port(100))
> >> + [Datapath actions: drop
> >> ])
> >>
> >> dnl Check GRE tunnel push
> >> AT_CHECK([ovs-ofctl add-flow int-br action=3]) AT_CHECK([ovs-appctl
> >> ofproto/trace ovs-dummy
> >> 'in_port(2),eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:01),eth_type(
> >> 0x0800),ipv4(src=1.1.3.88,dst=1.1.3.112,proto=47,tos=0,ttl=64,frag=no)
> >> '], [0], [stdout]) AT_CHECK([tail -1 stdout], [0],
> >> - [Datapath actions:
> >> tnl_push(tnl_port(3),header(size=62,type=3,eth(dst=f8:bc:12:44:34:b6,s
> >> rc=aa:55:aa:55:00:00,dl_type=0x86dd),ipv6(src=2001:cafe::88,dst=2001:c
> >> afe::92,label=0,proto=47,tclass=0x0,hlimit=64),gre((flags=0x2000,proto
> >> =0x6558),key=0x1c8)),out_port(100))
> >> + [Datapath actions: drop
> >> ])
> >>
> >> dnl Check Geneve tunnel push
> >> AT_CHECK([ovs-ofctl add-flow int-br
> >> "actions=set_field:2001:cafe::92->tun_ipv6_dst,5"])
> >> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> >> 'in_port(2),eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:01),eth_type(
> >> 0x0800),ipv4(src=1.1.3.88,dst=1.1.3.112,proto=47,tos=0,ttl=64,frag=no)
> >> '], [0], [stdout]) AT_CHECK([tail -1 stdout], [0],
> >> - [Datapath actions:
> >> tnl_push(tnl_port(6081),header(size=70,type=5,eth(dst=f8:bc:12:44:34:b
> >> 6,src=aa:55:aa:55:00:00,dl_type=0x86dd),ipv6(src=2001:cafe::88,dst=200
> >> 1:cafe::92,label=0,proto=17,tclass=0x0,hlimit=64),udp(src=0,dst=6081,c
> >> sum=0xffff),geneve(vni=0x7b)),out_port(100))
> >> + [Datapath actions: drop
> >> ])
> >>
> >> dnl Check Geneve tunnel push with options @@ -122,7 +122,7 @@
> >> AT_CHECK([ovs-ofctl add-tlv-map int-br
> >> "{class=0xffff,type=0x80,len=4}->tun_meta
> >> AT_CHECK([ovs-ofctl add-flow int-br
> >> "actions=set_field:2001:cafe::92->tun_ipv6_dst,set_field:0xa->tun_meta
> >> data0,5"]) AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> >> 'in_port(2),eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:01),eth_type(
> >> 0x0800),ipv4(src=1.1.3.88,dst=1.1.3.112,proto=47,tos=0,ttl=64,frag=no)
> >> '], [0], [stdout]) AT_CHECK([tail -1 stdout], [0],
> >> - [Datapath actions:
> >> tnl_push(tnl_port(6081),header(size=78,type=5,eth(dst=f8:bc:12:44:34:b
> >> 6,src=aa:55:aa:55:00:00,dl_type=0x86dd),ipv6(src=2001:cafe::88,dst=200
> >> 1:cafe::92,label=0,proto=17,tclass=0x0,hlimit=64),udp(src=0,dst=6081,c
> >> sum=0xffff),geneve(crit,vni=0x7b,options({class=0xffff,type=0x80,len=4
> >> ,0xa}))),out_port(100))
> >> + [Datapath actions: drop
> >> ])
> >>
> >> dnl Check decapsulation of GRE packet diff --git
> >> a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at index
> >> cc8b1b5..07832fc 100644
> >> --- a/tests/tunnel-push-pop.at
> >> +++ b/tests/tunnel-push-pop.at
> >> @@ -113,49 +113,49 @@ dnl Check VXLAN tunnel push AT_CHECK([ovs-ofctl
> >> add-flow int-br action=2]) AT_CHECK([ovs-appctl ofproto/trace
> >> ovs-dummy
> >> 'in_port(2),eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:00),eth_type(
> >> 0x0800),ipv4(src=1.1.3.88,dst=1.1.3.112,proto=47,tos=0,ttl=64,frag=no)
> >> '], [0], [stdout]) AT_CHECK([tail -1 stdout], [0],
> >> - [Datapath actions:
> >> tnl_push(tnl_port(4789),header(size=50,type=4,eth(dst=f8:bc:12:44:34:b
> >> 6,src=aa:55:aa:55:00:00,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1.2.92
> >> ,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan
> >> (flags=0x8000000,vni=0x7b)),out_port(100))
> >> + [Datapath actions: drop
> >> ])
> >>
> >> dnl Check VXLAN GPE tunnel push
> >> AT_CHECK([ovs-ofctl add-flow int-br action=8]) AT_CHECK([ovs-appctl
> >> ofproto/trace ovs-dummy
> >> 'in_port(2),eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:01),eth_type(
> >> 0x0800),ipv4(src=1.1.3.88,dst=1.1.3.112,proto=47,tos=0,ttl=64,frag=no)
> >> '], [0], [stdout]) AT_CHECK([tail -1 stdout], [0],
> >> - [Datapath actions:
> >> tnl_push(tnl_port(4789),header(size=50,type=4,eth(dst=f8:bc:12:44:34:b
> >> 6,src=aa:55:aa:55:00:00,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1.2.92
> >> ,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan
> >> (flags=0xc000003,vni=0x159)),out_port(100))
> >> + [Datapath actions: drop
> >> ])
> >>
> >> dnl Check VXLAN tunnel push set tunnel id by flow and checksum
> >> AT_CHECK([ovs-ofctl add-flow int-br "actions=set_tunnel:124,4"])
> >> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> >> 'in_port(2),eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:00),eth_type(
> >> 0x0800),ipv4(src=1.1.3.88,dst=1.1.3.112,proto=47,tos=0,ttl=64,frag=no)
> >> '], [0], [stdout]) AT_CHECK([tail -1 stdout], [0],
> >> - [Datapath actions:
> >> tnl_push(tnl_port(4789),header(size=50,type=4,eth(dst=f8:bc:12:44:34:b
> >> 7,src=aa:55:aa:55:00:00,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1.2.93
> >> ,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=4789,csum=0xffff),vx
> >> lan(flags=0x8000000,vni=0x7c)),out_port(100))
> >> + [Datapath actions: drop
> >> ])
> >>
> >> dnl Check GRE tunnel push
> >> AT_CHECK([ovs-ofctl add-flow int-br action=3]) AT_CHECK([ovs-appctl
> >> ofproto/trace ovs-dummy
> >> 'in_port(2),eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:00),eth_type(
> >> 0x0800),ipv4(src=1.1.3.88,dst=1.1.3.112,proto=47,tos=0,ttl=64,frag=no)
> >> '], [0], [stdout]) AT_CHECK([tail -1 stdout], [0],
> >> - [Datapath actions:
> >> tnl_push(tnl_port(3),header(size=42,type=3,eth(dst=f8:bc:12:44:34:b6,s
> >> rc=aa:55:aa:55:00:00,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1.2.92,pr
> >> oto=47,tos=0,ttl=64,frag=0x4000),gre((flags=0x2000,proto=0x6558),key=0
> >> x1c8)),out_port(100))
> >> + [Datapath actions: drop
> >> ])
> >>
> >> dnl Check L3GRE tunnel push
> >> AT_CHECK([ovs-ofctl add-flow int-br action=7]) AT_CHECK([ovs-appctl
> >> ofproto/trace ovs-dummy
> >> 'in_port(2),eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:01),eth_type(
> >> 0x0800),ipv4(src=1.1.3.88,dst=1.1.3.112,proto=47,tos=0,ttl=64,frag=no)
> >> '], [0], [stdout]) AT_CHECK([tail -1 stdout], [0],
> >> - [Datapath actions:
> >> pop_eth,tnl_push(tnl_port(3),header(size=42,type=3,eth(dst=f8:bc:12:44
> >> :34:b6,src=aa:55:aa:55:00:00,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1
> >> .2.92,proto=47,tos=0,ttl=64,frag=0x4000),gre((flags=0x2000,proto=0x800
> >> ),key=0x1c8)),out_port(100))
> >> + [Datapath actions: pop_eth
> >> ])
> >>
> >> dnl Check Geneve tunnel push
> >> AT_CHECK([ovs-ofctl add-flow int-br
> >> "actions=set_field:1.1.2.92->tun_dst,5"])
> >> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> >> 'in_port(2),eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:00),eth_type(
> >> 0x0800),ipv4(src=1.1.3.88,dst=1.1.3.112,proto=47,tos=0,ttl=64,frag=no)
> >> '], [0], [stdout]) AT_CHECK([tail -1 stdout], [0],
> >> - [Datapath actions:
> >> tnl_push(tnl_port(6081),header(size=50,type=5,eth(dst=f8:bc:12:44:34:b
> >> 6,src=aa:55:aa:55:00:00,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1.2.92
> >> ,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=6081,csum=0x0),genev
> >> e(vni=0x7b)),out_port(100))
> >> + [Datapath actions: drop
> >> ])
> >>
> >> dnl Check Geneve tunnel push with pkt-mark AT_CHECK([ovs-ofctl
> >> add-flow int-br "actions=set_tunnel:234,6"]) AT_CHECK([ovs-appctl
> >> ofproto/trace ovs-dummy
> >> 'in_port(2),eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:00),eth_type(
> >> 0x0800),ipv4(src=1.1.3.88,dst=1.1.3.112,proto=47,tos=0,ttl=64,frag=no)
> >> '], [0], [stdout]) AT_CHECK([tail -1 stdout], [0],
> >> - [Datapath actions:
> >> set(skb_mark(0x4d2)),tnl_push(tnl_port(6081),header(size=50,type=5,eth
> >> (dst=f8:bc:12:44:34:b7,src=aa:55:aa:55:00:00,dl_type=0x0800),ipv4(src=
> >> 1.1.2.88,dst=1.1.2.93,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst
> >> =6081,csum=0x0),geneve(vni=0xea)),out_port(100))
> >> + [Datapath actions: set(skb_mark(0x4d2))
> >> ])
> >>
> >> dnl Check Geneve tunnel push with options @@ -163,7 +163,7 @@
> >> AT_CHECK([ovs-ofctl add-tlv-map int-br
> >> "{class=0xffff,type=0x80,len=4}->tun_meta
> >> AT_CHECK([ovs-ofctl add-flow int-br
> >> "actions=set_field:1.1.2.92->tun_dst,set_field:0xa->tun_metadata0,5"])
> >> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> >> 'in_port(2),eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:00),eth_type(
> >> 0x0800),ipv4(src=1.1.3.88,dst=1.1.3.112,proto=47,tos=0,ttl=64,frag=no)
> >> '], [0], [stdout]) AT_CHECK([tail -1 stdout], [0],
> >> - [Datapath actions:
> >> tnl_push(tnl_port(6081),header(size=58,type=5,eth(dst=f8:bc:12:44:34:b
> >> 6,src=aa:55:aa:55:00:00,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1.2.92
> >> ,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=6081,csum=0x0),genev
> >> e(crit,vni=0x7b,options({class=0xffff,type=0x80,len=4,0xa}))),out_port
> >> (100))
> >> + [Datapath actions: drop
> >> ])
> >>
> >> dnl Check decapsulation of GRE packet @@ -325,7 +325,7 @@
> >> AT_CHECK([ovs-ofctl add-flow int-br action=3])
> >>
> >> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> >> 'in_port(2),eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:00),eth_type(
> >> 0x0800),ipv4(src=1.1.3.88,dst=1.1.3.112,proto=17,tos=0,ttl=64,frag=no)
> >> ,udp(src=51283,dst=4789)'], [0], [stdout]) AT_CHECK([tail -1 stdout],
> >> [0],
> >> - [Datapath actions:
> >> tnl_push(tnl_port(3),header(size=42,type=3,eth(dst=f8:bc:12:44:34:b6,s
> >> rc=aa:55:aa:55:00:00,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1.2.92,pr
> >> oto=47,tos=0,ttl=64,frag=0x4000),gre((flags=0x2000,proto=0x6558),key=0
> >> x1c8)),out_port(100))
> >> + [Datapath actions: drop
> >> ])
> >>
> >> dnl Verify outer L2 and L3 header flow fields can be matched in the
> >> underlay bridge
> >>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
