Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1398925 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] usb: gadget: f_fs: report error if excess data received changbin.du@intel.com - 2016-05-11 12:40 +0200
    Re: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <balbi@kernel.org> - 2016-05-11 13:10 +0200
      Re: [PATCH] usb: gadget: f_fs: report error if excess data received Michal Nazarewicz <mina86@mina86.com> - 2016-05-11 14:40 +0200
        RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-12 06:30 +0200
      RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-12 06:30 +0200
        RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <balbi@kernel.org> - 2016-05-12 09:00 +0200
          RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-12 09:40 +0200
            RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-12 09:50 +0200
              RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-12 10:20 +0200
                RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-12 11:20 +0200
                  RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-12 11:30 +0200
                    RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-12 12:00 +0200
                  RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-12 11:50 +0200
                    RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-12 12:20 +0200
                    RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-12 12:20 +0200
                      RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-12 12:50 +0200
                        RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-12 13:30 +0200
                          RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-13 08:00 +0200
                            RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-13 08:40 +0200
                              RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-13 12:40 +0200
                              RE: [PATCH] usb: gadget: f_fs: report error if excess data received Alan Stern <stern@rowland.harvard.edu> - 2016-05-13 16:30 +0200
                                Re: [PATCH] usb: gadget: f_fs: report error if excess data received Michal Nazarewicz <mina86@mina86.com> - 2016-05-14 22:40 +0200
                                RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-16 15:10 +0200
                                  Re: [PATCH] usb: gadget: f_fs: report error if excess data received Michal Nazarewicz <mina86@mina86.com> - 2016-05-16 15:10 +0200
                                    Re: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-16 15:20 +0200
                                      Re: [PATCH] usb: gadget: f_fs: report error if excess data received Michal Nazarewicz <mina86@mina86.com> - 2016-05-16 21:10 +0200
                                        RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-17 05:00 +0200
                                          Re: [PATCH] usb: gadget: f_fs: report error if excess data received Michal Nazarewicz <mina86@mina86.com> - 2016-05-18 11:50 +0200
                                            Re: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-18 12:20 +0200
                                              Re: [PATCH] usb: gadget: f_fs: report error if excess data received Michal Nazarewicz <mina86@mina86.com> - 2016-05-18 15:40 +0200
                                                RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-19 05:00 +0200
                                                  Re: [PATCH] usb: gadget: f_fs: report error if excess data received Michal Nazarewicz <mina86@mina86.com> - 2016-05-19 09:40 +0200
                                                    RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-19 11:00 +0200
                                              RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-19 04:40 +0200
    Re: [PATCH] usb: gadget: f_fs: report error if excess data received Michal Nazarewicz <mina86@mina86.com> - 2016-05-16 18:10 +0200
      Re: [PATCH] usb: gadget: f_fs: report error if excess data received Lars-Peter Clausen <lars@metafoo.de> - 2016-05-16 18:30 +0200
        Re: [PATCH] usb: gadget: f_fs: report error if excess data received Michal Nazarewicz <mina86@mina86.com> - 2016-05-16 18:50 +0200
      Re: [PATCH] usb: gadget: f_fs: report error if excess data received Krzysztof Opasiak <k.opasiak@samsung.com> - 2016-05-16 18:40 +0200

Page 1 of 2  [1] 2  Next page →

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

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

[toc] | [next] | [standalone]

#1398946

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1399022

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»

[toc] | [prev] | [next] | [standalone]

#1399664

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

[toc] | [prev] | [next] | [standalone]

#1399663

> 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

[toc] | [prev] | [next] | [standalone]

#1399698

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

[toc] | [prev] | [next] | [standalone]

#1399719

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

[toc] | [prev] | [next] | [standalone]

#1399721

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

[toc] | [prev] | [next] | [standalone]

#1399742

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

[toc] | [prev] | [next] | [standalone]

#1399815

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

[toc] | [prev] | [next] | [standalone]

#1399821

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

[toc] | [prev] | [next] | [standalone]

#1399845

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

[toc] | [prev] | [next] | [standalone]

#1399834

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

[toc] | [prev] | [next] | [standalone]

#1399861

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1399862

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

[toc] | [prev] | [next] | [standalone]

#1399908

> 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

[toc] | [prev] | [next] | [standalone]

#1399935

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

[toc] | [prev] | [next] | [standalone]

#1400520

> 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

[toc] | [prev] | [next] | [standalone]

#1400536

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1400688

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

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web