netdev-linux: Fix use-after-free when netdev_dump_queues() deletes queues.

iface_configure_qos() passes a callback to netdev_dump_queues() that can
delete queues.  The netdev-linux implementation of this function was
unprepared for the callback to delete queues, so this could cause a
use-after-free.  This fixes the problem in netdev_linux_dump_queues() and
documents that netdev_dump_queues() implementations must support deletions
in the callback.

Found by valgrind:

==1593== Invalid read of size 8
==1593==    at 0x4A8C43: netdev_linux_dump_queues (hmap.h:326)
==1593==    by 0x4305F7: bridge_reconfigure (bridge.c:3084)
==1593==    by 0x431384: bridge_run (bridge.c:1892)
==1593==    by 0x432749: main (ovs-vswitchd.c:96)
==1593==  Address 0x632e078 is 8 bytes inside a block of size 32 free'd
==1593==    at 0x4C240FD: free (vg_replace_malloc.c:366)
==1593==    by 0x4A4D74: hfsc_class_delete (netdev-linux.c:3250)
==1593==    by 0x42AA59: iface_delete_queues (bridge.c:3055)
==1593==    by 0x4A8C8C: netdev_linux_dump_queues (netdev-linux.c:1881)
==1593==    by 0x4305F7: bridge_reconfigure (bridge.c:3084)
==1593==    by 0x431384: bridge_run (bridge.c:1892)

Bug #10164.
Reported-by: Ram Jothikumar <ram@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 3dd5c8af08241650df4af613a262e17cd72bdcd0..be70a916e80deec766d1d09f0a5cca41e7a600a8 100644 (file)
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -2002,7 +2002,7 @@ netdev_linux_dump_queues(const struct netdev *netdev,
 {
     struct netdev_dev_linux *netdev_dev =
                                 netdev_dev_linux_cast(netdev_get_dev(netdev));
-    struct tc_queue *queue;
+    struct tc_queue *queue, *next_queue;
     struct shash details;
     int last_error;
     int error;
@@ -2016,7 +2016,8 @@ netdev_linux_dump_queues(const struct netdev *netdev,
 
     last_error = 0;
     shash_init(&details);
-    HMAP_FOR_EACH (queue, hmap_node, &netdev_dev->tc->queues) {
+    HMAP_FOR_EACH_SAFE (queue, next_queue, hmap_node,
+                        &netdev_dev->tc->queues) {
         shash_clear(&details);
 
         error = netdev_dev->tc->ops->class_get(netdev, queue, &details);

diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
index 2ef75b30c57dde36e88e6611b8f08ae8003e4793..dea171db268344edcb871c5756c6987b9578b048 100644 (file)
--- a/lib/netdev-provider.h
+++ b/lib/netdev-provider.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010, 2011 Nicira Networks.
+ * Copyright (c) 2009, 2010, 2011, 2012 Nicira Networks.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -464,7 +464,12 @@ struct netdev_class {
      * of iteration is unspecified, but (when successful) each queue is visited
      * exactly once.
      *
-     * 'cb' will not modify or free the 'details' argument passed in. */
+     * 'cb' will not modify or free the 'details' argument passed in.  It may
+     * delete or modify the queue passed in as its 'queue_id' argument.  It may
+     * modify but will not delete any other queue within 'netdev'.  If 'cb'
+     * adds new queues, then ->dump_queues is allowed to visit some queues
+     * twice or not at all.
+     */
     int (*dump_queues)(const struct netdev *netdev,
                        void (*cb)(unsigned int queue_id,
                                   const struct shash *details,

diff --git a/lib/netdev.c b/lib/netdev.c
index 5aa30a7a2c183095ab0a404d081166617136473a..305ad13f6c98853e651a726566c4221f95902c14 100644 (file)
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -1235,7 +1235,11 @@ netdev_get_queue_stats(const struct netdev *netdev, unsigned int queue_id,
  * Calling this function may be more efficient than calling netdev_get_queue()
  * for every queue.
  *
- * 'cb' must not modify or free the 'details' argument passed in.
+ * 'cb' must not modify or free the 'details' argument passed in.  It may
+ * delete or modify the queue passed in as its 'queue_id' argument.  It may
+ * modify but must not delete any other queue within 'netdev'.  'cb' should not
+ * add new queues because this may cause some queues to be visited twice or not
+ * at all.
  *
  * Returns 0 if successful, otherwise a positive errno value.  On error, some
  * configured queues may not have been included in the iteration. */