From: changbin.du@intel.com
To: balbi@ti.com, gregkh@linuxfoundation.org
Cc: viro@zeniv.linux.org.uk, mina86@mina86.com, r.baldyga@samsung.com,
        rui.silva@linaro.org, k.opasiak@samsung.com, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, "Du, Changbin" <changbin.du@intel.com>
Subject: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete
Date: Tue, 29 Dec 2015 14:36:58 +0800
Message-Id: <1451371018-14918-1-git-send-email-changbin.du@intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4460
Lines: 132

From: "Du, Changbin" <changbin.du@intel.com>

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>
---
 drivers/usb/gadget/function/f_fs.c | 45 ++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index cf43e9e..8050939 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -687,6 +687,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 	struct ffs_ep *ep;
 	char *data = NULL;
 	ssize_t ret, data_len = -EINVAL;
+	bool interrupted = false;
 	int halt;
 
 	/* Are we still active? */
@@ -829,26 +830,35 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 
 			spin_unlock_irq(&epfile->ffs->eps_lock);
 
-			if (unlikely(ret < 0)) {
-				/* nop */
-			} else if (unlikely(
+			if (unlikely(ret < 0))
+				goto error_mutex;
+
+			if (unlikely(
 				   wait_for_completion_interruptible(&done))) {
-				ret = -EINTR;
-				usb_ep_dequeue(ep->ep, req);
-			} else {
 				/*
-				 * XXX We may end up silently droping data
-				 * here.  Since data_len (i.e. req->length) may
-				 * be bigger than len (after being rounded up
-				 * to maxpacketsize), we may end up with more
-				 * data then user space has space for.
+				 * 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.
 				 */
-				ret = ep->status;
-				if (io_data->read && ret > 0) {
-					ret = copy_to_iter(data, ret, &io_data->data);
-					if (!ret)
-						ret = -EFAULT;
-				}
+				usb_ep_dequeue(ep->ep, req);
+				interrupted = true;
+			}
+
+			/*
+			 * XXX We may end up silently droping data
+			 * here.  Since data_len (i.e. req->length) may
+			 * be bigger than len (after being rounded up
+			 * to maxpacketsize), we may end up with more
+			 * data then user space has space for.
+			 */
+			ret = ep->status < 0 && interrupted ?
+					-EINTR : ep->status;
+			if (io_data->read && ret > 0) {
+				ret = copy_to_iter(data, ret, &io_data->data);
+				if (!ret)
+					ret = -EFAULT;
 			}
 			kfree(data);
 		}
@@ -859,6 +869,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 
 error_lock:
 	spin_unlock_irq(&epfile->ffs->eps_lock);
+error_mutex:
 	mutex_unlock(&epfile->mutex);
 error:
 	kfree(data);
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/