From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f44.google.com ([74.125.82.44]:35130 "EHLO mail-wm0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752366AbcKRVoL (ORCPT ); Fri, 18 Nov 2016 16:44:11 -0500 Received: by mail-wm0-f44.google.com with SMTP id a197so63732063wmd.0 for ; Fri, 18 Nov 2016 13:44:11 -0800 (PST) From: Michal Nazarewicz To: gregkh@linuxfoundation.org, chenyu56@huawei.com, felipe.balbi@linux.intel.com, john.stultz@linaro.org Cc: stable@vger.kernel.org Subject: Re: FAILED: patch "[PATCH] usb: gadget: f_fs: stop sleeping in ffs_func_eps_disable" failed to apply to 4.8-stable tree In-Reply-To: <14793689499361@kroah.com> References: <14793689499361@kroah.com> Date: Fri, 18 Nov 2016 22:44:07 +0100 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Sender: stable-owner@vger.kernel.org List-ID: On Thu, Nov 17 2016, gregkh wrote: > The patch below does not apply to the 4.8-stable tree. > If someone wants it applied there, or to any other stable or longterm > tree, then please email the backport, including the original git commit > id to . Uh? Works with no issues for me: $ cd ~/code/linux $ git checkout v4.8.9 $ git cherry-pick 454915d [detached HEAD 24f34df] usb: gadget: f_fs: edit epfile->ep under lock Date: Tue Oct 4 02:07:33 2016 +0200 1 file changed, 3 insertions(+), 3 deletions(-) $ git cherry-pick a9e6f83 [detached HEAD e575f39] usb: gadget: f_fs: stop sleeping in ffs_func_eps_di= sable Date: Tue Oct 4 02:07:34 2016 +0200 1 file changed, 93 insertions(+), 16 deletions(-) I can send those in emails if you want. > ------------------ original commit in Linus's tree ------------------ > > From a9e6f83c2df199187a5248f824f31b6787ae23ae Mon Sep 17 00:00:00 2001 > From: Michal Nazarewicz > Date: Tue, 4 Oct 2016 02:07:34 +0200 > Subject: [PATCH] usb: gadget: f_fs: stop sleeping in ffs_func_eps_disable > MIME-Version: 1.0 > Content-Type: text/plain; charset=3DUTF-8 > Content-Transfer-Encoding: 8bit > > ffs_func_eps_disable is called from atomic context so it cannot sleep > thus cannot grab a mutex. Change the handling of epfile->read_buffer > to use non-sleeping synchronisation method. > > Reported-by: Chen Yu > Signed-off-by: Micha=C5=82 Nazarewicz > Fixes: 9353afbbfa7b ("buffer data from =E2=80=98oversized=E2=80=99 OUT re= quests") > Tested-by: John Stultz > Tested-by: Chen Yu > Signed-off-by: Felipe Balbi > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/func= tion/f_fs.c > index b31aa9572723..e40d47d47d82 100644 > --- a/drivers/usb/gadget/function/f_fs.c > +++ b/drivers/usb/gadget/function/f_fs.c > @@ -136,8 +136,60 @@ struct ffs_epfile { > /* > * Buffer for holding data from partial reads which may happen since > * we=E2=80=99re rounding user read requests to a multiple of a max pac= ket size. > + * > + * The pointer is initialised with NULL value and may be set by > + * __ffs_epfile_read_data function to point to a temporary buffer. > + * > + * In normal operation, calls to __ffs_epfile_read_buffered will consume > + * data from said buffer and eventually free it. Importantly, while the > + * function is using the buffer, it sets the pointer to NULL. This is > + * all right since __ffs_epfile_read_data and __ffs_epfile_read_buffered > + * can never run concurrently (they are synchronised by epfile->mutex) > + * so the latter will not assign a new value to the pointer. > + * > + * Meanwhile ffs_func_eps_disable frees the buffer (if the pointer is > + * valid) and sets the pointer to READ_BUFFER_DROP value. This special > + * value is crux of the synchronisation between ffs_func_eps_disable and > + * __ffs_epfile_read_data. > + * > + * Once __ffs_epfile_read_data is about to finish it will try to set the > + * pointer back to its old value (as described above), but seeing as the > + * pointer is not-NULL (namely READ_BUFFER_DROP) it will instead free > + * the buffer. > + * > + * =3D=3D State transitions =3D=3D > + * > + * =E2=80=A2 ptr =3D=3D NULL: (initial state) > + * =E2=97=A6 __ffs_epfile_read_buffer_free: go to ptr =3D=3D DROP > + * =E2=97=A6 __ffs_epfile_read_buffered: nop > + * =E2=97=A6 __ffs_epfile_read_data allocates temp buffer: go to ptr = =3D=3D buf > + * =E2=97=A6 reading finishes: n/a, not in =E2=80=98and = reading=E2=80=99 state > + * =E2=80=A2 ptr =3D=3D DROP: > + * =E2=97=A6 __ffs_epfile_read_buffer_free: nop > + * =E2=97=A6 __ffs_epfile_read_buffered: go to ptr =3D=3D NULL > + * =E2=97=A6 __ffs_epfile_read_data allocates temp buffer: free buf, = nop > + * =E2=97=A6 reading finishes: n/a, not in =E2=80=98and = reading=E2=80=99 state > + * =E2=80=A2 ptr =3D=3D buf: > + * =E2=97=A6 __ffs_epfile_read_buffer_free: free buf, go to ptr =3D= =3D DROP > + * =E2=97=A6 __ffs_epfile_read_buffered: go to ptr =3D=3D NULL and= reading > + * =E2=97=A6 __ffs_epfile_read_data: n/a, __ffs_epfile_read_bu= ffered > + * is always called first > + * =E2=97=A6 reading finishes: n/a, not in =E2=80=98and = reading=E2=80=99 state > + * =E2=80=A2 ptr =3D=3D NULL and reading: > + * =E2=97=A6 __ffs_epfile_read_buffer_free: go to ptr =3D=3D DROP and= reading > + * =E2=97=A6 __ffs_epfile_read_buffered: n/a, mutex is held > + * =E2=97=A6 __ffs_epfile_read_data: n/a, mutex is held > + * =E2=97=A6 reading finishes and =E2=80=A6 > + * =E2=80=A6 all data read: free buf, go to ptr =3D= =3D NULL > + * =E2=80=A6 otherwise: go to ptr =3D=3D buf and = reading > + * =E2=80=A2 ptr =3D=3D DROP and reading: > + * =E2=97=A6 __ffs_epfile_read_buffer_free: nop > + * =E2=97=A6 __ffs_epfile_read_buffered: n/a, mutex is held > + * =E2=97=A6 __ffs_epfile_read_data: n/a, mutex is held > + * =E2=97=A6 reading finishes: free buf, go to ptr =3D= =3D DROP > */ > - struct ffs_buffer *read_buffer; /* P: epfile->mutex */ > + struct ffs_buffer *read_buffer; > +#define READ_BUFFER_DROP ((struct ffs_buffer *)ERR_PTR(-ESHUTDOWN)) >=20=20 > char name[5]; >=20=20 > @@ -736,25 +788,47 @@ static void ffs_epfile_async_io_complete(struct usb= _ep *_ep, > schedule_work(&io_data->work); > } >=20=20 > +static void __ffs_epfile_read_buffer_free(struct ffs_epfile *epfile) > +{ > + /* > + * See comment in struct ffs_epfile for full read_buffer pointer > + * synchronisation story. > + */ > + struct ffs_buffer *buf =3D xchg(&epfile->read_buffer, READ_BUFFER_DROP); > + if (buf && buf !=3D READ_BUFFER_DROP) > + kfree(buf); > +} > + > /* Assumes epfile->mutex is held. */ > static ssize_t __ffs_epfile_read_buffered(struct ffs_epfile *epfile, > struct iov_iter *iter) > { > - struct ffs_buffer *buf =3D epfile->read_buffer; > + /* > + * Null out epfile->read_buffer so ffs_func_eps_disable does not free > + * the buffer while we are using it. See comment in struct ffs_epfile > + * for full read_buffer pointer synchronisation story. > + */ > + struct ffs_buffer *buf =3D xchg(&epfile->read_buffer, NULL); > ssize_t ret; > - if (!buf) > + if (!buf || buf =3D=3D READ_BUFFER_DROP) > return 0; >=20=20 > ret =3D copy_to_iter(buf->data, buf->length, iter); > if (buf->length =3D=3D ret) { > kfree(buf); > - epfile->read_buffer =3D NULL; > - } else if (unlikely(iov_iter_count(iter))) { > + return ret; > + } > + > + if (unlikely(iov_iter_count(iter))) { > ret =3D -EFAULT; > } else { > buf->length -=3D ret; > buf->data +=3D ret; > } > + > + if (cmpxchg(&epfile->read_buffer, NULL, buf)) > + kfree(buf); > + > return ret; > } >=20=20 > @@ -783,7 +857,15 @@ static ssize_t __ffs_epfile_read_data(struct ffs_epf= ile *epfile, > buf->length =3D data_len; > buf->data =3D buf->storage; > memcpy(buf->storage, data + ret, data_len); > - epfile->read_buffer =3D buf; > + > + /* > + * At this point read_buffer is NULL or READ_BUFFER_DROP (if > + * ffs_func_eps_disable has been called in the meanwhile). See comment > + * in struct ffs_epfile for full read_buffer pointer synchronisation > + * story. > + */ > + if (unlikely(cmpxchg(&epfile->read_buffer, NULL, buf))) > + kfree(buf); >=20=20 > return ret; > } > @@ -1097,8 +1179,7 @@ ffs_epfile_release(struct inode *inode, struct file= *file) >=20=20 > ENTER(); >=20=20 > - kfree(epfile->read_buffer); > - epfile->read_buffer =3D NULL; > + __ffs_epfile_read_buffer_free(epfile); > ffs_data_closed(epfile->ffs); >=20=20 > return 0; > @@ -1724,24 +1805,20 @@ static void ffs_func_eps_disable(struct ffs_funct= ion *func) > unsigned count =3D func->ffs->eps_count; > unsigned long flags; >=20=20 > + spin_lock_irqsave(&func->ffs->eps_lock, flags); > do { > - spin_lock_irqsave(&func->ffs->eps_lock, flags); > /* pending requests get nuked */ > if (likely(ep->ep)) > usb_ep_disable(ep->ep); > ++ep; > - if (epfile) > - epfile->ep =3D NULL; > - spin_unlock_irqrestore(&func->ffs->eps_lock, flags); >=20=20 > if (epfile) { > - mutex_lock(&epfile->mutex); > - kfree(epfile->read_buffer); > - epfile->read_buffer =3D NULL; > - mutex_unlock(&epfile->mutex); > + epfile->ep =3D NULL; > + __ffs_epfile_read_buffer_free(epfile); > ++epfile; > } > } while (--count); > + spin_unlock_irqrestore(&func->ffs->eps_lock, flags); > } >=20=20 > static int ffs_func_eps_enable(struct ffs_function *func) > --=20 Best regards =E3=83=9F=E3=83=8F=E3=82=A6 =E2=80=9C=F0=9D=93=B6=F0=9D=93=B2=F0=9D=93=B7= =F0=9D=93=AA86=E2=80=9D =E3=83=8A=E3=82=B6=E3=83=AC=E3=83=B4=E3=82=A4=E3=83= =84 =C2=ABIf at first you don=E2=80=99t succeed, give up skydiving=C2=BB