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
