usb: f_fs: avoid race condition with ffs_epfile_io_complete

From: "Du, Changbin" <[email protected]>

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 <[email protected]>

---
 drivers/usb/gadget/function/f_fs.c | 45 ++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 17 deletions(-)

drivers/usb/gadget/function/f_fs.c +28 -17

index cf43e9e..8050939 100644
@@ -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);

Signature · 1 line

2.5.0

From: "Du, Changbin" <[email protected]>

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 <[email protected]>

---
 drivers/usb/gadget/function/f_fs.c | 45 ++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 17 deletions(-)

drivers/usb/gadget/function/f_fs.c +28 -17

index cf43e9e..8050939 100644
@@ -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);

Signature · 6 lines

2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 15 lines

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.

get unlock?

During the interrupt handler, it should only handle the "data complete" interrupt on queued request; if the "data complete" interrupt occurs, but it belongs to nobody, it will handle noop.

Best Regards,
Peter Chen

Quoted 23 lines

Quoted 15 lines

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.

get unlock?

During the interrupt handler, it should only handle the "data complete" interrupt on queued request; if the "data complete" interrupt occurs, but it belongs to nobody, it will handle noop.

Quoted 2 lines

Best Regards,
Peter Chen

You are right, but the problem is the request->status is wrong. If the data send out but report caller as -EINTR, it will introduce duplicate-send issue.

Quoted 5 lines

You are right, but the problem is the request->status is wrong. If the data send out but report caller as -EINTR, it will introduce duplicate-send issue.

Why -EINTR, the kernel-doc said it should return -ECONNRESET for active request, see include/linux/usb/gadget.h.

--

Best Regards,
Peter Chen

Why -EINTR, the kernel-doc said it should return -ECONNRESET for active request, see include/linux/usb/gadget.h.