From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753261AbcADUeB (ORCPT ); Mon, 4 Jan 2016 15:34:01 -0500 Received: from mail-wm0-f41.google.com ([74.125.82.41]:36924 "EHLO mail-wm0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752055AbcADUd6 convert rfc822-to-8bit (ORCPT ); Mon, 4 Jan 2016 15:33:58 -0500 From: Michal Nazarewicz To: changbin.du@intel.com, balbi@ti.com, gregkh@linuxfoundation.org Cc: viro@zeniv.linux.org.uk, r.baldyga@samsung.com, rui.silva@linaro.org, k.opasiak@samsung.com, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, "Du\, Changbin" Subject: Re: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete In-Reply-To: <1451371018-14918-1-git-send-email-changbin.du@intel.com> Organization: http://mina86.com/ References: <1451371018-14918-1-git-send-email-changbin.du@intel.com> User-Agent: Notmuch/0.19+53~g2e63a09 (http://notmuchmail.org) Emacs/25.1.50.1 (x86_64-unknown-linux-gnu) Face: iVBORw0KGgoAAAANSUhEUgAAADAAAAAwBAMAAAClLOS0AAAAJFBMVEWbfGlUPDDHgE57V0jUupKjgIObY0PLrom9mH4dFRK4gmjPs41MxjOgAAACP0lEQVQ4T23Sv2vbQBQHcBk1xE6WyALX107VUEgmn6+ouUwpEQQ6uRjttkWP4CkBg2M0BQLBdPFZYPsyFYo7qEtKDQ7on+t7+nF2Ux8ahD587717OmNYrOvycHsZ+o2r051wHTHysAvGb8ygvgu4QWT0sCmkgZCIEnlV2X8BtyraazFGDuxhmKSQJMlwHQ7v5MHSNxmz78rfElwAa3ieVD9e+hBhjaPDDG6NgFo2f4wBMNIo5YmRtF0RyDgFjJjlMIWbnuM4x9MMfABGTlN4qgIQB4A1DEyA1BHWtfeWNUMwiVJKoqh97KrkOO+qzgluVYLvFCUKAX73nONeBr7BGMdM6Sg0kuep03VywLaIzRiVr+GAzKlpQIsAFnWAG2e6DT5WmWDiudZMIc6hYrMOmeMQK9WX0B+/RfjzL9DI7Y9/Iayn29Ci0r2i4f9gMimMSZLCDMalgQGU5hnUtqAN0OGvEmO1Wnl0C0wWSCEHnuHBqmygxdxA8oWXwbipoc1EoNR9DqOpBpOJrnr0criQab9ZT4LL+wI+K7GBQH30CrhUruilgP9DRTrhVWZCiAyILP+wiuLeCKGTD6r/nc8LOJcAwR6IBTUs+7CASw3QFZ0MdA2PI3zNziH4ZKVhXCRMBjeZ1DWMekKwDCASwExy+NQ86TaykaDAFHO4aP48y4fIcDM5yOG8GcTLbOyp8A8azjJI93JFd1EA6yN8sSxMQJWoABqniRZVykYgRXErzrdqExAoUrRb0xfRp8p2A/4XmfilTtkDZ4cAAAAASUVORK5CYII= X-Face: -TR8(rDTHy/(xl?SfWd1|3:TTgDIatE^t'vop%*gVg[kn$t{EpK(P"VQ=~T2#ysNmJKN$"yTRLB4YQs$4{[.]Fc1)*O]3+XO^oXM>Q#b^ix,O)Zbn)q[y06$`e3?C)`CwR9y5riE=fv^X@x$y?D:XO6L&x4f-}}I4=VRNwiA^t1-ZrVK^07.Pi/57c_du'& X-PGP: 50751FF4 X-PGP-FP: AC1F 5F5C D418 88F8 CC84 5858 2060 4012 5075 1FF4 X-Hashcash: 1:20:160104:k.opasiak@samsung.com::PHIQtFgDZrW/cnWm:00000000000000000000000000000000000000000KQ5 X-Hashcash: 1:20:160104:viro@zeniv.linux.org.uk::Kmce1oXFkC4I5ulx:000000000000000000000000000000000000000ku/ X-Hashcash: 1:20:160104:linux-usb@vger.kernel.org::aMnYjR2L7IUUP9mu:00000000000000000000000000000000000013fr X-Hashcash: 1:20:160104:gregkh@linuxfoundation.org::cOgwtQAnU4fTRR+q:000000000000000000000000000000000002VoD X-Hashcash: 1:20:160104:rui.silva@linaro.org::cM1iFDD/1JZdNutz:000000000000000000000000000000000000000003OOQ X-Hashcash: 1:20:160104:changbin.du@intel.com::s4OoHUEA/FV9CL6n:00000000000000000000000000000000000000004dri X-Hashcash: 1:20:160104:linux-kernel@vger.kernel.org::x3X1ADBsNF0zV0C2:0000000000000000000000000000000006mzs X-Hashcash: 1:20:160104:balbi@ti.com::+CqhCLh2DM6GcYRG:000008vWN X-Hashcash: 1:20:160104:r.baldyga@samsung.com::wi5gZeJbZIbS2QEm:00000000000000000000000000000000000000009DGO X-Hashcash: 1:20:160104:changbin.du@intel.com::AAqz7aqn6iTMxkF7:0000000000000000000000000000000000000000FuYh Date: Mon, 04 Jan 2016 21:33:52 +0100 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Dec 29 2015, changbin.du@intel.com wrote: > From: "Du, Changbin" > > 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 Acked-by: Michal Nazarewicz 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 +------ooO--(_)--Ooo--