Merge pull request #18740 from maribu/chrysn-pull-requests/gnrc-netreg-lock

gnrc_netreg: Use locks around netreg
2025-01-18 12:52:44 +01:00 · 2022-10-14 15:10:35 +02:00 · 2022-10-14 15:10:35 +02:00 · 5d5e8d6163
commit 5d5e8d6163
parent ef8bd7c114 982ec4dcda
6 changed files with 212 additions and 0 deletions
--- a/examples/cord_ep/Makefile.ci
+++ b/examples/cord_ep/Makefile.ci
@ -27,6 +27,8 @@ BOARD_INSUFFICIENT_MEMORY := \
    nucleo-l031k6 \
    nucleo-l053r8 \
    samd10-xmini \
+    saml10-xpro \
+    saml11-xpro \
    slstk3400a \
    stk3200 \
    stm32f030f4-demo \
--- a/sys/include/net/gnrc/netreg.h
+++ b/sys/include/net/gnrc/netreg.h
@ -212,6 +212,44 @@ typedef struct gnrc_netreg_entry {
    } target;                   /**< Target for the registry entry */
 } gnrc_netreg_entry_t;

+/**
+ * @brief   The global locking of netregs
+ *
+ * A shared lock must be held across calls to @ref gnrc_netreg_lookup, and
+ * references obtained through that may only be used for the duration for which
+ * the lock is held.
+ *
+ * The shared lock is counting (i.e. multiple sections of code can acquire it
+ * concurrently), and may be held across blocking operations, as long as they
+ * don't involve the exclusive lock (e.g. by blocking on a socket creation).
+ * It's generally good practice to release them as soon as possible.
+ *
+ * There is an exclusive counterpart to the lock, which is
+ * internal to netreg (and used through functions such as @ref
+ * gnrc_netreg_register and @ref gnrc_netreg_unregister). The current
+ * implementation priorizes shared locks. This means that shared locks are
+ * generally acquired fast (they only block if an exclusive operation has
+ * already started), but constant access through shared locks might starve
+ * registration and deregistration.
+ *
+ * @{
+ */
+
+/** @brief Acquire a shared lock on the GNRC netreg
+ *
+ * This needs to be held around all calls to @ref gnrc_netreg_lookup, and for
+ * as long as any of its results are used. After that, call @ref
+ * gnrc_netreg_release_shared.
+ */
+void gnrc_netreg_acquire_shared(void);
+/** @brief Release a shared lock on the GNRC netreg
+ *
+ * @pre @ref gnrc_netreg_acquire_shared was called
+ */
+void gnrc_netreg_release_shared(void);
+
+/** @} */
+
 /**
 * @brief   Initializes module.
 */
@ -326,6 +364,10 @@ void gnrc_netreg_unregister(gnrc_nettype_t type, gnrc_netreg_entry_t *entry);
 * @brief   Searches for entries with given parameters in the registry and
 *          returns the first found.
 *
+ * @pre The caller must hold the lock of @ref gnrc_netreg_acquire_shared from
+ *      before calling this function, and must stop using any obtained pointers
+ *      before releasing the lock through @ref gnrc_netreg_release_shared.
+ *
 * @param[in] type      Type of the protocol.
 * @param[in] demux_ctx The demultiplexing context for the registered thread.
 *                      See gnrc_netreg_entry_t::demux_ctx.
@ -345,6 +387,13 @@ gnrc_netreg_entry_t *gnrc_netreg_lookup(gnrc_nettype_t type, uint32_t demux_ctx)
 *
 * @return  Number of entries with the same gnrc_netreg_entry_t::type and
 *          gnrc_netreg_entry_t::demux_ctx as the given parameters.
+ *
+ * Note that this returns a snapshot value, which may change at any time after
+ * that call. This is fine for most applications, as they just shortcut a code
+ * path if the number is zero. Callers that need that number to stay constant
+ * can acquire a shared lock through @ref gnrc_netreg_acquire_shared, and rely
+ * on the number staying constant until that lock is released through @ref
+ * gnrc_netreg_release_shared.
 */
 int gnrc_netreg_num(gnrc_nettype_t type, uint32_t demux_ctx);

@ -353,6 +402,11 @@ int gnrc_netreg_num(gnrc_nettype_t type, uint32_t demux_ctx);
 *          gnrc_netreg_entry_t::type and gnrc_netreg_entry_t::demux_ctx as the
 *          given entry.
 *
+ * The requirement on holding the global lock through @ref
+ * gnrc_netreg_acquire_shared from @ref gnrc_netreg_lookup extends to any
+ * results of this function: It may only be released when none of the pointers
+ * are used any more.
+ *
 * @param[in] entry     A registry entry retrieved by gnrc_netreg_lookup() or
 *                      gnrc_netreg_getnext(). Must not be NULL.
 *
--- a/sys/net/gnrc/netapi/gnrc_netapi.c
+++ b/sys/net/gnrc/netapi/gnrc_netapi.c
@ -85,6 +85,8 @@ static inline int _snd_rcv_mbox(mbox_t *mbox, uint16_t type, gnrc_pktsnip_t *pkt
 int gnrc_netapi_dispatch(gnrc_nettype_t type, uint32_t demux_ctx,
                         uint16_t cmd, gnrc_pktsnip_t *pkt)
 {
+    gnrc_netreg_acquire_shared();
+
    int numof = gnrc_netreg_num(type, demux_ctx);

    if (numof != 0) {
@ -134,5 +136,7 @@ int gnrc_netapi_dispatch(gnrc_nettype_t type, uint32_t demux_ctx,
        }
    }

+    gnrc_netreg_release_shared();
+
    return numof;
 }
--- a/sys/net/gnrc/netreg/gnrc_netreg.c
+++ b/sys/net/gnrc/netreg/gnrc_netreg.c
@ -14,6 +14,7 @@

 #include <errno.h>
 #include <string.h>
+#include <limits.h>

 #include "assert.h"
 #include "log.h"
@ -33,12 +34,101 @@
 /* The registry as lookup table by gnrc_nettype_t */
 static gnrc_netreg_entry_t *netreg[GNRC_NETTYPE_NUMOF];

+/** Held while accessing _lock_counter, and also while the exclusive lock is held */
+static mutex_t _lock_for_counter = MUTEX_INIT;
+/** Number of shared locks on netreg. Saturating arithmetic is used; if this
+ * reaches UINT_MAX, the lock will never be freed again. This is likely
+ * accurate, given that it will only happen when the lock was leaked, so it
+ * can't be freed any more anyway.
+ *
+ * This can be accessed only when _lock_for_counter is held; an alternative
+ * implementation with atomics would likely be possible, but for the case of "I
+ * can't do that right now" it would still need a mutex. But that's
+ * optimization that could be done without changing the public API.
+ * */
+static unsigned int _lock_counter = 0;
+/** Held while the netreg lists are being read. This is what the exclusive
+ * users block on if they can't grab the exclusive lock right away.
+ *
+ * This is largely used as a boolean flag (is locked / is unlocked) that is
+ * modified while the _lock_for_counter is held, and will not block because it
+ * is synchronized to _lock_counter being 0. It is only ever accessed outside
+ * _lock_for_counter when waiting for the counter to reach 0, and even then is
+ * released immediately to avoid deadlocks. (Instead, the exclusive acquisition
+ * tries to acquire the _lock_for_counter, and if that fails it queues up again
+ * after the reader that just snatched it).
+ * */
+static mutex_t _lock_wait_exclusive = MUTEX_INIT;
+
 void gnrc_netreg_init(void)
 {
    /* set all pointers in registry to NULL */
    memset(netreg, 0, GNRC_NETTYPE_NUMOF * sizeof(gnrc_netreg_entry_t *));
 }

+void gnrc_netreg_acquire_shared(void) {
+    mutex_lock(&_lock_for_counter);
+    if (_lock_counter == 0) {
+        /* At most, this blocks for the very short time until
+         * _gnrc_netreg_acquire_exclusive returns it immediately */
+        mutex_lock(&_lock_wait_exclusive);
+    }
+    if (_lock_counter != UINT_MAX) {
+        _lock_counter += 1;
+    }
+    mutex_unlock(&_lock_for_counter);
+}
+
+void gnrc_netreg_release_shared(void) {
+    mutex_lock(&_lock_for_counter);
+
+    assert(_lock_counter != 0); /* Release without acquire */
+
+    if (_lock_counter != UINT_MAX) {
+        _lock_counter -= 1;
+    }
+    if (_lock_counter == 0) {
+        mutex_unlock(&_lock_wait_exclusive);
+    }
+    mutex_unlock(&_lock_for_counter);
+}
+
+/** Assert that there is a shared lock on gnrc_netreg -- this should help weed
+ * out callers to @ref gnrc_netreg_lookup that don't properly lock. */
+static void _gnrc_netreg_assert_shared(void) {
+#if DEVELHELP
+    /* Even if we just peek: It's not an atomic, so it needs synchronization */
+    mutex_lock(&_lock_for_counter);
+    assert(_lock_counter != 0);
+    mutex_unlock(&_lock_for_counter);
+#endif
+}
+
+static void _gnrc_netreg_acquire_exclusive(void) {
+    while (true) {
+        mutex_lock(&_lock_for_counter);
+        if (_lock_counter == 0) {
+            /* At most, this blocks for the very short time until
+             * another caller of _gnrc_netreg_acquire_exclusive returns it
+             * immediately */
+            mutex_lock(&_lock_wait_exclusive);
+            /* Leaving both locked */
+            return;
+        }
+        mutex_unlock(&_lock_for_counter);
+
+        mutex_lock(&_lock_wait_exclusive);
+        /* ... but maybe someone just started grabbing _lock_for_counter, so we
+         * give them a chance to finish rather than deadlocking them */
+        mutex_unlock(&_lock_wait_exclusive);
+    }
+}
+
+static void _gnrc_netreg_release_exclusive(void) {
+     mutex_unlock(&_lock_wait_exclusive);
+     mutex_unlock(&_lock_for_counter);
+}
+
 int gnrc_netreg_register(gnrc_nettype_t type, gnrc_netreg_entry_t *entry)
 {
 #if DEVELHELP
@ -61,7 +151,9 @@ int gnrc_netreg_register(gnrc_nettype_t type, gnrc_netreg_entry_t *entry)
        return -EINVAL;
    }

+    _gnrc_netreg_acquire_exclusive();
    LL_PREPEND(netreg[type], entry);
+    _gnrc_netreg_release_exclusive();

    return 0;
 }
