* [PATCH 1/2] usb: gadget: f_fs: printk error when excess data is dropped on read
2016-05-21 18:47 [PATCH 0/2] f_fs: better handle excess data on read Michal Nazarewicz
@ 2016-05-21 18:47 ` Michal Nazarewicz
2016-05-21 18:47 ` [PATCH 2/2] usb: gadget: f_fs: buffer data from ‘oversized’ OUT requests Michal Nazarewicz
1 sibling, 0 replies; 3+ messages in thread
From: Michal Nazarewicz @ 2016-05-21 18:47 UTC (permalink / raw)
To: Felipe Balbi, Du, Changbin
Cc: Greg Kroah-Hartman, Al Viro, rui.silva, k.opasiak, lars,
linux-kernel, linux-usb, Michal Nazarewicz
Add a pr_err when host sent more data then the size of the buffer user
space gave us. This may happen on UDCs which require OUT requests to
be aligned to max packet size. The patch includes a description of the
situation.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
drivers/usb/gadget/function/f_fs.c | 61 ++++++++++++++++++++++++++++----------
1 file changed, 46 insertions(+), 15 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 2c314c1..e26a6b4 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -640,6 +640,44 @@ static void ffs_epfile_io_complete(struct usb_ep *_ep, struct usb_request *req)
}
}
+static ssize_t ffs_copy_to_iter(void *data, int data_len, struct iov_iter *iter)
+{
+ ssize_t ret = copy_to_iter(data, data_len, iter);
+ if (likely(ret == data_len))
+ return ret;
+
+ if (unlikely(iov_iter_count(iter)))
+ return -EFAULT;
+
+ /*
+ * Dear user space developer!
+ *
+ * TL;DR: To stop getting below error message in your kernel log, change
+ * user space code using functionfs to align read buffers to a max
+ * packet size.
+ *
+ * Some UDCs (e.g. dwc3) require request sizes to be a multiple of a max
+ * packet size. When unaligned buffer is passed to functionfs, it
+ * internally uses a larger, aligned buffer so that such UDCs are happy.
+ *
+ * Unfortunately, this means that host may send more data than was
+ * requested in read(2) system call. f_fs doesn’t know what to do with
+ * that excess data so it simply drops it.
+ *
+ * Was the buffer aligned in the first place, no such problem would
+ * happen.
+ *
+ * This only affects OUT endpoints, i.e. reading data with a read(2),
+ * aio_read(2) etc. system calls. Writing data to an IN endpoint is not
+ * affected.
+ */
+ pr_err("functionfs read size %d > requested size %zd, dropping excess data. "
+ "Align read buffer size to max packet size to avoid the problem.\n",
+ data_len, ret);
+
+ return ret;
+}
+
static void ffs_user_copy_worker(struct work_struct *work)
{
struct ffs_io_data *io_data = container_of(work, struct ffs_io_data,
@@ -649,9 +687,7 @@ static void ffs_user_copy_worker(struct work_struct *work)
if (io_data->read && ret > 0) {
use_mm(io_data->mm);
- ret = copy_to_iter(io_data->buf, ret, &io_data->data);
- if (ret != io_data->req->actual && iov_iter_count(&io_data->data))
- ret = -EFAULT;
+ ret = ffs_copy_to_iter(io_data->buf, ret, &io_data->data);
unuse_mm(io_data->mm);
}
@@ -804,18 +840,13 @@ 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) {
- ret = copy_to_iter(data, ret, &io_data->data);
- if (!ret)
- ret = -EFAULT;
- }
+ if (interrupted)
+ ret = -EINTR;
+ else if (io_data->read && ep->status > 0)
+ ret = ffs_copy_to_iter(data, ep->status,
+ &io_data->data);
+ else
+ ret = ep->status;
goto error_mutex;
} else if (!(req = usb_ep_alloc_request(ep->ep, GFP_KERNEL))) {
ret = -ENOMEM;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH 2/2] usb: gadget: f_fs: buffer data from ‘oversized’ OUT requests
2016-05-21 18:47 [PATCH 0/2] f_fs: better handle excess data on read Michal Nazarewicz
2016-05-21 18:47 ` [PATCH 1/2] usb: gadget: f_fs: printk error when excess data is dropped " Michal Nazarewicz
@ 2016-05-21 18:47 ` Michal Nazarewicz
1 sibling, 0 replies; 3+ messages in thread
From: Michal Nazarewicz @ 2016-05-21 18:47 UTC (permalink / raw)
To: Felipe Balbi, Du, Changbin
Cc: Greg Kroah-Hartman, Al Viro, rui.silva, k.opasiak, lars,
linux-kernel, linux-usb, Michal Nazarewicz
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 | 130 +++++++++++++++++++++++++++++++------
1 file changed, 109 insertions(+), 21 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index e26a6b4..08a1ac2 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; /* P: epfile->mutex */
+
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 {
@@ -667,6 +679,11 @@ static ssize_t ffs_copy_to_iter(void *data, int data_len, struct iov_iter *iter)
* Was the buffer aligned in the first place, no such problem would
* happen.
*
+ * Data may be dropped only in AIO reads. Synchronous reads are handled
+ * by splitting a request into multiple parts. This splitting may still
+ * be a problem though so it’s likely best to align the buffer
+ * regardless of it being AIO or not..
+ *
* This only affects OUT endpoints, i.e. reading data with a read(2),
* aio_read(2) etc. system calls. Writing data to an IN endpoint is not
* affected.
@@ -717,6 +734,56 @@ static void ffs_epfile_async_io_complete(struct usb_ep *_ep,
schedule_work(&io_data->work);
}
+/* 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;
+ ssize_t ret;
+ if (!buf)
+ 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))) {
+ ret = -EFAULT;
+ } else {
+ buf->length -= ret;
+ buf->data += ret;
+ }
+ return ret;
+}
+
+/* Assumes epfile->mutex is held. */
+static ssize_t __ffs_epfile_read_data(struct ffs_epfile *epfile,
+ void *data, int data_len,
+ struct iov_iter *iter)
+{
+ struct ffs_buffer *buf;
+
+ ssize_t ret = copy_to_iter(data, data_len, iter);
+ if (likely(data_len == ret))
+ return ret;
+
+ if (unlikely(iov_iter_count(iter)))
+ return -EFAULT;
+
+ /* See ffs_copy_to_iter for more context. */
+ pr_warn("functionfs read size %d > requested size %zd, splitting request into multiple reads.",
+ data_len, ret);
+
+ data_len -= ret;
+ buf = kmalloc(sizeof(*buf) + data_len, GFP_KERNEL);
+ buf->length = data_len;
+ buf->data = buf->storage;
+ memcpy(buf->storage, data + ret, data_len);
+ epfile->read_buffer = buf;
+
+ return ret;
+}
+
static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
{
struct ffs_epfile *epfile = file->private_data;
@@ -746,21 +813,40 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
if (halt && epfile->isoc)
return -EINVAL;
+ /* We will be using request and read_buffer */
+ ret = ffs_mutex_lock(&epfile->mutex, file->f_flags & O_NONBLOCK);
+ if (unlikely(ret))
+ goto error;
+
/* Allocate & copy */
if (!halt) {
+ struct usb_gadget *gadget;
+
+ /*
+ * 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 (!io_data->aio && io_data->read) {
+ ret = __ffs_epfile_read_buffered(epfile, &io_data->data);
+ if (ret)
+ goto error_mutex;
+ }
+
/*
* if we _do_ wait above, the epfile->ffs->gadget might be NULL
* before the waiting completes, so do not assign to 'gadget'
* earlier
*/
- struct usb_gadget *gadget = epfile->ffs->gadget;
- size_t copied;
+ gadget = epfile->ffs->gadget;
spin_lock_irq(&epfile->ffs->eps_lock);
/* In the meantime, endpoint got disabled or changed. */
if (epfile->ep != ep) {
- spin_unlock_irq(&epfile->ffs->eps_lock);
- return -ESHUTDOWN;
+ ret = -ESHUTDOWN;
+ goto error_lock;
}
data_len = iov_iter_count(&io_data->data);
/*
@@ -772,22 +858,17 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
spin_unlock_irq(&epfile->ffs->eps_lock);
data = kmalloc(data_len, GFP_KERNEL);
- if (unlikely(!data))
- return -ENOMEM;
- if (!io_data->read) {
- copied = copy_from_iter(data, data_len, &io_data->data);
- if (copied != data_len) {
- ret = -EFAULT;
- goto error;
- }
+ if (unlikely(!data)) {
+ ret = -ENOMEM;
+ goto error_mutex;
+ }
+ if (!io_data->read &&
+ copy_from_iter(data, data_len, &io_data->data) != data_len) {
+ ret = -EFAULT;
+ goto error_mutex;
}
}
- /* We will be using request */
- ret = ffs_mutex_lock(&epfile->mutex, file->f_flags & O_NONBLOCK);
- if (unlikely(ret))
- goto error;
-
spin_lock_irq(&epfile->ffs->eps_lock);
if (epfile->ep != ep) {
@@ -843,8 +924,8 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
if (interrupted)
ret = -EINTR;
else if (io_data->read && ep->status > 0)
- ret = ffs_copy_to_iter(data, ep->status,
- &io_data->data);
+ ret = __ffs_epfile_read_data(epfile, data, ep->status,
+ &io_data->data);
else
ret = ep->status;
goto error_mutex;
@@ -1012,6 +1093,8 @@ ffs_epfile_release(struct inode *inode, struct file *file)
ENTER();
+ kfree(epfile->read_buffer);
+ epfile->read_buffer = NULL;
ffs_data_closed(epfile->ffs);
return 0;
@@ -1637,19 +1720,24 @@ 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 {
+ if (epfile)
+ mutex_lock(&epfile->mutex);
+ spin_lock_irqsave(&func->ffs->eps_lock, flags);
/* pending requests get nuked */
if (likely(ep->ep))
usb_ep_disable(ep->ep);
++ep;
+ spin_unlock_irqrestore(&func->ffs->eps_lock, flags);
if (epfile) {
epfile->ep = NULL;
+ kfree(epfile->read_buffer);
+ epfile->read_buffer = NULL;
+ mutex_unlock(&epfile->mutex);
++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
^ permalink raw reply [flat|nested] 3+ messages in thread