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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> Signed-off-by: Alex Wang <[email protected]>
>> ---
>> 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
>> [email protected]
>> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