@ -72,7 +164,13 @@ void gnrc_netreg_unregister(gnrc_nettype_t type, gnrc_netreg_entry_t *entry)
        return;
    }

+    _gnrc_netreg_acquire_exclusive();
    LL_DELETE(netreg[type], entry);
+    /* We can release now already: No new references to this entry can be made
+     * any more, and the caller is only allowed to reuse the entry and the mbox
+     * target referenced by it after *this* function returned, not when the
+     * lock becomes available again. */
+    _gnrc_netreg_release_exclusive();

 #if defined(MODULE_GNRC_NETAPI_MBOX)
    /* drain packets still in the mbox */
@ -101,6 +199,8 @@ static gnrc_netreg_entry_t *_netreg_lookup(gnrc_netreg_entry_t *from,
                                           gnrc_nettype_t type,
                                           uint32_t demux_ctx)
 {
+    _gnrc_netreg_assert_shared();
+
    gnrc_netreg_entry_t *res = NULL;

    if (from || !_INVALID_TYPE(type)) {
@ -121,9 +221,14 @@ int gnrc_netreg_num(gnrc_nettype_t type, uint32_t demux_ctx)
    int num = 0;
    gnrc_netreg_entry_t *entry = NULL;

+    gnrc_netreg_acquire_shared();
+
    while((entry = _netreg_lookup(entry, type, demux_ctx)) != NULL) {
        num++;
    }
+
+    gnrc_netreg_release_shared();
+
    return num;
 }

--- a/tests/gcoap_fileserver/Makefile.ci
+++ b/tests/gcoap_fileserver/Makefile.ci
@ -8,6 +8,7 @@ BOARD_INSUFFICIENT_MEMORY := \
    atmega328p \
    atmega328p-xplained-mini \
    atxmega-a1-xplained \
+    atxmega-a1u-xpro \
    atxmega-a3bu-xplained \
    blackpill \
    bluepill \
--- a/tests/unittests/tests-netreg/tests-netreg.c
+++ b/tests/unittests/tests-netreg/tests-netreg.c
@ -38,23 +38,34 @@ static void test_netreg_register__inval_numof(void)

 static void test_netreg_register__success(void)
 {
+    gnrc_netreg_acquire_shared();
    gnrc_netreg_entry_t *res = gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16);

    TEST_ASSERT_NULL(res);
+    gnrc_netreg_release_shared();

    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0]));
+    gnrc_netreg_acquire_shared();
    TEST_ASSERT_NOT_NULL((res = gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16)));
    TEST_ASSERT_EQUAL_INT(TEST_UINT16, res->demux_ctx);
    TEST_ASSERT_EQUAL_INT(TEST_UINT8, res->target.pid);
    TEST_ASSERT_NULL((gnrc_netreg_getnext(res)));
+    gnrc_netreg_release_shared();
 }

 void test_netreg_unregister__success(void)
 {
    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0]));
+
+    gnrc_netreg_acquire_shared();
    TEST_ASSERT_NOT_NULL(gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16));
+    gnrc_netreg_release_shared();
+
    gnrc_netreg_unregister(GNRC_NETTYPE_TEST, &entries[0]);
+
+    gnrc_netreg_acquire_shared();
    TEST_ASSERT_NULL(gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16));
+    gnrc_netreg_release_shared();
 }

 void test_netreg_unregister__success2(void)
@ -64,11 +75,18 @@ void test_netreg_unregister__success2(void)
    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0]));
    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[1]));
    gnrc_netreg_unregister(GNRC_NETTYPE_TEST, &entries[0]);
