From 8de6ff3ea8642b29952949e6bcd532d1441b48c0 Mon Sep 17 00:00:00 2001 From: Justin Pettit Date: Thu, 4 Jan 2018 12:37:57 -0800 Subject: [PATCH] ofproto-dpif: Use a fixed size userspace cookie. This simplifies the cookie handling a bit. Signed-off-by: Justin Pettit Acked-by: Ben Pfaff --- lib/odp-util.c | 35 +++++++-------------- lib/odp-util.h | 58 ++++++++++++++++++----------------- ofproto/ofproto-dpif-ipfix.c | 2 +- ofproto/ofproto-dpif-ipfix.h | 3 +- ofproto/ofproto-dpif-sflow.c | 14 ++++----- ofproto/ofproto-dpif-sflow.h | 8 ++--- ofproto/ofproto-dpif-upcall.c | 53 ++++++++++++++++---------------- ofproto/ofproto-dpif-xlate.c | 50 +++++++++++++----------------- tests/odp.at | 2 -- 9 files changed, 103 insertions(+), 122 deletions(-) diff --git a/lib/odp-util.c b/lib/odp-util.c index ff08821595f..2910e151498 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -437,31 +437,25 @@ format_odp_userspace_action(struct ds *ds, const struct nlattr *attr, const uint8_t *userdata = nl_attr_get(userdata_attr); size_t userdata_len = nl_attr_get_size(userdata_attr); bool userdata_unspec = true; - union user_action_cookie cookie; + struct user_action_cookie cookie; - if (userdata_len >= sizeof cookie.type - && userdata_len <= sizeof cookie) { - - memset(&cookie, 0, sizeof cookie); - memcpy(&cookie, userdata, userdata_len); + if (userdata_len == sizeof cookie) { + memcpy(&cookie, userdata, sizeof cookie); userdata_unspec = false; - if (userdata_len == sizeof cookie.sflow - && cookie.type == USER_ACTION_COOKIE_SFLOW) { + if (cookie.type == USER_ACTION_COOKIE_SFLOW) { ds_put_format(ds, ",sFlow(" "vid=%"PRIu16",pcp=%d,output=%"PRIu32")", vlan_tci_to_vid(cookie.sflow.vlan_tci), vlan_tci_to_pcp(cookie.sflow.vlan_tci), cookie.sflow.output); - } else if (userdata_len == sizeof cookie.slow_path - && cookie.type == USER_ACTION_COOKIE_SLOW_PATH) { + } else if (cookie.type == USER_ACTION_COOKIE_SLOW_PATH) { ds_put_cstr(ds, ",slow_path("); format_flags(ds, slow_path_reason_to_string, cookie.slow_path.reason, ','); ds_put_format(ds, ")"); - } else if (userdata_len == sizeof cookie.flow_sample - && cookie.type == USER_ACTION_COOKIE_FLOW_SAMPLE) { + } else if (cookie.type == USER_ACTION_COOKIE_FLOW_SAMPLE) { ds_put_format(ds, ",flow_sample(probability=%"PRIu16 ",collector_set_id=%"PRIu32 ",obs_domain_id=%"PRIu32 @@ -479,8 +473,7 @@ format_odp_userspace_action(struct ds *ds, const struct nlattr *attr, ds_put_cstr(ds, ",egress"); } ds_put_char(ds, ')'); - } else if (userdata_len >= sizeof cookie.ipfix - && cookie.type == USER_ACTION_COOKIE_IPFIX) { + } else if (cookie.type == USER_ACTION_COOKIE_IPFIX) { ds_put_format(ds, ",ipfix(output_port="); odp_portno_name_format(portno_names, cookie.ipfix.output_odp_port, ds); @@ -1111,7 +1104,7 @@ static int parse_odp_userspace_action(const char *s, struct ofpbuf *actions) { uint32_t pid; - union user_action_cookie cookie; + struct user_action_cookie cookie; struct ofpbuf buf; odp_port_t tunnel_out_port; int n = -1; @@ -1125,7 +1118,10 @@ parse_odp_userspace_action(const char *s, struct ofpbuf *actions) } ofpbuf_init(&buf, 16); + memset(&cookie, 0, sizeof cookie); + user_data = &cookie; + user_data_size = sizeof cookie; { uint32_t output; uint32_t probability; @@ -1148,8 +1144,6 @@ parse_odp_userspace_action(const char *s, struct ofpbuf *actions) cookie.type = USER_ACTION_COOKIE_SFLOW; cookie.sflow.vlan_tci = htons(tci); cookie.sflow.output = output; - user_data = &cookie; - user_data_size = sizeof cookie.sflow; } else if (ovs_scan(&s[n], ",slow_path(%n", &n1)) { n += n1; @@ -1164,9 +1158,6 @@ parse_odp_userspace_action(const char *s, struct ofpbuf *actions) goto out; } n += res + 1; - - user_data = &cookie; - user_data_size = sizeof cookie.slow_path; } else if (ovs_scan(&s[n], ",flow_sample(probability=%"SCNi32"," "collector_set_id=%"SCNi32"," "obs_domain_id=%"SCNi32"," @@ -1183,8 +1174,6 @@ parse_odp_userspace_action(const char *s, struct ofpbuf *actions) cookie.flow_sample.obs_domain_id = obs_domain_id; cookie.flow_sample.obs_point_id = obs_point_id; cookie.flow_sample.output_odp_port = u32_to_odp(output); - user_data = &cookie; - user_data_size = sizeof cookie.flow_sample; if (ovs_scan(&s[n], ",ingress%n", &n1)) { cookie.flow_sample.direction = NX_ACTION_SAMPLE_INGRESS; @@ -1205,8 +1194,6 @@ parse_odp_userspace_action(const char *s, struct ofpbuf *actions) n += n1; cookie.type = USER_ACTION_COOKIE_IPFIX; cookie.ipfix.output_odp_port = u32_to_odp(output); - user_data = &cookie; - user_data_size = sizeof cookie.ipfix; } else if (ovs_scan(&s[n], ",userdata(%n", &n1)) { char *end; diff --git a/lib/odp-util.h b/lib/odp-util.h index f7ce206510f..b08ff719016 100644 --- a/lib/odp-util.h +++ b/lib/odp-util.h @@ -300,37 +300,39 @@ enum user_action_cookie_type { }; /* user_action_cookie is passed as argument to OVS_ACTION_ATTR_USERSPACE. */ -union user_action_cookie { +struct user_action_cookie { uint16_t type; /* enum user_action_cookie_type. */ - struct { - uint16_t type; /* USER_ACTION_COOKIE_SFLOW. */ - ovs_be16 vlan_tci; /* Destination VLAN TCI. */ - uint32_t output; /* SFL_FLOW_SAMPLE_TYPE 'output' value. */ - } sflow; - - struct { - uint16_t type; /* USER_ACTION_COOKIE_SLOW_PATH. */ - uint16_t unused; - uint32_t reason; /* enum slow_path_reason. */ - } slow_path; - - struct { - uint16_t type; /* USER_ACTION_COOKIE_FLOW_SAMPLE. */ - uint16_t probability; /* Sampling probability. */ - uint32_t collector_set_id; /* ID of IPFIX collector set. */ - uint32_t obs_domain_id; /* Observation Domain ID. */ - uint32_t obs_point_id; /* Observation Point ID. */ - odp_port_t output_odp_port; /* The output odp port. */ - enum nx_action_sample_direction direction; - } flow_sample; - - struct { - uint16_t type; /* USER_ACTION_COOKIE_IPFIX. */ - odp_port_t output_odp_port; /* The output odp port. */ - } ipfix; + union { + struct { + /* USER_ACTION_COOKIE_SFLOW. */ + ovs_be16 vlan_tci; /* Destination VLAN TCI. */ + uint32_t output; /* SFL_FLOW_SAMPLE_TYPE 'output' value. */ + } sflow; + + struct { + /* USER_ACTION_COOKIE_SLOW_PATH. */ + uint16_t unused; + uint32_t reason; /* enum slow_path_reason. */ + } slow_path; + + struct { + /* USER_ACTION_COOKIE_FLOW_SAMPLE. */ + uint16_t probability; /* Sampling probability. */ + uint32_t collector_set_id; /* ID of IPFIX collector set. */ + uint32_t obs_domain_id; /* Observation Domain ID. */ + uint32_t obs_point_id; /* Observation Point ID. */ + odp_port_t output_odp_port; /* The output odp port. */ + enum nx_action_sample_direction direction; + } flow_sample; + + struct { + /* USER_ACTION_COOKIE_IPFIX. */ + odp_port_t output_odp_port; /* The output odp port. */ + } ipfix; + }; }; -BUILD_ASSERT_DECL(sizeof(union user_action_cookie) == 24); +BUILD_ASSERT_DECL(sizeof(struct user_action_cookie) == 28); size_t odp_put_userspace_action(uint32_t pid, const void *userdata, size_t userdata_size, diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c index f691c700ec3..a420903e9dd 100644 --- a/ofproto/ofproto-dpif-ipfix.c +++ b/ofproto/ofproto-dpif-ipfix.c @@ -2732,7 +2732,7 @@ dpif_ipfix_bridge_sample(struct dpif_ipfix *di, const struct dp_packet *packet, void dpif_ipfix_flow_sample(struct dpif_ipfix *di, const struct dp_packet *packet, const struct flow *flow, - const union user_action_cookie *cookie, + const struct user_action_cookie *cookie, odp_port_t input_odp_port, const struct flow_tnl *output_tunnel_key, const struct dpif_ipfix_actions *ipfix_actions) diff --git a/ofproto/ofproto-dpif-ipfix.h b/ofproto/ofproto-dpif-ipfix.h index 1309da1fce4..1f42cd5275e 100644 --- a/ofproto/ofproto-dpif-ipfix.h +++ b/ofproto/ofproto-dpif-ipfix.h @@ -60,7 +60,8 @@ void dpif_ipfix_bridge_sample(struct dpif_ipfix *, const struct dp_packet *, odp_port_t, odp_port_t, const struct flow_tnl *, const struct dpif_ipfix_actions *); void dpif_ipfix_flow_sample(struct dpif_ipfix *, const struct dp_packet *, - const struct flow *, const union user_action_cookie *, + const struct flow *, + const struct user_action_cookie *, odp_port_t, const struct flow_tnl *, const struct dpif_ipfix_actions *); diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c index 2be586998cf..e30a411f5a6 100644 --- a/ofproto/ofproto-dpif-sflow.c +++ b/ofproto/ofproto-dpif-sflow.c @@ -1233,7 +1233,7 @@ dpif_sflow_encode_mpls_stack(SFLLabelStack *stack, * See http://sflow.org/sflow_version_5.txt "Input/Output port information" */ static uint32_t -dpif_sflow_cookie_num_outputs(const union user_action_cookie *cookie) +dpif_sflow_cookie_num_outputs(const struct user_action_cookie *cookie) { uint32_t format = cookie->sflow.output & 0xC0000000; uint32_t port_n = cookie->sflow.output & 0x3FFFFFFF; @@ -1248,9 +1248,9 @@ dpif_sflow_cookie_num_outputs(const union user_action_cookie *cookie) void dpif_sflow_received(struct dpif_sflow *ds, const struct dp_packet *packet, - const struct flow *flow, odp_port_t odp_in_port, - const union user_action_cookie *cookie, - const struct dpif_sflow_actions *sflow_actions) + const struct flow *flow, odp_port_t odp_in_port, + const struct user_action_cookie *cookie, + const struct dpif_sflow_actions *sflow_actions) OVS_EXCLUDED(mutex) { SFL_FLOW_SAMPLE_TYPE fs; @@ -1283,9 +1283,9 @@ dpif_sflow_received(struct dpif_sflow *ds, const struct dp_packet *packet, fs.input = SFL_DS_INDEX(in_dsp->dsi); } - /* Make the assumption that the random number generator in the datapath converges - * to the configured mean, and just increment the samplePool by the configured - * sampling rate every time. */ + /* Make the assumption that the random number generator in the + * datapath converges to the configured mean, and just increment the + * samplePool by the configured sampling rate every time. */ sampler->samplePool += sfl_sampler_get_sFlowFsPacketSamplingRate(sampler); /* Sampled header. */ diff --git a/ofproto/ofproto-dpif-sflow.h b/ofproto/ofproto-dpif-sflow.h index 014e6cce39c..74fe58007af 100644 --- a/ofproto/ofproto-dpif-sflow.h +++ b/ofproto/ofproto-dpif-sflow.h @@ -70,13 +70,13 @@ void dpif_sflow_run(struct dpif_sflow *); void dpif_sflow_wait(struct dpif_sflow *); void dpif_sflow_read_actions(const struct flow *, - const struct nlattr *actions, size_t actions_len, - struct dpif_sflow_actions *); + const struct nlattr *actions, size_t actions_len, + struct dpif_sflow_actions *); void dpif_sflow_received(struct dpif_sflow *, const struct dp_packet *, const struct flow *, odp_port_t odp_port, - const union user_action_cookie *, - const struct dpif_sflow_actions *); + const struct user_action_cookie *, + const struct dpif_sflow_actions *); int dpif_sflow_odp_port_to_ifindex(const struct dpif_sflow *, odp_port_t odp_port); diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 02cf5415bc3..ddae02dabb3 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -971,9 +971,6 @@ udpif_revalidator(void *arg) static enum upcall_type classify_upcall(enum dpif_upcall_type type, const struct nlattr *userdata) { - union user_action_cookie cookie; - size_t userdata_len; - /* First look at the upcall type. */ switch (type) { case DPIF_UC_ACTION: @@ -993,26 +990,22 @@ classify_upcall(enum dpif_upcall_type type, const struct nlattr *userdata) VLOG_WARN_RL(&rl, "action upcall missing cookie"); return BAD_UPCALL; } - userdata_len = nl_attr_get_size(userdata); - if (userdata_len < sizeof cookie.type - || userdata_len > sizeof cookie) { + + struct user_action_cookie cookie; + size_t userdata_len = nl_attr_get_size(userdata); + if (userdata_len != sizeof cookie) { VLOG_WARN_RL(&rl, "action upcall cookie has unexpected size %"PRIuSIZE, userdata_len); return BAD_UPCALL; } - memset(&cookie, 0, sizeof cookie); - memcpy(&cookie, nl_attr_get(userdata), userdata_len); - if (userdata_len == MAX(8, sizeof cookie.sflow) - && cookie.type == USER_ACTION_COOKIE_SFLOW) { + memcpy(&cookie, nl_attr_get(userdata), sizeof cookie); + if (cookie.type == USER_ACTION_COOKIE_SFLOW) { return SFLOW_UPCALL; - } else if (userdata_len == MAX(8, sizeof cookie.slow_path) - && cookie.type == USER_ACTION_COOKIE_SLOW_PATH) { + } else if (cookie.type == USER_ACTION_COOKIE_SLOW_PATH) { return MISS_UPCALL; - } else if (userdata_len == MAX(8, sizeof cookie.flow_sample) - && cookie.type == USER_ACTION_COOKIE_FLOW_SAMPLE) { + } else if (cookie.type == USER_ACTION_COOKIE_FLOW_SAMPLE) { return FLOW_SAMPLE_UPCALL; - } else if (userdata_len == MAX(8, sizeof cookie.ipfix) - && cookie.type == USER_ACTION_COOKIE_IPFIX) { + } else if (cookie.type == USER_ACTION_COOKIE_IPFIX) { return IPFIX_UPCALL; } else { VLOG_WARN_RL(&rl, "invalid user cookie of type %"PRIu16 @@ -1029,7 +1022,7 @@ compose_slow_path(struct udpif *udpif, struct xlate_out *xout, struct ofpbuf *buf, uint32_t slowpath_meter_id, uint32_t controller_meter_id) { - union user_action_cookie cookie; + struct user_action_cookie cookie; odp_port_t port; uint32_t pid; @@ -1056,7 +1049,7 @@ compose_slow_path(struct udpif *udpif, struct xlate_out *xout, nl_msg_put_u32(buf, OVS_ACTION_ATTR_METER, meter_id); } - odp_put_userspace_action(pid, &cookie, sizeof cookie.slow_path, + odp_put_userspace_action(pid, &cookie, sizeof cookie, ODPP_NONE, false, buf); if (meter_id != UINT32_MAX) { @@ -1349,12 +1342,14 @@ process_upcall(struct udpif *udpif, struct upcall *upcall, case SFLOW_UPCALL: if (upcall->sflow) { - union user_action_cookie cookie; + struct user_action_cookie cookie; struct dpif_sflow_actions sflow_actions; + if (nl_attr_get_size(userdata) != sizeof cookie) { + return EINVAL; + } + memcpy(&cookie, nl_attr_get(userdata), sizeof cookie); memset(&sflow_actions, 0, sizeof sflow_actions); - memset(&cookie, 0, sizeof cookie); - memcpy(&cookie, nl_attr_get(userdata), sizeof cookie.sflow); actions_len = dpif_read_actions(udpif, upcall, flow, upcall_type, &sflow_actions); @@ -1366,12 +1361,14 @@ process_upcall(struct udpif *udpif, struct upcall *upcall, case IPFIX_UPCALL: if (upcall->ipfix) { - union user_action_cookie cookie; + struct user_action_cookie cookie; struct flow_tnl output_tunnel_key; struct dpif_ipfix_actions ipfix_actions; - memset(&cookie, 0, sizeof cookie); - memcpy(&cookie, nl_attr_get(userdata), sizeof cookie.ipfix); + if (nl_attr_get_size(userdata) != sizeof cookie) { + return EINVAL; + } + memcpy(&cookie, nl_attr_get(userdata), sizeof cookie); memset(&ipfix_actions, 0, sizeof ipfix_actions); if (upcall->out_tun_key) { @@ -1391,12 +1388,14 @@ process_upcall(struct udpif *udpif, struct upcall *upcall, case FLOW_SAMPLE_UPCALL: if (upcall->ipfix) { - union user_action_cookie cookie; + struct user_action_cookie cookie; struct flow_tnl output_tunnel_key; struct dpif_ipfix_actions ipfix_actions; - memset(&cookie, 0, sizeof cookie); - memcpy(&cookie, nl_attr_get(userdata), sizeof cookie.flow_sample); + if (nl_attr_get_size(userdata) != sizeof cookie) { + return EINVAL; + } + memcpy(&cookie, nl_attr_get(userdata), sizeof cookie); memset(&ipfix_actions, 0, sizeof ipfix_actions); if (upcall->out_tun_key) { diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index c77477c9641..c7fe197b384 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -2853,19 +2853,18 @@ xlate_normal(struct xlate_ctx *ctx) /* Appends a "sample" action for sFlow or IPFIX to 'ctx->odp_actions'. The * 'probability' is the number of packets out of UINT32_MAX to sample. The - * 'cookie' (of length 'cookie_size' bytes) is passed back in the callback for - * each sampled packet. 'tunnel_out_port', if not ODPP_NONE, is added as the - * OVS_USERSPACE_ATTR_EGRESS_TUN_PORT attribute. If 'include_actions', an - * OVS_USERSPACE_ATTR_ACTIONS attribute is added. If 'emit_set_tunnel', - * sample(sampling_port=1) would translate into datapath sample action - * set(tunnel(...)), sample(...) and it is used for sampling egress tunnel - * information. + * 'cookie' is passed back in the callback for each sampled packet. + * 'tunnel_out_port', if not ODPP_NONE, is added as the + * OVS_USERSPACE_ATTR_EGRESS_TUN_PORT attribute. If 'include_actions', + * an OVS_USERSPACE_ATTR_ACTIONS attribute is added. If + * 'emit_set_tunnel', sample(sampling_port=1) would translate into + * datapath sample action set(tunnel(...)), sample(...) and it is used + * for sampling egress tunnel information. */ static size_t compose_sample_action(struct xlate_ctx *ctx, const uint32_t probability, - const union user_action_cookie *cookie, - const size_t cookie_size, + const struct user_action_cookie *cookie, const odp_port_t tunnel_out_port, bool include_actions) { @@ -2900,7 +2899,7 @@ compose_sample_action(struct xlate_ctx *ctx, ctx->xbridge, ctx->xin->flow.in_port.ofp_port); uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port, flow_hash_5tuple(&ctx->xin->flow, 0)); - int cookie_offset = odp_put_userspace_action(pid, cookie, cookie_size, + int cookie_offset = odp_put_userspace_action(pid, cookie, sizeof *cookie, tunnel_out_port, include_actions, ctx->odp_actions); @@ -2928,10 +2927,9 @@ compose_sflow_action(struct xlate_ctx *ctx) return 0; } - union user_action_cookie cookie = { .type = USER_ACTION_COOKIE_SFLOW }; + struct user_action_cookie cookie = { .type = USER_ACTION_COOKIE_SFLOW }; return compose_sample_action(ctx, dpif_sflow_get_probability(sflow), - &cookie, sizeof cookie.sflow, ODPP_NONE, - true); + &cookie, ODPP_NONE, true); } /* If flow IPFIX is enabled, make sure IPFIX flow sample action @@ -2969,30 +2967,27 @@ compose_ipfix_action(struct xlate_ctx *ctx, odp_port_t output_odp_port) } } - union user_action_cookie cookie = { - .ipfix = { - .type = USER_ACTION_COOKIE_IPFIX, - .output_odp_port = output_odp_port, - } + struct user_action_cookie cookie = { + .type = USER_ACTION_COOKIE_IPFIX, + .ipfix.output_odp_port = output_odp_port, }; compose_sample_action(ctx, dpif_ipfix_get_bridge_exporter_probability(ipfix), - &cookie, sizeof cookie.ipfix, tunnel_out_port, - false); + &cookie, tunnel_out_port, false); } /* Fix "sample" action according to data collected while composing ODP actions, * as described in compose_sflow_action(). * - * 'user_cookie_offset' must be the offset returned by add_sflow_action(). */ + * 'user_cookie_offset' must be the offset returned by + * compose_sflow_action(). */ static void fix_sflow_action(struct xlate_ctx *ctx, unsigned int user_cookie_offset) { const struct flow *base = &ctx->base_flow; - union user_action_cookie *cookie; + struct user_action_cookie *cookie; - cookie = ofpbuf_at(ctx->odp_actions, user_cookie_offset, - sizeof cookie->sflow); + cookie = ofpbuf_at(ctx->odp_actions, user_cookie_offset, sizeof *cookie); ovs_assert(cookie->type == USER_ACTION_COOKIE_SFLOW); cookie->type = USER_ACTION_COOKIE_SFLOW; @@ -5273,9 +5268,9 @@ xlate_sample_action(struct xlate_ctx *ctx, } } - union user_action_cookie cookie = { + struct user_action_cookie cookie = { + .type = USER_ACTION_COOKIE_FLOW_SAMPLE, .flow_sample = { - .type = USER_ACTION_COOKIE_FLOW_SAMPLE, .probability = os->probability, .collector_set_id = os->collector_set_id, .obs_domain_id = os->obs_domain_id, @@ -5284,8 +5279,7 @@ xlate_sample_action(struct xlate_ctx *ctx, .direction = os->direction, } }; - compose_sample_action(ctx, probability, &cookie, sizeof cookie.flow_sample, - tunnel_out_port, false); + compose_sample_action(ctx, probability, &cookie, tunnel_out_port, false); } /* Determine if an datapath action translated from the openflow action diff --git a/tests/odp.at b/tests/odp.at index 1a80322890e..4891653eb81 100644 --- a/tests/odp.at +++ b/tests/odp.at @@ -246,8 +246,6 @@ AT_CLEANUP AT_SETUP([OVS datapath actions parsing and formatting - valid forms]) AT_DATA([actions.txt], [dnl 1,2,3 -userspace(pid=555666777) -userspace(pid=555666777,tunnel_out_port=10) userspace(pid=6633,sFlow(vid=9,pcp=7,output=10),actions) userspace(pid=6633,sFlow(vid=9,pcp=7,output=10),actions,tunnel_out_port=10) userspace(pid=9765,slow_path(0))