dpif-linux: Fix revalidator core caused by flow dumps.

The dpif flow dump API guarantees that keys it returns are not deleted
unless dpif_flow_dump_next_may_destroy_keys() warns that they might
be.  When dpif_linux_flow_dump_next() needed extra memory for a
datapath flow's actions, it would use a special temporary buffer.
Unfortunately, it also used memory from this temporary buffer for the
keys it returned.  Thus, on the next loop this memory would be freed,
breaking our invariant.

The correct solution to this problem is probably to fix this rather
awkward API.  However, this patch's solution is small and simple, so
it's fine for now.

The following is a valgrind stack trace which lead to finding this
bug.

Invalid read of size 2
  at 0x4C2D050: memcpy@@GLIBC_2.14 (in ...)
  by 0x4D40B8: dpif_linux_flow_to_ofpbuf (dpif-linux.c:2373)
  by 0x4D4DE4: dpif_linux_operate__ (dpif-linux.c:1394)
  by 0x4D5055: dpif_linux_operate (dpif-linux.c:1480)
  by 0x450D14: dpif_operate (dpif.c:1225)
  by 0x42D706: push_dump_ops__.isra.6 (ofproto-dpif-upcall.c:1362)
  by 0x42E5A8: revalidate.isra.11 (ofproto-dpif-upcall.c:1531)
  by 0x42EB0E: udpif_revalidator (ofproto-dpif-upcall.c:592)
  by 0x495070: ovsthread_wrapper (ovs-thread.c:302)
  by 0x5472E99: start_thread (pthread_create.c:308)
  by 0x5C7C4BC: clone (clone.S:112)
Address 0x227e12d0 is 160 bytes inside a block of size 4,976 free'd
  at 0x4C2A82E: free (in ...)
  by 0x494227: ofpbuf_uninit (ofpbuf.c:136)
  by 0x4D5D34: dpif_linux_flow_dump_next (ofpbuf.h:182)
  by 0x4509BA: dpif_flow_dump_next (dpif.c:1036)
  by 0x42E137: revalidate.isra.11 (ofproto-dpif-upcall.c:1451)
  by 0x42EB0E: udpif_revalidator (ofproto-dpif-upcall.c:592)
  by 0x495070: ovsthread_wrapper (ovs-thread.c:302)
  by 0x5472E99: start_thread (pthread_create.c:308)
  by 0x5C7C4BC: clone (clone.S:112)

Signed-off-by: Ethan Jackson <ethan@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>

diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index 4af0607..6a8564a 100644 (file)
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -1221,9 +1221,19 @@ dpif_linux_flow_dump_next(const struct dpif *dpif_, void *iter_, void *state_,
         }
 
         if (actions && !state->flow.actions) {
+            struct dpif_linux_flow reply;
+
+            /* Keys are required to be allocated from 'state->buffer' so
+             * they're preserved across calls.  Therefore we need a separate
+             * reply to prevent them from being overwritten.  Actions, however,
+             * don't have this requirement, so it's that fine they're destroyed
+             * on the next call. */
             error = dpif_linux_flow_get__(dpif, state->flow.key,
                                           state->flow.key_len,
-                                          &state->flow, &state->tmp);
+                                          &reply, &state->tmp);
+            state->flow.actions = reply.actions;
+            state->flow.actions_len = reply.actions_len;
+
             if (error == ENOENT) {
                 VLOG_DBG("dumped flow disappeared on get");
             } else if (error) {