From e995e3df57ea4e27678bc0bea5eb30872994155b Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 15 Feb 2013 16:48:32 -0800 Subject: [PATCH] Allow OVS_USERSPACE_ATTR_USERDATA to be variable length. Until now, the optional OVS_USERSPACE_ATTR_USERDATA attribute had to be exactly 64 bits long, if it was present. However, 64 bits is not enough space to associate as much information with a flow as would be convenient for some userspace features now under development. This commit generalizes the attribute, allowing it to be any length. This generalization is backward-compatible: if userspace only uses 64-bit attributes, then it will not see any change in behavior. CC: Romain Lenglet Signed-off-by: Ben Pfaff Acked-by: Jesse Gross --- datapath/datapath.c | 11 +++--- datapath/datapath.h | 2 +- include/linux/openvswitch.h | 11 +++--- lib/dpif-linux.c | 6 +-- lib/dpif-netdev.c | 74 +++++++++++++++++++++---------------- lib/dpif.h | 10 ++--- lib/odp-util.c | 61 ++++++++++++++++++++++-------- lib/odp-util.h | 2 +- ofproto/ofproto-dpif.c | 20 +++++++--- tests/odp.at | 1 + 10 files changed, 125 insertions(+), 73 deletions(-) diff --git a/datapath/datapath.c b/datapath/datapath.c index 5d18def8f..0920a30a4 100644 --- a/datapath/datapath.c +++ b/datapath/datapath.c @@ -374,8 +374,8 @@ static int queue_userspace_packet(struct net *net, int dp_ifindex, len = sizeof(struct ovs_header); len += nla_total_size(skb->len); len += nla_total_size(FLOW_BUFSIZE); - if (upcall_info->cmd == OVS_PACKET_CMD_ACTION) - len += nla_total_size(8); + if (upcall_info->userdata) + len += NLA_ALIGN(upcall_info->userdata->nla_len); user_skb = genlmsg_new(len, GFP_ATOMIC); if (!user_skb) { @@ -392,8 +392,9 @@ static int queue_userspace_packet(struct net *net, int dp_ifindex, nla_nest_end(user_skb, nla); if (upcall_info->userdata) - nla_put_u64(user_skb, OVS_PACKET_ATTR_USERDATA, - nla_get_u64(upcall_info->userdata)); + __nla_put(user_skb, OVS_PACKET_ATTR_USERDATA, + upcall_info->userdata->nla_len - NLA_HDRLEN, + nla_data(upcall_info->userdata)); nla = __nla_reserve(user_skb, OVS_PACKET_ATTR_PACKET, skb->len); @@ -679,7 +680,7 @@ static int validate_userspace(const struct nlattr *attr) { static const struct nla_policy userspace_policy[OVS_USERSPACE_ATTR_MAX + 1] = { [OVS_USERSPACE_ATTR_PID] = {.type = NLA_U32 }, - [OVS_USERSPACE_ATTR_USERDATA] = {.type = NLA_U64 }, + [OVS_USERSPACE_ATTR_USERDATA] = {.type = NLA_UNSPEC }, }; struct nlattr *a[OVS_USERSPACE_ATTR_MAX + 1]; int error; diff --git a/datapath/datapath.h b/datapath/datapath.h index 2b9334835..9bc98fb76 100644 --- a/datapath/datapath.h +++ b/datapath/datapath.h @@ -120,7 +120,7 @@ struct ovs_skb_cb { * struct dp_upcall - metadata to include with a packet to send to userspace * @cmd: One of %OVS_PACKET_CMD_*. * @key: Becomes %OVS_PACKET_ATTR_KEY. Must be nonnull. - * @userdata: If nonnull, its u64 value is extracted and passed to userspace as + * @userdata: If nonnull, its variable-length value is passed to userspace as * %OVS_PACKET_ATTR_USERDATA. * @portid: Netlink PID to which packet should be sent. If @portid is 0 then no * packet is sent and the packet is accounted in the datapath's @n_lost diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h index 7c6e3abc6..07f088792 100644 --- a/include/linux/openvswitch.h +++ b/include/linux/openvswitch.h @@ -148,7 +148,8 @@ enum ovs_packet_cmd { * for %OVS_PACKET_CMD_EXECUTE. It has nested %OVS_ACTION_ATTR_* attributes. * @OVS_PACKET_ATTR_USERDATA: Present for an %OVS_PACKET_CMD_ACTION * notification if the %OVS_ACTION_ATTR_USERSPACE action specified an - * %OVS_USERSPACE_ATTR_USERDATA attribute. + * %OVS_USERSPACE_ATTR_USERDATA attribute, with the same length and content + * specified there. * * These attributes follow the &struct ovs_header within the Generic Netlink * payload for %OVS_PACKET_* commands. @@ -158,7 +159,7 @@ enum ovs_packet_attr { OVS_PACKET_ATTR_PACKET, /* Packet data. */ OVS_PACKET_ATTR_KEY, /* Nested OVS_KEY_ATTR_* attributes. */ OVS_PACKET_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_* attributes. */ - OVS_PACKET_ATTR_USERDATA, /* u64 OVS_ACTION_ATTR_USERSPACE arg. */ + OVS_PACKET_ATTR_USERDATA, /* OVS_ACTION_ATTR_USERSPACE arg. */ __OVS_PACKET_ATTR_MAX }; @@ -448,13 +449,13 @@ enum ovs_sample_attr { * enum ovs_userspace_attr - Attributes for %OVS_ACTION_ATTR_USERSPACE action. * @OVS_USERSPACE_ATTR_PID: u32 Netlink PID to which the %OVS_PACKET_CMD_ACTION * message should be sent. Required. - * @OVS_USERSPACE_ATTR_USERDATA: If present, its u64 argument is copied to the - * %OVS_PACKET_CMD_ACTION message as %OVS_PACKET_ATTR_USERDATA, + * @OVS_USERSPACE_ATTR_USERDATA: If present, its variable-length argument is + * copied to the %OVS_PACKET_CMD_ACTION message as %OVS_PACKET_ATTR_USERDATA. */ enum ovs_userspace_attr { OVS_USERSPACE_ATTR_UNSPEC, OVS_USERSPACE_ATTR_PID, /* u32 Netlink PID to receive upcalls. */ - OVS_USERSPACE_ATTR_USERDATA, /* u64 optional user-specified cookie. */ + OVS_USERSPACE_ATTR_USERDATA, /* Optional user-specified cookie. */ __OVS_USERSPACE_ATTR_MAX }; diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c index aa1739d0a..6a2afd364 100644 --- a/lib/dpif-linux.c +++ b/lib/dpif-linux.c @@ -1237,7 +1237,7 @@ parse_odp_packet(struct ofpbuf *buf, struct dpif_upcall *upcall, [OVS_PACKET_ATTR_KEY] = { .type = NL_A_NESTED }, /* OVS_PACKET_CMD_ACTION only. */ - [OVS_PACKET_ATTR_USERDATA] = { .type = NL_A_U64, .optional = true }, + [OVS_PACKET_ATTR_USERDATA] = { .type = NL_A_UNSPEC, .optional = true }, }; struct ovs_header *ovs_header; @@ -1275,9 +1275,7 @@ parse_odp_packet(struct ofpbuf *buf, struct dpif_upcall *upcall, upcall->key = CONST_CAST(struct nlattr *, nl_attr_get(a[OVS_PACKET_ATTR_KEY])); upcall->key_len = nl_attr_get_size(a[OVS_PACKET_ATTR_KEY]); - upcall->userdata = (a[OVS_PACKET_ATTR_USERDATA] - ? nl_attr_get_u64(a[OVS_PACKET_ATTR_USERDATA]) - : 0); + upcall->userdata = a[OVS_PACKET_ATTR_USERDATA]; *dp_ifindex = ovs_header->dp_ifindex; return 0; diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index d315b5941..305e196be 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -151,7 +151,7 @@ static int dpif_netdev_open(const struct dpif_class *, const char *name, bool create, struct dpif **); static int dp_netdev_output_userspace(struct dp_netdev *, const struct ofpbuf *, int queue_no, const struct flow *, - uint64_t arg); + const struct nlattr *userdata); static void dp_netdev_execute_actions(struct dp_netdev *, struct ofpbuf *, struct flow *, const struct nlattr *actions, @@ -1043,7 +1043,7 @@ dp_netdev_port_input(struct dp_netdev *dp, struct dp_netdev_port *port, dp->n_hit++; } else { dp->n_missed++; - dp_netdev_output_userspace(dp, packet, DPIF_UC_MISS, &key, 0); + dp_netdev_output_userspace(dp, packet, DPIF_UC_MISS, &key, NULL); } } @@ -1108,37 +1108,51 @@ dp_netdev_output_port(struct dp_netdev *dp, struct ofpbuf *packet, static int dp_netdev_output_userspace(struct dp_netdev *dp, const struct ofpbuf *packet, - int queue_no, const struct flow *flow, uint64_t arg) + int queue_no, const struct flow *flow, + const struct nlattr *userdata) { struct dp_netdev_queue *q = &dp->queues[queue_no]; - struct dp_netdev_upcall *u; - struct dpif_upcall *upcall; - struct ofpbuf *buf; - size_t key_len; - - if (q->head - q->tail >= MAX_QUEUE_LEN) { - dp->n_lost++; - return ENOBUFS; - } + if (q->head - q->tail < MAX_QUEUE_LEN) { + struct dp_netdev_upcall *u = &q->upcalls[q->head++ & QUEUE_MASK]; + struct dpif_upcall *upcall = &u->upcall; + struct ofpbuf *buf = &u->buf; + size_t buf_size; + + upcall->type = queue_no; + + /* Allocate buffer big enough for everything. */ + buf_size = ODPUTIL_FLOW_KEY_BYTES + 2 + packet->size; + if (userdata) { + buf_size += NLA_ALIGN(userdata->nla_len); + } + ofpbuf_init(buf, buf_size); - u = &q->upcalls[q->head++ & QUEUE_MASK]; + /* Put ODP flow. */ + odp_flow_key_from_flow(buf, flow, flow->in_port); + upcall->key = buf->data; + upcall->key_len = buf->size; - buf = &u->buf; - ofpbuf_init(buf, ODPUTIL_FLOW_KEY_BYTES + 2 + packet->size); - odp_flow_key_from_flow(buf, flow, flow->in_port); - key_len = buf->size; - ofpbuf_pull(buf, key_len); - ofpbuf_reserve(buf, 2); - ofpbuf_put(buf, packet->data, packet->size); + /* Put userdata. */ + if (userdata) { + upcall->userdata = ofpbuf_put(buf, userdata, + NLA_ALIGN(userdata->nla_len)); + } - upcall = &u->upcall; - upcall->type = queue_no; - upcall->packet = buf; - upcall->key = buf->base; - upcall->key_len = key_len; - upcall->userdata = arg; + /* Put packet. + * + * We adjust 'data' and 'size' in 'buf' so that only the packet itself + * is visible in 'upcall->packet'. The ODP flow and (if present) + * userdata become part of the headroom. */ + ofpbuf_put_zeros(buf, 2); + buf->data = ofpbuf_put(buf, packet->data, packet->size); + buf->size = packet->size; + upcall->packet = buf; - return 0; + return 0; + } else { + dp->n_lost++; + return ENOBUFS; + } } static void @@ -1180,11 +1194,9 @@ dp_netdev_action_userspace(struct dp_netdev *dp, struct ofpbuf *packet, struct flow *key, const struct nlattr *a) { - const struct nlattr *userdata_attr; - uint64_t userdata; + const struct nlattr *userdata; - userdata_attr = nl_attr_find_nested(a, OVS_USERSPACE_ATTR_USERDATA); - userdata = userdata_attr ? nl_attr_get_u64(userdata_attr) : 0; + userdata = nl_attr_find_nested(a, OVS_USERSPACE_ATTR_USERDATA); dp_netdev_output_userspace(dp, packet, DPIF_UC_ACTION, key, userdata); } diff --git a/lib/dpif.h b/lib/dpif.h index c5e3fc810..fd05b2f20 100644 --- a/lib/dpif.h +++ b/lib/dpif.h @@ -540,10 +540,10 @@ const char *dpif_upcall_type_to_string(enum dpif_upcall_type); /* A packet passed up from the datapath to userspace. * - * If 'key' or 'actions' is nonnull, then it points into data owned by - * 'packet', so their memory cannot be freed separately. (This is hardly a - * great way to do things but it works out OK for the dpif providers and - * clients that exist so far.) + * If 'key', 'actions', or 'userdata' is nonnull, then it points into data + * owned by 'packet', so their memory cannot be freed separately. (This is + * hardly a great way to do things but it works out OK for the dpif providers + * and clients that exist so far.) */ struct dpif_upcall { /* All types. */ @@ -553,7 +553,7 @@ struct dpif_upcall { size_t key_len; /* Length of 'key' in bytes. */ /* DPIF_UC_ACTION only. */ - uint64_t userdata; /* Argument to OVS_ACTION_ATTR_USERSPACE. */ + struct nlattr *userdata; /* Argument to OVS_ACTION_ATTR_USERSPACE. */ }; int dpif_recv_set(struct dpif *, bool enable); diff --git a/lib/odp-util.c b/lib/odp-util.c index f3f66b7cc..54a240873 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -248,9 +248,11 @@ format_odp_userspace_action(struct ds *ds, const struct nlattr *attr) { static const struct nl_policy ovs_userspace_policy[] = { [OVS_USERSPACE_ATTR_PID] = { .type = NL_A_U32 }, - [OVS_USERSPACE_ATTR_USERDATA] = { .type = NL_A_U64, .optional = true }, + [OVS_USERSPACE_ATTR_USERDATA] = { .type = NL_A_UNSPEC, + .optional = true }, }; struct nlattr *a[ARRAY_SIZE(ovs_userspace_policy)]; + const struct nlattr *userdata_attr; if (!nl_parse_nested(attr, ovs_userspace_policy, a, ARRAY_SIZE(a))) { ds_put_cstr(ds, "userspace(error)"); @@ -260,7 +262,8 @@ format_odp_userspace_action(struct ds *ds, const struct nlattr *attr) ds_put_format(ds, "userspace(pid=%"PRIu32, nl_attr_get_u32(a[OVS_USERSPACE_ATTR_PID])); - if (a[OVS_USERSPACE_ATTR_USERDATA]) { + userdata_attr = a[OVS_USERSPACE_ATTR_USERDATA]; + if (userdata_attr && nl_attr_get_size(userdata_attr) == sizeof(uint64_t)) { uint64_t userdata = nl_attr_get_u64(a[OVS_USERSPACE_ATTR_USERDATA]); union user_action_cookie cookie; @@ -287,6 +290,16 @@ format_odp_userspace_action(struct ds *ds, const struct nlattr *attr) ds_put_format(ds, ",userdata=0x%"PRIx64, userdata); break; } + } else if (userdata_attr) { + const uint8_t *userdata = nl_attr_get(userdata_attr); + size_t len = nl_attr_get_size(userdata_attr); + size_t i; + + ds_put_format(ds, ",userdata("); + for (i = 0; i < len; i++) { + ds_put_format(ds, "%02x", userdata[i]); + } + ds_put_char(ds, ')'); } ds_put_char(ds, ')'); @@ -449,7 +462,7 @@ parse_odp_action(const char *s, const struct simap *port_names, int n = -1; if (sscanf(s, "userspace(pid=%lli)%n", &pid, &n) > 0 && n > 0) { - odp_put_userspace_action(pid, NULL, actions); + odp_put_userspace_action(pid, NULL, 0, actions); return n; } else if (sscanf(s, "userspace(pid=%lli,sFlow(vid=%i," "pcp=%i,output=%lli))%n", @@ -465,7 +478,7 @@ parse_odp_action(const char *s, const struct simap *port_names, cookie.type = USER_ACTION_COOKIE_SFLOW; cookie.sflow.vlan_tci = htons(tci); cookie.sflow.output = output; - odp_put_userspace_action(pid, &cookie, actions); + odp_put_userspace_action(pid, &cookie, sizeof cookie, actions); return n; } else if (sscanf(s, "userspace(pid=%lli,slow_path%n", &pid, &n) > 0 && n > 0) { @@ -487,18 +500,29 @@ parse_odp_action(const char *s, const struct simap *port_names, } n++; - odp_put_userspace_action(pid, &cookie, actions); + odp_put_userspace_action(pid, &cookie, sizeof cookie, actions); return n; } else if (sscanf(s, "userspace(pid=%lli,userdata=" "%31[x0123456789abcdefABCDEF])%n", &pid, userdata_s, &n) > 0 && n > 0) { - union user_action_cookie cookie; uint64_t userdata; userdata = strtoull(userdata_s, NULL, 0); - memcpy(&cookie, &userdata, sizeof cookie); - odp_put_userspace_action(pid, &cookie, actions); + odp_put_userspace_action(pid, &userdata, sizeof(userdata), + actions); return n; + } else if (sscanf(s, "userspace(pid=%lli,userdata(%n", &pid, &n) > 0 + && n > 0) { + struct ofpbuf buf; + char *end; + + ofpbuf_init(&buf, 16); + end = ofpbuf_put_hex(&buf, &s[n], NULL); + if (end[0] == ')' && end[1] == ')') { + odp_put_userspace_action(pid, buf.data, buf.size, actions); + ofpbuf_uninit(&buf); + return (end + 2) - s; + } } } @@ -2113,25 +2137,30 @@ odp_key_fitness_to_string(enum odp_key_fitness fitness) } /* Appends an OVS_ACTION_ATTR_USERSPACE action to 'odp_actions' that specifies - * Netlink PID 'pid'. If 'cookie' is nonnull, adds a userdata attribute whose - * contents contains 'cookie' and returns the offset within 'odp_actions' of - * the start of the cookie. (If 'cookie' is null, then the return value is not - * meaningful.) */ + * Netlink PID 'pid'. If 'userdata' is nonnull, adds a userdata attribute + * whose contents are the 'userdata_size' bytes at 'userdata' and returns the + * offset within 'odp_actions' of the start of the cookie. (If 'userdata' is + * null, then the return value is not meaningful.) */ size_t -odp_put_userspace_action(uint32_t pid, const union user_action_cookie *cookie, +odp_put_userspace_action(uint32_t pid, + const void *userdata, size_t userdata_size, struct ofpbuf *odp_actions) { + size_t userdata_ofs; size_t offset; offset = nl_msg_start_nested(odp_actions, OVS_ACTION_ATTR_USERSPACE); nl_msg_put_u32(odp_actions, OVS_USERSPACE_ATTR_PID, pid); - if (cookie) { + if (userdata) { + userdata_ofs = odp_actions->size + NLA_HDRLEN; nl_msg_put_unspec(odp_actions, OVS_USERSPACE_ATTR_USERDATA, - cookie, sizeof *cookie); + userdata, userdata_size); + } else { + userdata_ofs = 0; } nl_msg_end_nested(odp_actions, offset); - return cookie ? odp_actions->size - NLA_ALIGN(sizeof *cookie) : 0; + return userdata_ofs; } void diff --git a/lib/odp-util.h b/lib/odp-util.h index ccf6c2a27..62401fc56 100644 --- a/lib/odp-util.h +++ b/lib/odp-util.h @@ -152,7 +152,7 @@ union user_action_cookie { BUILD_ASSERT_DECL(sizeof(union user_action_cookie) == 8); size_t odp_put_userspace_action(uint32_t pid, - const union user_action_cookie *, + const void *userdata, size_t userdata_size, struct ofpbuf *odp_actions); void odp_put_tunnel_action(const struct flow_tnl *tunnel, struct ofpbuf *odp_actions); diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index c1b9b698b..1e6fdc8b2 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -3888,7 +3888,16 @@ classify_upcall(const struct dpif_upcall *upcall) } /* "action" upcalls need a closer look. */ - memcpy(&cookie, &upcall->userdata, sizeof(cookie)); + if (!upcall->userdata) { + VLOG_WARN_RL(&rl, "action upcall missing cookie"); + return BAD_UPCALL; + } + if (nl_attr_get_size(upcall->userdata) != sizeof(cookie)) { + VLOG_WARN_RL(&rl, "action upcall cookie has unexpected size %zu", + nl_attr_get_size(upcall->userdata)); + return BAD_UPCALL; + } + memcpy(&cookie, nl_attr_get(upcall->userdata), sizeof(cookie)); switch (cookie.type) { case USER_ACTION_COOKIE_SFLOW: return SFLOW_UPCALL; @@ -3898,7 +3907,8 @@ classify_upcall(const struct dpif_upcall *upcall) case USER_ACTION_COOKIE_UNSPEC: default: - VLOG_WARN_RL(&rl, "invalid user cookie : 0x%"PRIx64, upcall->userdata); + VLOG_WARN_RL(&rl, "invalid user cookie : 0x%"PRIx64, + nl_attr_get_u64(upcall->userdata)); return BAD_UPCALL; } } @@ -3918,7 +3928,7 @@ handle_sflow_upcall(struct dpif_backer *backer, return; } - memcpy(&cookie, &upcall->userdata, sizeof(cookie)); + memcpy(&cookie, nl_attr_get(upcall->userdata), sizeof(cookie)); dpif_sflow_received(ofproto->sflow, upcall->packet, &flow, odp_in_port, &cookie); } @@ -5578,7 +5588,7 @@ compose_slow_path(const struct ofproto_dpif *ofproto, const struct flow *flow, ofpbuf_use_stack(&buf, stub, stub_size); if (slow & (SLOW_CFM | SLOW_LACP | SLOW_STP)) { uint32_t pid = dpif_port_get_pid(ofproto->backer->dpif, UINT32_MAX); - odp_put_userspace_action(pid, &cookie, &buf); + odp_put_userspace_action(pid, &cookie, sizeof cookie, &buf); } else { put_userspace_action(ofproto, &buf, flow, &cookie); } @@ -5597,7 +5607,7 @@ put_userspace_action(const struct ofproto_dpif *ofproto, pid = dpif_port_get_pid(ofproto->backer->dpif, ofp_port_to_odp_port(ofproto, flow->in_port)); - return odp_put_userspace_action(pid, cookie, odp_actions); + return odp_put_userspace_action(pid, cookie, sizeof *cookie, odp_actions); } static void diff --git a/tests/odp.at b/tests/odp.at index bd45cf706..dcf746546 100644 --- a/tests/odp.at +++ b/tests/odp.at @@ -89,6 +89,7 @@ userspace(pid=9765,slow_path()) userspace(pid=9765,slow_path(cfm)) userspace(pid=9765,slow_path(cfm,match)) userspace(pid=9123,userdata=0x815309) +userspace(pid=1234567,userdata(0102030405060708090a0b0c0d0e0f)) set(tun_id(0x7f10354)) set(in_port(2)) set(eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15)) -- 2.43.0