[ovs-dev] [PATCH] odp-util: Make sure vlan tci mask has exact match for VLAN_CFI.
Jarno Rajahalme
jrajahalme at nicira.com
Sat Jun 6 18:38:18 UTC 2015
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 <jrajahalme at 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 <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)".
>>
>> 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>
>> ---
>> 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 at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>
More information about the dev
mailing list