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

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

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

Since the buffer size for req is rounded up to maxpacketsize,
then we may end up with more data then user space has space
for.

If it happen, we can keep the excess data for next i/o, or
report an error. But we cannot silently drop data, because
USB layer should ensure the data integrality it has transferred,
otherwise applications may get corrupt data if it doesn't
detect this case.

Here, we simply report an error to userspace to let userspace
proccess. Actually, userspace applications should negotiate
with host side for how many bytes it should receive.

Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
 drivers/usb/gadget/function/f_fs.c | 48 +++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 15b648c..411ed2d 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -640,6 +640,36 @@ static void ffs_epfile_io_complete(struct usb_ep *_ep, struct usb_request *req)
 	}
 }
 
+static size_t ffs_copy_to_user(const void *buf, size_t bytes,
+				struct ffs_io_data *io_data)
+{
+	size_t count = iov_iter_count(&io_data->data);
+	int ret;
+
+	/**
+	 * Since the buffer size for req is rounded up to maxpacketsize,
+	 * then we may end up with more data then user space has space for.
+	 * We can keep the excess data for next i/o, or report an error.
+	 * But we cannot silently drop data, because USB layer should ensure
+	 * the data integrality it has transferred.
+	 *
+	 * Here, we simply report an error to userspace to let userspace
+	 * proccess. Actually, userspace applications should negotiate with
+	 * each other for how many bytes host send.
+	 */
+	if (bytes > count) {
+		pr_err("ffs read size %zu bigger than requested size %zu\n",
+			bytes, count);
+		return -EOVERFLOW;
+	}
+
+	ret = copy_to_iter(buf, bytes, &io_data->data);
+	if (ret != bytes)
+		return -EFAULT;
+
+	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,
@@ -650,9 +680,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 (iov_iter_count(&io_data->data))
-			ret = -EFAULT;
+		ret = ffs_copy_to_user(io_data->buf, ret, io_data);
 		unuse_mm(io_data->mm);
 	}
 
@@ -803,18 +831,10 @@ 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 (io_data->read && ret > 0)
+			ret = ffs_copy_to_user(data, ret, io_data);
+
 		goto error_mutex;
 	} else if (!(req = usb_ep_alloc_request(ep->ep, GFP_KERNEL))) {
 		ret = -ENOMEM;
-- 
2.7.4

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]


Hi,

changbin.du@intel.com writes:
> From: "Du, Changbin" <changbin.du@intel.com>
>
> Since the buffer size for req is rounded up to maxpacketsize,
> then we may end up with more data then user space has space
> for.

only for OUT direction with the controller you're using ;-)

> If it happen, we can keep the excess data for next i/o, or
> report an error. But we cannot silently drop data, because
> USB layer should ensure the data integrality it has transferred,
> otherwise applications may get corrupt data if it doesn't
> detect this case.

and when has this actually happened ? Host should not send more data in
this case, if it does, it's an error on the host side. Also, returning
-EOVERFLOW is not exactly correct here, because you'd violate POSIX
specification of read(), right ?

> Here, we simply report an error to userspace to let userspace
> proccess. Actually, userspace applications should negotiate

no, this violates POSIX. Care to explain what problem are you actually
facing ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Michal Nazarewicz]

On Wed, May 11 2016, Felipe Balbi wrote:
> Also, returning -EOVERFLOW is not exactly correct here, because you'd
> violate POSIX specification of read(), right ?

Maybe we could piggyback on:

       EINVAL fd was created via a call to timerfd_create(2) and the
              wrong size buffer was given to read();

But I kinda agree.  I’m not sure how much we need to care about this
instead of having user space round their buffers up to the nearest max
packet size boundary.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

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

[Du, Changbin]

> On Wed, May 11 2016, Felipe Balbi wrote:
> > Also, returning -EOVERFLOW is not exactly correct here, because you'd
> > violate POSIX specification of read(), right ?
> 
> Maybe we could piggyback on:
> 
>        EINVAL fd was created via a call to timerfd_create(2) and the
>               wrong size buffer was given to read();
> 
> But I kinda agree.  I’m not sure how much we need to care about this
> instead of having user space round their buffers up to the nearest max
> packet size boundary.
> 
> --
> Best regards
> ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
> «If at first you don’t succeed, give up skydiving»

This is a good idea that "having user space round their buffers". But kernel
Still cannot hide error silently. :)

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

[Du, Changbin]

> Hi,
> 
> changbin.du@intel.com writes:
> > From: "Du, Changbin" <changbin.du@intel.com>
> >
> > Since the buffer size for req is rounded up to maxpacketsize,
> > then we may end up with more data then user space has space
> > for.
> 
> only for OUT direction with the controller you're using ;-)
For sure.

> 
> > If it happen, we can keep the excess data for next i/o, or
> > report an error. But we cannot silently drop data, because
> > USB layer should ensure the data integrality it has transferred,
> > otherwise applications may get corrupt data if it doesn't
> > detect this case.
> 
> and when has this actually happened ? Host should not send more data in
> this case, if it does, it's an error on the host side. Also, returning
> -EOVERFLOW is not exactly correct here, because you'd violate POSIX
> specification of read(), right ?
> 
This can happen if the host side app force kill-restart, not taking care of this
special condition(and we are not documented), or even it is a bug. Usually APPs
may has  a protocol to control the packet size, but protocol mismatch can happen
if either side encounter an error.

Anyway, this is real. If kernel return success and drop data, the error may 
explosion later, or its totally hided (but why some data lost in kernel?
Kernel cannot tell userspace we cannot be trusted sometimes, right?). 
so IMO, if this is an error, we need report an error or fix it, not hide it.

The POSIX didn't say read cannot return "-EOVERFLOW", it says:
" Other errors may occur, depending on the object connected to fd."

If "-EOVERFLOW" is not suitable, EFAULT, or any suggestions?

> > Here, we simply report an error to userspace to let userspace
> > proccess. Actually, userspace applications should negotiate
> 
> no, this violates POSIX. Care to explain what problem are you actually
> facing ?
> 
Why this violates POSIX? Could you give more details?

The problem is device side app sometimes received incorrect data caused
by the dropping. Most times the error can be detected by APP itself, but
sometimes cannot. It depends on the design of its communication protocol.

> --
> Balbi

Best Regards,
Du, Changbin

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

[Felipe Balbi]

Hi,

"Du, Changbin" <changbin.du@intel.com> writes:
>> > If it happen, we can keep the excess data for next i/o, or
>> > report an error. But we cannot silently drop data, because
>> > USB layer should ensure the data integrality it has transferred,
>> > otherwise applications may get corrupt data if it doesn't
>> > detect this case.
>> 
>> and when has this actually happened ? Host should not send more data in
>> this case, if it does, it's an error on the host side. Also, returning
>> -EOVERFLOW is not exactly correct here, because you'd violate POSIX
>> specification of read(), right ?
>> 
> This can happen if the host side app force kill-restart, not taking care of this
> special condition(and we are not documented), or even it is a bug. Usually APPs
> may has  a protocol to control the packet size, but protocol mismatch can happen
> if either side encounter an error.
>
> Anyway, this is real. If kernel return success and drop data, the error may 
> explosion later, or its totally hided (but why some data lost in kernel?
> Kernel cannot tell userspace we cannot be trusted sometimes, right?). 
> so IMO, if this is an error, we need report an error or fix it, not hide it.
>
> The POSIX didn't say read cannot return "-EOVERFLOW", it says:
> " Other errors may occur, depending on the object connected to fd."
>
> If "-EOVERFLOW" is not suitable, EFAULT, or any suggestions?
>
>> > Here, we simply report an error to userspace to let userspace
>> > proccess. Actually, userspace applications should negotiate
>> 
>> no, this violates POSIX. Care to explain what problem are you actually
>> facing ?
>> 
> Why this violates POSIX? Could you give more details?

