From 6dec79e68c20c3ac446eb0d3eb549bf274cfc23f Mon Sep 17 00:00:00 2001 From: Jack Pham Date: Thu, 27 Sep 2018 12:06:10 -0700 Subject: [PATCH 1/2] usb: gadget: f_fs: Fail waiting IO after eps disabled Normally in epfile_io() and epfile_ioctl(), if the endpoint is not enabled the system call will block waiting until the endpoint is enabled. During a USB disconnect, the function's endpoints get disabled, so that if an IO call occurs right after that, it would perform the same blocking wait. From the kernel perspective this is indistinguishable from an IO call happening after the function is first initialized but not yet enabled. Hence a userspace process would be blocked and unaware of the cable disconnection. For userspace daemons such as ADB, this can be a problem since its read/write threads cannot be reaped until the I/O call returns, which won't happen until the cable is reconnected. In that case, the previous pending read/write would proceed to get successfully queued. In the case of a read, the previous read consumes data meant for the new session, and in the case of a write, stale data could be sent to the host. To fix this, add a counter to keep track of the epfile's open status. Upon the endpoint getting disabled mark the endpoint as invalid so as to fail subsequent IOs until the epfile is fully closed. Returning -ENODEV upon failure should allow the userspace process to close the file, so that it can reopen and begin a new session. Change-Id: I655ffdaa03485753431ff05dc521bf8d1b463ff1 Signed-off-by: Jack Pham --- drivers/usb/gadget/function/f_fs.c | 36 ++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c index 511125a2d3b3..e7de34d1a939 100644 --- a/drivers/usb/gadget/function/f_fs.c +++ b/drivers/usb/gadget/function/f_fs.c @@ -167,6 +167,7 @@ struct ffs_epfile { struct ffs_data *ffs; struct ffs_ep *ep; /* P: ffs->eps_lock */ + atomic_t opened; struct dentry *dentry; @@ -233,7 +234,7 @@ struct ffs_epfile { unsigned char in; /* P: ffs->eps_lock */ unsigned char isoc; /* P: ffs->eps_lock */ - unsigned char _pad; + bool invalid; }; struct ffs_buffer { @@ -1044,6 +1045,16 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) if (file->f_flags & O_NONBLOCK) return -EAGAIN; + /* + * epfile->invalid is set when EPs are disabled. Userspace + * might have stale threads continuing to do I/O and may be + * unaware of that especially if we block here. Instead return + * an error immediately here and don't allow any more I/O + * until the epfile is reopened. + */ + if (epfile->invalid) + return -ENODEV; + ret = wait_event_interruptible( epfile->ffs->wait, (ep = epfile->ep)); if (ret) @@ -1246,15 +1257,16 @@ ffs_epfile_open(struct inode *inode, struct file *file) ENTER(); - ffs_log("%s: state %d setup_state %d flag %lu", epfile->name, - epfile->ffs->state, epfile->ffs->setup_state, - epfile->ffs->flags); + ffs_log("%s: state %d setup_state %d flag %lu opened %u", + epfile->name, epfile->ffs->state, epfile->ffs->setup_state, + epfile->ffs->flags, atomic_read(&epfile->opened)); if (WARN_ON(epfile->ffs->state != FFS_ACTIVE)) return -ENODEV; file->private_data = epfile; ffs_data_opened(epfile->ffs); + atomic_inc(&epfile->opened); return 0; } @@ -1395,9 +1407,12 @@ ffs_epfile_release(struct inode *inode, struct file *file) ENTER(); __ffs_epfile_read_buffer_free(epfile); - ffs_log("%s: state %d setup_state %d flag %lu", epfile->name, - epfile->ffs->state, epfile->ffs->setup_state, - epfile->ffs->flags); + ffs_log("%s: state %d setup_state %d flag %lu opened %u", + epfile->name, epfile->ffs->state, epfile->ffs->setup_state, + epfile->ffs->flags, atomic_read(&epfile->opened)); + + if (atomic_dec_and_test(&epfile->opened)) + epfile->invalid = false; ffs_data_closed(epfile->ffs); @@ -1427,6 +1442,10 @@ static long ffs_epfile_ioctl(struct file *file, unsigned code, if (file->f_flags & O_NONBLOCK) return -EAGAIN; + /* don't allow any I/O until file is reopened */ + if (epfile->invalid) + return -ENODEV; + ret = wait_event_interruptible( epfile->ffs->wait, (ep = epfile->ep)); if (ret) @@ -2091,6 +2110,8 @@ static int ffs_epfiles_create(struct ffs_data *ffs) ffs_epfiles_destroy(epfiles, i - 1); return -ENOMEM; } + + atomic_set(&epfile->opened, 0); } ffs->epfiles = epfiles; @@ -2137,6 +2158,7 @@ static void ffs_func_eps_disable(struct ffs_function *func) ++ep; if (epfile) { + epfile->invalid = true; /* until file is reopened */ epfile->ep = NULL; __ffs_epfile_read_buffer_free(epfile); ++epfile; From df30b04c71ffd9ae5f937108025fa9e06c80842e Mon Sep 17 00:00:00 2001 From: Jack Pham Date: Tue, 13 Feb 2018 18:36:37 -0800 Subject: [PATCH 2/2] USB: f_fs: Set ffs->func to NULL after disabling endpoint in set_alt() When adb root is performed, userspace will close and open ffs_epsfile. Closing this file will call ffs_functionfs_callback() which does call remove_config(). This will call ffs_function_eps_disable to disable endpoints and then calls ffs_func_unbind(). Unbind() will also call endpoint disable which might lead to disabling endpoint which is already disabled. Hence set ffs->func to NULL after disabling endpoints in set_alt(). CRs-Fixed: 557532 Change-Id: I3052bdee74a1793d4e003de4b991d353e5d699b0 Signed-off-by: Vijayavardhan Vennapusa Signed-off-by: Mayank Rana Signed-off-by: Jack Pham --- drivers/usb/gadget/function/f_fs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c index e7de34d1a939..c92f8e8d1e56 100644 --- a/drivers/usb/gadget/function/f_fs.c +++ b/drivers/usb/gadget/function/f_fs.c @@ -3561,8 +3561,10 @@ static int ffs_func_set_alt(struct usb_function *f, return intf; } - if (ffs->func) + if (ffs->func) { ffs_func_eps_disable(ffs->func); + ffs->func = NULL; + } if (ffs->state == FFS_DEACTIVATED) { ffs->state = FFS_CLOSING;