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

[Michal Nazarewicz <mina86@mina86.com>]

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