Merge #19601

19601: SUIT: Prepared manifests r=chrysn a=chrysn

### Contribution description

While SUIT can generally be used already with manifests that are "dropped into memory" (by any mechanism), the convenient SUIT worker thread mechanism so far could not be used with it.

This adds the suit_worker_try_prepare / suit_worker_trigger_prepared
pair for using the SUIT worker, and breaks suit_handle_manifest_buf out
of suit_handle_url (where the latter now calls the former).

#### By-catch

As part of factoring out reaping of the zombie worker thread, a locking
error that deadlocks the SUIT worker in case the race between the mutex
being unlocked and the thread being reaped hits the necessary handling
code is fixed (and mutex_unlock is called in the error path).

### Testing procedure

SUIT tests should pass.

https://github.com/RIOT-OS/RIOT/pull/19659 provides a demo of how the new API is used.

### Issues/PRs references

I think that the currently employed mechanism of having a resource to which a URL with the manifest gets posted is contrary to the design goals of SUIT -- the signed manifest should be what justifies the device to spend resources (eg. get data from a server), not a URI that is just *not* signed.

This PR makes it easier to implement a resource to which the manifest can be POSTed, rather than a CoAP URI that represents the manifest, as is proposed (but not mature) in 
https://github.com/RIOT-OS/RIOT/pull/19659.

[edit: Adjusted to reflect decisions made during review]

Co-authored-by: chrysn <chrysn@fsfe.org>

sys/include/suit/transport/worker.h

@@ -32,7 +32,7 @@ extern "C" {
 #endif
 
 /**
- * @brief   Trigger a SUIT udate via a worker thread
+ * @brief   Trigger a SUIT update via a worker thread
  *
  * @param[in] url       url pointer containing the full coap url to the manifest
  * @param[in] len       length of the url
@@ -40,11 +40,59 @@ extern "C" {
 void suit_worker_trigger(const char *url, size_t len);
 
 /**
- * @brief   Trigger a SUIT udate
+ * @brief   Trigger a SUIT update via a worker thread
+ *
+ * @pre The caller called @ref suit_worker_try_prepare to obtain the @p buf, and
+ * populated @p size bytes into it.
+ *
+ * This can be called with a size of 0 when writing a manifest was started, but
+ * could not be completed.
+ *
+ * @param[in] manifest  Pointer to the manifest. This must be the return value
+ *                      of a previous call to @ref suit_worker_try_prepare, and is
+ *                      invalidated at some point during or after the call.
+ * @param[in] size      Number of bytes that have been prepared in
+ *                      the buffer before the call.
+ */
+void suit_worker_trigger_prepared(const uint8_t *manifest, size_t size);
+
+/**
+ * @brief    Prepare for a worker run with a preloaded manifest
+ *
+ * This obtains a lock on the SUIT worker, and provides a pointer to a memory
+ * area into which the manifest is to be written. The lock must be released by
+ * calling @ref suit_worker_trigger_prepared later.
+ *
+ * @param[out]   buffer    On success, buffer into which the image may be
+ *                         written.
+ * @param[inout] size      Requested buffer size. On some errors, this will be
+ *                         decreased to a size that would be acceptable.
+ *
+ * @return 0 on success
+ * @return -EAGAIN if the worker is currently busy.
+ * @return -ENOMEM if the worker is available but the requested size is too
+ *         large. (In this case, an acceptable size is indicated in size).
+ *
+ * @note There is no blocking version of this (it behaves like @ref
+ * mutex_trylock, not like @ref mutex_lock). This allows a simpler
+ * implementation on the thread handling side<!-- possibly it's not even
+ * possible without priority inversion tricks -->, and is also what typical use
+ * cases need.
+ */
+int suit_worker_try_prepare(uint8_t **buffer, size_t *size);
+
+/**
+ * @brief   Trigger a SUIT update
  *
  * @note Make sure the thread calling this function has enough stack space to fit
  *       a whole flash page.
  *
+ * This function accesses global state shared with @ref
+ * suit_handle_manifest_buf; only one of those may be called at the same time.
+ * You may use @ref suit_worker_trigger instead, which manages locking and also
+ * does away with the stack space requirement by being executed on an own
+ * stack.
+ *
  * @param[in] url       url pointer containing the full coap url to the manifest
  *
  * @return 0 on success
@@ -52,6 +100,26 @@ void suit_worker_trigger(const char *url, size_t len);
  */
 int suit_handle_url(const char *url);
 
+/**
+ * @brief   Trigger a SUIT update on an in-memory manifest
+ *
+ * @note Make sure the thread calling this function has enough stack space to fit
+ *       a whole flash page.
+ *
+ * This function accesses global state shared with @ref suit_handle_url; only
+ * one of those may be called at the same time. You may use @ref
+ * suit_worker_try_prepare / @ref suit_worker_trigger_prepared instead, which
+ * manage locking and also do away with the stack space requirement by being
+ * executed on an own stack.
+ *
+ * @param[in] buffer     Memory area containing a SUIT manifest.
+ * @param[in] size       Size of the manifest in @p buffer.
+ *
+ * @return 0 on success
+ *        <0 on error
+ */
+int suit_handle_manifest_buf(const uint8_t *buffer, size_t size);
+
 #ifdef __cplusplus
 }
 #endif

sys/suit/transport/worker.c

@@ -35,6 +35,8 @@
 #include "periph/pm.h"
 #include "ztimer.h"
 
+#include "suit/transport/worker.h"
+
 #ifdef MODULE_SUIT_TRANSPORT_COAP
 #include "net/nanocoap_sock.h"
 #include "suit/transport/coap.h"
