Re: [PATCH v4 00/54] tree-in-dcache stuff

Al Viro
2026-02-01T00:10:00Z

On Fri, Jan 30, 2026 at 05:11:39PM -0800, Linus Torvalds wrote:
> On Fri, 30 Jan 2026 at 17:06, Al Viro <viro@zeniv.linux.org.uk> 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 <viro@zeniv.linux.org.uk>
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 <wusamuel@google.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
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;