Re: [PATCH] usb: gadget: f_mass_storage: reject relative paths to fix sb_writers deadlock

[Lei, Xue]

That's good questions.

> What happens if the user gives an absolute path that just happens to 
be below the configfs mount point?

For example: echo "/sys/kernel/config/usb_gadget/g1/idVendor" > lun.0/file
This path starts with /, so it passes the buf[0] != '/' check introduced 
by the patch.
The path then enters fsg_lun_open() → 
filp_open("/sys/kernel/config/...", O_RDWR). During path resolution, the 
VFS traverses into the configfs mount, and
open_last_lookups() calls mnt_want_write() on the configfs vfsmount 
because O_RDWR is set. This triggers sb_start_write() on the same 
configfs superblock
whose sb_writers is already held by the outer vfs_write() → 
file_start_write().
This is the same deadlock scenario

> What happens if the user gives a relative path of the form "../filename"?

For example: echo "../some_file" > lun.0/file
The first character of "../some_file" is ., which is not /. The patch 
rejects it immediately with -EINVAL before fsg_lun_open() is ever called.
This case is fully handled by the patch. No deadlock, no file open 
attempt — clean rejection.


How about the patch 
0001-usb-gadget-f_mass_storage-defer-filp_open-to-workque.patch?

The deadlock fundamentally occurs because filp_open() executes in the 
same task context that holds sb_writers on the configfs superblock. Any 
path-based
validation approach (rejecting relative paths, checking the target 
superblock after kern_path(), etc.) is inherently a heuristic — it tries 
to predict
which paths would lead to the recursive lock acquisition and reject them 
upfront.

And this patch eliminates the problem at its root: by moving 
fsg_lun_open() to a worker thread, filp_open() runs in a context that 
simply does not
hold sb_writers on any superblock. The recursive acquisition becomes 
impossible regardless of what path the user provides — relative, 
absolute, pointing to
configfs, or anywhere else. There is no need for path validation 
heuristics or filesystem-type checks.

The trade-off is one additional context switch per file attribute write, 
which is negligible given that this is a configuration-time operation 
(not a data
path), and fsg_lun_open() already performs blocking I/O (opening and 
validating a backing file).


BestRegards,

Xue Lei