@@ -70,12 +72,14 @@
 #define SUIT_COAP_WORKER_PRIO THREAD_PRIORITY_MAIN - 1
 #endif
 
+/** Maximum size of SUIT manifests processable by the suit_worker mechanisms */
 #ifndef SUIT_MANIFEST_BUFSIZE
 #define SUIT_MANIFEST_BUFSIZE   640
 #endif
 
 static char _stack[SUIT_WORKER_STACKSIZE];
 static char _url[CONFIG_SOCK_URLPATH_MAXLEN];
+static ssize_t _size;
 static uint8_t _manifest_buf[SUIT_MANIFEST_BUFSIZE];
 
 static mutex_t _worker_lock;
@@ -114,7 +118,11 @@ int suit_handle_url(const char *url)
 
     LOG_INFO("suit_worker: got manifest with size %u\n", (unsigned)size);
 
-#ifdef MODULE_SUIT
+    return suit_handle_manifest_buf(_manifest_buf, size);
+}
+
+int suit_handle_manifest_buf(const uint8_t *buffer, size_t size)
+{
     suit_manifest_t manifest;
     memset(&manifest, 0, sizeof(manifest));
 
@@ -122,11 +130,11 @@ int suit_handle_url(const char *url)
     manifest.urlbuf_len = CONFIG_SOCK_URLPATH_MAXLEN;
 
     int res;
-    if ((res = suit_parse(&manifest, _manifest_buf, size)) != SUIT_OK) {
+    if ((res = suit_parse(&manifest, buffer, size)) != SUIT_OK) {
         LOG_INFO("suit_worker: suit_parse() failed. res=%i\n", res);
         return res;
     }
-#endif
+
 #ifdef MODULE_SUIT_STORAGE_FLASHWRITE
     const riotboot_hdr_t *hdr = riotboot_slot_get_hdr(riotboot_slot_other());
     riotboot_hdr_print(hdr);
@@ -144,7 +152,14 @@ static void *_suit_worker_thread(void *arg)
 
     LOG_INFO("suit_worker: started.\n");
 
-    if (suit_handle_url(_url) == 0) {
+    int res;
+    if (_url[0] == '\0') {
+        res = suit_handle_manifest_buf(_manifest_buf, _size);
+    } else {
+        res = suit_handle_url(_url);
+    }
+
+    if (res == 0) {
         LOG_INFO("suit_worker: update successful\n");
         if (IS_USED(MODULE_SUIT_STORAGE_FLASHWRITE)) {
             LOG_INFO("suit_worker: rebooting...\n");
@@ -161,18 +176,40 @@ static void *_suit_worker_thread(void *arg)
     return NULL;
 }
 
-void suit_worker_trigger(const char *url, size_t len)
+/** Reap the zombie worker, or return false if it's still running
+ *
+ * To call this, the _worker_lock must be held.
+ *
+ * In the rare case of the worker still running, this releases the
+ * _worker_lock, and the caller needs to start over, acquire the _worker_lock
+ * and possibly populate the manifest buffer, reap the worker again, and then
+ * start the worker thread (with a URL if the manifest was not populated).
+ *
+ * Otherwise, the worker thread will eventually unlock the mutex.
+ * */
+static bool _worker_reap(void)
 {
-    mutex_lock(&_worker_lock);
     if (_worker_pid != KERNEL_PID_UNDEF) {
         if (thread_kill_zombie(_worker_pid) != 1) {
             /* This will only happen if the SUIT thread runs on a lower
              * priority than the caller */
             LOG_WARNING("Ignoring SUIT trigger: worker is still busy.\n");
-            return;
+            mutex_unlock(&_worker_lock);
+            return false;
         }
     }
+    return true;
+}
 
+void suit_worker_trigger(const char *url, size_t len)
+{
+    mutex_lock(&_worker_lock);
+    if (!_worker_reap()) {
+        return;
+    }
+
+    assert(len != 0); /* A zero-length URI is invalid, but _url[0] == '\0' is
+                         special to _suit_worker_thread */
     memcpy(_url, url, len);
     _url[len] = '\0';
 
@@ -180,3 +217,44 @@ void suit_worker_trigger(const char *url, size_t len)
                   THREAD_CREATE_STACKTEST,
                   _suit_worker_thread, NULL, "suit worker");
 }
+
+void suit_worker_trigger_prepared(const uint8_t *buffer, size_t size)
+{
+    /* As we're being handed the data and the lock (and not just any manifest),
+     * we can only accept what was handed out in suit_worker_try_prepare. */
+    (void)buffer;
+    assert(buffer = _manifest_buf);
+
+    if (!_worker_reap()) {
+        return;
+    }
+
+    _url[0] = '\0';
+    _size = size;
+
+    if (size == 0) {
+        _worker_pid = KERNEL_PID_UNDEF;
+        mutex_unlock(&_worker_lock);
+        return;
+    }
+
+    _worker_pid = thread_create(_stack, SUIT_WORKER_STACKSIZE, SUIT_COAP_WORKER_PRIO,
+                  THREAD_CREATE_STACKTEST,
+                  _suit_worker_thread, NULL, "suit worker");
+}
+
+int suit_worker_try_prepare(uint8_t **buffer, size_t *size)
+{
+    if (!mutex_trylock(&_worker_lock)) {
+        return -EAGAIN;
+    }
+
+    if (*size > SUIT_MANIFEST_BUFSIZE) {
+        *size = SUIT_MANIFEST_BUFSIZE;
+        mutex_unlock(&_worker_lock);
+        return -ENOMEM;
+    }
+
+    *buffer = _manifest_buf;
+    return 0;
+}