[PATCH v4] mtd: block2mtd: defer device open out of param/sysfs write

[PATCH v4] mtd: block2mtd: defer device open out of param/sysfs write

[Chris Roy]

block2mtd_setup() opens the named block device while still under
param_lock, and on the sysfs write path under kernfs (and possibly a
splice pipe lock). That nests VFS locking the wrong way relative to
overlayfs and trips lockdep.

Drop param_lock and run setup on a dedicated ordered workqueue. Keep
the call synchronous with wait_for_completion(). Allocate the work on
the heap so DEBUG_OBJECTS_WORK stays quiet.

Reported-by: syzbot+7cab6a19619f1b8efc00@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=7cab6a19619f1b8efc00
Tested-by: syzbot+7cab6a19619f1b8efc00@syzkaller.appspotmail.com
Assisted-by: Claude:claude-sonnet-5
Signed-off-by: Chris Roy <iam@thechris.in>
---
v4:
 - rename list_mutex / setup_wq to say what they are for
 - tidy new comments
 - fix Assisted-by tag format (checkpatch: AGENT_NAME:MODEL_VERSION)
 - drop redundant setup_wq check in block2mtd_setup_defer() (the sole
   caller already gates on it)
 - claim Tested-by from syzbot (granted on v2, v3, and this content)
v3:
 - rewrite the new comments to match the rest of the file
 - add Assisted-by
v2:
 - heap-allocated work (v1 tripped DEBUG_OBJECTS_WORK)
 - dedicated ordered workqueue instead of system_wq
 - module reference across the deferred open
 - flush/destroy the workqueue before exit teardown
 - serialize setup2 on the worker under list_mutex
 - early-boot paramline updates under that mutex

Not proposed for stable. block2mtd has no known production use (per
Richard Weinberger, testing is the only real use case), so there is
no backport trail worth chasing here.

 drivers/mtd/devices/block2mtd.c | 116 ++++++++++++++++++++++++++------
 1 file changed, 95 insertions(+), 21 deletions(-)

diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
index 03e80b2..a540089 100644
--- a/drivers/mtd/devices/block2mtd.c
+++ b/drivers/mtd/devices/block2mtd.c
@@ -27,6 +27,8 @@
 #include <linux/init.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mutex.h>
+#include <linux/workqueue.h>
+#include <linux/completion.h>
 #include <linux/mount.h>
 #include <linux/slab.h>
 #include <linux/major.h>
@@ -45,6 +47,9 @@ struct block2mtd_dev {
 
 /* Static info about the MTD, used in cleanup_module */
 static LIST_HEAD(blkmtd_device_list);
+/* Protects blkmtd_device_list and early-boot paramline updates */
+static DEFINE_MUTEX(list_mutex);
+static struct workqueue_struct *setup_wq;
 
 
 static struct page *page_read(struct address_space *mapping, pgoff_t index)
@@ -461,31 +466,85 @@ static int block2mtd_setup2(const char *val)
 	return 0;
 }
 
+struct block2mtd_setup_work {
+	struct work_struct work;
+	struct completion done;
+	char *val;
+	int ret;
+};
+
+static void block2mtd_setup_workfn(struct work_struct *work)
+{
+	struct block2mtd_setup_work *w =
+		container_of(work, struct block2mtd_setup_work, work);
+
+	mutex_lock(&list_mutex);
+	w->ret = block2mtd_setup2(w->val);
+	mutex_unlock(&list_mutex);
+	complete(&w->done);
+}
+
+/* Runs block2mtd_setup2() on setup_wq, blocking until it completes */
+static int block2mtd_setup_defer(const char *val)
+{
+	struct block2mtd_setup_work *w;
+	int ret;
+
+	w = kzalloc(sizeof(*w), GFP_KERNEL);
+	if (!w)
+		return -ENOMEM;
+
+	w->val = kstrdup(val, GFP_KERNEL);
+	if (!w->val) {
+		kfree(w);
+		return -ENOMEM;
+	}
+
+	init_completion(&w->done);
+	INIT_WORK(&w->work, block2mtd_setup_workfn);
+	queue_work(setup_wq, &w->work);
+	wait_for_completion(&w->done);
+
+	ret = w->ret;
+	kfree(w->val);
+	kfree(w);
+	return ret;
+}
 
 static int block2mtd_setup(const char *val, const struct kernel_param *kp)
 {
-#ifdef MODULE
-	return block2mtd_setup2(val);
-#else
-	/* If more parameters are later passed in via
-	   /sys/module/block2mtd/parameters/block2mtd
-	   and block2mtd_init() has already been called,
-	   we can parse the argument now. */
-
-	if (block2mtd_init_called)
-		return block2mtd_setup2(val);
-
-	/* During early boot stage, we only save the parameters
-	   here. We must parse them later: if the param passed
-	   from kernel boot command line, block2mtd_setup() is
-	   called so early that it is not possible to resolve
-	   the device (even kmalloc() fails). Deter that work to
-	   block2mtd_setup2(). */
-
-	strscpy(block2mtd_paramline, val, sizeof(block2mtd_paramline));
+	int ret = 0;
 
-	return 0;
+	if (!try_module_get(kp->mod))
+		return -ENODEV;
+
+	kernel_param_unlock(kp->mod);
+
+#ifndef MODULE
+	mutex_lock(&list_mutex);
+	if (!block2mtd_init_called) {
+		/* Cannot resolve block devices this early */
+		strscpy(block2mtd_paramline, val, sizeof(block2mtd_paramline));
+		mutex_unlock(&list_mutex);
+		kernel_param_lock(kp->mod);
+		module_put(kp->mod);
+		return 0;
+	}
+	mutex_unlock(&list_mutex);
 #endif
+
+	if (setup_wq) {
+		ret = block2mtd_setup_defer(val);
+	} else {
+		/* Not yet deferred to setup_wq; safe to call setup2 directly */
+		mutex_lock(&list_mutex);
+		ret = block2mtd_setup2(val);
+		mutex_unlock(&list_mutex);
+	}
+
+	kernel_param_lock(kp->mod);
+	module_put(kp->mod);
+	return ret;
 }
 
 
