seq: Fix deadlock with the time_init.

There is an ABBA deadlock between time_init() and seq_wait():

    Thread 1:
        poll_block()
        time_poll()
        time_init()
        pthread_once() <-- lock A
        do_time_init()
        seq_create()
        pthread_mutex_lock(seq_mutex) <-- lock B

    Thread 2:
        seq_wait(different seqno)
        pthread_mutex_lock(seq_mutex) <-- lock B
        poll_immediate_wake()
        poll_timer_wait()
        time_msec()
        time_init()
        pthread_once() <-- lock A

This is likely the same deadlock Intel CI saw last year before the lab
was shut down.

The issue should not happen with normal applications as those would
normally have the time module initialized early in the process before
waiting on any sequence numbers, but it happens in the test-barrier
application from time to time causing the test suite to hang.

Fix that by making sure we're not calling poll_immediate_wake() under
the seq_mutex.  The time and seq modules are independent and it's hard
to ensure the dependency without exporting some of their internals.
Instead re-defining the prototype of the poll_immediate_wake_at(),
adding the thread safety annotation, so we have some basic protection
from this deadlock if the code ever changes.  Compiler will warn on
the prototype mismatch as well if it ever happens, so it's not a big
problem.  Having this prototype also gives us a spot in the code where
we can place a comment explaining the locking order.

Reportde-at: https://mail.openvswitch.org/pipermail/ovs-dev/2024-July/415436.html
Reported-at: https://issues.redhat.com/browse/FDP-1493
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

diff --git a/lib/seq.c b/lib/seq.c
index 7c2fa0c69f388cc4cd781ef813cc2107f0f0e767..b0e6d5ce42b256583215a0d3e7a6a9934dcaadd0 100644 (file)
--- a/lib/seq.c
+++ b/lib/seq.c
@@ -163,9 +163,14 @@ seq_read(const struct seq *seq)
     return value;
 }
 
+/* poll_immediate_wake_at() must not be called while holding seq_mutex
+ * in order to avoid potential deadlock with time_init() that calls
+ * seq_create() if the timeval module is not initialized yet.  */
+void poll_immediate_wake_at(const char *where) OVS_EXCLUDED(seq_mutex);
+
 static void
 seq_wait__(struct seq *seq, uint64_t value, const char *where)
-    OVS_REQUIRES(seq_mutex)
+    OVS_RELEASES(seq_mutex)
 {
     unsigned int id = ovsthread_id_self();
     uint32_t hash = hash_int(id, 0);
@@ -176,9 +181,11 @@ seq_wait__(struct seq *seq, uint64_t value, const char *where)
             if (waiter->value != value) {
                 /* The current value is different from the value we've already
                  * waited for, */
+                ovs_mutex_unlock(&seq_mutex);
                 poll_immediate_wake_at(where);
             } else {
                 /* Already waiting on 'value', nothing more to do. */
+                ovs_mutex_unlock(&seq_mutex);
             }
             return;
         }
@@ -196,6 +203,7 @@ seq_wait__(struct seq *seq, uint64_t value, const char *where)
         latch_wait_at(&waiter->thread->latch, where);
         waiter->thread->waiting = true;
     }
+    ovs_mutex_unlock(&seq_mutex);
 }
 
 /* Causes the following poll_block() to wake up when 'seq''s sequence number
@@ -219,9 +227,9 @@ seq_wait_at(const struct seq *seq_, uint64_t value, const char *where)
     if (value == seq_read(seq_)) {
         seq_wait__(seq, value, where);
     } else {
+        ovs_mutex_unlock(&seq_mutex);
         poll_immediate_wake_at(where);
     }
-    ovs_mutex_unlock(&seq_mutex);
 }
 
 /* Called by poll_block() just before it returns, this function destroys any