Fix driver open modes and mount lifetime during hotplug work

CMakeLists.txt

@@ -91,7 +91,7 @@ dmod_add_library(${DMOD_MODULE_NAME} ${DMOD_MODULE_VERSION}
 # for them.
 dmod_link_modules(${DMOD_MODULE_NAME}
     dmfsi
-    dmdrvi@>=2.6
+    dmdrvi
     dmini
     dmlist
     dmosi

docs/open-mode-validation.md

@@ -0,0 +1,54 @@
+# Device access and mount lifetime validation
+
+DMFSI represents read/write access as 3, whereas DMDRVI uses 4. dmdevfs now
+maps the access bits at its filesystem/driver boundary and preserves the
+existing non-access flags. Invalid access mode zero is rejected before calling
+the driver. Each test handle records the actual flags received by the driver.
+
+Validation on 2026-10-16:
+
+- Native `test_dmdevfs`: 10/10 steps, including the new read, write, read/write,
+  create/truncate, append and invalid-mode regression.
+- Native `test_partitions` passed; STM32F7 build passed.
+- STM32F746G-DISCO booted with the changed dmdevfs: RTC 11/11 and RNG 6/6.
+  The generic rtc CLI sets time through an `r+b` VFS handle successfully.
+- Before the lifetime fix below, full native ctest: 7/9 targets passed. `test_dmdevmon` and `test_order`
+  terminate with SIGSEGV in repeated mount/configuration operations. Both
+  reproduce with the unchanged `origin/main` dmdevfs source and the same test
+  environment (exit -11). A GDB capture of test_order places the fault in the
+  hotplug thread's Dmod_GetDifFunction/Dmod_Context_IsValid with an invalid
+  module-context pointer. Those failures are not covered by the passing
+  access-mode regression; the lifetime fix below resolves them.
+
+The test CMake scope clears inherited DMOD_MAL_IMPLS: the filesystem, rather
+than its consumers, implements dmdrvi notification callbacks. Dependencies are
+unversioned in CMake; the packager derives required API versions.
+
+## Mount/worker lifetime fix
+
+The class-probing worker released g_devfs_mutex after its live-mount lookup,
+then accessed the mount's driver nodes and module context without retaining
+them. Concurrent filesystem deinit removed the mount from the registry and
+freed those objects. Repeated mount/unmount tests therefore raced a real
+use-after-free, independently of filesystem open-mode translation.
+
+Worker operations now retain a mount reference under the registry mutex.
+Unmount unregisters it to block new worker references, then sleeps on a
+per-mount semaphore until active operations finish before destroying drivers.
+No global mutex is held while waiting or invoking driver callbacks. Pending
+mount notifications carry a generation so reuse of a freed mount address
+cannot make a stale notification operate on a newer mount. Failed mount
+initialization follows the same teardown ordering.
+
+Validation on 2026-10-16:
+
+- Full native ctest: 10/10, including test_order and test_dmdevmon.
+- Both previous failures plus the lifetime regression: 20 consecutive passes
+  per target (60 executions).
+- The new regression pauses a real mock-driver class probe with semaphores,
+  starts unmount concurrently and verifies the driver is not freed and
+  unmount cannot complete before releasing the probe. A second mount with a
+  queued notification can be unmounted while the worker is blocked elsewhere.
+- The regression fails against the exact pre-fix dmdevfs source: driver free
+  and unmount complete prematurely (0/1 steps, exit 1).
+- STM32F7 build passed.

src/dmdevfs.c

@@ -171,6 +171,7 @@ typedef struct
     dmdrvi_context_t context;
     dmdrvi_dev_num_t dev_num;
     bool available;   // true: device became available, false: no longer available
+    uint64_t mount_generation; // Reject queued notifications for a reused mount address.
     dmfsi_context_t mounted;    // Not NULL: report the nodes of this mount instead (see dmfsi_dmdevfs_mounted())
 } hotplug_event_t;
 
@@ -242,6 +243,10 @@ struct dmfsi_context
     uint32_t    magic;
     char* config_path;          // Path with the configuration files
     dmlist_context_t* drivers;  // List of loaded drivers
+    uint64_t generation;
+    unsigned worker_refs;        // Active unlocked worker operations; guarded by g_devfs_mutex.
+    bool stopping;
+    dmosi_semaphore_t workers_idle;
     bool ready;                 // True once dmfsi_dmdevfs_mounted() has been called by the mounter
     path_t mount_path;          // This mount's own absolute path, valid once `ready` is true
 };
@@ -255,6 +260,7 @@ struct dmfsi_context
  * the right mount (and driver_node_t) to update.
  */
 static dmlist_context_t* g_dmdevfs_mounts = NULL;
