Hey Jarno,
Thx a lot for the comments, and sorry about my careless testing...
I saw for dpif-netdev, the upcall processing also calls
odp_flow_key_from_mask()... but totally missed that the wildcard is
actually obtains from: *wc = upcall.xout.wc; /* in upcall_cb(). */
I'll adopt your suggested changes!
Thanks,
Alex Wang,
On Sat, Jun 6, 2015 at 11:38 AM, Jarno Rajahalme <jrajaha...@nicira.com>
wrote:
> Alex,
>
> I took a closer look at the test case and it actually verifies that the
> CFI in the mask is NOT set:
>
> +AT_CHECK([cat ovs-vswitchd.log | grep 'in_port=[[1]]' |
> FILTER_FLOW_INSTALL | STRIP_XOUT], [0], [dnl
> +recirc_id=0,ip,in_port=1,vlan_tci=0x100a/0x0fff,nw_frag=no, actions: <del>
>
> Note the mask: 0x0fff!
>
> How about this instead:
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 71b8bef..6bb8518 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -5033,6 +5033,10 @@ xlate_actions(struct xlate_in *xin, struct
> xlate_out *xout)
> wc->masks.tp_src &= htons(UINT8_MAX);
> wc->masks.tp_dst &= htons(UINT8_MAX);
> }
> + /* VLAN_TCI CFI bit must be matched if any of the TCI is matched.
> */
> + if (wc->masks.vlan_tci) {
> + wc->masks.vlan_tci |= htons(VLAN_CFI);
> + }
> }
> }
>
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index b5a9ad9..f9015c7 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -6673,3 +6673,19 @@
> icmp6,vlan_tci=0x0000,dl_src=00:00:86:05:80:da,dl_dst=00:60:97:07:69:ea,ipv6_src
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +# Tests the exact match of CFI bit in installed datapath flows matching
> VLAN.
> +AT_SETUP([ofproto-dpif - vlan matching])
> +OVS_VSWITCHD_START(
> + [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1])
> +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
> +
> +AT_CHECK([ovs-ofctl del-flows br0])
> +AT_CHECK([ovs-ofctl add-flow br0
> "vlan_tci=0x000a/0x0fff,action=output:local"])
> +
> +AT_CHECK([ovs-appctl netdev-dummy/receive p0
> 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x8100),vlan(vid=10,pcp=0),encap(eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0))'])
> +
> +AT_CHECK([cat ovs-vswitchd.log | grep 'in_port=[[1]]' |
> FILTER_FLOW_INSTALL | STRIP_XOUT], [0], [dnl
> +recirc_id=0,ip,in_port=1,dl_vlan=10,nw_frag=no, actions: <del>
> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
>
> Jarno
>
> > On Jun 6, 2015, at 10:31 AM, Jarno Rajahalme <jrajaha...@nicira.com>
> wrote:
> >
> > Alex,
> >
> > I though it surprising that fixing the mask in the netlink attributes
> would fix the problem for the userspace datapath used in the test case, as
> the userspace datapath does not use the netlink attribute format in flow
> setup at all.
> >
> > I tested just the test case without the OVS code change, and the test
> case passes without the fix. Maybe this is due to the fact that we do not
> have the same VLAN_CFI validation in the userspace datapath at all. In
> general we assume in userspace datapath that the wildcards are correct as
> initialized via xlate_actions() during the upcall callback.
> >
> > So, IMO the correct fix is to ensure the mask (‘wc’) has the CFI bit set
> when needed at the end of the ofproto-dpif-xlate.c/xlate_actions(), right
> after the other sanity checks for the wildcards. No change needed in
> odp-util.c.
> >
> > Then, maybe we should add a kernel module testcase to make sure the fix
> works also for linux kernel datapath.
> >
> > I do not think we should start adding validations for the userspace
> datapath, though. After all, the ‘wc’ is now correct again as generated by
> late_actions() :-)
> >
> > Jarno
> >
> >> On Jun 6, 2015, at 8:48 AM, Alex Wang <al...@nicira.com> wrote:
> >>
> >> OVS datapath has check which prevents the installation of flow
> >> that matches VLAN TCI but does not have exact match for VLAN_CFI
> >> bit. To follow this rule, ovs userspace must make sure the
> >> flow key for datapath flow matching VLAN TCI has exact match for
> >> VLAN_CFI bit.
> >>
> >> Before this commit, this is not enforced, so OpenFlow flow like
> >> "vlan_tci=0x000a/0x0fff,action=output:local" can generate datapath
> >> flow like "vlan(vid=5/0xfff,pcp=0/0x0,cfi=1/0)".
> >>
> >> With the OVS datapath check, the installation of such datapath
> >> flow will be rejected with:
> >> "|WARN|system@ovs-system: failed to put[create][modify] (Invalid
> argument)"
> >>
> >> This commit makes ovs userspace always exact match the VLAN_CFI
> >> bit if the flow matches VLAN TCI.
> >>
> >> Reported-by: Ronald Lee <ronald...@vmware.com>
> >> Signed-off-by: Alex Wang <al...@nicira.com>
> >> ---
> >> lib/odp-util.c | 6 +++++-
> >> tests/ofproto-dpif.at | 16 ++++++++++++++++
> >> 2 files changed, 21 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/odp-util.c b/lib/odp-util.c
> >> index 3204d16..63f2660 100644
> >> --- a/lib/odp-util.c
> >> +++ b/lib/odp-util.c
> >> @@ -3486,10 +3486,14 @@ odp_flow_key_from_flow__(struct ofpbuf *buf,
> const struct flow *flow,
> >> if (flow->vlan_tci != htons(0) || flow->dl_type ==
> htons(ETH_TYPE_VLAN)) {
> >> if (export_mask) {
> >> nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, OVS_BE16_MAX);
> >> + /* When there is a VLAN, the VLAN match must always
> include match
> >> + * on "present" bit. */
> >> + nl_msg_put_be16(buf, OVS_KEY_ATTR_VLAN,
> >> + data->vlan_tci | htons(VLAN_CFI));
> >> } else {
> >> nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE,
> htons(ETH_TYPE_VLAN));
> >> + nl_msg_put_be16(buf, OVS_KEY_ATTR_VLAN, data->vlan_tci);
> >> }
> >> - nl_msg_put_be16(buf, OVS_KEY_ATTR_VLAN, data->vlan_tci);
> >> encap = nl_msg_start_nested(buf, OVS_KEY_ATTR_ENCAP);
> >> if (flow->vlan_tci == htons(0)) {
> >> goto unencap;
> >> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> >> index b5a9ad9..ec885a5 100644
> >> --- a/tests/ofproto-dpif.at
> >> +++ b/tests/ofproto-dpif.at
> >> @@ -6673,3 +6673,19 @@
> icmp6,vlan_tci=0x0000,dl_src=00:00:86:05:80:da,dl_dst=00:60:97:07:69:ea,ipv6_src
> >> OVS_VSWITCHD_STOP
> >> AT_CLEANUP
> >>
> >> +# Tests the exact match of CFI bit in installed datapath flows
> matching VLAN.
> >> +AT_SETUP([ofproto-dpif - vlan matching])
> >> +OVS_VSWITCHD_START(
> >> + [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1])
> >> +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
> >> +
> >> +AT_CHECK([ovs-ofctl del-flows br0])
> >> +AT_CHECK([ovs-ofctl add-flow br0
> "vlan_tci=0x000a/0x0fff,action=output:local"])
> >> +
> >> +AT_CHECK([ovs-appctl netdev-dummy/receive p0
> 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x8100),vlan(vid=10,pcp=0),encap(eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0))'])
> >> +
> >> +AT_CHECK([cat ovs-vswitchd.log | grep 'in_port=[[1]]' |
> FILTER_FLOW_INSTALL | STRIP_XOUT], [0], [dnl
> >> +recirc_id=0,ip,in_port=1,vlan_tci=0x100a/0x0fff,nw_frag=no, actions:
> <del>
> >> +])
> >> +OVS_VSWITCHD_STOP
> >> +AT_CLEANUP
> >> \ No newline at end of file
> >> --
> >> 1.7.9.5
> >>
> >> _______________________________________________
> >> dev mailing list
> >> dev@openvswitch.org
> >> http://openvswitch.org/mailman/listinfo/dev
> >
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
