Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1301263 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] usb: f_fs: avoid race condition with  ffs_epfile_io_complete Peter Chen <hzpeterchen@gmail.com> - 2016-01-05 04:40 +0100
    RE: [PATCH] usb: f_fs: avoid race condition with  ffs_epfile_io_complete "Du, Changbin" <changbin.du@intel.com> - 2016-01-05 05:20 +0100
      Re: [PATCH] usb: f_fs: avoid race condition with  ffs_epfile_io_complete Peter Chen <hzpeterchen@gmail.com> - 2016-01-05 07:00 +0100
        RE: [PATCH] usb: f_fs: avoid race condition with  ffs_epfile_io_complete "Du, Changbin" <changbin.du@intel.com> - 2016-01-05 07:30 +0100
        Re: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete Michal Nazarewicz <mina86@mina86.com> - 2016-01-05 13:50 +0100

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

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

[toc] | [next] | [standalone]

#1301270

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

[toc] | [prev] | [next] | [standalone]

#1301283

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

[toc] | [prev] | [next] | [standalone]

#1301288

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web