From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754552AbcI1VjK (ORCPT ); Wed, 28 Sep 2016 17:39:10 -0400 Received: from mail-wm0-f45.google.com ([74.125.82.45]:38773 "EHLO mail-wm0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753890AbcI1VjA (ORCPT ); Wed, 28 Sep 2016 17:39:00 -0400 From: Michal Nazarewicz To: Chen Yu , Felipe Balbi , gregkh@linuxfoundation.org Cc: wangbinghui@hisilicon.com, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org, John Stultz , Amit Pundir , Guodong Xu Subject: Re: BUG: scheduling while atomic in f_fs when gadget remove driver In-Reply-To: Organization: http://mina86.com/ References: <205cfce1-d54c-262d-f939-ad9f37b0c52c@huawei.com> <878tud4q6x.fsf@linux.intel.com> <261ada71-8a5d-6e89-7fac-6b6ba88218d7@huawei.com> User-Agent: Notmuch/0.19+53~g2e63a09 (http://notmuchmail.org) Emacs/25.1.50.2 (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:160928:amit.pundir@linaro.org::jTv/MFXCxvZzMeKk:0000000000000000000000000000000000000001bmD X-Hashcash: 1:20:160928:john.stultz@linaro.org::XfHNKqTaY8D2AQJ8:0000000000000000000000000000000000000000OWA X-Hashcash: 1:20:160928:linux-kernel@vger.kernel.org::msR7jqFYzERGZnQG:0000000000000000000000000000000003SKj X-Hashcash: 1:20:160928:felipe.balbi@linux.intel.com::LjqDXcuh94e57j64:0000000000000000000000000000000001YBl X-Hashcash: 1:20:160928:chenyu56@huawei.com::EZcuDurFF6z3O6Vr:0000000000000000000000000000000000000000000otn X-Hashcash: 1:20:160928:gregkh@linuxfoundation.org::qQmjla8JGCLN2uKu:0000000000000000000000000000000000069St X-Hashcash: 1:20:160928:guodong.xu@linaro.org::qO3Og06X8igoWFio:00000000000000000000000000000000000000005gbq X-Hashcash: 1:20:160928:wangbinghui@hisilicon.com::BGgrBgnOijb0R/pK:0000000000000000000000000000000000004za2 X-Hashcash: 1:20:160928:linux-usb@vger.kernel.org::KhCMmaPWvoAK3spA:000000000000000000000000000000000000FfOf Date: Wed, 28 Sep 2016 23:38:56 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id u8SLdEgZ017877 On Wed, Sep 28 2016, Michal Nazarewicz wrote: > With that done, the only thing which needs a mutex is > epfile->read_buffer. Perhaps this would do: ---- >8 -------------------------------------------------- ------------- >>From 6416a1065203a39328311f6c58083089efe169aa Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Wed, 28 Sep 2016 23:36:56 +0200 Subject: [RFC] usb: gadget: f_fs: stop sleeping in ffs_func_eps_disable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-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ł Nazarewicz Fixes: 9353afbbfa7b ("buffer data from ‘oversized’ OUT requests") --- drivers/usb/gadget/function/f_fs.c | 89 +++++++++++++++++++++++++++++++------- 1 file changed, 73 insertions(+), 16 deletions(-) diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c index 759f5d4..8db53da 100644 --- a/drivers/usb/gadget/function/f_fs.c +++ b/drivers/usb/gadget/function/f_fs.c @@ -136,8 +136,50 @@ struct ffs_epfile { /* * Buffer for holding data from partial reads which may happen since * we’re rounding user read requests to a multiple of a max packet size. + * + * The pointer starts with NULL value and may be initialised to other + * value by __ffs_epfile_read_data function which may need to allocate + * the temporary buffer. + * + * In normal operation, subsequent calls to __ffs_epfile_read_buffered + * will consume data from the 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 (as they are + * protected by epfile->mutex) so the latter will not assign a new value + * to the buffer. + * + * 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. + * + * This how concurrent calls to the two functions would look like (‘<->’ + * denotes xchg operation): + * + * read_buffer = some buffer + * + * THREAD A THREAD B + * __ffs_epfile_read_data: + * buf = NULL + * buf <-> read_buffer + * … do stuff on buf … + * __ffs_func_eps_disable: + * buf = READ_BUFFER_DROP + * buf <-> read_buffer + * kfree(buf); + * + * old = cmpxchg(read_buffer, NULL, buf) + * if (old) + * kfree(buf) */ - struct ffs_buffer *read_buffer; /* P: epfile->mutex */ + struct ffs_buffer *read_buffer; +#define READ_BUFFER_DROP ((struct ffs_buffer *)ERR_PTR(-ESHUTDOWN)) char name[5]; @@ -740,21 +782,31 @@ static void ffs_epfile_async_io_complete(struct usb_ep *_ep, static ssize_t __ffs_epfile_read_buffered(struct ffs_epfile *epfile, struct iov_iter *iter) { - struct ffs_buffer *buf = epfile->read_buffer; + /* + * Null out epfile->read_buffer so ffs_func_eps_disable does not free + * the buffer while we are using it. + */ + struct ffs_buffer *buf = xchg(&epfile->read_buffer, NULL); ssize_t ret; - if (!buf) + if (!buf || buf == READ_BUFFER_DROP) return 0; ret = copy_to_iter(buf->data, buf->length, iter); if (buf->length == ret) { kfree(buf); - epfile->read_buffer = NULL; - } else if (unlikely(iov_iter_count(iter))) { + return ret; + } + + if (unlikely(iov_iter_count(iter))) { ret = -EFAULT; } else { buf->length -= ret; buf->data += ret; } + + if (cmpxchg(&epfile->read_buffer, NULL, buf)) + kfree(buf); + return ret; } @@ -783,7 +835,10 @@ static ssize_t __ffs_epfile_read_data(struct ffs_epfile *epfile, buf->length = data_len; buf->data = buf->storage; memcpy(buf->storage, data + ret, data_len); - epfile->read_buffer = buf; + + buf = xchg(&epfile->read_buffer, buf); + if (buf && buf != READ_BUFFER_DROP) + kfree(buf); return ret; } @@ -1094,11 +1149,14 @@ static int ffs_epfile_release(struct inode *inode, struct file *file) { struct ffs_epfile *epfile = inode->i_private; + struct ffs_buffer *buf; ENTER(); - kfree(epfile->read_buffer); - epfile->read_buffer = NULL; + buf = xchg(&epfile->read_buffer, NULL); + if (buf && buf != READ_BUFFER_DROP) + kfree(buf); + ffs_data_closed(epfile->ffs); return 0; @@ -1721,27 +1779,26 @@ static void ffs_func_eps_disable(struct ffs_function *func) { struct ffs_ep *ep = func->eps; struct ffs_epfile *epfile = func->ffs->epfiles; + struct ffs_buffer *buf; unsigned count = func->ffs->eps_count; unsigned long flags; + 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 = NULL; - spin_unlock_irqrestore(&func->ffs->eps_lock, flags); if (epfile) { - mutex_lock(&epfile->mutex); - kfree(epfile->read_buffer); - epfile->read_buffer = NULL; - mutex_unlock(&epfile->mutex); + epfile->ep = NULL; + buf = xchg(&epfile->read_buffer, READ_BUFFER_DROP); + if (buf && buf != READ_BUFFER_DROP) + kfree(buf); ++epfile; } } while (--count); + spin_unlock_irqrestore(&func->ffs->eps_lock, flags); } static int ffs_func_eps_enable(struct ffs_function *func) ---- >8 -------------------------------------------------- ------------- Note: This has not been tested in *any* way. It’s more to demonstrate the concept even though it is likely that it does actually work. -- Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving»