[ovs-dev] [PATCH V2] odp-util: Make sure vlan tci mask has exact match for VLAN_CFI.

[Jarno Rajahalme]

Two small nits, otherwise:

Acked-by: Jarno Rajahalme <jrajahalme at nicira.com> 

  Jarno


> On Jun 6, 2015, at 23:37, Alex Wang <alexw at 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)".
> 

I'd think exact match on hex 'a' would yield vid=10 instead of 5.

> With the OVS datapath check, the installation of such datapath
> flow will be rejected with:
> "|WARN|system at 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 <ronaldlee at vmware.com>
> Signed-off-by: Alex Wang <alexw at nicira.com>
> 
> ---
> PATCH->V2:
> - Fix careless testing.
> - Adopt Jarno's suggestions.
> ---
> ofproto/ofproto-dpif-xlate.c |    4 ++++
> tests/ofproto-dpif.at        |   16 ++++++++++++++++
> 2 files changed, 20 insertions(+)
> 
> 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..ef7c5c0 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
> \ No newline at end of file

Maybe add the new line to the end of file?

> -- 
> 1.7.9.5
>