+static uint64_t g_mount_generation;
 
 /**
  * @brief Guards g_dmdevfs_mounts and every mount's ctx->drivers list against
@@ -353,7 +359,7 @@ static void hotplug_worker_thread( void* arg );
 static bool queue_hotplug_event( dmdrvi_context_t context, const dmdrvi_dev_num_t* dev_num, bool available );
 static void process_device_available( dmdrvi_context_t context, const dmdrvi_dev_num_t* dev_num );
 static void process_device_unavailable( dmdrvi_context_t context, const dmdrvi_dev_num_t* dev_num );
-static void process_mount_ready( dmfsi_context_t mount );
+static void process_mount_ready( dmfsi_context_t mount, uint64_t generation );
 
 // ============================================================================
 //                      Module Interface Implementation
@@ -454,6 +460,39 @@ int dmod_deinit(void)
 //                      DMFSI Interface Implementation
 // ============================================================================
 
+/* Retain under g_devfs_mutex after a registry lookup, before invoking driver
+ * code without that mutex. Unmount removes the registry entry first and waits
+ * for the last active worker to finish before freeing any driver or list. */
+static void retain_worker_mount_locked(dmfsi_context_t mount)
+{
+    ++mount->worker_refs;
+}
+
+static void release_worker_mount(dmfsi_context_t mount)
+{
+    dmosi_mutex_lock(g_devfs_mutex);
+    if (--mount->worker_refs == 0 && mount->stopping)
+    {
+        // Post before dropping the lock: unmount may destroy the semaphore
+        // as soon as its wait completes. Do not access mount after this post.
+        dmosi_semaphore_post(mount->workers_idle, 1);
+    }
+    dmosi_mutex_unlock(g_devfs_mutex);
+}
+
+static void detach_mount_and_wait(dmfsi_context_t mount)
+{
+    dmosi_mutex_lock(g_devfs_mutex);
+    dmlist_remove(g_dmdevfs_mounts, mount, compare_mount_context);
+    mount->stopping = true;
+    bool busy = mount->worker_refs != 0;
+    dmosi_mutex_unlock(g_devfs_mutex);
+    if (busy)
+    {
+        dmosi_semaphore_wait(mount->workers_idle, 1, -1);
+    }
+}
+
 /**
  * @brief Initialize the file system
  */
