Re: BUG: scheduling while atomic in f_fs when gadget remove driver

[Michal Nazarewicz <mina86@mina86.com>]

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 <mina86@mina86.com>
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 <chenyu56@huawei.com>
Signed-off-by: Michał Nazarewicz <mina86@mina86.com>
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»