[PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[changbin.du]

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

Re: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Michal Nazarewicz]

On Tue, Dec 29 2015, changbin.du@intel.com wrote:
> 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>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

While reviewing this patch I found two other bugs in ffs_epfile_io and
some more opportunities to refactor the code.

As soon as the kernel compiles I’ll send a patch set which will include
a modified version of this patch which has a smaller difference.

> ---
>  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
>

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  ミハウ “mina86” ナザレヴイツ  (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

Re: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Peter Chen]

On Tue, Dec 29, 2015 at 02:36:58PM +0800, changbin.du@intel.com wrote:
> 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.
> 

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.

> 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-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Best Regards,
Peter Chen

RE: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Du, Changbin]

> > 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

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.

Regards,
Du, Changbin

Re: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Peter Chen]

On Tue, Jan 05, 2016 at 04:09:47AM +0000, Du, Changbin wrote:
> > > 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
> 
> 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

RE: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Du, Changbin]

> >
> > 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
F_fs return -EINTER in its dequeuer case, not udc driver. What I want
to say is driver should return the right status for each usb request.

Re: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete

[Michal Nazarewicz]

On Tue, Jan 05 2016, Peter Chen wrote:
> Why -EINTR, the kernel-doc said it should return -ECONNRESET for
> active request, see include/linux/usb/gadget.h.

Because EINTR is what read returns to the user if the operation has been
interrupted by a signal, see ‘man 2 read’:

       EINTR The call was interrupted by a signal before any data was
       read; see signal(7).


-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  ミハウ “mina86” ナザレヴイツ  (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--