[ovs-dev] [PATCH v2] rtbsd: Make rtbsd module thread-safe.

[Ryan Wilson]

Due to patch fe83f8 (netdev: Remove netdev from global shash when
the user is changing interface configuration), netdevs can be
destructed and deallocated by revalidator and RCU threads. When
netdevs with class bsd are destroyed, the rtbsd notifier is
unregistered, possibly causing memory to be freed. This causes a
race condition with the main thread which calls rtbsd_notifier_run
periodically to check for any netdev change events.

This patch makes the rtbsd module thread-safe via a mutex.

Note this patch removes rtbsd_notifier_run() in
rtbsd_notifier_register() due to locking requirements. Since the
rtbsd_notifier_run() is always run by the main thread often,
receiving stale notifications from the notifier is unlikely.

Bug #1258532
Signed-off-by: Ryan Wilson <wr...@nicira.com>
Acked-by: YAMAMOTO Takashi <yamam...@valinux.co.jp>

---
v2: Fix OVS_EXCLUDES/OVS_EXCLUDED typo
---
 lib/rtbsd.c |   39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/lib/rtbsd.c b/lib/rtbsd.c
index b706c20..ffffbb7 100644
--- a/lib/rtbsd.c
+++ b/lib/rtbsd.c
@@ -32,14 +32,17 @@
 VLOG_DEFINE_THIS_MODULE(rtbsd);
 COVERAGE_DEFINE(rtbsd_changed);
 
+static struct ovs_mutex rtbsd_mutex = OVS_MUTEX_INITIALIZER;
+
 /* PF_ROUTE socket. */
 static int notify_sock = -1;
 
 /* All registered notifiers. */
 static struct list all_notifiers = LIST_INITIALIZER(&all_notifiers);
 
-static void rtbsd_report_change(const struct if_msghdr *);
-static void rtbsd_report_notify_error(void);
+static void rtbsd_report_change(const struct if_msghdr *)
+    OVS_REQUIRES(rtbsd_mutex);
+static void rtbsd_report_notify_error(void) OVS_REQUIRES(rtbsd_mutex);
 
 /* Registers 'cb' to be called with auxiliary data 'aux' with network device
  * change notifications.  The notifier is stored in 'notifier', which the
@@ -49,53 +52,63 @@ static void rtbsd_report_notify_error(void);
 int
 rtbsd_notifier_register(struct rtbsd_notifier *notifier,
                             rtbsd_notify_func *cb, void *aux)
+    OVS_EXCLUDED(rtbsd_mutex)
 {
+    int error = 0;
+
+    ovs_mutex_lock(&rtbsd_mutex);
     if (notify_sock < 0) {
-        int error;
         notify_sock = socket(PF_ROUTE, SOCK_RAW, 0);
         if (notify_sock < 0) {
             VLOG_WARN("could not create PF_ROUTE socket: %s",
                       ovs_strerror(errno));
-            return errno;
+            error = errno;
+            goto out;
         }
         error = set_nonblocking(notify_sock);
         if (error) {
             VLOG_WARN("error set_nonblocking PF_ROUTE socket: %s",
                     ovs_strerror(error));
-            return error;
+            goto out;
         }
-    } else {
-        /* Catch up on notification work so that the new notifier won't
-         * receive any stale notifications. XXX*/
-        rtbsd_notifier_run();
     }
 
     list_push_back(&all_notifiers, &notifier->node);
     notifier->cb = cb;
     notifier->aux = aux;
-    return 0;
+
+out:
+    ovs_mutex_unlock(&rtbsd_mutex);
+    return error;
 }
 
 /* Cancels notification on 'notifier', which must have previously been
  * registered with rtbsd_notifier_register(). */
 void
 rtbsd_notifier_unregister(struct rtbsd_notifier *notifier)
+    OVS_EXCLUDED(rtbsd_mutex)
 {
+    ovs_mutex_lock(&rtbsd_mutex);
     list_remove(&notifier->node);
     if (list_is_empty(&all_notifiers)) {
         close(notify_sock);
         notify_sock = -1;
     }
+    ovs_mutex_unlock(&rtbsd_mutex);
 }
 
 /* Calls all of the registered notifiers, passing along any as-yet-unreported
  * netdev change events. */
 void
 rtbsd_notifier_run(void)
+    OVS_EXCLUDED(rtbsd_mutex)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
     struct if_msghdr msg;
+
+    ovs_mutex_lock(&rtbsd_mutex);
     if (notify_sock < 0) {
+        ovs_mutex_unlock(&rtbsd_mutex);
         return;
     }
 
@@ -114,6 +127,7 @@ rtbsd_notifier_run(void)
                 rtbsd_report_change(&msg);
             }
         } else if (errno == EAGAIN) {
+            ovs_mutex_unlock(&rtbsd_mutex);
             return;
         } else {
             if (errno == ENOBUFS) {
@@ -131,14 +145,18 @@ rtbsd_notifier_run(void)
  * ready. */
 void
 rtbsd_notifier_wait(void)
+    OVS_EXCLUDED(rtbsd_mutex)
 {
+    ovs_mutex_lock(&rtbsd_mutex);
     if (notify_sock >= 0) {
         poll_fd_wait(notify_sock, POLLIN);
     }
+    ovs_mutex_unlock(&rtbsd_mutex);
 }
 
 static void
 rtbsd_report_change(const struct if_msghdr *msg)
+    OVS_REQUIRES(rtbsd_mutex)
 {
     struct rtbsd_notifier *notifier;
     struct rtbsd_change change;
@@ -158,6 +176,7 @@ rtbsd_report_change(const struct if_msghdr *msg)
 /* If an error occurs the notifiers' callbacks are called with NULL changes */
 static void
 rtbsd_report_notify_error(void)
+    OVS_REQUIRES(rtbsd_mutex)
 {
     struct rtbsd_notifier *notifier;
 
-- 
1.7.9.5

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