read(5) should return at mode 5 bytes. If there are more, than 5 bytes,
we don't error out, we just return the requested 5 bytes and wait for a
further read.

What I'm more concerned, however, is why we received more than expected
data. What's on the extra bytes ? Can you capture dwc3 traces ? Perhaps
add a few traces doing a hexdump (using __print_hex()) of the data in
req->buf.

> The problem is device side app sometimes received incorrect data caused
> by the dropping. Most times the error can be detected by APP itself, but

why ? app did e.g. read(5), that caused driver to queue a usb_request
with length set to 512. Host sent more data than the expected 5 bytes,
why did host do that ? And if that data was needed, why didn't userspace
read() more than 5 ?

-- 
balbi

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

[Du, Changbin]

Hi,

> >> and when has this actually happened ? Host should not send more data in
> >> this case, if it does, it's an error on the host side. Also, returning
> >> -EOVERFLOW is not exactly correct here, because you'd violate POSIX
> >> specification of read(), right ?
> >>
> > This can happen if the host side app force kill-restart, not taking care of this
> > special condition(and we are not documented), or even it is a bug. Usually
> APPs
> > may has  a protocol to control the packet size, but protocol mismatch can
> happen
> > if either side encounter an error.
> >
> > Anyway, this is real. If kernel return success and drop data, the error may
> > explosion later, or its totally hided (but why some data lost in kernel?
> > Kernel cannot tell userspace we cannot be trusted sometimes, right?).
> > so IMO, if this is an error, we need report an error or fix it, not hide it.
> >
> > The POSIX didn't say read cannot return "-EOVERFLOW", it says:
> > " Other errors may occur, depending on the object connected to fd."
> >
> > If "-EOVERFLOW" is not suitable, EFAULT, or any suggestions?
> >
> >> > Here, we simply report an error to userspace to let userspace
> >> > proccess. Actually, userspace applications should negotiate
> >>
> >> no, this violates POSIX. Care to explain what problem are you actually
> >> facing ?
> >>
> > Why this violates POSIX? Could you give more details?
> 
> read(5) should return at mode 5 bytes. If there are more, than 5 bytes,
> we don't error out, we just return the requested 5 bytes and wait for a
> further read.
> 
Yes, it is true. As I mentioned before, we also can keep the extra data for
next read. This need more work to maintain a buffer. Here I just simply 
report an error(let userspace know something goes wrong.) before the
logic is implemented by someone.
(Maybe ioctl approach may be more appropriate for usb transfer, like usbfs.)

> What I'm more concerned, however, is why we received more than
> expected
> data. What's on the extra bytes ? Can you capture dwc3 traces ? Perhaps
> add a few traces doing a hexdump (using __print_hex()) of the data in
> req->buf.
> 
The extra bytes can be anything(random), they just data from APP layer.
It doesn't make sense for you to check. So I will not dump them, sorry.

> > The problem is device side app sometimes received incorrect data caused
> > by the dropping. Most times the error can be detected by APP itself, but
> 
> why ? app did e.g. read(5), that caused driver to queue a usb_request
> with length set to 512. Host sent more data than the expected 5 bytes,
> why did host do that ? And if that data was needed, why didn't userspace
> read() more than 5 ?
> 
> --
> Balbi
Well, first, there must be a protocol upon usb between host side and device side.
Second device side didn't know how many bytes to receive, it need host side tell
it.  But host could be buggy, or the application is killed and restart. These all can lead
host send more than device wanted bytes. For sure it wrong at host side, but device
side don't know.

Best Regards,
Du, Changbin

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

[Felipe Balbi]

Hi,

"Du, Changbin" <changbin.du@intel.com> writes:
> Hi,
>
>> >> and when has this actually happened ? Host should not send more data in
>> >> this case, if it does, it's an error on the host side. Also, returning
>> >> -EOVERFLOW is not exactly correct here, because you'd violate POSIX
>> >> specification of read(), right ?
>> >>
>> > This can happen if the host side app force kill-restart, not taking care of this
>> > special condition(and we are not documented), or even it is a bug. Usually
>> APPs
>> > may has  a protocol to control the packet size, but protocol mismatch can
>> happen
>> > if either side encounter an error.
>> >
>> > Anyway, this is real. If kernel return success and drop data, the error may
>> > explosion later, or its totally hided (but why some data lost in kernel?
>> > Kernel cannot tell userspace we cannot be trusted sometimes, right?).
>> > so IMO, if this is an error, we need report an error or fix it, not hide it.
>> >
>> > The POSIX didn't say read cannot return "-EOVERFLOW", it says:
>> > " Other errors may occur, depending on the object connected to fd."
>> >
>> > If "-EOVERFLOW" is not suitable, EFAULT, or any suggestions?
>> >
>> >> > Here, we simply report an error to userspace to let userspace
>> >> > proccess. Actually, userspace applications should negotiate
>> >>
>> >> no, this violates POSIX. Care to explain what problem are you actually
>> >> facing ?
>> >>
>> > Why this violates POSIX? Could you give more details?
>> 
>> read(5) should return at mode 5 bytes. If there are more, than 5 bytes,
>> we don't error out, we just return the requested 5 bytes and wait for a
>> further read.
>> 
> Yes, it is true. As I mentioned before, we also can keep the extra data for
> next read. This need more work to maintain a buffer. Here I just simply 
> report an error(let userspace know something goes wrong.) before the
> logic is implemented by someone.

no, this is not how we do things here. If we find a bug we actually fix
it, we don't just work around it ;-)

> (Maybe ioctl approach may be more appropriate for usb transfer, like usbfs.)

heh, no :-)

>> What I'm more concerned, however, is why we received more than
>> expected
>> data. What's on the extra bytes ? Can you capture dwc3 traces ? Perhaps
>> add a few traces doing a hexdump (using __print_hex()) of the data in
>> req->buf.
>> 
> The extra bytes can be anything(random), they just data from APP layer.
> It doesn't make sense for you to check. So I will not dump them, sorry.

interesting, so you claim to have found a bug, but when asked to provide
more information your answer is "no" ? Thanks :-)

>> > The problem is device side app sometimes received incorrect data caused
>> > by the dropping. Most times the error can be detected by APP itself, but
>> 
>> why ? app did e.g. read(5), that caused driver to queue a usb_request
>> with length set to 512. Host sent more data than the expected 5 bytes,
>> why did host do that ? And if that data was needed, why didn't userspace
>> read() more than 5 ?
>> 
>
> Well, first, there must be a protocol upon usb between host side and
> device side.

sorry, I don't know what mean here. USB does not *require* a protocol
running on top of USB. There usually is one, but that's not a
requirement.

> Second device side didn't know how many bytes to receive, it need host
> side tell it.

well, many protocols work like this. See Mass Storage, for example.

> But host could be buggy,

if host is buggy, why should we fix host on the peripheral side ?

> or the application is killed and restart.

