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 <rlenglet@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>

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)
+	if (upcall_info->userdata)
-		len += nla_total_size(8);
+		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_put(user_skb, OVS_PACKET_ATTR_USERDATA,
-			    nla_get_u64(upcall_info->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;

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

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_USERSPACE_ATTR_USERDATA: If present, its variable-length argument is
- * %OVS_PACKET_CMD_ACTION message as %OVS_PACKET_ATTR_USERDATA,
+ * 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
 };
 

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]
+    upcall->userdata = a[OVS_PACKET_ATTR_USERDATA];
-                        ? nl_attr_get_u64(a[OVS_PACKET_ATTR_USERDATA])
-                        : 0);
     *dp_ifindex = ovs_header->dp_ifindex;
 
     return 0;

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;
+    if (q->head - q->tail < MAX_QUEUE_LEN) {
-    struct dpif_upcall *upcall;
+        struct dp_netdev_upcall *u = &q->upcalls[q->head++ & QUEUE_MASK];
-    struct ofpbuf *buf;
+        struct dpif_upcall *upcall = &u->upcall;
-    size_t key_len;
+        struct ofpbuf *buf = &u->buf;
+        size_t buf_size;
 
-    if (q->head - q->tail >= MAX_QUEUE_LEN) {
+        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);
+
+        /* Put ODP flow. */
+        odp_flow_key_from_flow(buf, flow, flow->in_port);
+        upcall->key = buf->data;
+        upcall->key_len = buf->size;
+
+        /* Put userdata. */
+        if (userdata) {
+            upcall->userdata = ofpbuf_put(buf, userdata,
+                                          NLA_ALIGN(userdata->nla_len));
+        }
+
+        /* 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;
+    } else {
         dp->n_lost++;
         return ENOBUFS;
     }
-
-    u = &q->upcalls[q->head++ & QUEUE_MASK];
-
-    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);
-
-    upcall = &u->upcall;
-    upcall->type = queue_no;
-    upcall->packet = buf;
-    upcall->key = buf->base;
-    upcall->key_len = key_len;
-    upcall->userdata = arg;
-
-    return 0;
 }
 
 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;
+    const struct nlattr *userdata;
-    uint64_t userdata;
 
-    userdata_attr = nl_attr_find_nested(a, OVS_USERSPACE_ATTR_USERDATA);
+    userdata = nl_attr_find_nested(a, OVS_USERSPACE_ATTR_USERDATA);
-    userdata = userdata_attr ? nl_attr_get_u64(userdata_attr) : 0;
     dp_netdev_output_userspace(dp, packet, DPIF_UC_ACTION, key, userdata);
 }
 

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
+ * If 'key', 'actions', or 'userdata' is nonnull, then it points into data
- * 'packet', so their memory cannot be freed separately.  (This is hardly a
+ * owned by 'packet', so their memory cannot be freed separately.  (This is
- * great way to do things but it works out OK for the dpif providers and
+ * hardly a great way to do things but it works out OK for the dpif providers
- * clients that exist so far.)
+ * 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);

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, &userdata, sizeof(userdata),
-            odp_put_userspace_action(pid, &cookie, actions);
+                                     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
+ * Netlink PID 'pid'.  If 'userdata' is nonnull, adds a userdata attribute
- * contents contains 'cookie' and returns the offset within 'odp_actions' of
+ * whose contents are the 'userdata_size' bytes at 'userdata' and returns the
- * the start of the cookie.  (If 'cookie' is null, then the return value is not
+ * offset within 'odp_actions' of the start of the cookie.  (If 'userdata' is
- * meaningful.) */
+ * 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

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);

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

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))