On Fri, Jan 30, 2026 at 06:09:00PM -0800, Samuel Wu wrote:
> On Fri, Jan 30, 2026 at 5:16 PM Al Viro <[email protected]> wrote:
> >
> > On Fri, Jan 30, 2026 at 05:05:34PM -0800, Samuel Wu wrote:
> >
> > > > How lovely... Could you slap
> > > > WARN_ON(ret == -EAGAIN);
> > > > right before that
> > > > if (ret < 0)
> > > > return ret;
> > >
> > > Surprisingly ret == 0 every time, so no difference in dmesg logs with
> > > this addition.
> >
> > What the hell? Other than that mutex_lock(), the only change in there
> > is the order of store to file->private_data and call of ffs_data_opened();
> > that struct file pointer is not visible to anyone at that point...
>
> Agree, 09e88dc22ea2 (serialize ffs_ep0_open() on ffs->mutex) in itself
> is quite straightforward. Not familiar with this code path so just
> speculating, but is there any interaction with previous patches (e.g.
> refcounting)?
>
> > Wait, it also brings ffs_data_reset() on that transition under ffs->mutex...
> > For a quick check: does
> > git fetch git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git
> > for-wsamuel2
> > git switch --detach FETCH_HEAD
> > demonstrate the same breakage?
>
> Had to adjust forward declaration of ffs_data_reset() to build, but
> unfortunately same breakage.
That really looks like a badly racy userland on top of everything else...
I mean, it smells like userland open() from one process while another
is in the middle of configuring that stuff getting delayed too much
for the entire thing to work. Bloody wonderful...
OK, let's see if a variant with serialization on spinlock works - how does
the following do on top of mainline?
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 05c6750702b6..fa467a40949d 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -59,7 +59,6 @@ static struct ffs_data *__must_check ffs_data_new(const char
*dev_name)
__attribute__((malloc));
/* Opened counter handling. */
-static void ffs_data_opened(struct ffs_data *ffs);
static void ffs_data_closed(struct ffs_data *ffs);
/* Called with ffs->mutex held; take over ownership of data. */
@@ -636,23 +635,25 @@ static ssize_t ffs_ep0_read(struct file *file, char
__user *buf,
return ret;
}
+
+static void ffs_data_reset(struct ffs_data *ffs);
+
static int ffs_ep0_open(struct inode *inode, struct file *file)
{
struct ffs_data *ffs = inode->i_sb->s_fs_info;
- int ret;
- /* Acquire mutex */
- ret = ffs_mutex_lock(&ffs->mutex, file->f_flags & O_NONBLOCK);
- if (ret < 0)
- return ret;
-
- ffs_data_opened(ffs);
+ spin_lock_irq(&ffs->eps_lock);
if (ffs->state == FFS_CLOSING) {
- ffs_data_closed(ffs);
- mutex_unlock(&ffs->mutex);
+ spin_unlock_irq(&ffs->eps_lock);
return -EBUSY;
}
- mutex_unlock(&ffs->mutex);
+ if (!ffs->opened++ && ffs->state == FFS_DEACTIVATED) {
+ ffs->state = FFS_CLOSING;
+ spin_unlock_irq(&ffs->eps_lock);
+ ffs_data_reset(ffs);
+ } else {
+ spin_unlock_irq(&ffs->eps_lock);
+ }
file->private_data = ffs;
return stream_open(inode, file);
@@ -1202,15 +1203,10 @@ ffs_epfile_open(struct inode *inode, struct file *file)
{
struct ffs_data *ffs = inode->i_sb->s_fs_info;
struct ffs_epfile *epfile;
- int ret;
-
- /* Acquire mutex */
- ret = ffs_mutex_lock(&ffs->mutex, file->f_flags & O_NONBLOCK);
- if (ret < 0)
- return ret;
- if (!atomic_inc_not_zero(&ffs->opened)) {
- mutex_unlock(&ffs->mutex);
+ spin_lock_irq(&ffs->eps_lock);
+ if (!ffs->opened) {
+ spin_unlock_irq(&ffs->eps_lock);
return -ENODEV;
}
/*
@@ -1220,11 +1216,11 @@ ffs_epfile_open(struct inode *inode, struct file *file)
*/
epfile = smp_load_acquire(&inode->i_private);
if (unlikely(ffs->state != FFS_ACTIVE || !epfile)) {
- mutex_unlock(&ffs->mutex);
- ffs_data_closed(ffs);
+ spin_unlock_irq(&ffs->eps_lock);
return -ENODEV;
}
- mutex_unlock(&ffs->mutex);
+ ffs->opened++;
+ spin_unlock_irq(&ffs->eps_lock);
file->private_data = epfile;
return stream_open(inode, file);
@@ -2092,8 +2088,6 @@ static int ffs_fs_init_fs_context(struct fs_context *fc)
return 0;
}
-static void ffs_data_reset(struct ffs_data *ffs);
-
static void
ffs_fs_kill_sb(struct super_block *sb)
{
@@ -2150,15 +2144,6 @@ static void ffs_data_get(struct ffs_data *ffs)
refcount_inc(&ffs->ref);
}
-static void ffs_data_opened(struct ffs_data *ffs)
-{
- if (atomic_add_return(1, &ffs->opened) == 1 &&
- ffs->state == FFS_DEACTIVATED) {
- ffs->state = FFS_CLOSING;
- ffs_data_reset(ffs);
- }
-}
-
static void ffs_data_put(struct ffs_data *ffs)
{
if (refcount_dec_and_test(&ffs->ref)) {
@@ -2176,28 +2161,29 @@ static void ffs_data_put(struct ffs_data *ffs)
static void ffs_data_closed(struct ffs_data *ffs)
{
- if (atomic_dec_and_test(&ffs->opened)) {
- if (ffs->no_disconnect) {
- struct ffs_epfile *epfiles;
- unsigned long flags;
-
- ffs->state = FFS_DEACTIVATED;
- spin_lock_irqsave(&ffs->eps_lock, flags);
- epfiles = ffs->epfiles;
- ffs->epfiles = NULL;
- spin_unlock_irqrestore(&ffs->eps_lock,
- flags);
-
- if (epfiles)
- ffs_epfiles_destroy(ffs->sb, epfiles,
- ffs->eps_count);
-
- if (ffs->setup_state == FFS_SETUP_PENDING)
- __ffs_ep0_stall(ffs);
- } else {
- ffs->state = FFS_CLOSING;
- ffs_data_reset(ffs);
- }
+ spin_lock_irq(&ffs->eps_lock);
+ if (--ffs->opened) { // not the last opener?
+ spin_unlock_irq(&ffs->eps_lock);
+ return;
+ }
+ if (ffs->no_disconnect) {
+ struct ffs_epfile *epfiles;
+
+ ffs->state = FFS_DEACTIVATED;
+ epfiles = ffs->epfiles;
+ ffs->epfiles = NULL;
+ spin_unlock_irq(&ffs->eps_lock);
+
+ if (epfiles)
+ ffs_epfiles_destroy(ffs->sb, epfiles,
+ ffs->eps_count);
+
+ if (ffs->setup_state == FFS_SETUP_PENDING)
+ __ffs_ep0_stall(ffs);
+ } else {
+ ffs->state = FFS_CLOSING;
+ spin_unlock_irq(&ffs->eps_lock);
+ ffs_data_reset(ffs);
}
}
@@ -2214,7 +2200,7 @@ static struct ffs_data *ffs_data_new(const char *dev_name)
}
refcount_set(&ffs->ref, 1);
- atomic_set(&ffs->opened, 0);
+ ffs->opened = 0;
ffs->state = FFS_READ_DESCRIPTORS;
mutex_init(&ffs->mutex);
spin_lock_init(&ffs->eps_lock);
@@ -2266,6 +2252,7 @@ static void ffs_data_reset(struct ffs_data *ffs)
{
ffs_data_clear(ffs);
+ spin_lock_irq(&ffs->eps_lock);
ffs->raw_descs_data = NULL;
ffs->raw_descs = NULL;
ffs->raw_strings = NULL;
@@ -2289,6 +2276,7 @@ static void ffs_data_reset(struct ffs_data *ffs)
ffs->ms_os_descs_ext_prop_count = 0;
ffs->ms_os_descs_ext_prop_name_len = 0;
ffs->ms_os_descs_ext_prop_data_len = 0;
+ spin_unlock_irq(&ffs->eps_lock);
}
@@ -3756,6 +3744,7 @@ static int ffs_func_set_alt(struct usb_function *f,
{
struct ffs_function *func = ffs_func_from_usb(f);
struct ffs_data *ffs = func->ffs;
+ unsigned long flags;
int ret = 0, intf;
if (alt > MAX_ALT_SETTINGS)
@@ -3768,12 +3757,15 @@ static int ffs_func_set_alt(struct usb_function *f,
if (ffs->func)
ffs_func_eps_disable(ffs->func);
+ spin_lock_irqsave(&ffs->eps_lock, flags);
if (ffs->state == FFS_DEACTIVATED) {
ffs->state = FFS_CLOSING;
+ spin_unlock_irqrestore(&ffs->eps_lock, flags);
INIT_WORK(&ffs->reset_work, ffs_reset_work);
schedule_work(&ffs->reset_work);
return -ENODEV;
}
+ spin_unlock_irqrestore(&ffs->eps_lock, flags);
if (ffs->state != FFS_ACTIVE)
return -ENODEV;
@@ -3791,16 +3783,20 @@ static void ffs_func_disable(struct usb_function *f)
{
struct ffs_function *func = ffs_func_from_usb(f);
struct ffs_data *ffs = func->ffs;
+ unsigned long flags;
if (ffs->func)
ffs_func_eps_disable(ffs->func);
+ spin_lock_irqsave(&ffs->eps_lock, flags);
if (ffs->state == FFS_DEACTIVATED) {
ffs->state = FFS_CLOSING;
+ spin_unlock_irqrestore(&ffs->eps_lock, flags);
INIT_WORK(&ffs->reset_work, ffs_reset_work);
schedule_work(&ffs->reset_work);
return;
}
+ spin_unlock_irqrestore(&ffs->eps_lock, flags);
if (ffs->state == FFS_ACTIVE) {
ffs->func = NULL;
diff --git a/drivers/usb/gadget/function/u_fs.h
b/drivers/usb/gadget/function/u_fs.h
index 4b3365f23fd7..6a80182aadd7 100644
--- a/drivers/usb/gadget/function/u_fs.h
+++ b/drivers/usb/gadget/function/u_fs.h
@@ -176,7 +176,7 @@ struct ffs_data {
/* reference counter */
refcount_t ref;
/* how many files are opened (EP0 and others) */
- atomic_t opened;
+ int opened;
/* EP0 state */
enum ffs_state state;
