From 8c301900fc6f7faface4a2cbd016411f966d0601 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Wed, 11 Dec 2013 11:07:01 -0800 Subject: [PATCH] dpif-netdev: Properly create exact match masks. Normally OVS userspace supplies a mask along with a flow key for each new data path flow that should be created. OVS also provides an option to disable the kernel wildcarding, in which case the flows are created without a mask. When kernel wildcarding is disabled, the datapath should use exact match, i.e. not wildcard any bits in the flow key. Currently, what happens with the userspace datapath instead is that a datapath flow with mostly empty mask is created (i.e., most fields are wildcarded), as the current code does not examine the given mask key length to find out that the mask key is actually empty. This results in the same datapath flow matching on packets of multiple different flows, wrong actions being processed, and stats being incorrect. This patch refactors userspace datapath code to explicitly initialize a suitable exact match mask when a flow put without a mask is executed. Signed-off-by: Jarno Rajahalme Acked-by: Ben Pfaff --- lib/dpif-netdev.c | 92 ++++++++++++++++++++++++++++++------------- tests/ofproto-dpif.at | 22 +++++++++++ 2 files changed, 86 insertions(+), 28 deletions(-) diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 911cb5dc1..20f4a9e22 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -40,6 +40,7 @@ #include "flow.h" #include "hmap.h" #include "list.h" +#include "meta-flow.h" #include "netdev.h" #include "netdev-vport.h" #include "netlink.h" @@ -788,29 +789,72 @@ get_dpif_flow_stats(struct dp_netdev_flow *netdev_flow, } static int -dpif_netdev_flow_mask_from_nlattrs(const struct nlattr *key, uint32_t key_len, - const struct nlattr *mask_key, - uint32_t mask_key_len, struct flow *flow, - struct flow *mask) +dpif_netdev_mask_from_nlattrs(const struct nlattr *key, uint32_t key_len, + const struct nlattr *mask_key, + uint32_t mask_key_len, const struct flow *flow, + struct flow *mask) +{ + if (mask_key_len) { + if (odp_flow_key_to_mask(mask_key, mask_key_len, mask, flow)) { + /* This should not happen: it indicates that + * odp_flow_key_from_mask() and odp_flow_key_to_mask() + * disagree on the acceptable form of a mask. Log the problem + * as an error, with enough details to enable debugging. */ + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); + + if (!VLOG_DROP_ERR(&rl)) { + struct ds s; + + ds_init(&s); + odp_flow_format(key, key_len, mask_key, mask_key_len, NULL, &s, + true); + VLOG_ERR("internal error parsing flow mask %s", ds_cstr(&s)); + ds_destroy(&s); + } + + return EINVAL; + } + /* Force unwildcard the in_port. */ + mask->in_port.odp_port = u32_to_odp(UINT32_MAX); + } else { + enum mf_field_id id; + /* No mask key, unwildcard everything except fields whose + * prerequisities are not met. */ + memset(mask, 0x0, sizeof *mask); + + for (id = 0; id < MFF_N_IDS; ++id) { + /* Skip registers and metadata. */ + if (!(id >= MFF_REG0 && id < MFF_REG0 + FLOW_N_REGS) + && id != MFF_METADATA) { + const struct mf_field *mf = mf_from_id(id); + if (mf_are_prereqs_ok(mf, flow)) { + mf_mask_field(mf, mask); + } + } + } + } + + return 0; +} + +static int +dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len, + struct flow *flow) { odp_port_t in_port; - if (odp_flow_key_to_flow(key, key_len, flow) - || (mask_key - && odp_flow_key_to_mask(mask_key, mask_key_len, mask, flow))) { + if (odp_flow_key_to_flow(key, key_len, flow)) { /* This should not happen: it indicates that odp_flow_key_from_flow() - * and odp_flow_key_to_flow() disagree on the acceptable form of a flow - * or odp_flow_key_from_mask() and odp_flow_key_to_mask() disagree on - * the acceptable form of a mask. Log the problem as an error, with - * enough details to enable debugging. */ + * and odp_flow_key_to_flow() disagree on the acceptable form of a + * flow. Log the problem as an error, with enough details to enable + * debugging. */ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); if (!VLOG_DROP_ERR(&rl)) { struct ds s; ds_init(&s); - odp_flow_format(key, key_len, mask_key, mask_key_len, NULL, &s, - true); + odp_flow_format(key, key_len, NULL, 0, NULL, &s, true); VLOG_ERR("internal error parsing flow key %s", ds_cstr(&s)); ds_destroy(&s); } @@ -818,11 +862,6 @@ dpif_netdev_flow_mask_from_nlattrs(const struct nlattr *key, uint32_t key_len, return EINVAL; } - if (mask_key) { - /* Force unwildcard the in_port. */ - mask->in_port.odp_port = u32_to_odp(UINT32_MAX); - } - in_port = flow->in_port.odp_port; if (!is_valid_port_number(in_port) && in_port != ODPP_NONE) { return EINVAL; @@ -831,14 +870,6 @@ dpif_netdev_flow_mask_from_nlattrs(const struct nlattr *key, uint32_t key_len, return 0; } -static int -dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len, - struct flow *flow) -{ - return dpif_netdev_flow_mask_from_nlattrs(key, key_len, NULL, 0, flow, - NULL); -} - static int dpif_netdev_flow_get(const struct dpif *dpif, const struct nlattr *nl_key, size_t nl_key_len, @@ -934,8 +965,13 @@ dpif_netdev_flow_put(struct dpif *dpif, const struct dpif_flow_put *put) struct flow_wildcards wc; int error; - error = dpif_netdev_flow_mask_from_nlattrs(put->key, put->key_len, - put->mask, put->mask_len, &flow, &wc.masks); + error = dpif_netdev_flow_from_nlattrs(put->key, put->key_len, &flow); + if (error) { + return error; + } + error = dpif_netdev_mask_from_nlattrs(put->key, put->key_len, + put->mask, put->mask_len, + &flow, &wc.masks); if (error) { return error; } diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 4d8d4602d..f38257116 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -2953,6 +2953,28 @@ skb_priority(0),in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_ OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif megaflow - disabled]) +OVS_VSWITCHD_START +ADD_OF_PORTS([br0], [1], [2]) +AT_DATA([flows.txt], [dnl +table=0 in_port=1,ip,nw_dst=10.0.0.1 actions=output(2) +table=0 in_port=1,ip,nw_dst=10.0.0.3 actions=drop +]) +AT_CHECK([ovs-appctl dpif/disable-megaflows], [0], [megaflows disabled +], []) +AT_CHECK([ovs-appctl vlog/set dpif_netdev:dbg], [0], [], []) +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) +for i in 1 2 3 4; do + AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),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([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) +done +AT_CHECK([ovs-appctl dpif/dump-flows br0 | STRIP_USED], [0], [dnl +skb_priority(0),skb_mark(0),in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),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), packets:3, bytes:180, used:0.0s, actions:2 +skb_priority(0),skb_mark(0),in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0), packets:3, bytes:180, used:0.0s, actions:drop +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([ofproto-dpif - datapath port number change]) OVS_VSWITCHD_START([set Bridge br0 fail-mode=standalone]) ADD_OF_PORTS([br0], 1) -- 2.47.0