@@ -481,6 +520,17 @@ dmod_dmfsi_dif_api_declaration( 1.0, dmdevfs, dmfsi_context_t, _init, (const cha
     ctx->magic = DMDEVFS_CONTEXT_MAGIC;
     ctx->config_path = Dmod_StrDup(config);
     ctx->drivers = dmlist_create();
+    ctx->worker_refs = 0;
+    ctx->stopping = false;
+    ctx->workers_idle = dmosi_semaphore_create(0, 1);
+    if (ctx->config_path == NULL || ctx->drivers == NULL || ctx->workers_idle == NULL)
+    {
+        if (ctx->workers_idle != NULL) dmosi_semaphore_destroy(ctx->workers_idle);
+        if (ctx->drivers != NULL) dmlist_destroy(ctx->drivers);
+        Dmod_Free(ctx->config_path);
+        Dmod_Free(ctx);
+        return NULL;
+    }
     ctx->ready = false;
     ctx->mount_path[0] = '\0';
 
@@ -497,26 +547,21 @@ dmod_dmfsi_dif_api_declaration( 1.0, dmdevfs, dmfsi_context_t, _init, (const cha
     // mount even while a driver's own dmdrvi_create() is still running as
     // part of configure_drivers() below.
     dmosi_mutex_lock(g_devfs_mutex);
-    bool registered = dmlist_push_back(g_dmdevfs_mounts, ctx);
+    ctx->generation = g_mount_generation == UINT64_MAX ? 0 : ++g_mount_generation;
+    bool registered = ctx->generation != 0 && dmlist_push_back(g_dmdevfs_mounts, ctx);
     dmosi_mutex_unlock(g_devfs_mutex);
     if (!registered)
     {
         DMOD_LOG_ERROR("Failed to register mount for hot-plug notifications\n");
     }
 
-    int res = configure_drivers(ctx, NULL, ctx->config_path);
+    int res = registered ? configure_drivers(ctx, NULL, ctx->config_path) : DMFSI_ERR_GENERAL;
     if (res != DMFSI_OK)
     {
         DMOD_LOG_ERROR("Failed to configure drivers\n");
+        detach_mount_and_wait(ctx);
         unconfigure_drivers(ctx);
-
-        if (registered)
-        {
-            dmosi_mutex_lock(g_devfs_mutex);
-            dmlist_remove(g_dmdevfs_mounts, ctx, compare_mount_context);
-            dmosi_mutex_unlock(g_devfs_mutex);
-        }
-
+        dmosi_semaphore_destroy(ctx->workers_idle);
         dmlist_destroy(ctx->drivers);
         Dmod_Free(ctx->config_path);
         Dmod_Free(ctx);
@@ -545,20 +590,31 @@ dmod_dmfsi_dif_api_declaration( 1.0, dmdevfs, int, _deinit, (dmfsi_context_t ctx
         return DMFSI_ERR_INVALID;
     }
 
-    // Deregister before tearing down ctx->drivers: once removed, the hot-plug
-    // worker thread can no longer find (or touch) this mount, so the rest of
-    // teardown below can safely run without holding g_devfs_mutex.
-    dmosi_mutex_lock(g_devfs_mutex);
-    dmlist_remove(g_dmdevfs_mounts, ctx, compare_mount_context);
-    dmosi_mutex_unlock(g_devfs_mutex);
-
+    detach_mount_and_wait(ctx);
     unconfigure_drivers(ctx);
+    dmosi_semaphore_destroy(ctx->workers_idle);
     dmlist_destroy(ctx->drivers);
     Dmod_Free(ctx->config_path);
     Dmod_Free(ctx);
     return DMFSI_OK;
 }
 
+/* dmfsi encodes RDWR as RDONLY|WRONLY (3), while dmdrvi has a
+ * separate RDWR bit (4). Translate at this interface boundary, retaining
+ * the non-access flags that were already forwarded to drivers. */
+static int driver_open_mode(int mode)
+{
+    int access = mode & (DMFSI_O_RDONLY | DMFSI_O_WRONLY);
+    int extra = mode & ~(DMFSI_O_RDONLY | DMFSI_O_WRONLY);
+    switch (access)
+    {
+        case DMFSI_O_RDONLY: return extra | DMDRVI_O_RDONLY;
+        case DMFSI_O_WRONLY: return extra | DMDRVI_O_WRONLY;
+        case DMFSI_O_RDWR: return extra | DMDRVI_O_RDWR;
+        default: return -1;
+    }
+}
+
 /**
  * @brief Open a file
  */
@@ -576,6 +632,10 @@ dmod_dmfsi_dif_api_declaration( 1.0, dmdevfs, int, _fopen, (dmfsi_context_t ctx,
         return DMFSI_ERR_INVALID;
     }
 
+    *fp = NULL;
+    int driver_mode = driver_open_mode(mode);
+    if (driver_mode < 0) return DMFSI_ERR_INVALID;
+
     // Find the driver node for this file, taking a reference on it so it
     // cannot be freed out from under this handle by a concurrent
     // process_device_unavailable() - see find_and_ref_driver_node().
@@ -616,7 +676,7 @@ dmod_dmfsi_dif_api_declaration( 1.0, dmdevfs, int, _fopen, (dmfsi_context_t ctx,
 
         // Open the device through the driver, identifying which device within the
         // context this is (the context's own device, or a hot-plugged sub-device)
-        handle->driver_handle = dmdrvi_open(driver_node->driver_context, mode, &driver_node->dev_num);
+        handle->driver_handle = dmdrvi_open(driver_node->driver_context, driver_mode, &driver_node->dev_num);
         if(handle->driver_handle == NULL)
         {
             DMOD_LOG_ERROR("Driver failed to open device: %s\n", path);
@@ -3768,6 +3828,7 @@ dmod_dmfsi_dif_api_declaration( 1.0, dmdevfs, void, _mounted, (dmfsi_context_t c
     }
     memset(event, 0, sizeof(*event));
     event->mounted = ctx;
+    event->mount_generation = ctx->generation;
     dmosi_mutex_lock(g_hotplug_mutex);
     bool queued = dmlist_push_back(g_hotplug_events, event);
     dmosi_mutex_unlock(g_hotplug_mutex);
@@ -3789,13 +3850,16 @@ dmod_dmfsi_dif_api_declaration( 1.0, dmdevfs, void, _mounted, (dmfsi_context_t c
  * may need (report_node_ready()). A node reported meanwhile (a hot-plug
  * event queued earlier) is left alone, its partitions too.
  */
-static void process_mount_ready( dmfsi_context_t mount )
+static void process_mount_ready( dmfsi_context_t mount, uint64_t generation )
 {
     dmosi_mutex_lock(g_devfs_mutex);
     bool live = dmlist_find(g_dmdevfs_mounts, mount, compare_mount_context) != NULL;
+    // A stale queued pointer may equal the address of a later mount.
+    live = live && mount->generation == generation;
     path_t mount_path;
     if (live)
     {
+        retain_worker_mount_locked(mount);
         strncpy(mount_path, mount->mount_path, sizeof(mount_path));
         mount_path[sizeof(mount_path) - 1] = '\0';
     }
@@ -3821,6 +3885,7 @@ static void process_mount_ready( dmfsi_context_t mount )
         report_node_ready(mount_path, node);
         scan_partitions(mount, mount_path, node);
     }
+    release_worker_mount(mount);
 }
 
 /**
@@ -3853,7 +3918,7 @@ static void hotplug_worker_thread( void* arg )
         }
         if (event->mounted != NULL)
         {
-            process_mount_ready(event->mounted);
+            process_mount_ready(event->mounted, event->mount_generation);
             Dmod_Free(event);
             continue;
         }
@@ -3887,9 +3952,9 @@ static void hotplug_worker_thread( void* arg )
  *     (stable, pointer-only) fields needed to build the new node.
  *  2. Unlocked: allocate the node and compute its path (may log).
  *  3. Locked again: re-validate the mount is still registered - if _deinit()
- *     raced us and won, mount may now be a dangling pointer, so this check
- *     must only ever compare it (never dereference it) until proven live -
- *     and only then insert the node.
+ *     started unmounting, discard the new node instead. A worker reference
+ *     keeps the owner, driver module and mount alive across all unlocked
+ *     work until release_worker_mount(), including error exits.
  */
 static void process_device_available( dmdrvi_context_t context, const dmdrvi_dev_num_t* dev_num )
 {
@@ -3911,6 +3976,7 @@ static void process_device_available( dmdrvi_context_t context, const dmdrvi_dev
         return;
     }
 
+    retain_worker_mount_locked(mount);
     driver_node_t* owner = (driver_node_t*)dmlist_find(mount->drivers, (void*)context, compare_driver_context);
     dmdrvi_context_t owner_context = owner->driver_context;
     Dmod_Context_t* owner_driver = owner->driver;
@@ -3927,6 +3993,7 @@ static void process_device_available( dmdrvi_context_t context, const dmdrvi_dev
         DMOD_LOG_ERROR("dmdrvi_device_available: failed to retain friends metadata\n");
         if (owner_role != NULL) Dmod_Free(owner_role);
         if (owner_group != NULL) Dmod_Free(owner_group);
+        release_worker_mount(mount);
         return;
     }
 
@@ -3936,6 +4003,7 @@ static void process_device_available( dmdrvi_context_t context, const dmdrvi_dev
         DMOD_LOG_ERROR("dmdrvi_device_available: failed to allocate device node\n");
         if (owner_role != NULL) Dmod_Free(owner_role);
         if (owner_group != NULL) Dmod_Free(owner_group);
+        release_worker_mount(mount);
         return;
     }
 
@@ -3961,6 +4029,7 @@ static void process_device_available( dmdrvi_context_t context, const dmdrvi_dev
     {
         DMOD_LOG_ERROR("dmdrvi_device_available: failed to compute device path\n");
         free_driver_node(new_node);
+        release_worker_mount(mount);
         return;
     }
 
@@ -3982,12 +4051,14 @@ static void process_device_available( dmdrvi_context_t context, const dmdrvi_dev
     {
         DMOD_LOG_ERROR("dmdrvi_device_available: mount was unmounted while processing event\n");
         free_driver_node(new_node);
+        release_worker_mount(mount);
         return;
     }
     if (!inserted)
     {
         DMOD_LOG_ERROR("dmdrvi_device_available: failed to register device node\n");
         free_driver_node(new_node);
+        release_worker_mount(mount);
         return;
     }
 
@@ -4003,6 +4074,7 @@ static void process_device_available( dmdrvi_context_t context, const dmdrvi_dev
     }
 
     DMOD_LOG_INFO("Hot-plugged device now available: %s\n", new_node->path);
+    release_worker_mount(mount);
 }
 
 /**
@@ -4024,6 +4096,7 @@ static void process_device_unavailable( dmdrvi_context_t context, const dmdrvi_d
         return;
     }
 
+    retain_worker_mount_locked(mount);
     driver_node_t* node = (driver_node_t*)dmlist_find(mount->drivers, &lookup, compare_dynamic_device);
     path_t removed_path;
     strncpy(removed_path, node->path, sizeof(removed_path));
@@ -4054,4 +4127,5 @@ static void process_device_unavailable( dmdrvi_context_t context, const dmdrvi_d
 
     release_driver_node_ref(node);
     DMOD_LOG_INFO("Hot-plugged device no longer available: %s\n", removed_path);
+    release_worker_mount(mount);
 }