If application is killed (why was the application killed? Which
application was killed?), then why are we still connected to host at
all? It's clear that this gadget can't work without its userspace
counterpart. If that userspace isn't available, we should drop data
pullup and disconnect from host.

> These all can lead host send more than device wanted bytes. For sure
> it wrong at host side, but device side don't know.

but none of this means we have a bug at device side. In fact, by
allowing these extra bytes to reach userspace, we could be creating a
possible attack vector.

Your explanation is unsatisfactory, so I won't apply your patch, sorry.

-- 
balbi

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

[Du, Changbin]

> > The extra bytes can be anything(random), they just data from APP layer.
> > It doesn't make sense for you to check. So I will not dump them, sorry.
> 
> interesting, so you claim to have found a bug, but when asked to provide
> more information your answer is "no" ? Thanks :-)
> 
Do you really want a random hex string? I don't think it is useful.

> >> > The problem is device side app sometimes received incorrect data
> caused
> >> > by the dropping. Most times the error can be detected by APP itself, but
> >>
> >> why ? app did e.g. read(5), that caused driver to queue a usb_request
> >> with length set to 512. Host sent more data than the expected 5 bytes,
> >> why did host do that ? And if that data was needed, why didn't userspace
> >> read() more than 5 ?
> >>
> >
> > Well, first, there must be a protocol upon usb between host side and
> > device side.
> 
> sorry, I don't know what mean here. USB does not *require* a protocol
> running on top of USB. There usually is one, but that's not a
> requirement.
> 
Communication between two endpoints must has a protocol, even it may
very simple. Without protocol, they cannot exchange information.

> > Second device side didn't know how many bytes to receive, it need host
> > side tell it.
> 
> well, many protocols work like this. See Mass Storage, for example.
> 
> > But host could be buggy,
> 
> if host is buggy, why should we fix host on the peripheral side ?
> 
True it is bug of host, but is it a reason kernel can drop data then? 

> > or the application is killed and restart.
> 
> If application is killed (why was the application killed? Which
> application was killed?), then why are we still connected to host at
> all? It's clear that this gadget can't work without its userspace
> counterpart. If that userspace isn't available, we should drop data
> pullup and disconnect from host.
> 
Usb no need disconnect if the application exit (host side). Seems you
only care about device side.

> > These all can lead host send more than device wanted bytes. For sure
> > it wrong at host side, but device side don't know.
> 
> but none of this means we have a bug at device side. In fact, by
> allowing these extra bytes to reach userspace, we could be creating a
> possible attack vector.
> 
> Your explanation is unsatisfactory, so I won't apply your patch, sorry.
> 
> --
> balbi
It is fine. Then need userspace take care of all the data it received. Because
Kernel may drop some data for it. Kernel ffs driver is unauthentic sometimes.

Best Regards,
Du, Changbin

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

[Felipe Balbi]

Hi,

"Du, Changbin" <changbin.du@intel.com> writes:
>> >> > The problem is device side app sometimes received incorrect data
>> caused
>> >> > by the dropping. Most times the error can be detected by APP itself, but
>> >>
>> >> why ? app did e.g. read(5), that caused driver to queue a usb_request
>> >> with length set to 512. Host sent more data than the expected 5 bytes,
>> >> why did host do that ? And if that data was needed, why didn't userspace
>> >> read() more than 5 ?
>> >>
>> >
>> > Well, first, there must be a protocol upon usb between host side and
>> > device side.
>> 
>> sorry, I don't know what mean here. USB does not *require* a protocol
>> running on top of USB. There usually is one, but that's not a
>> requirement.
>> 
> Communication between two endpoints must has a protocol, even it may
> very simple. Without protocol, they cannot exchange information.

that protocol is USB :-)

>> > Second device side didn't know how many bytes to receive, it need host
>> > side tell it.
>> 
>> well, many protocols work like this. See Mass Storage, for example.
>> 
>> > But host could be buggy,
>> 
>> if host is buggy, why should we fix host on the peripheral side ?
>> 
> True it is bug of host, but is it a reason kernel can drop data then? 

sure is :-) For all we know, that data means nothing to us

>> > or the application is killed and restart.
>> 
>> If application is killed (why was the application killed? Which
>> application was killed?), then why are we still connected to host at
>> all? It's clear that this gadget can't work without its userspace
>> counterpart. If that userspace isn't available, we should drop data
>> pullup and disconnect from host.
>> 
> Usb no need disconnect if the application exit (host side). Seems you
> only care about device side.

oh, application was killed on host side. I thought it was on device
side. Yeah, then we shouldn't disconnect.

>> > These all can lead host send more than device wanted bytes. For sure
>> > it wrong at host side, but device side don't know.
>> 
>> but none of this means we have a bug at device side. In fact, by
>> allowing these extra bytes to reach userspace, we could be creating a
>> possible attack vector.
>> 
>> Your explanation is unsatisfactory, so I won't apply your patch, sorry.
>> 
>> --
>> balbi
> It is fine. Then need userspace take care of all the data it received. Because
> Kernel may drop some data for it. Kernel ffs driver is unauthentic sometimes.

I really cannot understand what you mean sometimes. You're saying that
userspace needs to take care of all the data it received because kernel
can drop data. If kernel is dropping data, there's no extra data
reaching userspace, right?

Is the problem that we *are* giving more data than expected to
userspace? Are we overflowing some userspace buffer? If that's the case,
then below should be enough for the time being:

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 73515d54e1cc..d1bd53c895ca 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -156,6 +156,8 @@ struct ffs_io_data {
 	struct usb_request *req;
 
 	struct ffs_data *ffs;
+
+	ssize_t expected_len;
 };
 
 struct ffs_desc_helper {
@@ -730,8 +732,10 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 		 * Controller may require buffer size to be aligned to
 		 * maxpacketsize of an out endpoint.
 		 */
-		if (io_data->read)
+		if (io_data->read) {
+			io_data->expected_len = data_len;
 			data_len = usb_ep_align_maybe(gadget, ep->ep, data_len);
+		}
 		spin_unlock_irq(&epfile->ffs->eps_lock);
 
 		data = kmalloc(data_len, GFP_KERNEL);
@@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 		 */
 		ret = interrupted ? -EINTR : ep->status;
 		if (io_data->read && ret > 0) {
-			ret = copy_to_iter(data, ret, &io_data->data);
+			if (ret > io_data->expected_len)
+				pr_debug("FFS: size mismatch: %zd for %zd",
+						ret, io_data->expected_len);
+
+			ret = copy_to_iter(data, io_data->expected_len,
+					&io_data->data);
 			if (!ret)
 				ret = -EFAULT;
 		}

that we can get merged during v4.7-rc and Cc stable and backport this to
anything containing Al's commit c993c39b8639 ("gadget/function/f_fs.c:
use put iov_iter into io_data").

-- 
balbi

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

[Felipe Balbi]

Hi again,

Felipe Balbi <felipe.balbi@linux.intel.com> writes:
> @@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
>  		 */
>  		ret = interrupted ? -EINTR : ep->status;
>  		if (io_data->read && ret > 0) {
> -			ret = copy_to_iter(data, ret, &io_data->data);
> +			if (ret > io_data->expected_len)
> +				pr_debug("FFS: size mismatch: %zd for %zd",
> +						ret, io_data->expected_len);
> +
> +			ret = copy_to_iter(data, io_data->expected_len,
> +					&io_data->data);

we need a min() here. Better version below:

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 73515d54e1cc..6c49b152f46e 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -156,6 +156,8 @@ struct ffs_io_data {
 	struct usb_request *req;
 
 	struct ffs_data *ffs;
+
+	ssize_t expected_len;
 };
 
 struct ffs_desc_helper {
@@ -730,8 +732,10 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 		 * Controller may require buffer size to be aligned to
 		 * maxpacketsize of an out endpoint.
 		 */
-		if (io_data->read)
+		if (io_data->read) {
+			io_data->expected_len = data_len;
 			data_len = usb_ep_align_maybe(gadget, ep->ep, data_len);
+		}
 		spin_unlock_irq(&epfile->ffs->eps_lock);
 
 		data = kmalloc(data_len, GFP_KERNEL);
@@ -811,7 +815,15 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 		 */
 		ret = interrupted ? -EINTR : ep->status;
 		if (io_data->read && ret > 0) {
-			ret = copy_to_iter(data, ret, &io_data->data);
+			ssize_t bytes;
+
+			if (ret > io_data->expected_len)
+				pr_debug("FFS: size mismatch: %zd for %zd",
+						ret, io_data->expected_len);
+
+			bytes = min(ret, io_data->expected_len);
+
+			ret = copy_to_iter(data, bytes, &io_data->data);
 			if (!ret)
 				ret = -EFAULT;
 		}


-- 
balbi

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

[Du, Changbin]

Hi,
> 
> we need a min() here. Better version below:
No need. copy_to_iter will do it for us.

Best Regards,
Du, Changbin

> 
> diff --git a/drivers/usb/gadget/function/f_fs.c
> b/drivers/usb/gadget/function/f_fs.c
> index 73515d54e1cc..6c49b152f46e 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -156,6 +156,8 @@ struct ffs_io_data {
>  	struct usb_request *req;
> 
>  	struct ffs_data *ffs;
> +
> +	ssize_t expected_len;
>  };
> 
>  struct ffs_desc_helper {
> @@ -730,8 +732,10 @@ static ssize_t ffs_epfile_io(struct file *file, struct
> ffs_io_data *io_data)
>  		 * Controller may require buffer size to be aligned to
>  		 * maxpacketsize of an out endpoint.
>  		 */
> -		if (io_data->read)
> +		if (io_data->read) {
> +			io_data->expected_len = data_len;
>  			data_len = usb_ep_align_maybe(gadget, ep->ep,
> data_len);
> +		}
>  		spin_unlock_irq(&epfile->ffs->eps_lock);
> 
>  		data = kmalloc(data_len, GFP_KERNEL);
> @@ -811,7 +815,15 @@ static ssize_t ffs_epfile_io(struct file *file, struct
> ffs_io_data *io_data)
>  		 */
>  		ret = interrupted ? -EINTR : ep->status;
>  		if (io_data->read && ret > 0) {
> -			ret = copy_to_iter(data, ret, &io_data->data);
> +			ssize_t bytes;
> +
> +			if (ret > io_data->expected_len)
> +				pr_debug("FFS: size mismatch: %zd for %zd",
> +						ret, io_data->expected_len);
> +
> +			bytes = min(ret, io_data->expected_len);
> +
> +			ret = copy_to_iter(data, bytes, &io_data->data);
>  			if (!ret)
>  				ret = -EFAULT;
>  		}
> 
> 
> --
> balbi

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

[Du, Changbin]

> >> > These all can lead host send more than device wanted bytes. For sure
> >> > it wrong at host side, but device side don't know.
> >>
> >> but none of this means we have a bug at device side. In fact, by
> >> allowing these extra bytes to reach userspace, we could be creating a
> >> possible attack vector.
> >>
> >> Your explanation is unsatisfactory, so I won't apply your patch, sorry.
> >>
> >> --
> >> balbi
> > It is fine. Then need userspace take care of all the data it received. Because
> > Kernel may drop some data for it. Kernel ffs driver is unauthentic
> sometimes.
> 
> I really cannot understand what you mean sometimes. You're saying that
> userspace needs to take care of all the data it received because kernel
> can drop data. If kernel is dropping data, there's no extra data
> reaching userspace, right?
> 
For sure, maybe I didn't describe it well so let you confused. :)

> Is the problem that we *are* giving more data than expected to
> userspace? Are we overflowing some userspace buffer? If that's the case,
> then below should be enough for the time being:
> 
No, the problem is we drop data but silently. We cannot give more data to
userspace since buffer is limited.

> diff --git a/drivers/usb/gadget/function/f_fs.c
> b/drivers/usb/gadget/function/f_fs.c
> index 73515d54e1cc..d1bd53c895ca 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -156,6 +156,8 @@ struct ffs_io_data {
>  	struct usb_request *req;
> 
>  	struct ffs_data *ffs;
> +
> +	ssize_t expected_len;
>  };
> 
>  struct ffs_desc_helper {
> @@ -730,8 +732,10 @@ static ssize_t ffs_epfile_io(struct file *file, struct
> ffs_io_data *io_data)
>  		 * Controller may require buffer size to be aligned to
>  		 * maxpacketsize of an out endpoint.
>  		 */
> -		if (io_data->read)
> +		if (io_data->read) {
> +			io_data->expected_len = data_len;
>  			data_len = usb_ep_align_maybe(gadget, ep->ep,
> data_len);
> +		}
>  		spin_unlock_irq(&epfile->ffs->eps_lock);
> 
>  		data = kmalloc(data_len, GFP_KERNEL);
> @@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct
> ffs_io_data *io_data)
>  		 */
>  		ret = interrupted ? -EINTR : ep->status;
>  		if (io_data->read && ret > 0) {
> -			ret = copy_to_iter(data, ret, &io_data->data);
> +			if (ret > io_data->expected_len)
> +				pr_debug("FFS: size mismatch: %zd for %zd",
> +						ret, io_data->expected_len);
> +
> +			ret = copy_to_iter(data, io_data->expected_len,
> +					&io_data->data);
>  			if (!ret)
>  				ret = -EFAULT;
>  		}
> 
> that we can get merged during v4.7-rc and Cc stable and backport this to
> anything containing Al's commit c993c39b8639 ("gadget/function/f_fs.c:
> use put iov_iter into io_data").
> 

The different for this code is just give warning but not return error. It is also
fine for me that at least this let development can find some key message to
find What happed under kernel. But the message should be *error* I think.

And this missed AIO path. This is identify to my patch after remove the
"return -EOVERFLOW;" line.

Byw, we not need add the field "expected_len", we can get it from the
struct ffs_io_data.

If this is fine for you, I can publish a new patch.

> --
> Balbi

Best Regards,
Du, Changbin

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3259 bytes --]


Hi,

"Du, Changbin" <changbin.du@intel.com> writes:
>> >> > These all can lead host send more than device wanted bytes. For sure
>> >> > it wrong at host side, but device side don't know.
>> >>
>> >> but none of this means we have a bug at device side. In fact, by
>> >> allowing these extra bytes to reach userspace, we could be creating a
>> >> possible attack vector.
>> >>
>> >> Your explanation is unsatisfactory, so I won't apply your patch, sorry.
>> >>
>> >> --
>> >> balbi
>> > It is fine. Then need userspace take care of all the data it received. Because
>> > Kernel may drop some data for it. Kernel ffs driver is unauthentic
>> sometimes.
>> 
>> I really cannot understand what you mean sometimes. You're saying that
>> userspace needs to take care of all the data it received because kernel
>> can drop data. If kernel is dropping data, there's no extra data
>> reaching userspace, right?
>> 
> For sure, maybe I didn't describe it well so let you confused. :)

okay

>> Is the problem that we *are* giving more data than expected to
>> userspace? Are we overflowing some userspace buffer? If that's the case,
>> then below should be enough for the time being:
>> 
> No, the problem is we drop data but silently. We cannot give more data to

okay, but does that create any problems for device side userspace? What
problem is that?

> userspace since buffer is limited.

right, and that was my point: if we copy more to userspace, then we have
a real big problem.

>> @@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct
>> ffs_io_data *io_data)
>>  		 */
>>  		ret = interrupted ? -EINTR : ep->status;
>>  		if (io_data->read && ret > 0) {
>> -			ret = copy_to_iter(data, ret, &io_data->data);
>> +			if (ret > io_data->expected_len)
>> +				pr_debug("FFS: size mismatch: %zd for %zd",
>> +						ret, io_data->expected_len);
>> +
>> +			ret = copy_to_iter(data, io_data->expected_len,
>> +					&io_data->data);
>>  			if (!ret)
>>  				ret = -EFAULT;
>>  		}
>> 
>> that we can get merged during v4.7-rc and Cc stable and backport this to
>> anything containing Al's commit c993c39b8639 ("gadget/function/f_fs.c:
>> use put iov_iter into io_data").
>> 
>
> The different for this code is just give warning but not return
> error. It is also fine for me that at least this let development can
> find some key message to find What happed under kernel. But the
> message should be *error* I think.

I'm fine with pr_error()

> And this missed AIO path. This is identify to my patch after remove the

right, it's more of a debug patch since I don't have the setup to
trigger this (I'm assuming you're using adb?)

> "return -EOVERFLOW;" line.

there's one key difference, see below

> Byw, we not need add the field "expected_len", we can get it from the
> struct ffs_io_data.

without expected_len we can copy more data to userspace, right ? If
req->actual > data_len_before_aligning_to_maxpacket, then we will copy
more data then we should to userspace and this was a regression caused
by Al's commit, AFAICT.

> If this is fine for you, I can publish a new patch.
>
>> --
>> Balbi
>
> Best Regards,
> Du, Changbin

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 0 bytes --]

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

[Felipe Balbi]

Hi,

"Du, Changbin" <changbin.du@intel.com> writes:
>> >> > These all can lead host send more than device wanted bytes. For sure
>> >> > it wrong at host side, but device side don't know.
>> >>
>> >> but none of this means we have a bug at device side. In fact, by
>> >> allowing these extra bytes to reach userspace, we could be creating a
>> >> possible attack vector.
>> >>
>> >> Your explanation is unsatisfactory, so I won't apply your patch, sorry.
>> >>
>> >> --
>> >> balbi
>> > It is fine. Then need userspace take care of all the data it received. Because
>> > Kernel may drop some data for it. Kernel ffs driver is unauthentic
>> sometimes.
>> 
>> I really cannot understand what you mean sometimes. You're saying that
>> userspace needs to take care of all the data it received because kernel
>> can drop data. If kernel is dropping data, there's no extra data
>> reaching userspace, right?
>> 
> For sure, maybe I didn't describe it well so let you confused. :)

okay

>> Is the problem that we *are* giving more data than expected to
>> userspace? Are we overflowing some userspace buffer? If that's the case,
>> then below should be enough for the time being:
>> 
> No, the problem is we drop data but silently. We cannot give more data to

okay, but does that create any problems for device side userspace? What
problem is that?

> userspace since buffer is limited.

right, and that was my point: if we copy more to userspace, then we have
a real big problem.

>> @@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct
>> ffs_io_data *io_data)
>>  		 */
>>  		ret = interrupted ? -EINTR : ep->status;
>>  		if (io_data->read && ret > 0) {
>> -			ret = copy_to_iter(data, ret, &io_data->data);
>> +			if (ret > io_data->expected_len)
>> +				pr_debug("FFS: size mismatch: %zd for %zd",
>> +						ret, io_data->expected_len);
>> +
>> +			ret = copy_to_iter(data, io_data->expected_len,
>> +					&io_data->data);
>>  			if (!ret)
>>  				ret = -EFAULT;
>>  		}
>> 
>> that we can get merged during v4.7-rc and Cc stable and backport this to
>> anything containing Al's commit c993c39b8639 ("gadget/function/f_fs.c:
>> use put iov_iter into io_data").
>> 
>
> The different for this code is just give warning but not return
> error. It is also fine for me that at least this let development can
> find some key message to find What happed under kernel. But the
> message should be *error* I think.

I'm fine with pr_error()

> And this missed AIO path. This is identify to my patch after remove the

right, it's more of a debug patch since I don't have the setup to
trigger this (I'm assuming you're using adb?)

> "return -EOVERFLOW;" line.

there's one key difference, see below

> Byw, we not need add the field "expected_len", we can get it from the
> struct ffs_io_data.

without expected_len we can copy more data to userspace, right ? If
req->actual > data_len_before_aligning_to_maxpacket, then we will copy
more data then we should to userspace and this was a regression caused
by Al's commit, AFAICT.

> If this is fine for you, I can publish a new patch.
>
>> --
>> Balbi
>
> Best Regards,
> Du, Changbin

-- 
balbi

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

[Du, Changbin]

> Hi,
> 
> "Du, Changbin" <changbin.du@intel.com> writes:
> >> >> > These all can lead host send more than device wanted bytes. For
> sure
> >> >> > it wrong at host side, but device side don't know.
> >> >>
> >> >> but none of this means we have a bug at device side. In fact, by
> >> >> allowing these extra bytes to reach userspace, we could be creating a
> >> >> possible attack vector.
> >> >>
> >> >> Your explanation is unsatisfactory, so I won't apply your patch, sorry.
> >> >>
> >> >> --
> >> >> balbi
> >> > It is fine. Then need userspace take care of all the data it received.
> Because
> >> > Kernel may drop some data for it. Kernel ffs driver is unauthentic
> >> sometimes.
> >>
> >> I really cannot understand what you mean sometimes. You're saying that
> >> userspace needs to take care of all the data it received because kernel
> >> can drop data. If kernel is dropping data, there's no extra data
> >> reaching userspace, right?
> >>
> > For sure, maybe I didn't describe it well so let you confused. :)
> 
> okay
> 
> >> Is the problem that we *are* giving more data than expected to
> >> userspace? Are we overflowing some userspace buffer? If that's the case,
> >> then below should be enough for the time being:
> >>
> > No, the problem is we drop data but silently. We cannot give more data to
> 
> okay, but does that create any problems for device side userspace? What
> problem is that?
> 
> > userspace since buffer is limited.
> 
> right, and that was my point: if we copy more to userspace, then we have
> a real big problem.
> 
Yes, we drop the data because we userspace buffer is not enough this time.
The problem here is that really can we just drop it silently? Maybe not.

> >> @@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct
> >> ffs_io_data *io_data)
> >>  		 */
> >>  		ret = interrupted ? -EINTR : ep->status;
> >>  		if (io_data->read && ret > 0) {
> >> -			ret = copy_to_iter(data, ret, &io_data->data);
> >> +			if (ret > io_data->expected_len)
> >> +				pr_debug("FFS: size mismatch: %zd for %zd",
> >> +						ret, io_data->expected_len);
> >> +
> >> +			ret = copy_to_iter(data, io_data->expected_len,
> >> +					&io_data->data);
> >>  			if (!ret)
> >>  				ret = -EFAULT;
> >>  		}
> >>
> >> that we can get merged during v4.7-rc and Cc stable and backport this to
> >> anything containing Al's commit c993c39b8639 ("gadget/function/f_fs.c:
> >> use put iov_iter into io_data").
> >>
> >
> > The different for this code is just give warning but not return
> > error. It is also fine for me that at least this let development can
> > find some key message to find What happed under kernel. But the
> > message should be *error* I think.
> 
> I'm fine with pr_error()
> 
> > And this missed AIO path. This is identify to my patch after remove the
> 
> right, it's more of a debug patch since I don't have the setup to
> trigger this (I'm assuming you're using adb?)
> 
Right. And adb can detect this unexpected behavior(data mismatch) quickly
because it has some selfcheck for the data content.

> > "return -EOVERFLOW;" line.
> 
> there's one key difference, see below
> 
> > Byw, we not need add the field "expected_len", we can get it from the
> > struct ffs_io_data.
> 
> without expected_len we can copy more data to userspace, right ? If
> req->actual > data_len_before_aligning_to_maxpacket, then we will copy
> more data then we should to userspace and this was a regression caused
> by Al's commit, AFAICT.
> 
No, expected_len equals to iov_iter_count(&io_data->data), right? So we
do not need a new field.

> > If this is fine for you, I can publish a new patch.
> >
> >> --
> >> Balbi
> >
> > Best Regards,
> > Du, Changbin
> 
> --
> balbi

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

[Felipe Balbi]

Hi,

"Du, Changbin" <changbin.du@intel.com> writes:
>> right, and that was my point: if we copy more to userspace, then we have
>> a real big problem.
>> 
> Yes, we drop the data because we userspace buffer is not enough this time.
> The problem here is that really can we just drop it silently? Maybe not.

Yeah, it probably deserves a pr_err() or pr_debug(), but host sending
more data than it should, is another problem altogether which needs to
be addressed at the host.

Adding a print to aid debugging is a good idea, but bailing out on the
peripheral side is not :-s

>> > And this missed AIO path. This is identify to my patch after remove the
>> 
>> right, it's more of a debug patch since I don't have the setup to
>> trigger this (I'm assuming you're using adb?)
>> 
> Right. And adb can detect this unexpected behavior(data mismatch) quickly
> because it has some selfcheck for the data content.

cool

>> > Byw, we not need add the field "expected_len", we can get it from the
>> > struct ffs_io_data.
>> 
>> without expected_len we can copy more data to userspace, right ? If
>> req->actual > data_len_before_aligning_to_maxpacket, then we will copy
>> more data then we should to userspace and this was a regression caused
>> by Al's commit, AFAICT.
>> 
> No, expected_len equals to iov_iter_count(&io_data->data), right? So we
> do not need a new field.

/me goes read iov_iter_count()

you're right, we don't need expected len at all ;-)

in any case, did you figure out if the extra data host sends is
important data at all or just garbage ?

-- 
balbi

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

[Du, Changbin]

> Hi,
> 
> "Du, Changbin" <changbin.du@intel.com> writes:
> >> right, and that was my point: if we copy more to userspace, then we have
> >> a real big problem.
> >>
> > Yes, we drop the data because we userspace buffer is not enough this time.
> > The problem here is that really can we just drop it silently? Maybe not.
> 
> Yeah, it probably deserves a pr_err() or pr_debug(), but host sending
> more data than it should, is another problem altogether which needs to
> be addressed at the host.
> 
> Adding a print to aid debugging is a good idea, but bailing out on the
> peripheral side is not :-s
> 
Ok, if we think this is a problem at host side that the transfer is not device
expected, then device side should not accept the data or deliver the
transferred data to userspace. But now we take part of the data to userspace
and says it is ok.
Do you agree with this point?

IMO, we expose usb transfer as a file on device side. But file read() doesn't
have a requirement that "sorry, you cannot read so little! you need read all
once, else we may drop data for you. :) ".
And some library that may retry read() until get enough data (which is normal
For a general read). Then sometimes the buffer size for sys_read may not as
expected. This is why I think ioctl approach is more appropriate for usb transfer.

> --
> Balbi

Best Regards,
Du, Changbin

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2381 bytes --]


Hi,

"Du, Changbin" <changbin.du@intel.com> writes:
>> "Du, Changbin" <changbin.du@intel.com> writes:
>> >> right, and that was my point: if we copy more to userspace, then we have
>> >> a real big problem.
>> >>
>> > Yes, we drop the data because we userspace buffer is not enough this time.
>> > The problem here is that really can we just drop it silently? Maybe not.
>> 
>> Yeah, it probably deserves a pr_err() or pr_debug(), but host sending
>> more data than it should, is another problem altogether which needs to
>> be addressed at the host.
>> 
>> Adding a print to aid debugging is a good idea, but bailing out on the
>> peripheral side is not :-s
>> 
> Ok, if we think this is a problem at host side that the transfer is not device
> expected, then device side should not accept the data or deliver the
> transferred data to userspace. But now we take part of the data to userspace
> and says it is ok.
> Do you agree with this point?

We deliver to userspace the part userspace requested, right? So that's
okay. The USB details WRT e.g. babble or host trying to send more data
than expected, needs to be handled within the kernel.

> IMO, we expose usb transfer as a file on device side. But file read() doesn't
> have a requirement that "sorry, you cannot read so little! you need read all
> once, else we may drop data for you. :) ".

but that's not how read() semantics work. When userspace asks to read(x)
bytes, we have three possible outcomes:

i. We have x bytes to return, so we copy_to_user(x)

ii. We have y < x bytes to return, so we copy_to_user(y)

iii. We have y > x bytes to return, so we copy_to_user(x)

This is exactly how the kernel is behaving. The only "detail" we have is
that, for some reason, host is sending too much data. what I still don't
know is if this extra data is garbage or something userspace genuinely
cares about. Do you know the answer to this?

> And some library that may retry read() until get enough data (which is
> normal For a general read). Then sometimes the buffer size for
> sys_read may not as expected. This is why I think ioctl approach is
> more appropriate for usb transfer.

no, this won't change anything. Besides, it's a pointless discussion as
cannot break userspace ABI. GadgetFS and FunctionFS have been shipping
in kernel for many years.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Du, Changbin]

Hi,

> >> Yeah, it probably deserves a pr_err() or pr_debug(), but host sending
> >> more data than it should, is another problem altogether which needs to
> >> be addressed at the host.
> >>
> >> Adding a print to aid debugging is a good idea, but bailing out on the
> >> peripheral side is not :-s
> >>
> > Ok, if we think this is a problem at host side that the transfer is not device
> > expected, then device side should not accept the data or deliver the
> > transferred data to userspace. But now we take part of the data to
> userspace
> > and says it is ok.
> > Do you agree with this point?
> 
> We deliver to userspace the part userspace requested, right? So that's
> okay. The USB details WRT e.g. babble or host trying to send more data
> than expected, needs to be handled within the kernel.
> 
*babble* error is a good example. As you know, when xhci received more
data than expected, how does it process it? It doesn't deliver to software
the part userspace requested, but just give an error. And the xhci driver
set the urb status to -EOVERFLOW. This is similar to device side between
kernel and userspace.

> > IMO, we expose usb transfer as a file on device side. But file read() doesn't
> > have a requirement that "sorry, you cannot read so little! you need read all
> > once, else we may drop data for you. :) ".
> 
> but that's not how read() semantics work. When userspace asks to read(x)
> bytes, we have three possible outcomes:
> 
> i. We have x bytes to return, so we copy_to_user(x)
> 
> ii. We have y < x bytes to return, so we copy_to_user(y)
> 
> iii. We have y > x bytes to return, so we copy_to_user(x)
> 
I totally agree with these. They are all right. But what if userspace read
Twice? EG. When it want read a its packet, it may first read a head size,
Then read body.
read(20) = read(5) + read(15)
If this normal for a file, and works well, right? But if it happens on 
FunctionFS, the first read success, but the remailing 15 bytes lost because
they dropped by kernel. You may think it is host's bug, which also means
FunctionFS file does be different. You can compare usb with network,
Does network driver drop data? Afaik, You can read any bytes from a
socket, and data never lost.

For example, the host may send "aaabbb" and "cccddd", and device
side app May get "aaaccc" and all read success! Host send also success! 
But "bbb" "ddd" are lost.

The different views we have is on how to process the extra data. So we
can focus discussion here.

> This is exactly how the kernel is behaving. The only "detail" we have is
> that, for some reason, host is sending too much data. what I still don't
> know is if this extra data is garbage or something userspace genuinely
> cares about. Do you know the answer to this?
> 
IMO, FunctionFS should not care about what the data is. That is userspace' s
logic and meaningless for kernel. Kernel should not assume the data is garbage
or not. Is it right?

PS, for adb, it will re-open FunctionFS file after it detect unexpected data.

> > And some library that may retry read() until get enough data (which is
> > normal For a general read). Then sometimes the buffer size for
> > sys_read may not as expected. This is why I think ioctl approach is
> > more appropriate for usb transfer.
> 
> no, this won't change anything. Besides, it's a pointless discussion as
> cannot break userspace ABI. GadgetFS and FunctionFS have been shipping
> in kernel for many years.
> 
That because developers know the special requirement of FunctionFS,
just like adb.

> --
> Balbi

Do you mind if I modify my original patch for print error instead of return
error?

Best Regards,
Du, Changbin

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

[Alan Stern]

On Fri, 13 May 2016, Felipe Balbi wrote:

> We deliver to userspace the part userspace requested, right? So that's
> okay. The USB details WRT e.g. babble or host trying to send more data
> than expected, needs to be handled within the kernel.

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.

Alan Stern

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

[Michal Nazarewicz]

On Fri, May 13 2016, Alan Stern wrote:
> 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.

I was wondering about that for a while actually.  So far, f_fs’ model
was: one read, one request.  Splitting requests would certainly be
possible, but is that what f_fs’ users would expect to happen if host
rounds the request up?

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> On Fri, 13 May 2016, Felipe Balbi wrote:
>
>> We deliver to userspace the part userspace requested, right? So that's
>> okay. The USB details WRT e.g. babble or host trying to send more data
>> than expected, needs to be handled within the kernel.
>
> 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.

right, but that just means we need to buffer the data instead of bailing
out of the first read() completely.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Michal Nazarewicz]

> 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.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 904 bytes --]

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(?)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Michal Nazarewicz]

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»

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

[Du, Changbin]

> 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.
> 

Congratulations finally reach an agreement, thanks Alan Stern and Michal.
Here just have a comment - the buffered data need be dropped when the
epfile is closed, because it means the session is terminated.

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

[Michal Nazarewicz]

On Tue, May 17 2016, Changbin Du wrote:
>> 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.
>> 
>
> Congratulations finally reach an agreement,

To be honest, if it was up to me, I would rip request size rounding up
out of the code.

> thanks Alan Stern and Michal.
> Here just have a comment - the buffered data need be dropped when the
> epfile is closed, because it means the session is terminated.

I blame that on sleep deprivation.  Another issue is what to do when
endpoint is disabled.  Should the buffer be cleared as soon as the
endpoint is disabled?  Or maybe when the endpoint is enabled again?  Or
maybe it should never be cleared?

If the buffer is cleared when endpoint is disabled, we again silently
drop data.  On the other hand, if we don’t do that, read() on the
endpoint will may succeed even if the configuration is disabled which
may be surprising for users.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2258 bytes --]


Hi,

Michal Nazarewicz <mina86@mina86.com> writes:
> On Tue, May 17 2016, Changbin Du wrote:
>>> 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.
>>> 
>>
>> Congratulations finally reach an agreement,
>
> To be honest, if it was up to me, I would rip request size rounding up
> out of the code.

we've been through this before. This needs to be done at the gadget
layer. Gadget driver can over-allocate ahead of time if
gadget->quirk_ep_out_aligned_size is true, then we avoid memcpy() at the
UDC driver level.

>> thanks Alan Stern and Michal.
>> Here just have a comment - the buffered data need be dropped when the
>> epfile is closed, because it means the session is terminated.
>
> I blame that on sleep deprivation.  Another issue is what to do when
> endpoint is disabled.  Should the buffer be cleared as soon as the
> endpoint is disabled?  Or maybe when the endpoint is enabled again?  Or
> maybe it should never be cleared?
>
> If the buffer is cleared when endpoint is disabled, we again silently
> drop data.  On the other hand, if we don’t do that, read() on the
> endpoint will may succeed even if the configuration is disabled which
> may be surprising for users.

tough decision... but seems like clearing the buffer as soon as ep is
disabled is the way to go.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Michal Nazarewicz]

On Wed, May 18 2016, Felipe Balbi wrote:
> we've been through this before. This needs to be done at the gadget
> layer. Gadget driver can over-allocate ahead of time if
> gadget->quirk_ep_out_aligned_size is true, then we avoid memcpy() at
> the UDC driver level.

Right, all right, so let’s look at it from a regular USB function point
of view.  If a USB function allocates a request which is not aligned,
UDC will align the buffer and *drop* excess data.  Seeing how ugly
f_fs’s code is becoming, I’m now leaning to letting to f_fs do the same
thing: if user space makes an unaligned read, f_fs aligns the buffer and
then drops excess data.

Any arguments for f_fs to not drop the data apply to UDC, so they should
behave identically.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

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

[Du, Changbin]

> On Wed, May 18 2016, Felipe Balbi wrote:
> > we've been through this before. This needs to be done at the gadget
> > layer. Gadget driver can over-allocate ahead of time if
> > gadget->quirk_ep_out_aligned_size is true, then we avoid memcpy() at
> > the UDC driver level.
> 
> Right, all right, so let’s look at it from a regular USB function point
> of view.  If a USB function allocates a request which is not aligned,
> UDC will align the buffer and *drop* excess data.  Seeing how ugly
>
Do you mean UDC driver align the buffer? I searched the code, currently
only DWC3 needs buffer size to be aligned to MaxPacketSize on ep out.
And the align is done in f_fs driver.

> f_fs’s code is becoming, I’m now leaning to letting to f_fs do the same
> thing: if user space makes an unaligned read, f_fs aligns the buffer and
> then drops excess data.
> 
> Any arguments for f_fs to not drop the data apply to UDC, so they should
> behave identically.
> 
I'd prefer fail the request at all, and it is better done in HW. Because per the
USB Spec that device can return NAK if a function was unable to accept data
>From the host. the DWC3 has not been design as this, if software fail the
transfer, it is a little weird for host.

So, now we have 3 choices:
1) buffer the excess data
2) fail the transfer
3) drop the excess data, then print an warning message

Which one do you prefer?

> --
> Best regards
> ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
> «If at first you don’t succeed, give up skydiving»

Best Regards,
Du, Changbin

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

[Michal Nazarewicz]

On Thu, May 19 2016, Changbin Du wrote:
>> On Wed, May 18 2016, Felipe Balbi wrote:
>> > we've been through this before. This needs to be done at the gadget
>> > layer. Gadget driver can over-allocate ahead of time if
>> > gadget->quirk_ep_out_aligned_size is true, then we avoid memcpy() at
>> > the UDC driver level.
>> 
>> Right, all right, so let’s look at it from a regular USB function point
>> of view.  If a USB function allocates a request which is not aligned,
>> UDC will align the buffer and *drop* excess data.  Seeing how ugly

> Do you mean UDC driver align the buffer? I searched the code, currently
> only DWC3 needs buffer size to be aligned to MaxPacketSize on ep out.
> And the align is done in f_fs driver.

I thought that was what was happening based on Felipe’s comment about
avoiding memcpy.  I looked at the code now and dunno what actually
happens.

>> f_fs’s code is becoming, I’m now leaning to letting to f_fs do the same
>> thing: if user space makes an unaligned read, f_fs aligns the buffer and
>> then drops excess data.
>> 
>> Any arguments for f_fs to not drop the data apply to UDC, so they should
>> behave identically.
>> 
> I'd prefer fail the request at all, and it is better done in HW.
> Because per the USB Spec that device can return NAK if a function was
> unable to accept data From the host.  The DWC3 has not been design as
> this, if software fail the transfer, it is a little weird for host.
>
> So, now we have 3 choices:
> 1) buffer the excess data
> 2) fail the transfer

You mean fail when more data has been sent (i.e. drop the whole packet)
or fail at entry to read() if the buffer is not aligned?

> 3) drop the excess data, then print an warning message
>
> Which one do you prefer?

I think f_fs should mimic whatever happens if unaligned request is
queued on dwc3.  As far as I understand, this is not 1.

I’ll be travelling again on Friday so I’ll finish up the patch doing 1
so we will have a choice between 1 (my patch) and 3 (your patch).

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

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

[Du, Changbin]

Hi,
> > I'd prefer fail the request at all, and it is better done in HW.
> > Because per the USB Spec that device can return NAK if a function was
> > unable to accept data From the host.  The DWC3 has not been design as
> > this, if software fail the transfer, it is a little weird for host.
> >
> > So, now we have 3 choices:
> > 1) buffer the excess data
> > 2) fail the transfer
> 
> You mean fail when more data has been sent (i.e. drop the whole packet)
> or fail at entry to read() if the buffer is not aligned?
> 
I mean the first one.

> > 3) drop the excess data, then print an warning message
> >
> > Which one do you prefer?
> 
> I think f_fs should mimic whatever happens if unaligned request is
> queued on dwc3.  As far as I understand, this is not 1.
> 
> I’ll be travelling again on Friday so I’ll finish up the patch doing 1
> so we will have a choice between 1 (my patch) and 3 (your patch).
> 
Great! Prefer your patch if #1 works good.

> --
> Best regards
> ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
> «If at first you don’t succeed, give up skydiving»

Best Regards,
Du, Changbin

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

[Du, Changbin]

> >> thanks Alan Stern and Michal.
> >> Here just have a comment - the buffered data need be dropped when
> the
> >> epfile is closed, because it means the session is terminated.
> >
> > I blame that on sleep deprivation.  Another issue is what to do when
> > endpoint is disabled.  Should the buffer be cleared as soon as the
> > endpoint is disabled?  Or maybe when the endpoint is enabled again?  Or
> > maybe it should never be cleared?
> >
> > If the buffer is cleared when endpoint is disabled, we again silently
> > drop data.  On the other hand, if we don’t do that, read() on the
> > endpoint will may succeed even if the configuration is disabled which
> > may be surprising for users.
> 
> tough decision... but seems like clearing the buffer as soon as ep is
> disabled is the way to go.
> 
> --
> Balbi

I agree with Balbi, seems it is not easy to maintain the excess buffer... I was to
implement it at the beginning but I am not confident everything is done correctly.

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

[Michal Nazarewicz]

So I’ve been looking at AIO handling in f_fs and either I’m stupid or
the code is broken.  Here’s part of ffs_user_copy_worker:

	int ret = io_data->req->status ? io_data->req->status :
					 io_data->req->actual;
	if (io_data->read && ret > 0) {
		use_mm(io_data->mm);
		ret = copy_to_iter(io_data->buf, ret, &io_data->data);
		if (iov_iter_count(&io_data->data))
			ret = -EFAULT;
		unuse_mm(io_data->mm);
	}

First of all, shouldn’t the copy_to_iter invocation be:

		if (copy_to_iter(io_data->buf, ret, &io_data->data))
                	ret = -EFAULT;

Second of all, if the request reads fewer bytes than user requested,
iov_iter_count(…) will be non-zero (namely it will be the difference
between user’s buffer size and data read).  This should not result in
EFAULT though.

So, am I going crazy? Or does this need to be fixed as well?

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

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

[Lars-Peter Clausen]

On 05/16/2016 06:05 PM, Michal Nazarewicz wrote:
> So I’ve been looking at AIO handling in f_fs and either I’m stupid or
> the code is broken.

The code was broken. Fixed in commit 332a5b446b791 ("usb: gadget: f_fs: Fix
EFAULT generation for async read operations:).

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

[Michal Nazarewicz]

> On 05/16/2016 06:05 PM, Michal Nazarewicz wrote:
>> So I’ve been looking at AIO handling in f_fs and either I’m stupid or
>> the code is broken.

On Mon, May 16 2016, Lars-Peter Clausen wrote:
> The code was broken. Fixed in commit 332a5b446b791 ("usb: gadget:
> f_fs: Fix EFAULT generation for async read operations:).

Ah, now I remember, thanks!

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

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

[Krzysztof Opasiak]

Hi Michal,

On 05/16/2016 06:05 PM, Michal Nazarewicz wrote:
> So I’ve been looking at AIO handling in f_fs and either I’m stupid or
> the code is broken.  Here’s part of ffs_user_copy_worker:
> 
> 	int ret = io_data->req->status ? io_data->req->status :
> 					 io_data->req->actual;
> 	if (io_data->read && ret > 0) {
> 		use_mm(io_data->mm);
> 		ret = copy_to_iter(io_data->buf, ret, &io_data->data);
> 		if (iov_iter_count(&io_data->data))
> 			ret = -EFAULT;
> 		unuse_mm(io_data->mm);
> 	}
> 
> First of all, shouldn’t the copy_to_iter invocation be:
> 
> 		if (copy_to_iter(io_data->buf, ret, &io_data->data))
>                 	ret = -EFAULT;
> 
> Second of all, if the request reads fewer bytes than user requested,
> iov_iter_count(…) will be non-zero (namely it will be the difference
> between user’s buffer size and data read).  This should not result in
> EFAULT though.
> 
> So, am I going crazy? Or does this need to be fixed as well?
> 

I think it has been already fixed:

http://permalink.gmane.org/gmane.linux.usb.general/139316

Cheers,
-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics