Merge pull request RIOT-OS#21113 from maribu/sys/ztimer/ztimer_mbox_g…

…et_timeout

sys/ztimer: implement ztimer_mbox_get_timeout() and use it to fix race in gnrc_sock_recv()

sys/include/ztimer.h

@@ -264,10 +264,11 @@
 
 #include <stdint.h>
 
-#include "sched.h"
+#include "mbox.h"
 #include "msg.h"
 #include "mutex.h"
 #include "rmutex.h"
+#include "sched.h"
 
 #ifdef __cplusplus
 extern "C" {
@@ -530,6 +531,22 @@ int ztimer_msg_receive_timeout(ztimer_clock_t *clock, msg_t *msg,
 /* created with dist/tools/define2u16.py */
 #define MSG_ZTIMER 0xc83e   /**< msg type used by ztimer_msg_receive_timeout */
 
+/**
+ * @brief Get message from mailbox, blocking with a timeout
+ *
+ * If the mailbox is empty, this function will block until a message becomes
+ * available or the timeout triggers
+ *
+ * @param[in]   clock           ztimer clock to operate on
+ * @param[in]   mbox            ptr to mailbox to operate on
+ * @param[in]   msg             ptr to storage for retrieved message
+ * @param[in]   timeout         relative timeout, in @p clock time units
+ *
+ * @retval  0           Got a message
+ * @retval -ETIMEDOUT   Timeout triggered before a message was obtained
+ */
+int ztimer_mbox_get_timeout(ztimer_clock_t *clock, mbox_t *mbox, msg_t *msg, uint32_t timeout);
+
 /**
  * @brief ztimer_now() for extending timers
  *

sys/net/gnrc/sock/gnrc_sock.c

@@ -28,38 +28,18 @@
 #include "net/ipv6/hdr.h"
 #include "net/udp.h"
 #include "utlist.h"
-#if IS_USED(MODULE_ZTIMER_USEC) || IS_USED(MODULE_ZTIMER_MSEC)
-#include "ztimer.h"
-#endif
-#if IS_USED(MODULE_XTIMER)
-#include "xtimer.h"
-#endif
-
 #include "sock_types.h"
 #include "gnrc_sock_internal.h"
 
+#if IS_USED(MODULE_ZTIMER_USEC) || IS_USED(MODULE_ZTIMER_MSEC)
+#  include "ztimer.h"
+#endif
+
 #ifdef MODULE_FUZZING
 extern gnrc_pktsnip_t *gnrc_pktbuf_fuzzptr;
 gnrc_pktsnip_t *gnrc_sock_prevpkt = NULL;
 #endif
 
-#if IS_USED(MODULE_XTIMER) || IS_USED(MODULE_ZTIMER_USEC) || IS_USED(MODULE_ZTIMER_MSEC)
-#define _TIMEOUT_MAGIC      (0xF38A0B63U)
-#define _TIMEOUT_MSG_TYPE   (0x8474)
-
-static void _callback_put(void *arg)
-{
-    msg_t timeout_msg = { .sender_pid = KERNEL_PID_UNDEF,
-                          .type = _TIMEOUT_MSG_TYPE,
-                          .content = { .value = _TIMEOUT_MAGIC } };
-    gnrc_sock_reg_t *reg = arg;
-
-    /* should be safe, because otherwise if mbox were filled this callback is
-     * senseless */
-    mbox_try_put(&reg->mbox, &timeout_msg);
-}
-#endif
-
 #ifdef SOCK_HAS_ASYNC
 static void _netapi_cb(uint16_t cmd, gnrc_pktsnip_t *pkt, void *ctx)
 {
@@ -121,71 +101,51 @@ ssize_t gnrc_sock_recv(gnrc_sock_reg_t *reg, gnrc_pktsnip_t **pkt_out,
     if (mbox_size(&reg->mbox) != GNRC_SOCK_MBOX_SIZE) {
         return -EINVAL;
     }
-#if IS_USED(MODULE_ZTIMER_USEC)
-    ztimer_t timeout_timer = { .base = { .next = NULL } };
-    if ((timeout != SOCK_NO_TIMEOUT) && (timeout != 0)) {
-        timeout_timer.callback = _callback_put;
-        timeout_timer.arg = reg;
-        ztimer_set(ZTIMER_USEC, &timeout_timer, timeout);
-    }
-#elif IS_USED(MODULE_ZTIMER_MSEC)
-    ztimer_t timeout_timer = { .base = { .next = NULL } };
-    if ((timeout != SOCK_NO_TIMEOUT) && (timeout != 0)) {
-        timeout_timer.callback = _callback_put;
-        timeout_timer.arg = reg;
-        ztimer_set(ZTIMER_MSEC, &timeout_timer, DIV_ROUND_INF(timeout, US_PER_MS));
-    }
-#elif IS_USED(MODULE_XTIMER)
-    xtimer_t timeout_timer = { .callback = NULL };
 
-    /* xtimer_spin would make this never receive anything.
-     * Avoid that by setting the minimal not spinning timeout. */
-    if (timeout < XTIMER_BACKOFF && timeout > 0) {
-        timeout = XTIMER_BACKOFF;
-    }
-
-    if ((timeout != SOCK_NO_TIMEOUT) && (timeout != 0)) {
-        timeout_timer.callback = _callback_put;
-        timeout_timer.arg = reg;
-        xtimer_set(&timeout_timer, timeout);
-    }
-#else
-    assume((timeout == SOCK_NO_TIMEOUT) || (timeout == 0));
-#endif
-    if (timeout != 0) {
 #if defined(DEVELHELP) && IS_ACTIVE(SOCK_HAS_ASYNC)
-        if (reg->async_cb.generic) {
-            /* this warning is a false positive when sock_*_recv() was not called from
-             * the asynchronous handler */
-            LOG_WARNING("gnrc_sock: timeout != 0 within the asynchronous callback lead "
-                        "to unexpected delays within the asynchronous handler.\n");
-        }
+    if ((timeout != 0) && (reg->async_cb.generic)) {
+        /* this warning is a false positive when sock_*_recv() was not called from
+         * the asynchronous handler */
+        LOG_WARNING("gnrc_sock: timeout != 0 within the asynchronous callback lead "
+                    "to unexpected delays within the asynchronous handler.\n");
+    }
 #endif
+
+    if (timeout == SOCK_NO_TIMEOUT) {
         mbox_get(&reg->mbox, &msg);
     }
-    else {
+    else if (timeout == 0) {
         if (!mbox_try_get(&reg->mbox, &msg)) {
             return -EAGAIN;
         }
     }
-#if IS_USED(MODULE_ZTIMER_USEC)
-    ztimer_remove(ZTIMER_USEC, &timeout_timer);
-#elif IS_USED(MODULE_ZTIMER_MSEC)
-    ztimer_remove(ZTIMER_MSEC, &timeout_timer);
-#elif IS_USED(MODULE_XTIMER)
-    xtimer_remove(&timeout_timer);
-#endif
+    else {
+        /* Preferring low power over us precision here if both options are
+         * possible. This is typically the better trade-off, as even on fast
+         * networks round-trip-times are typically measured in ms rather than
+         * in us */
+        if (IS_USED(MODULE_ZTIMER_MSEC)) {
+            /* rounding up seems more sensible here */
+            uint32_t timeout_ms = (timeout + US_PER_MS - 1) / US_PER_MS;
+            if (ztimer_mbox_get_timeout(ZTIMER_MSEC, &reg->mbox, &msg, timeout_ms)) {
+                return -ETIMEDOUT;
+            }
+        }
+        else if (IS_USED(MODULE_ZTIMER_USEC)) {
+            if (ztimer_mbox_get_timeout(ZTIMER_USEC, &reg->mbox, &msg, timeout)) {
+                return -ETIMEDOUT;
+            }
+        }
+        else {
+            /* cannot do timeout without a timer */
+            assert(0);
+            return -ENOTSUP;
+        }
+    }
     switch (msg.type) {
         case GNRC_NETAPI_MSG_TYPE_RCV:
             pkt = msg.content.ptr;
             break;
-#if IS_USED(MODULE_XTIMER) || IS_USED(MODULE_ZTIMER_USEC) || IS_USED(MODULE_ZTIMER_MSEC)
-        case _TIMEOUT_MSG_TYPE:
-            if (msg.content.value == _TIMEOUT_MAGIC) {
-                return -ETIMEDOUT;
-            }
-#endif
-            /* Falls Through. */
         default:
             return -EINVAL;
     }

sys/ztimer/util.c

@@ -30,6 +30,9 @@
 #include "thread.h"
 #include "ztimer.h"
 
+#define ENABLE_DEBUG    0
+#include "debug.h"
+
 static void _callback_unlock_mutex(void *arg)
 {
     mutex_t *mutex = (mutex_t *)arg;
@@ -128,6 +131,49 @@ int ztimer_msg_receive_timeout(ztimer_clock_t *clock, msg_t *msg,
 
 #endif /* MODULE_CORE_MSG */
 
+#ifdef MODULE_CORE_MBOX
+struct ztimer_mbox_thread_status {
+    ztimer_t timer;
+    mbox_t *mbox;
+    thread_t *thread;
+    int status;
+};
+
+static void _ztimer_mbox_get_timeout(void *arg)
+{
+    struct ztimer_mbox_thread_status *data = arg;
+    if (!list_remove(&data->mbox->readers, &data->thread->rq_entry)) {
+        /* timeout triggered just after the message was received but before the
+         * timer was canceled. --> ignore the timeout */
+        DEBUG_PUTS("ztimer_mbox_get_timeout: timeout triggered, but message already received");
+        return;
+    }
+
+    sched_set_status(data->thread, STATUS_PENDING);
+    data->status = -ETIMEDOUT;
+    thread_yield_higher();
+    DEBUG_PUTS("ztimer_mbox_get_timeout: timeout triggered");
+}
+
+int ztimer_mbox_get_timeout(ztimer_clock_t *clock, mbox_t *mbox, msg_t *msg, uint32_t timeout)
+{
+    struct ztimer_mbox_thread_status data = {
+        .timer = {
+            .arg = &data,
+            .callback = _ztimer_mbox_get_timeout,
+        },
+        .mbox = mbox,
+        .thread = thread_get_active(),
+        .status = 0,
+    };
+
+    ztimer_set(clock, &data.timer, timeout);
+    mbox_get(mbox, msg);
+    ztimer_remove(clock, &data.timer);
+    return data.status;
+}
+#endif
+
 #ifdef MODULE_CORE_THREAD_FLAGS
 static void _set_timeout_flag_callback(void *arg)
 {

tests/sys/ztimer_mbox_get_timeout/Makefile

@@ -0,0 +1,7 @@
+include ../Makefile.sys_common
+
+USEMODULE += ztimer_usec
+USEMODULE += core_thread_flags
+USEMODULE += core_mbox
+
+include $(RIOTBASE)/Makefile.include

tests/sys/ztimer_mbox_get_timeout/Makefile.ci

@@ -0,0 +1,4 @@
+BOARD_INSUFFICIENT_MEMORY := \
+    atmega8 \
+    nucleo-l011k4 \
+    #