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 05c6750702b609565d7a4adc1ebae16ba3ffeeb7..fa467a40949d2ebbb05ef564877600bdf02f9bb7 100644 (file)
--- 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 @@ done_mutex:
        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 4b3365f23fd7508f0ef363a2374b738ce69f0cac..6a80182aadd70d5adea564ecdcf4a39ad1f40654 100644 (file)
--- 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;