On 14 July 2016 at 02:22, Jarno Rajahalme <ja...@ovn.org> wrote:
>
> > On Jul 13, 2016, at 9:01 PM, Ansis Atteka <aatt...@ovn.org> wrote:
> >
> > There are two issues that this patch fixes:
> > 1. it was impossible to set skb mark at all through
> > NXM_NX_PKT_MARK register for tunnel packets; AND
> > 2. ipsec_xxx tunnels would not be marked with the default
> > IPsec mark (broken by d23df9a87 "lib/odp: Use masked set
> > actions.").
> >
> > This patch also adds anti-regression tests to prevent such
> > breakages in the future.
> >
> > Signed-off-by: Ansis Atteka <aatt...@ovn.org>
> > VMware-BZ: #1653178
> > ---
> > ofproto/tunnel.c | 4 +-
> > tests/tunnel.at | 120
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 122 insertions(+), 2 deletions(-)
> >
> > diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
> > index 9695c54..206299e 100644
> > --- a/ofproto/tunnel.c
> > +++ b/ofproto/tunnel.c
> > @@ -357,7 +357,6 @@ tnl_process_ecn(struct flow *flow)
> > flow->nw_tos |= IP_ECN_CE;
> > }
> >
> > - flow->pkt_mark &= ~IPSEC_MARK;
>
> If we do not clear the IPSEC_MARK bit on input, then the bit will remain
> set also on output, since the change below will not change the mark if
> match.pkt_mark is 0. For example, we could have input from ipsec tunnel
> (mark has IPSEC_MARK set ?), which is then output to a non-IPSEC tunnel,
> but the combination of the changes above an below will fail to clear the
> mark. Would this work as intended?
>
I think you are right. IPsec skb_mark Mark should be cleared when
outputting packet to the next tunnel, because the next tunnel could be in
unencrypted mode:
1) My patch behavior:
recirc_id(0),tunnel(src=192.168.122.101,dst=192.168.122.13,flags(-df-csum)),in_port(2),skb_mark(0x1),eth(src=be:7a:9d:e2:4f:44,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=1.1.1.2,tip=1.1.1.3,op=1/0xff),
packets:3, bytes:126, used:0.540s,
actions:1,set(tunnel(dst=8.8.8.8,ttl=64,flags(df))),2
2) Behavior after your suggested changes:
recirc_id(0),tunnel(src=192.168.122.13,dst=192.168.122.101,flags(-df-csum)),in_port(2),skb_mark(0x1),eth(src=ea:55:cf:a8:21:4c,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=1.1.1.1,tip=1.1.1.3,op=1/0xff),
packets:4, bytes:168, used:0.900s,
actions:set(skb_mark(0)),1,set(tunnel(dst=8.8.8.8,ttl=64,flags(df))),2
I sent PATCHv2 for that! Thanks for pointing this out.
> Jarno
>
> > return true;
> > }
> >
> > @@ -435,7 +434,8 @@ tnl_port_send(const struct ofport_dpif *ofport,
> struct flow *flow,
> > flow->tunnel.ipv6_dst = in6addr_any;
> > }
> > }
> > - flow->pkt_mark = tnl_port->match.pkt_mark;
> > + flow->pkt_mark |= tnl_port->match.pkt_mark;
> > + wc->masks.pkt_mark |= tnl_port->match.pkt_mark;
> >
> > if (!cfg->out_key_flow) {
> > flow->tunnel.tun_id = cfg->out_key;
> > diff --git a/tests/tunnel.at b/tests/tunnel.at
> > index 7f82785..15ae5cf 100644
> > --- a/tests/tunnel.at
> > +++ b/tests/tunnel.at
> > @@ -142,6 +142,126 @@ AT_CHECK([tail -1 stdout], [0],
> > OVS_VSWITCHD_STOP
> > AT_CLEANUP
> >
> > +AT_SETUP([tunnel - unencrypted tunnel and not setting skb_mark])
> > +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=gre \
> > + options:remote_ip=1.1.1.1 options:local_ip=2.2.2.2 \
> > + options:key=5 ofport_request=1\
> > + -- add-port br0 p2 -- set Interface p2 type=dummy \
> > + ofport_request=2 ofport_request=2])
> > +AT_DATA([flows.txt], [dnl
> > +actions=output:1
> > +])
> > +OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP
> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=4,ttl=128,frag=no),tcp(src=8,dst=9)'],
> [0], [stdout])
> > +AT_CHECK([tail -1 stdout], [0],
> > + [Datapath actions:
> set(tunnel(tun_id=0x5,src=2.2.2.2,dst=1.1.1.1,ttl=64,flags(df|key))),1
> > +])
> > +OVS_VSWITCHD_STOP
> > +AT_CLEANUP
> > +
> > +AT_SETUP([tunnel - unencrypted tunnel and setting skb_mark to 1])
> > +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=gre \
> > + options:remote_ip=1.1.1.1 options:local_ip=2.2.2.2 \
> > + options:key=5 ofport_request=1\
> > + -- add-port br0 p2 -- set Interface p2 type=dummy \
> > + ofport_request=2 ofport_request=2])
> > +AT_DATA([flows.txt], [dnl
> > +actions=load:0x1->NXM_NX_PKT_MARK[[]],output:1
> > +])
> > +OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP
> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=4,ttl=128,frag=no),tcp(src=8,dst=9)'],
> [0], [stdout])
> > +AT_CHECK([tail -1 stdout], [0],
> > + [Datapath actions:
> set(tunnel(tun_id=0x5,src=2.2.2.2,dst=1.1.1.1,ttl=64,flags(df|key))),set(skb_mark(0x1)),1
> > +])
> > +OVS_VSWITCHD_STOP
> > +AT_CLEANUP
> > +
> > +AT_SETUP([tunnel - unencrypted tunnel and setting skb_mark to 2])
> > +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=gre \
> > + options:remote_ip=1.1.1.1 options:local_ip=2.2.2.2 \
> > + options:key=5 ofport_request=1\
> > + -- add-port br0 p2 -- set Interface p2 type=dummy \
> > + ofport_request=2 ofport_request=2])
> > +AT_DATA([flows.txt], [dnl
> > +actions=load:0x2->NXM_NX_PKT_MARK[[]],output:1
> > +])
> > +OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP
> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=4,ttl=128,frag=no),tcp(src=8,dst=9)'],
> [0], [stdout])
> > +AT_CHECK([tail -1 stdout], [0],
> > + [Datapath actions:
> set(tunnel(tun_id=0x5,src=2.2.2.2,dst=1.1.1.1,ttl=64,flags(df|key))),set(skb_mark(0x2)),1
> > +])
> > +OVS_VSWITCHD_STOP
> > +AT_CLEANUP
> > +
> > +AT_SETUP([tunnel - encrypted tunnel and not setting skb_mark])
> > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > +AT_SKIP_IF([$non_ascii_cwd])
> > +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=gre \
> > + options:remote_ip=1.1.1.1 options:local_ip=2.2.2.2 \
> > + options:key=5 ofport_request=1\
> > + -- add-port br0 p2 -- set Interface p2 type=dummy \
> > + ofport_request=2 ofport_request=2])
> > +AT_DATA([flows.txt], [dnl
> > +actions=output:1
> > +])
> > +OVS_MONITOR_IPSEC_START
> > +AT_CHECK([ovs-vsctl set interface p1 type=ipsec_gre
> options:psk=1234567890])
> > +OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP
> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=4,ttl=128,frag=no),tcp(src=8,dst=9)'],
> [0], [stdout])
> > +AT_CHECK([tail -1 stdout], [0],
> > + [Datapath actions:
> set(tunnel(tun_id=0x5,src=2.2.2.2,dst=1.1.1.1,ttl=64,flags(df|key))),set(skb_mark(0x1/0x1)),1
> > +])
> > +OVS_VSWITCHD_STOP
> > +AT_CLEANUP
> > +
> > +AT_SETUP([tunnel - encrypted tunnel and setting skb_mark to 1])
> > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > +AT_SKIP_IF([$non_ascii_cwd])
> > +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=gre \
> > + options:remote_ip=1.1.1.1 options:local_ip=2.2.2.2 \
> > + options:key=5 ofport_request=1\
> > + -- add-port br0 p2 -- set Interface p2 type=dummy \
> > + ofport_request=2 ofport_request=2])
> > +AT_DATA([flows.txt], [dnl
> > +actions=load:0x1->NXM_NX_PKT_MARK[[]],output:1
> > +])
> > +OVS_MONITOR_IPSEC_START
> > +AT_CHECK([ovs-vsctl set interface p1 type=ipsec_gre
> options:psk=1234567890])
> > +OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP
> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=4,ttl=128,frag=no),tcp(src=8,dst=9)'],
> [0], [stdout])
> > +AT_CHECK([tail -1 stdout], [0],
> > + [Datapath actions:
> set(tunnel(tun_id=0x5,src=2.2.2.2,dst=1.1.1.1,ttl=64,flags(df|key))),set(skb_mark(0x1)),1
> > +])
> > +OVS_VSWITCHD_STOP
> > +AT_CLEANUP
> > +
> > +AT_SETUP([tunnel - encrypted tunnel and setting skb_mark to 2])
> > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > +AT_SKIP_IF([$non_ascii_cwd])
> > +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=gre \
> > + options:remote_ip=1.1.1.1 options:local_ip=2.2.2.2 \
> > + options:key=5 ofport_request=1\
> > + -- add-port br0 p2 -- set Interface p2 type=dummy \
> > + ofport_request=2 ofport_request=2])
> > +AT_DATA([flows.txt], [dnl
> > +actions=load:0x2->NXM_NX_PKT_MARK[[]],output:1
> > +])
> > +OVS_MONITOR_IPSEC_START
> > +AT_CHECK([ovs-vsctl set interface p1 type=ipsec_gre
> options:psk=1234567890])
> > +OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP
> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=4,ttl=128,frag=no),tcp(src=8,dst=9)'],
> [0], [stdout])
> > +AT_CHECK([tail -1 stdout], [0],
> > + [Datapath actions:
> set(tunnel(tun_id=0x5,src=2.2.2.2,dst=1.1.1.1,ttl=64,flags(df|key))),set(skb_mark(0x3)),1
> > +])
> > +OVS_VSWITCHD_STOP
> > +AT_CLEANUP
> > +
> > AT_SETUP([tunnel - ToS and TTL inheritance])
> > OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=gre \
> > options:remote_ip=1.1.1.1 options:tos=inherit \
> > --
> > 2.7.4
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
