On Fri, Jan 30, 2026 at 05:11:39PM -0800, Linus Torvalds wrote: > On Fri, 30 Jan 2026 at 17:06, Al Viro <[email protected]> wrote: > > > > I'd rather go for a spinlock there, protecting these FFS_DEACTIVATED > > transitions; > > Yes, that's the much better solution. The locking in this thing is > horrendous. > > But judging by Samuel's recent email, there's something else than the > open locking thing going on.
I've put the following into viro/vfs.git#fixes; diff is identical to the one that is reported to fix things on top of -rc7... Objections? commit 99a706fa47949ece1fb02b5b1206efd4fb031d25 Author: Al Viro <[email protected]> Date: Sat Jan 31 18:24:41 2026 -0500 functionfs: use spinlock for FFS_DEACTIVATED/FFS_CLOSING transitions When all files are closed, functionfs needs ffs_data_reset() to be done before any further opens are allowed. During that time we have ffs->state set to FFS_CLOSING; that makes ->open() fail with -EBUSY. Once ffs_data_reset() is done, it switches state (to FFS_READ_DESCRIPTORS) indicating that opening that thing is allowed again. There's a couple of additional twists: * mounting with -o no_disconnect delays ffs_data_reset() from doing that at the final ->release() to the first subsequent open(). That's indicated by ffs->state set to FFS_DEACTIVATED; if open() sees that, it immediately switches to FFS_CLOSING and proceeds with doing ffs_data_reset() before returning to userland. * a couple of usb callbacks need to force the delayed transition; unfortunately, they are done in locking environment that does not allow blocking and ffs_data_reset() can block. As the result, if these callbacks see FFS_DEACTIVATED, they change state to FFS_CLOSING and use schedule_work() to get ffs_data_reset() executed asynchronously. Unfortunately, the locking is rather insufficient. A fix attempted in e5bf5ee26663 ("functionfs: fix the open/removal races") had closed a bunch of UAF, but it didn't do anything to the callbacks, lacked barriers in transition from FFS_CLOSING to FFS_READ_DESCRIPTORS _and_ it had been too heavy-handed in open()/open() serialization - I've used ffs->mutex for that, and it's being held over actual IO on ep0, complete with copy_from_user(), etc. Even more unfortunately, the userland side is apparently racy enough to have the resulting timing changes (no failures, just a delayed return of open(2)) disrupt the things quite badly. Userland bugs or not, it's a clear regression that needs to be dealt with. Solution is to use a spinlock for serializing these state checks and transitions - unlike ffs->mutex it can be taken in these callbacks and it doesn't disrupt the timings in open(). We could introduce a new spinlock, but it's easier to use the one that is already there (ffs->eps_lock) instead - the locking environment is safe for it in all affected places. Since now it is held over all places that alter or check the open count (ffs->opened), there's no need to keep that atomic_t - int would serve just fine and it's simpler that way. Fixes: e5bf5ee26663 ("functionfs: fix the open/removal races") Fixes: 18d6b32fca38 ("usb: gadget: f_fs: add "no_disconnect" mode") # v4.0 Tested-by: Samuel Wu <[email protected]> Signed-off-by: Al Viro <[email protected]> --- 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;
