On Tue, Dec 29 2015, changbin.du@xxxxxxxxx wrote:
> From: "Du, Changbin" <changbin.du@xxxxxxxxx>
>
> 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@xxxxxxxxx>
Acked-by: Michal Nazarewicz <mina86@xxxxxxxxxx>
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@xxxxxxxxxx>--<xmpp:mina86@xxxxxxxxxx>--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
