datapath: Dump flow actions only if there is room.

Expanding an skbuff in a netlink dump handler doesn't work well.  We
weren't updating the truesize of the skb or the allocation within the
socket that netlink_dump() had put the skb in.  The code had other bugs
too.

This commit fixes the problem (in my tests, anyway) by avoiding expanding
the reply skbuff to fill in the actions.  Instead, in such a case the
userspace client has to do a separate "get" action to get the actions.
This commit also updates userspace to do this automatically for dumps in
the cases where the caller cares (only "ovs-dpctl dump-flows" currently
cares).

Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>
Bug #4520.

diff --git a/datapath/datapath.c b/datapath/datapath.c
index babe63c..74f276b 100644 (file)
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -827,7 +827,6 @@ static int odp_flow_cmd_fill_info(struct sw_flow *flow, struct datapath *dp,
        struct nlattr *nla;
        unsigned long used;
        u8 tcp_flags;
-       int nla_len;
        int err;
 
        sf_acts = rcu_dereference_protected(flow->sf_acts,
@@ -863,23 +862,20 @@ static int odp_flow_cmd_fill_info(struct sw_flow *flow, struct datapath *dp,
        if (tcp_flags)
                NLA_PUT_U8(skb, ODP_FLOW_ATTR_TCP_FLAGS, tcp_flags);
 
-       /* If ODP_FLOW_ATTR_ACTIONS doesn't fit, and this is the first flow to
-        * be dumped into 'skb', then expand the skb.  This is unusual for
-        * Netlink but individual action lists can be longer than a page and
-        * thus entirely undumpable if we didn't do this. */
-       nla_len = nla_total_size(sf_acts->actions_len);
-       if (nla_len > skb_tailroom(skb) && !skb_orig_len) {
-               int hdr_off = (unsigned char *)odp_header - skb->data;
-
-               err = pskb_expand_head(skb, 0, nla_len - skb_tailroom(skb), GFP_KERNEL);
-               if (err)
-                       goto error;
-
-               odp_header = (struct odp_header *)(skb->data + hdr_off);
-       }
-       nla = nla_nest_start(skb, ODP_FLOW_ATTR_ACTIONS);
-       memcpy(__skb_put(skb, sf_acts->actions_len), sf_acts->actions, sf_acts->actions_len);
-       nla_nest_end(skb, nla);
+       /* If ODP_FLOW_ATTR_ACTIONS doesn't fit, skip dumping the actions if
+        * this is the first flow to be dumped into 'skb'.  This is unusual for
+        * Netlink but individual action lists can be longer than
+        * NLMSG_GOODSIZE and thus entirely undumpable if we didn't do this.
+        * The userspace caller can always fetch the actions separately if it
+        * really wants them.  (Most userspace callers in fact don't care.)
+        *
+        * This can only fail for dump operations because the skb is always
+        * properly sized for single flows.
+        */
+       err = nla_put(skb, ODP_FLOW_ATTR_ACTIONS, sf_acts->actions_len,
+                     sf_acts->actions);
+       if (err < 0 && skb_orig_len)
+               goto error;
 
        return genlmsg_end(skb, odp_header);
 

diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index 2e35857..f1d42dc 100644 (file)
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -506,21 +506,31 @@ dpif_linux_port_poll_wait(const struct dpif *dpif_)
 }
 
 static int
-dpif_linux_flow_get(const struct dpif *dpif_,
-                    const struct nlattr *key, size_t key_len,
-                    struct ofpbuf **actionsp, struct dpif_flow_stats *stats)
+dpif_linux_flow_get__(const struct dpif *dpif_,
+                      const struct nlattr *key, size_t key_len,
+                      struct dpif_linux_flow *reply, struct ofpbuf **bufp)
 {
     struct dpif_linux *dpif = dpif_linux_cast(dpif_);
-    struct dpif_linux_flow request, reply;
-    struct ofpbuf *buf;
-    int error;
+    struct dpif_linux_flow request;
 
     dpif_linux_flow_init(&request);
     request.cmd = ODP_FLOW_CMD_GET;
     request.dp_ifindex = dpif->dp_ifindex;
     request.key = key;
     request.key_len = key_len;
-    error = dpif_linux_flow_transact(&request, &reply, &buf);
+    return dpif_linux_flow_transact(&request, reply, bufp);
+}
+
+static int
+dpif_linux_flow_get(const struct dpif *dpif_,
+                    const struct nlattr *key, size_t key_len,
+                    struct ofpbuf **actionsp, struct dpif_flow_stats *stats)
+{
+    struct dpif_linux_flow reply;
+    struct ofpbuf *buf;
+    int error;
+
+    error = dpif_linux_flow_get__(dpif_, key, key_len, &reply, &buf);
     if (!error) {
         if (stats) {
             dpif_linux_flow_get_stats(&reply, stats);
@@ -599,6 +609,7 @@ struct dpif_linux_flow_state {
     struct nl_dump dump;
     struct dpif_linux_flow flow;
     struct dpif_flow_stats stats;
+    struct ofpbuf *buf;
 };
 
 static int
@@ -620,6 +631,8 @@ dpif_linux_flow_dump_start(const struct dpif *dpif_, void **statep)
     nl_dump_start(&state->dump, genl_sock, buf);
     ofpbuf_delete(buf);
 
+    state->buf = NULL;
+
     return 0;
 }
 
@@ -633,24 +646,42 @@ dpif_linux_flow_dump_next(const struct dpif *dpif_ OVS_UNUSED, void *state_,
     struct ofpbuf buf;
     int error;
 
-    if (!nl_dump_next(&state->dump, &buf)) {
-        return EOF;
-    }
+    do {
+        ofpbuf_delete(state->buf);
+        state->buf = NULL;
 
-    error = dpif_linux_flow_from_ofpbuf(&state->flow, &buf);
-    if (!error) {
-        if (key) {
-            *key = state->flow.key;
-            *key_len = state->flow.key_len;
+        if (!nl_dump_next(&state->dump, &buf)) {
+            return EOF;
         }
-        if (actions) {
-            *actions = state->flow.actions;
-            *actions_len = state->flow.actions_len;
+
+        error = dpif_linux_flow_from_ofpbuf(&state->flow, &buf);
+        if (error) {
+            return error;
         }
-        if (stats) {
-            dpif_linux_flow_get_stats(&state->flow, &state->stats);
-            *stats = &state->stats;
+
+        if (actions && !state->flow.actions) {
+            error = dpif_linux_flow_get__(dpif_, state->flow.key,
+                                          state->flow.key_len,
+                                          &state->flow, &state->buf);
+            if (error == ENOENT) {
+                VLOG_DBG("dumped flow disappeared on get");
+            } else if (error) {
+                VLOG_WARN("error fetching dumped flow: %s", strerror(error));
+            }
         }
+    } while (error);
+
+    if (actions) {
+        *actions = state->flow.actions;
+        *actions_len = state->flow.actions_len;
+    }
+    if (key) {
+        *key = state->flow.key;
+        *key_len = state->flow.key_len;
+    }
+    if (stats) {
+        dpif_linux_flow_get_stats(&state->flow, &state->stats);
+        *stats = &state->stats;
     }
     return error;
 }
@@ -660,6 +691,7 @@ dpif_linux_flow_dump_done(const struct dpif *dpif OVS_UNUSED, void *state_)
 {
     struct dpif_linux_flow_state *state = state_;
     int error = nl_dump_done(&state->dump);
+    ofpbuf_delete(state->buf);
     free(state);
     return error;
 }