@@ -496,10 +555,17 @@ static int __init block2mtd_init(void)
 {
 	int ret = 0;
 
+	setup_wq = alloc_ordered_workqueue("block2mtd", 0);
+	if (!setup_wq)
+		return -ENOMEM;
+
 #ifndef MODULE
+	mutex_lock(&list_mutex);
 	if (strlen(block2mtd_paramline))
 		ret = block2mtd_setup2(block2mtd_paramline);
+	/* Avoid racing sysfs with the early paramline */
 	block2mtd_init_called = 1;
+	mutex_unlock(&list_mutex);
 #endif
 
 	return ret;
@@ -510,9 +576,16 @@ static void block2mtd_exit(void)
 {
 	struct list_head *pos, *next;
 
-	/* Remove the MTD devices */
+	if (setup_wq) {
+		flush_workqueue(setup_wq);
+		destroy_workqueue(setup_wq);
+		setup_wq = NULL;
+	}
+
+	mutex_lock(&list_mutex);
 	list_for_each_safe(pos, next, &blkmtd_device_list) {
 		struct block2mtd_dev *dev = list_entry(pos, typeof(*dev), list);
+
 		block2mtd_sync(&dev->mtd);
 		mtd_device_unregister(&dev->mtd);
 		mutex_destroy(&dev->write_mutex);
@@ -522,6 +595,7 @@ static void block2mtd_exit(void)
 		list_del(&dev->list);
 		block2mtd_free_device(dev);
 	}
+	mutex_unlock(&list_mutex);
 }
 
 late_initcall(block2mtd_init);

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v4] mtd: block2mtd: defer device open out of param/sysfs write

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mtd: block2mtd: use-after-free and resource leak on init failure
- [High] mtd: block2mtd: sysfs lock inversion bypasses workqueue deferral
--

commit c3f3ebd86ecc63dd07e6a22d8527eadaeb761633
Author: Chris Roy <iam@thechris.in>

mtd: block2mtd: defer device open out of param/sysfs write

This patch defers block2mtd_setup() device opening to a dedicated ordered
workqueue to prevent lockdep-detected lock inversion between VFS and
kernfs/overlayfs during sysfs writes. The setup work is executed via a
dynamically allocated work item, preserving synchronous behavior with
wait_for_completion().

Closes: https://syzkaller.appspot.com/bug?extid=7cab6a19619f1b8efc00

> diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
> index 349fa07be314c..30ac9325bdd2b 100644
> --- a/drivers/mtd/devices/block2mtd.c
> +++ b/drivers/mtd/devices/block2mtd.c
[ ... ]
> @@ -461,31 +466,85 @@ static int block2mtd_setup2(const char *val)
[ ... ]
>  static int block2mtd_setup(const char *val, const struct kernel_param *kp)
>  {
[ ... ]
> +	if (setup_wq) {
> +		ret = block2mtd_setup_defer(val);
> +	} else {
> +		/* Not yet deferred to setup_wq; safe to call setup2 directly */
> +		mutex_lock(&list_mutex);
> +		ret = block2mtd_setup2(val);
> +		mutex_unlock(&list_mutex);
> +	}

[Severity: High]
Does evaluating setup_wq here bypass the workqueue deferral and re-introduce
the sysfs lock inversion?

Since mod_sysfs_setup() exposes module parameters to sysfs before
do_init_module() calls block2mtd_init(), setup_wq can be NULL during a
concurrent sysfs write.

This would cause the sysfs writer to execute block2mtd_setup2() inline under
the sysfs kernfs lock.

Additionally, since setup_wq is assigned in block2mtd_init() without memory
barriers (such as smp_store_release() and smp_load_acquire()), concurrent
sysfs writers might observe a stale NULL pointer even after initialization
completes.

> +
> +	kernel_param_lock(kp->mod);
> +	module_put(kp->mod);
> +	return ret;
>  }
>  
>  
> @@ -496,10 +555,17 @@ static int __init block2mtd_init(void)
>  {
>  	int ret = 0;
>  
> +	setup_wq = alloc_ordered_workqueue("block2mtd", 0);
> +	if (!setup_wq)
> +		return -ENOMEM;

[Severity: High]
Can returning -ENOMEM here lead to a use-after-free and resource leak?

If parameters are provided during module load
(e.g., insmod block2mtd.ko block2mtd=...), block2mtd_setup() is executed
before block2mtd_init(). Because setup_wq is NULL at that time, it
synchronously registers the MTD device in the block2mtd_setup() else branch
shown above.

If alloc_ordered_workqueue() subsequently fails here, it returns -ENOMEM and
aborts the module load.

The kernel frees the module memory without calling block2mtd_exit(), leaving
the registered MTD device holding dangling function pointers to the unloaded
module text.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920185132.1266699-1-iam@thechris.in?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/