+
+    gnrc_netreg_acquire_shared();
    TEST_ASSERT_NOT_NULL((res = gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16)));
    TEST_ASSERT_EQUAL_INT(TEST_UINT16, res->demux_ctx);
    TEST_ASSERT_EQUAL_INT(TEST_UINT8 + 1, res->target.pid);
+    gnrc_netreg_release_shared();
+
    gnrc_netreg_unregister(GNRC_NETTYPE_TEST, &entries[1]);
+
+    gnrc_netreg_acquire_shared();
    TEST_ASSERT_NULL(gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16));
+    gnrc_netreg_release_shared();
 }

 void test_netreg_unregister__success3(void)
@ -78,56 +96,82 @@ void test_netreg_unregister__success3(void)
    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0]));
    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[1]));
    gnrc_netreg_unregister(GNRC_NETTYPE_TEST, &entries[1]);
+
+    gnrc_netreg_acquire_shared();
    TEST_ASSERT_NOT_NULL((res = gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16)));
    TEST_ASSERT_EQUAL_INT(TEST_UINT16, res->demux_ctx);
    TEST_ASSERT_EQUAL_INT(TEST_UINT8, res->target.pid);
+    gnrc_netreg_release_shared();
+
    gnrc_netreg_unregister(GNRC_NETTYPE_TEST, &entries[0]);
+
+    gnrc_netreg_acquire_shared();
    TEST_ASSERT_NULL(gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16));
+    gnrc_netreg_release_shared();
 }

 void test_netreg_lookup__wrong_type_undef(void)
 {
    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0]));
+    gnrc_netreg_acquire_shared();
    TEST_ASSERT_NULL(gnrc_netreg_lookup(GNRC_NETTYPE_UNDEF, TEST_UINT16));
+    gnrc_netreg_release_shared();
 }

 void test_netreg_lookup__wrong_type_numof(void)
 {
    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0]));
+    gnrc_netreg_acquire_shared();
    TEST_ASSERT_NULL(gnrc_netreg_lookup(GNRC_NETTYPE_NUMOF, TEST_UINT16));
+    gnrc_netreg_release_shared();
 }

 void test_netreg_num__empty(void)
 {
+    gnrc_netreg_acquire_shared();
    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_num(GNRC_NETTYPE_TEST, TEST_UINT16));
    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_num(GNRC_NETTYPE_TEST, TEST_UINT16 + 1));
    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_num(GNRC_NETTYPE_TEST, GNRC_NETREG_DEMUX_CTX_ALL));
+    gnrc_netreg_release_shared();
 }

 void test_netreg_num__wrong_type_undef(void)
 {
    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0]));
+    gnrc_netreg_acquire_shared();
    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_num(GNRC_NETTYPE_UNDEF, TEST_UINT16));
+    gnrc_netreg_release_shared();
 }

 void test_netreg_num__wrong_type_numof(void)
 {
    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0]));
+    gnrc_netreg_acquire_shared();
    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_num(GNRC_NETTYPE_NUMOF, TEST_UINT16));
+    gnrc_netreg_release_shared();
 }

 void test_netreg_num__2_entries(void)
 {
    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0]));
+
+    gnrc_netreg_acquire_shared();
    TEST_ASSERT_EQUAL_INT(1, gnrc_netreg_num(GNRC_NETTYPE_TEST, TEST_UINT16));
+    gnrc_netreg_release_shared();
+
    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[1]));
+
+    gnrc_netreg_acquire_shared();
    TEST_ASSERT_EQUAL_INT(2, gnrc_netreg_num(GNRC_NETTYPE_TEST, TEST_UINT16));
+    gnrc_netreg_release_shared();
 }

 void test_netreg_getnext__NULL(void)
 {
    TEST_ASSERT_EQUAL_INT(0, gnrc_netreg_register(GNRC_NETTYPE_TEST, &entries[0]));
+    gnrc_netreg_acquire_shared();
    TEST_ASSERT_NULL(gnrc_netreg_getnext(NULL));
+    gnrc_netreg_release_shared();
 }

 void test_netreg_getnext__2_entries(void)
@ -135,8 +179,10 @@ void test_netreg_getnext__2_entries(void)
    gnrc_netreg_entry_t *res = NULL;

    test_netreg_num__2_entries();
+    gnrc_netreg_acquire_shared();
    TEST_ASSERT_NOT_NULL((res = gnrc_netreg_lookup(GNRC_NETTYPE_TEST, TEST_UINT16)));
    TEST_ASSERT_NOT_NULL(gnrc_netreg_getnext(res));
+    gnrc_netreg_release_shared();
 }

 Test *tests_netreg_tests(void)