[ovs-dev] [bugfixes 3/3] datapath: Dump flow actions only if there is room.

Ben Pfaff blp at nicira.com
Tue Feb 1 00:48:25 UTC 2011


Expanding an skbuff in a netlink dump handler doesn't work well.  First,
we weren't updating the truesize of the skb or the allocation within the
socket that netlink_dump() had put the skb in.  Even when I fixed that, I
still got mysterious crashes and reboots.  I don't know why these happened.

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 at nicira.com>
---
 datapath/datapath.c |   28 ++++++-------------
 lib/dpif-linux.c    |   74 ++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 62 insertions(+), 40 deletions(-)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index babe63c..ea1b8d2 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -820,14 +820,12 @@ static struct genl_multicast_group dp_flow_multicast_group = {
 static int odp_flow_cmd_fill_info(struct sw_flow *flow, struct datapath *dp,
 				  struct sk_buff *skb, u32 pid, u32 seq, u32 flags, u8 cmd)
 {
-	const int skb_orig_len = skb->len;
 	const struct sw_flow_actions *sf_acts;
 	struct odp_flow_stats stats;
 	struct odp_header *odp_header;
 	struct nlattr *nla;
 	unsigned long used;
 	u8 tcp_flags;
-	int nla_len;
 	int err;
 
 	sf_acts = rcu_dereference_protected(flow->sf_acts,
@@ -863,23 +861,15 @@ 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);
+	/* Put the actions if there is room.  For fetching a single flow there
+	 * will always be enough room, because the skbuff was sized properly
+	 * for the actions, but for dumping there may not be enough room (even
+	 * if this is the only flow dumped) because the skbuff is sized by
+	 * netlink_dump().  In such a case the userspace caller can always
+	 * fetch the actions separately if it really wants them.  (Most
+	 * userspace callers in fact don't care.)
+	 */
+	nla_put(skb, ODP_FLOW_ATTR_ACTIONS, sf_acts->actions_len, sf_acts->actions);
 
 	return genlmsg_end(skb, odp_header);
 
diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index 2e35857..f1d42dc 100644
--- 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;
 }
-- 
1.7.1





More information about the dev mailing list