Re: [PATCH] usb: gadget: f_fs: report error if excess data received

[Michal Nazarewicz <mina86@mina86.com>]

On Mon, May 16 2016, Felipe Balbi wrote:
> Michal Nazarewicz <mina86@mina86.com> writes:
>
>>> Alan Stern <stern@rowland.harvard.edu> writes:
>>>> The point is that you don't know whether the host sent more data than
>>>> expected.  All you know is that the host sent more data than the user
>>>> asked the kernel for -- but maybe the user didn't ask for all the
>>>> data that he expected.  Maybe the user wanted to retrieve the full
>>>> set of data using two read() system calls.
>>
>> On Mon, May 16 2016, Felipe Balbi wrote:
>>> right, but that just means we need to buffer the data instead of bailing
>>> out of the first read() completely.
>>
>> Correct.
>>
>> I have a ~4h bus ride ahead of me so I’ll try to implement it.  If you
>> don’t hear from me by the end of the day, there probably wasn’t enough
>> space/comfort in the bus to use a laptop.
>
> Cool, Michal. Thanks
>
> seems like a kfifo would do well here(?)

There appears to be no kfifo support for iov_iter though, so I just went
with a simple buffer.

I haven’t looked at the patch too carefully so this is an RFC rather
than an actual patch at this point.  It does compile at least.

Regardless, the more I thin about it, the more I’m under the impression
that the whole rounding up in f_fs was a mistake.  And the more I’m
leaning towards ignoring the excess data set by the host.

---------- >8 ----------------------------------------------------------
Subject: usb: gadget: f_fs: buffer data from ‘oversized’ OUT requests

f_fs rounds up read(2) requests to a multiple of a max packet size
which means that host may provide more data than user has space for.
So far, the excess data has been silently ignored.

This introduces a buffer for a tail of such requests so that they are
returned on next read instead of being ignored.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/function/f_fs.c | 63 +++++++++++++++++++++++++++++++++-----
 1 file changed, 56 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 2c314c1..7d3c51a 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -130,6 +130,12 @@ struct ffs_epfile {
 
 	struct dentry			*dentry;
 
+	/*
+	 * 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.
+	 */
+	struct ffs_buffer		*read_buffer;
+
 	char				name[5];
 
 	unsigned char			in;	/* P: ffs->eps_lock */
@@ -138,6 +144,12 @@ struct ffs_epfile {
 	unsigned char			_pad;
 };
 
+struct ffs_buffer {
+	size_t length;
+	char *data;
+	char storage[];
+};
+
 /*  ffs_io_data structure ***************************************************/
 
 struct ffs_io_data {
@@ -681,6 +693,24 @@ static void ffs_epfile_async_io_complete(struct usb_ep *_ep,
 	schedule_work(&io_data->work);
 }
 
+static ssize_t ffs_epfile_read_buffered(struct ffs_epfile *epfile,
+					struct iov_iter *iter)
+{
+	struct ffs_buffer *buf = epfile->read_buffer;
+	ssize_t ret = 0;
+	if (buf) {
+		ret = copy_to_iter(buf->data, buf->length, iter);
+		buf->length -= ret;
+		if (buf->length) {
+			buf->data += ret;
+		} else {
+			kfree(buf);
+			epfile->read_buffer = NULL;
+		}
+	}
+	return ret;
+}
+
 static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 {
 	struct ffs_epfile *epfile = file->private_data;
@@ -710,6 +740,18 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 	if (halt && epfile->isoc)
 		return -EINVAL;
 
+	/*
+	 * Do we have buffered data from previous partial read?  Check that for
+	 * synchronous case only because we do not have facility to ‘wake up’
+	 * a pending asynchronous read and push buffered data to it which we
+	 * would need to make things behave consistently.
+	 */
+	if (!halt && !io_data->aio && io_data->read) {
+		ret = ffs_epfile_read_buffered(epfile, &io_data->data);
+		if (ret)
+			return ret;
+	}
+
 	/* Allocate & copy */
 	if (!halt) {
 		/*
@@ -804,17 +846,24 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 			interrupted = ep->status < 0;
 		}
 
-		/*
-		 * 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 = interrupted ? -EINTR : ep->status;
 		if (io_data->read && ret > 0) {
+			size_t left;
 			ret = copy_to_iter(data, ret, &io_data->data);
-			if (!ret)
+			left = ep->status - ret;
+			if (!left) {
+				/* nop */
+			} else if (iov_iter_count(&io_data->data)) {
 				ret = -EFAULT;
+			} else {
+				struct ffs_buffer *buf = kmalloc(
+					sizeof(*epfile->read_buffer) + left,
+					GFP_KERNEL);
+				buf->length = left;
+				buf->data = buf->storage;
+				memcpy(buf->storage, data + ret, left);
+				epfile->read_buffer = buf;
+			}
 		}
 		goto error_mutex;
 	} else if (!(req = usb_ep_alloc_request(ep->ep, GFP_KERNEL))) {
-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»