On 7/29/26 22:12, Alan Stern wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> On Wed, Jul 29, 2026 at 06:32:45PM +0800, Xue Lei wrote:
> > A deadlock can occur when writing a relative path to the mass_storage
> > lun file attribute while the process's CWD is on the same configfs
> > mount:
> >
> >    write(configfs_fd, "relative_path", ...)
> >      -> vfs_write()
> >        -> file_start_write()          -- acquires sb_writers (configfs sb)
> >          -> configfs_write_iter()
> >            -> fsg_store_file()
> >              -> fsg_lun_open()
> >                -> filp_open("relative_path", O_RDWR, ...)
> >                  -> path_openat()
> >                    -> open_last_lookups()
> >                      -> mnt_want_write() -- tries to acquire same sb_writers
> >                         *** DEADLOCK ***
> >
> > This happens because filp_open() resolves relative paths against the
> > task's CWD. If the CWD is on the same configfs superblock, path_openat()
> > calls mnt_want_write() which calls sb_start_write() on the same
> > superblock, causing a recursive lock acquisition that can deadlock
> > during filesystem freeze.
> >
> > Backing file paths for mass_storage LUNs should always be absolute
> > paths pointing to block devices or regular files. Reject relative
> > paths early in fsg_store_file() to prevent this deadlock.
> What happens if the user gives an absolute path that just happens to be
> below the configfs mount point?
>
> What happens if the user gives a relative path of the form
> "../filename"?
>
> Alan Stern
>
> > Reported-by: syzbot+4c9318af45f0bf2af153@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=4c9318af45f0bf2af153
> > Fixes: ef0aa4b92cf1 ("usb: gadget: f_mass_storage: add configfs support")
> > Signed-off-by: Xue Lei <Xue.Lei@xxxxxxxxxxxxx>
> > ---
> >   drivers/usb/gadget/function/storage_common.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/usb/gadget/function/storage_common.c b/drivers/usb/gadget/function/storage_common.c
> > index 75831f2c7abe..d45c7a34df99 100644
> > --- a/drivers/usb/gadget/function/storage_common.c
> > +++ b/drivers/usb/gadget/function/storage_common.c
> > @@ -448,6 +448,12 @@ ssize_t fsg_store_file(struct fsg_lun *curlun, struct rw_semaphore *filesem,
> >        if (count > 0 && buf[count-1] == '\n')
> >                ((char *) buf)[count-1] = 0;            /* Ugh! */
> >
> > +     /* Reject relative paths to prevent sb_writers deadlock when
> > +      * CWD is on the same filesystem (e.g., configfs).
> > +      */
> > +     if (count > 0 && buf[0] && buf[0] != '/')
> > +             return -EINVAL;
> > +
> >        /* Load new medium */
> >        down_write(filesem);
> >        if (count > 0 && buf[0]) {
> > --
> > 2.49.1
From 112a252cba7e2789ae9aab0d45d5c4722f300b95 Mon Sep 17 00:00:00 2001
From: Xue Lei <Xue.Lei@xxxxxxxxxxxxx>
Date: Thu, 30 Jul 2026 16:45:36 +0800
Subject: [PATCH] usb: gadget: f_mass_storage: defer filp_open to workqueue to
 prevent sb_writers deadlock

When the mass_storage function is configured via configfs, writing a
backing file path to the "file" attribute invokes fsg_lun_open() ->
filp_open() within the context of vfs_write() on the configfs
attribute file.  vfs_write() holds sb_writers on the configfs
superblock via file_start_write().  If the path given to filp_open()
resolves on the same configfs superblock (either via a relative path
when CWD is on configfs, or an absolute path pointing into configfs),
mnt_want_write() inside path_openat() attempts to recursively acquire
the same sb_writers, creating a deadlock opportunity:

  CPU0 (write path)            CPU1 (freeze path)
  ----------------             ------------------
  sb_start_write()  [ok]
                               freeze_super()
                                 percpu_down_write() [blocks]
  sb_start_write()  [blocks]
          --> mutual deadlock

The key insight is that the deadlock occurs because filp_open() runs
in the same task context that holds sb_writers.  If we move the
filp_open() call to a different context that does NOT hold sb_writers,
the deadlock is impossible regardless of what path is provided.

Fix this by deferring the fsg_lun_open() call to a system workqueue
via schedule_work() + wait_for_completion().  The worker thread does
not hold any sb_writers lock, so filp_open() can safely acquire
sb_writers on any superblock without causing recursion.  The original
caller waits synchronously for the result, preserving the existing
synchronous store() semantics expected by configfs.

This approach is comprehensive: it handles relative paths, absolute
paths into configfs, and any future scenarios where the filesystem
topology might create similar sb_writers conflicts -- without needing
path validation heuristics.

Reported-by: syzbot+4c9318af45f0bf2af153@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=4c9318af45f0bf2af153
Fixes: ef0aa4b92cf1 ("usb: gadget: f_mass_storage: add configfs support")
Signed-off-by: Xue Lei <Xue.Lei@xxxxxxxxxxxxx>
---
 drivers/usb/gadget/function/storage_common.c | 49 +++++++++++++++++++-
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/storage_common.c b/drivers/usb/gadget/function/storage_common.c
index 75831f2c7abe..c33591326086 100644
--- a/drivers/usb/gadget/function/storage_common.c
+++ b/drivers/usb/gadget/function/storage_common.c
@@ -21,9 +21,11 @@
 
 #include <linux/module.h>
 #include <linux/blkdev.h>
+#include <linux/completion.h>
 #include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/kstrtox.h>
+#include <linux/workqueue.h>
 #include <linux/usb/composite.h>
 
 #include "storage_common.h"
@@ -176,6 +178,49 @@ void fsg_lun_close(struct fsg_lun *curlun)
 }
 EXPORT_SYMBOL_GPL(fsg_lun_close);
 
+/*
+ * When fsg_lun_open() is called from a configfs store handler, the calling
+ * task holds sb_writers on the configfs superblock (via vfs_write() ->
+ * file_start_write()).  If filp_open() inside fsg_lun_open() resolves the
+ * path to the same superblock, mnt_want_write() in path_openat() attempts
+ * to re-acquire sb_writers, which deadlocks against concurrent freeze_super().
+ *
+ * Deferring fsg_lun_open() to a workqueue worker ensures filp_open() runs in
+ * a context that does not hold any sb_writers, eliminating the deadlock
+ * regardless of the path provided.
+ */
+struct fsg_lun_open_work {
+	struct work_struct	work;
+	struct completion	done;
+	struct fsg_lun		*curlun;
+	const char		*filename;
+	int			result;
+};
+
+static void fsg_lun_open_work_fn(struct work_struct *work)
+{
+	struct fsg_lun_open_work *ow =
+		container_of(work, struct fsg_lun_open_work, work);
+
+	ow->result = fsg_lun_open(ow->curlun, ow->filename);
+	complete(&ow->done);
+}
+
+static int fsg_lun_open_deferred(struct fsg_lun *curlun, const char *filename)
+{
+	struct fsg_lun_open_work ow;
+
+	INIT_WORK_ONSTACK(&ow.work, fsg_lun_open_work_fn);
+	init_completion(&ow.done);
+	ow.curlun = curlun;
+	ow.filename = filename;
+	ow.result = -ETIMEDOUT;
+	schedule_work(&ow.work);
+	wait_for_completion(&ow.done);
+	destroy_work_on_stack(&ow.work);
+	return ow.result;
+}
+
 int fsg_lun_open(struct fsg_lun *curlun, const char *filename)
 {
 	int				ro;
@@ -451,8 +496,8 @@ ssize_t fsg_store_file(struct fsg_lun *curlun, struct rw_semaphore *filesem,
 	/* Load new medium */
 	down_write(filesem);
 	if (count > 0 && buf[0]) {
-		/* fsg_lun_open() will close existing file if any. */
-		rc = fsg_lun_open(curlun, buf);
+		/* Defer to workqueue to avoid sb_writers deadlock. */
+		rc = fsg_lun_open_deferred(curlun, buf);
 		if (rc == 0)
 			curlun->unit_attention_data =
 					SS_NOT_READY_TO_READY_TRANSITION;
-- 
2.49.1