[ovs-dev] [PATCH] dpif-linux: Fix ovs-vswitchd crash.

[Alex Wang]

Commits 1579cf677fc (dpif-linux: Implement the API functions to allow
multiple handler threads read upcall.) causes the ovs-vswitchd crash in
the following scenario:

- when udpif_flush() is called, it sets both n-handlers and n-revalidators
  to zero.  and it calls the dpif_handlers_set() to refresh the dpif-level
  channels.
- in dpif_linux_refresh_channels(), the code try to 'xzalloc(0)' which is
  equivalent to 'malloc(1)', and make 'dpif->handlers' point to it.  so,
  'dpif->handlers' points to a 1-byte memory.
- the following dereference of the 'dpif->handlers' results in SEGFAULT.

This commit fixes the above issue by assigning NULL to 'dpif->handlers'
when 'n-handlers' is zero.  So, the functions that check the non-NULL
of 'dpif->handlers' will not dereference it.

Reported-by: Andy Zhou <az...@nicira.com>
Signed-off-by: Alex Wang <al...@nicira.com>
---
 lib/dpif-linux.c |   30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index a575b78..2b11913 100644
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -148,7 +148,11 @@ struct dpif_linux {
 
     /* Upcall messages. */
     struct fat_rwlock upcall_lock;
-    struct dpif_handler *handlers;
+    bool upcall_enabled;           /* Upcall recv enabled?  If false, */
+                                   /* 'handlers' must be NULL. */
+    struct dpif_handler *handlers; /* Array of handlers.  NULL if either */
+                                   /* upcall_enabled is false or n_handlers */
+                                   /* is zero. */
     uint32_t n_handlers;           /* Num of upcall handlers. */
     int uc_array_size;             /* Size of 'handler->channels' and */
                                    /* 'handler->epoll_events'. */
@@ -1489,7 +1493,8 @@ dpif_linux_refresh_channels(struct dpif_linux *dpif, 
uint32_t n_handlers)
 
     if (dpif->n_handlers != n_handlers) {
         destroy_all_channels(dpif);
-        dpif->handlers = xzalloc(n_handlers * sizeof *dpif->handlers);
+        dpif->handlers = n_handlers
+                         ? xzalloc(n_handlers * sizeof *dpif->handlers) : NULL;
         for (i = 0; i < n_handlers; i++) {
             struct dpif_handler *handler = &dpif->handlers[i];
 
@@ -1515,6 +1520,11 @@ dpif_linux_refresh_channels(struct dpif_linux *dpif, 
uint32_t n_handlers)
         handler->event_offset = handler->n_events = 0;
     }
 
+    /* Skips port dumping if there is no handler. */
+    if (!dpif->handlers) {
+        return retval;
+    }
+
     keep_channels_nbits = dpif->uc_array_size;
     keep_channels = bitmap_allocate(keep_channels_nbits);
 
@@ -1604,13 +1614,17 @@ static int
 dpif_linux_recv_set__(struct dpif_linux *dpif, bool enable)
     OVS_REQ_WRLOCK(dpif->upcall_lock)
 {
-    if ((dpif->handlers != NULL) == enable) {
-        return 0;
-    } else if (!enable) {
-        destroy_all_channels(dpif);
+    if (dpif->upcall_enabled == enable) {
         return 0;
     } else {
-        return dpif_linux_refresh_channels(dpif, 1);
+        dpif->upcall_enabled = enable;
+
+        if (!enable) {
+            destroy_all_channels(dpif);
+            return 0;
+        } else {
+            return dpif_linux_refresh_channels(dpif, 1);
+        }
     }
 }
 
@@ -1634,7 +1648,7 @@ dpif_linux_handlers_set(struct dpif *dpif_, uint32_t 
n_handlers)
     int error = 0;
 
     fat_rwlock_wrlock(&dpif->upcall_lock);
-    if (dpif->handlers) {
+    if (dpif->upcall_enabled) {
         error = dpif_linux_refresh_channels(dpif, n_handlers);
     }
     fat_rwlock_unlock(&dpif->upcall_lock);
-- 
1.7.9.5

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev