From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754568AbcEPTJ2 (ORCPT ); Mon, 16 May 2016 15:09:28 -0400 Received: from mail-wm0-f53.google.com ([74.125.82.53]:35607 "EHLO mail-wm0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753736AbcEPTJ0 convert rfc822-to-8bit (ORCPT ); Mon, 16 May 2016 15:09:26 -0400 From: Michal Nazarewicz To: Felipe Balbi , Alan Stern Cc: "Du\, Changbin" , Al Viro , "gregkh\@linuxfoundation.org" , "rui.silva\@linaro.org" , "k.opasiak\@samsung.com" , "lars\@metafoo.de" , "linux-usb\@vger.kernel.org" , "linux-kernel\@vger.kernel.org" Subject: Re: [PATCH] usb: gadget: f_fs: report error if excess data received In-Reply-To: <87bn46p2gk.fsf@linux.intel.com> Organization: http://mina86.com/ References: <87eg92p3cn.fsf@linux.intel.com> <87bn46p2gk.fsf@linux.intel.com> User-Agent: Notmuch/0.19+53~g2e63a09 (http://notmuchmail.org) Emacs/25.1.50.1 (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:160516:k.opasiak@samsung.com::NJ++6+IEw/IBdgzP:000000000000000000000000000000000000000010UN X-Hashcash: 1:20:160516:stern@rowland.harvard.edu::6ZkEnDXL5kyaYkHm:0000000000000000000000000000000000001ohH X-Hashcash: 1:20:160516:gregkh@linuxfoundation.org::48c/OL3rC++ABIKr:0000000000000000000000000000000000027p+ X-Hashcash: 1:20:160516:rui.silva@linaro.org::0VBwB52DgjCgmrgp:000000000000000000000000000000000000000002bPD X-Hashcash: 1:20:160516:lars@metafoo.de::tZnk+6+5km04In1t:0040yA X-Hashcash: 1:20:160516:viro@zeniv.linux.org.uk::5KMsrkS+twEAEXVr:0000000000000000000000000000000000000069fa X-Hashcash: 1:20:160516:changbin.du@intel.com::lYfzea1BbJHVHC+T:000000000000000000000000000000000000000090SF X-Hashcash: 1:20:160516:linux-usb@vger.kernel.org::8os7TMMXVnX9k0E3:0000000000000000000000000000000000009ufH X-Hashcash: 1:20:160516:felipe.balbi@linux.intel.com::KAqEwkYhrI5rSQSj:000000000000000000000000000000000FGZl X-Hashcash: 1:20:160516:linux-kernel@vger.kernel.org::nUScl6penj+le/Li:000000000000000000000000000000000DdSb Date: Mon, 16 May 2016 21:09:08 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, May 16 2016, Felipe Balbi wrote: > Michal Nazarewicz writes: > >>> Alan Stern 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 --- 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»