summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDu, Changbin <changbin.du@intel.com>2015-12-29 14:36:58 +0800
committerJaehoon Chung <jh80.chung@samsung.com>2019-01-29 11:25:41 +0900
commite99035c34bfbff6754d2c66489fcd52e49d93eb4 (patch)
tree01b1a58bc2b2b7b324dc2f4e785e26b43a4e16b1
parent8a38f77918a5d0e34da98c952aaa835e7cdf55ff (diff)
downloadlinux-artik7-e99035c34bfbff6754d2c66489fcd52e49d93eb4.tar.gz
linux-artik7-e99035c34bfbff6754d2c66489fcd52e49d93eb4.tar.bz2
linux-artik7-e99035c34bfbff6754d2c66489fcd52e49d93eb4.zip
usb: f_fs: avoid race condition with ffs_epfile_io_complete
ffs_epfile_io and ffs_epfile_io_complete runs in different context, but there is no synchronization between them. consider the following scenario: 1) ffs_epfile_io interrupted by sigal while wait_for_completion_interruptible 2) then ffs_epfile_io set ret to -EINTR 3) just before or during usb_ep_dequeue, the request completed 4) ffs_epfile_io return with -EINTR In this case, ffs_epfile_io tell caller no transfer success but actually it may has been done. This break the caller's pipe. Below script can help test it (adbd is the process which lies on f_fs). while true do pkill -19 adbd #SIGSTOP pkill -18 adbd #SIGCONT sleep 0.1 done To avoid this, just dequeue the request first. After usb_ep_dequeue, the request must be done or canceled. With this change, we can ensure no race condition in f_fs driver. But actually I found some of the udc driver has analogical issue in its dequeue implementation. For example, 1) the dequeue function hold the controller's lock. 2) before driver request controller to stop transfer, a request completed. 3) the controller trigger a interrupt, but its irq handler need wait dequeue function to release the lock. 4) dequeue function give back the request with negative status, and release lock. 5) irq handler get lock but the request has already been given back. So, the dequeue implementation should take care of this case. IMO, it can be done as below steps to dequeue a already started request, 1) request controller to stop transfer on the given ep. HW know the actual transfer status. 2) after hw stop transfer, driver scan if there are any completed one. 3) if found, process it with real status. if no, the request can canceled. Signed-off-by: "Du, Changbin" <changbin.du@intel.com> [mina86@mina86.com: rebased on top of refactoring commits] Signed-off-by: Michal Nazarewicz <mina86@mina86.com> Signed-off-by: Felipe Balbi <balbi@kernel.org> Signed-off-by: Dongwoo Lee <dwoo08.lee@samsung.com> Change-Id: I12832c8cd8060302eff13565cf2f1372288d07ed
-rw-r--r--drivers/usb/gadget/function/f_fs.c12
1 files changed, 9 insertions, 3 deletions
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 2c66aa9b3d68..f14b1f4f3dc4 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -797,6 +797,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
ret = -EINVAL;
} else if (!io_data->aio) {
DECLARE_COMPLETION_ONSTACK(done);
+ bool interrupted = false;
req = ep->req;
req->buf = data;
@@ -812,9 +813,14 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
spin_unlock_irq(&epfile->ffs->eps_lock);
if (unlikely(wait_for_completion_interruptible(&done))) {
- ret = -EINTR;
+ /*
+ * To avoid race condition with ffs_epfile_io_complete,
+ * dequeue the request first then check
+ * status. usb_ep_dequeue API should guarantee no race
+ * condition with req->complete callback.
+ */
usb_ep_dequeue(ep->ep, req);
- goto error_mutex;
+ interrupted = ep->status < 0;
}
/*
@@ -824,7 +830,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
* to maxpacketsize), we may end up with more
* data then user space has space for.
*/
- ret = ep->status;
+ ret = interrupted ? -EINTR : ep->status;
if (io_data->read && ret > 0) {
ret = copy_to_iter(data, ret, &io_data->data);
if (!ret)