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

[Alan Stern]

On Thu, Jul 30, 2026 at 05:27:32PM +0800, Lei, Xue wrote:
> 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).

Did you try testing the new patch?  While fsg_lun_open_deferred() is 
waiting for the completion, it still holds the sb_writers lock.  So 
fsg_lun_open_work_fn() will block waiting for the lock to be released, 
which will never happen because fsg_lun_open_work_fn() can't signal the 
completion until it finishes waiting.

I think there is no way to solve this problem (at least, not unless 
fsg_lun_open() returns before the file has actually been opened -- which 
would cause other problems), and the best we can do is warn people not 
to put the backing storage file in the configfs filesystem (which they 
shouldn't be doing anyway).

If you still believe this problem needs a better fix, perhaps the way to 
find a more suitable approach would be to ask the VFS filesystem people 
how to do what we want.

Alan Stern

> 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
>