[ovs-dev] [PATCH] ofproto-dpif-upcall: Fix additional use-after-free in revalidate().

Ben Pfaff blp at nicira.com
Wed May 21 21:33:51 UTC 2014


Commit 1340ce0c175 (ofproto-dpif-upcall: Avoid use-after-free in
revalidate() corner cases.) fixed one use-after-free error in revalidate(),
but missed one more subtle case, in which dpif_linux_flow_dump_next()
attempts to retrieve actions for a flow that didn't have them in the main
dump result.  If retrieving those actions fails,
dpif_linux_flow_dump_next() goes on to the next flow, and as part of that
overwrites the old dumped flows in its buffer.  This is a problem because
dpif_linux_flow_dump_next_may_destroy_keys() would have indicated that
the old dumped flows would not have been destroyed, which means that the
data the caller relied on has gone away.  In the worst case, this causes
a segfault and core dump.

The best way to fix this problem is the refactoring that has already
happened on master in commit ac64794acb85 (dpif: Refactor flow dumping
interface to make better sense for batching.)  That is a fairly large
change, and not yet well-tested, so it doesn't yet seem suitable for a
stable branch.  For now, this commit simply turns off batching of the
deletions that happen during revalidation.  If this proves in practice to
hurt performance too much (e.g. in a TCP_CRR netperf when megaflows don't
avoid per-microflow upcalls), we can always backport the refactoring.

Bug #1249988.
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 lib/dpif.c                    |   15 +++------------
 ofproto/ofproto-dpif-upcall.c |   30 ++++++++----------------------
 2 files changed, 11 insertions(+), 34 deletions(-)

diff --git a/lib/dpif.c b/lib/dpif.c
index 41b8eb7..677816f 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1065,18 +1065,9 @@ dpif_flow_dump_next(struct dpif_flow_dump *dump, void *state,
     return !error;
 }
 
-/* Determines whether the next call to 'dpif_flow_dump_next' for 'dump' and
- * 'state' will modify or free the keys that it previously returned. 'state'
- * must have been initialized by a call to 'dpif_flow_dump_state_init' for
- * 'dump'.
- *
- * 'dpif' guarantees that data returned by flow_dump_next() will remain
- * accessible and unchanging until the next call. This function provides a way
- * for callers to determine whether that guarantee extends beyond the next
- * call.
- *
- * Returns true if the next call to flow_dump_next() is expected to be
- * destructive to previously returned keys for 'state', false otherwise. */
+/* Returns true if the next call to flow_dump_next() is expected to be
+ * destructive to previously returned keys for 'state', false otherwise.
+ * The return value is only a heuristic; the caller must not rely on it. */
 bool
 dpif_flow_dump_next_may_destroy_keys(struct dpif_flow_dump *dump, void *state)
 {
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 193f4cd..c8ec22c 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1434,16 +1434,13 @@ revalidate(struct revalidator *revalidator)
 {
     struct udpif *udpif = revalidator->udpif;
 
-    struct dump_op ops[REVALIDATE_MAX_BATCH];
     const struct nlattr *key, *mask, *actions;
     size_t key_len, mask_len, actions_len;
     const struct dpif_flow_stats *stats;
     long long int now;
     unsigned int flow_limit;
-    size_t n_ops;
     void *state;
 
-    n_ops = 0;
     now = time_msec();
     atomic_read(&udpif->flow_limit, &flow_limit);
 
@@ -1451,7 +1448,7 @@ revalidate(struct revalidator *revalidator)
     while (dpif_flow_dump_next(&udpif->dump, state, &key, &key_len, &mask,
                                &mask_len, &actions, &actions_len, &stats)) {
         struct udpif_key *ukey;
-        bool mark, may_destroy;
+        bool mark;
         long long int used, max_idle;
         uint32_t hash;
         size_t n_flows;
@@ -1508,31 +1505,20 @@ revalidate(struct revalidator *revalidator)
         }
 
         if (!mark) {
-            dump_op_init(&ops[n_ops++], key, key_len, ukey);
+            struct dump_op op;
+
+            dump_op_init(&op, key, key_len, ukey);
+            push_dump_ops__(udpif, &op, 1);
         }
 
     next:
-        may_destroy = dpif_flow_dump_next_may_destroy_keys(&udpif->dump,
-                                                           state);
-
-        /* Only update 'now' immediately before 'buffer' will be updated.
+        /* Only update 'now' immediately before 'buffer' will be updated
+         * (typically, at least, since this function only yields a heuristic).
          * This gives us the current time relative to the time the datapath
          * will write into 'stats'. */
-        if (may_destroy) {
+        if (dpif_flow_dump_next_may_destroy_keys(&udpif->dump, state)) {
             now = time_msec();
         }
-
-        /* Only do a dpif_operate when we've hit our maximum batch, or when our
-         * memory is about to be clobbered by the next call to
-         * dpif_flow_dump_next(). */
-        if (n_ops == REVALIDATE_MAX_BATCH || (n_ops && may_destroy)) {
-            push_dump_ops__(udpif, ops, n_ops);
-            n_ops = 0;
-        }
-    }
-
-    if (n_ops) {
-        push_dump_ops__(udpif, ops, n_ops);
     }
 
     dpif_flow_dump_state_uninit(udpif->dpif, state);
-- 
1.7.10.4




More information about the dev mailing list