On Fri, Jan 30, 2026 at 6:41 PM Al Viro <[email protected]> wrote:
>
> 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?
Excellent, this is working consistently for me on the latest 6.19-rc7.
>
> 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;
