[PATCH 2/2] usb: gadget: f_fs: stop sleeping in ffs_func_eps_disable

[Michal Nazarewicz]

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@xxxxxxxxxx>
Signed-off-by: MichaÅ Nazarewicz <mina86@xxxxxxxxxx>
Fixes: 9353afbbfa7b ("buffer data from âoversizedâ OUT requests")
Tested-by: Chen Yu <chenyu56@xxxxxxxxxx>
---
 drivers/usb/gadget/function/f_fs.c | 109 +++++++++++++++++++++++++++++++------
 1 file changed, 93 insertions(+), 16 deletions(-)

Compared to the previous version:
â this one has a bit more comments (I feel like itâs a bad sign that
  this needs so much documentation);
â ffs_epfile_realese sets read_buffer to READ_BUFFER_DROP (which
  doesnât matter since on entry __ffs_epfile_read_buffered behaves
  the same way when read_buffer is NULL or READ_BUFFER_DROP); and
â __ffs_epfile_read_data will drop the temporary data if read_buffer
  is READ_BUFFER_DROP (which may happen if ep is disabled between ep
  request finishes and data is copied to user space).

Chen, John, if you could test this version as well, that would be
swell.

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 759f5d4..e15fc1b 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âre rounding user read requests to a multiple of a max packet 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.
+	 *
+	 * == State transitions ==
+	 *
+	 * â ptr == NULL:  (initial state)
+	 *   â __ffs_epfile_read_buffer_free: go to ptr == DROP
+	 *   â __ffs_epfile_read_buffered:    nop
+	 *   â __ffs_epfile_read_data allocates temp buffer: go to ptr == buf
+	 *   â reading finishes:              n/a, not in âand readingâ state
+	 * â ptr == DROP:
+	 *   â __ffs_epfile_read_buffer_free: nop
+	 *   â __ffs_epfile_read_buffered:    go to ptr == NULL
+	 *   â __ffs_epfile_read_data allocates temp buffer: free buf, nop
+	 *   â reading finishes:              n/a, not in âand readingâ state
+	 * â ptr == buf:
+	 *   â __ffs_epfile_read_buffer_free: free buf, go to ptr == DROP
+	 *   â __ffs_epfile_read_buffered:    go to ptr == NULL and reading
+	 *   â __ffs_epfile_read_data:        n/a, __ffs_epfile_read_buffered
+	 *                                    is always called first
+	 *   â reading finishes:              n/a, not in âand readingâ state
+	 * â ptr == NULL and reading:
+	 *   â __ffs_epfile_read_buffer_free: go to ptr == DROP and reading
+	 *   â __ffs_epfile_read_buffered:    n/a, mutex is held
+	 *   â __ffs_epfile_read_data:        n/a, mutex is held
+	 *   â reading finishes and â
+	 *     â all data read:               free buf, go to ptr == NULL
+	 *     â otherwise:                   go to ptr == buf and reading
+	 * â ptr == DROP and reading:
+	 *   â __ffs_epfile_read_buffer_free: nop
+	 *   â __ffs_epfile_read_buffered:    n/a, mutex is held
+	 *   â __ffs_epfile_read_data:        n/a, mutex is held
+	 *   â reading finishes:              free buf, go to ptr == DROP
 	 */
-	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];
 
@@ -736,25 +788,47 @@ static void ffs_epfile_async_io_complete(struct usb_ep *_ep,
 	schedule_work(&io_data->work);
 }
 
+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 = xchg(&epfile->read_buffer, READ_BUFFER_DROP);
+	if (buf && buf != 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 = 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 = 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 +857,15 @@ 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;
+
+	/*
+	 * 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);
 
 	return ret;
 }
@@ -1097,8 +1179,7 @@ ffs_epfile_release(struct inode *inode, struct file *file)
 
 	ENTER();
 
-	kfree(epfile->read_buffer);
-	epfile->read_buffer = NULL;
+	__ffs_epfile_read_buffer_free(epfile);
 	ffs_data_closed(epfile->ffs);
 
 	return 0;
@@ -1724,24 +1805,20 @@ static void ffs_func_eps_disable(struct ffs_function *func)
 	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;
+			__ffs_epfile_read_buffer_free(epfile);
 			++epfile;
 		}
 	} while (--count);
+	spin_unlock_irqrestore(&func->ffs->eps_lock, flags);
 }
 
 static int ffs_func_eps_enable(struct ffs_function *func)
-- 
2.8.0.rc3.226.g39d4020