usb: gadget: f_fs: report error if excess data received
PATCH 1/1 usb: gadget: f_fs: report error if excess data received
From: "Du, Changbin" <[email protected]>
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.
--- drivers/usb/gadget/function/f_fs.c | 48 +++++++++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 14 deletions(-)
drivers/usb/gadget/function/f_fs.c +34 -14
@@ -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 ;
Signature · 1 line
2.7.4
Hi,
Quoted 5 lines · [email protected] writes:
From: "Du, Changbin" <[email protected]>
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 ;-)
Quoted 5 lines
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 ?
Quoted 2 lines
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 ?
Signature · 1 line
balbi
Quoted 2 lines · Felipe Balbi
Also, returning
-EOVERFLOWis not exactly correct here, because you'd violate POSIX specification ofread(), 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.
Signature · 3 lines
Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving»
Quoted 9 lines · Felipe Balbi
Quoted 2 lines
Also, returning
-EOVERFLOWis not exactly correct here, because you'd violate POSIX specification ofread(), right ?Maybe we could piggyback on:
EINVAL fd was created via a call to
timerfd_create(2)and the
wrong size buffer was given toread();
Quoted 8 lines
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. :)
Quoted 10 lines
Hi,
Quoted 5 lines · [email protected] writes:
From: "Du, Changbin" <[email protected]>
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.
Quoted 7 lines
Quoted 6 lines
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.
Quoted 5 lines
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
-EOVERFLOWis not exactly correct here, because you'd violate POSIX specification ofread(), 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?
Quoted 6 lines
Quoted 2 lines
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.
Quoted 2 lines
--
Balbi
Best Regards,
Du, Changbin
Hi,
Quoted 26 lines · Changbin
Quoted 11 lines
Quoted 5 lines
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
-EOVERFLOWis not exactly correct here, because you'd violate POSIX specification ofread(), 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?
Quoted 7 lines
Quoted 6 lines
Quoted 2 lines
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.
Quoted 2 lines
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 ?
Signature · 1 line
balbi
Hi,
Quoted 25 lines
Quoted 7 lines
Quoted 5 lines
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
-EOVERFLOWis not exactly correct here, because you'd violate POSIX specification ofread(), 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
Quoted 1 line
may has a protocol to control the packet size, but protocol mismatch can
happen
Quoted 15 lines
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?Quoted 3 lines
Quoted 2 lines
Here, we simply report an error to userspace to let userspace proccess. Actually, userspace applications should negotiate
Quoted 9 lines
Quoted 4 lines
Quoted 3 lines
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.)
Quoted 6 lines
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 inreq->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.
Quoted 10 lines
Quoted 2 lines
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_requestwith 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 userspaceread()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
Hi,
Quoted 32 lines · Changbin
Hi,
Quoted 30 lines
Quoted 7 lines
Quoted 5 lines
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
-EOVERFLOWis not exactly correct here, because you'd violate POSIX specification ofread(), 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
Quoted 1 line
may has a protocol to control the packet size, but protocol mismatch can
happen
Quoted 19 lines
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?Quoted 6 lines
Quoted 2 lines
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?
Quoted 8 lines
Quoted 4 lines
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 ;-)
Quoted 1 line
(Maybe ioctl approach may be more appropriate for usb transfer, like usbfs.)
heh, no :-)
Quoted 8 lines
Quoted 6 lines
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 inreq->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 :-)
Quoted 11 lines
Quoted 8 lines
Quoted 2 lines
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_requestwith 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 userspaceread()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.
Quoted 2 lines
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.
Quoted 1 line
But host could be buggy,
if host is buggy, why should we fix host on the peripheral side ?
Quoted 1 line
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.
Quoted 2 lines
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.
Signature · 1 line
balbi
Quoted 6 lines
Quoted 2 lines
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.
Quoted 10 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
The problem is device side app sometimes received incorrect data
caused
Quoted 8 lines
Quoted 7 lines
Quoted 1 line
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_requestwith 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 userspaceread()more than 5 ?
Quoted 7 lines
Quoted 2 lines
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.
Quoted 9 lines
Quoted 2 lines
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.
Quoted 1 line
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?
Quoted 8 lines
Quoted 1 line
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.
Quoted 11 lines
Quoted 2 lines
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
Hi,
Quoted 13 lines · Changbin
Quoted 13 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
The problem is device side app sometimes received incorrect data
caused
Quoted 10 lines
Quoted 7 lines
Quoted 1 line
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_requestwith 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 userspaceread()more than 5 ?Well, first, there must be a protocol upon usb between host side and device side.
Quoted 6 lines
Quoted 4 lines
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 :-)
Quoted 10 lines
Quoted 9 lines
Quoted 2 lines
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.
Quoted 1 line
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
Quoted 10 lines
Quoted 8 lines
Quoted 1 line
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.
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
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.
Quoted 6 lines
Quoted 4 lines
Your explanation is unsatisfactory, so I won't apply your patch, sorry.
--
balbiIt 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:
drivers/usb/gadget/function/f_fs.c +11 -2
@@ -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").
Signature · 1 line
balbi
Hi again,
Review static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) · line 823
Quoted 11 lines · Felipe Balbi
(quoted diff) +6
-1@@ -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:
drivers/usb/gadget/function/f_fs.c +14 -2
@@ -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 ; }
Signature · 1 line
balbi
Hi,
Quoted 2 lines
we need a
min()here. Better version below:
No need. copy_to_iter will do it for us.
Best Regards,
Du, Changbin
Quoted 51 lines
drivers/usb/gadget/function/f_fs.c +5
-1@@ -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, structffs_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);drivers/usb/gadget/function/f_fs.c +9
-1data =kmalloc (data_len,GFP_KERNEL );@@ -811,7 +815,15 @@ static ssize_t ffs_epfile_io(struct file *file, structffs_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
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 2 lines
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
Quoted 9 lines
Quoted 2 lines
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. :)
Quoted 4 lines
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.
Quoted 44 lines
drivers/usb/gadget/function/f_fs.c +5
-1@@ -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, structffs_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);drivers/usb/gadget/function/f_fs.c +6
-1data =kmalloc (data_len,GFP_KERNEL );@@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, structffs_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 ;}
Quoted 4 lines
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 putiov_iterintoio_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.
Quoted 2 lines
--
Balbi
Best Regards,
Du, Changbin
Hi,
Quoted 13 lines · Changbin
Quoted 13 lines
Quoted 13 lines
Quoted 11 lines
Quoted 2 lines
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.
--
balbiIt 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
Quoted 8 lines
Quoted 7 lines
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
Quoted 5 lines
Quoted 4 lines
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?
Quoted 1 line
userspace since buffer is limited.
right, and that was my point: if we copy more to userspace, then we have a real big problem.
Quoted 16 lines
Quoted 16 lines
drivers/usb/gadget/function/f_fs.c +6
-1@@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, structffs_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 ;}
Quoted 9 lines
Quoted 4 lines
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 putiov_iterintoio_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()
Quoted 1 line
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?)
Quoted 1 line
"return
-EOVERFLOW;" line.
there's one key difference, see below
Quoted 2 lines
Byw, we not need add the field "
expected_len", we can get it from the structffs_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.
Quoted 7 lines
If this is fine for you, I can publish a new patch.
Quoted 2 lines
--
BalbiBest Regards,
Du, Changbin
Signature · 1 line
balbi
Hi,
Quoted 13 lines · Changbin
Quoted 13 lines
Quoted 13 lines
Quoted 11 lines
Quoted 2 lines
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.
--
balbiIt 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
Quoted 8 lines
Quoted 7 lines
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
Quoted 5 lines
Quoted 4 lines
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?
Quoted 1 line
userspace since buffer is limited.
right, and that was my point: if we copy more to userspace, then we have a real big problem.
Quoted 16 lines
Quoted 16 lines
drivers/usb/gadget/function/f_fs.c +6
-1@@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, structffs_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 ;}
Quoted 9 lines
Quoted 4 lines
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 putiov_iterintoio_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()
Quoted 1 line
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?)
Quoted 1 line
"return
-EOVERFLOW;" line.
there's one key difference, see below
Quoted 2 lines
Byw, we not need add the field "
expected_len", we can get it from the structffs_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.
Quoted 7 lines
If this is fine for you, I can publish a new patch.
Quoted 2 lines
--
BalbiBest Regards,
Du, Changbin
Signature · 1 line
balbi
Quoted 35 lines
Hi,
Quoted 1 line · Changbin
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
These all can lead host send more than device wanted bytes. For
sure
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 1 line
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.
--
balbiIt is fine. Then need userspace take care of all the data it received.
Because
Quoted 9 lines
Quoted 8 lines
Quoted 1 line
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
Quoted 5 lines
Quoted 4 lines
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
Quoted 8 lines
okay, but does that create any problems for device side userspace? What problem is that?
Quoted 1 line
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.
Quoted 26 lines
Quoted 16 lines
Quoted 16 lines
drivers/usb/gadget/function/f_fs.c +6
-1@@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, structffs_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 ;}
Quoted 9 lines
Quoted 4 lines
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 putiov_iterintoio_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.
Quoted 7 lines
I'm fine with
pr_error()Quoted 1 line
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.
Quoted 4 lines
Quoted 1 line
"return
-EOVERFLOW;" line.there's one key difference, see below
Quoted 8 lines
Quoted 2 lines
Byw, we not need add the field "
expected_len", we can get it from the structffs_io_data.without
expected_lenwe can copy more data to userspace, right ? Ifreq->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.
Quoted 10 lines
Quoted 7 lines
If this is fine for you, I can publish a new patch.
Quoted 2 lines
--
BalbiBest Regards,
Du, Changbin--
balbi
Hi,
Quoted 5 lines · Changbin
Quoted 3 lines
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
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
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
Quoted 10 lines
Quoted 8 lines
Quoted 2 lines
Byw, we not need add the field "
expected_len", we can get it from the structffs_io_data.without
expected_lenwe can copy more data to userspace, right ? Ifreq->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_lenequals toiov_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 ?
Signature · 1 line
balbi
Quoted 9 lines
Hi,
Quoted 5 lines · Changbin
Quoted 3 lines
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.
Quoted 7 lines
Yeah, it probably deserves a
pr_err()orpr_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.
Quoted 2 lines
--
Balbi
Best Regards,
Du, Changbin
Hi,
Quoted 11 lines · Changbin
Quoted 11 lines
Quoted 5 lines · Changbin
Quoted 3 lines
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()orpr_debug(), but host sending more data than it should, is another problem altogether which needs to be addressed at the host.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 3 lines
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?
Quoted 4 lines
And some library that may retry
read()until get enough data (which is normal For a general read). Then sometimes the buffer size forsys_readmay 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.
Signature · 1 line
balbi
Hi,
Quoted 10 lines
Quoted 10 lines
Quoted 7 lines
Yeah, it probably deserves a
pr_err()orpr_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
Quoted 8 lines
userspace
Quoted 2 lines
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.
Quoted 4 lines
Quoted 3 lines
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. :) ".
Quoted 9 lines
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.
Quoted 5 lines
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.
Quoted 9 lines
Quoted 4 lines
And some library that may retry
read()until get enough data (which is normal For a general read). Then sometimes the buffer size forsys_readmay 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.
Quoted 2 lines
--
Balbi
Do you mind if I modify my original patch for print error instead of return error?
Best Regards,
Du, Changbin
Quoted 3 lines · Felipe Balbi
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
Quoted 5 lines · Alan Stern
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?
Signature · 3 lines
Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving»
Hi,
Quoted 11 lines · Alan Stern
Quoted 3 lines · Felipe Balbi
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.
Signature · 1 line
balbi
Quoted 6 lines
Quoted 5 lines · Alan Stern
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.
Quoted 2 lines · Felipe Balbi
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.
Signature · 3 lines
Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving»
Quoted 8 lines · Michal Nazarewicz
Quoted 6 lines
Quoted 5 lines · Alan Stern
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:
Quoted 8 lines
Quoted 2 lines
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(?)
Signature · 1 line
balbi
Quoted 15 lines · Felipe Balbi
Quoted 13 lines · Michal Nazarewicz
Quoted 6 lines
Quoted 5 lines · Alan Stern
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.Quoted 2 lines · Felipe Balbi
right, but that just means we need to buffer the data instead of bailing out of the first
read()completely.Correct.
Quoted 7 lines
Quoted 3 lines
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.
--- drivers/usb/gadget/function/f_fs.c | 63 +++++++++++++++++++++++++++++++++----- 1 file changed, 56 insertions(+), 7 deletions(-)
drivers/usb/gadget/function/f_fs.c +56 -7
@@ -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 ))) {
Signature · 3 lines
Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving»
Quoted 13 lines
There appears to be no kfifo support for
iov_iterthough, 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_fswas 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
Quoted 7 lines
f_fsrounds 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.
Quoted 17 lines · Changbin Du
Quoted 17 lines
There appears to be no kfifo support for
iov_iterthough, 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_fswas 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_fsrounds 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.
Quoted 5 lines
Quoted 3 lines
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.
Quoted 3 lines
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.
Signature · 3 lines
Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving»
Hi,
Quoted 18 lines · Michal Nazarewicz
Quoted 17 lines · Changbin Du
Quoted 17 lines
There appears to be no kfifo support for
iov_iterthough, 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_fswas 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_fsrounds 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.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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.
Quoted 9 lines
Quoted 3 lines
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?
Quoted 4 lines
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.
Signature · 1 line
balbi
Quoted 4 lines · Felipe Balbi
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_sizeis true, then we avoidmemcpy()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.
Signature · 3 lines
Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving»
Quoted 10 lines · Felipe Balbi
Quoted 4 lines
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_sizeis true, then we avoidmemcpy()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.
Quoted 7 lines
f_fs’s code is becoming, I’m now leaning to letting to
f_fsdo the same
thing: if user space makes an unaligned read,f_fsaligns the buffer and then drops excess data.Any arguments for
f_fsto 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
Quoted 1 line
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?
Quoted 4 lines
--
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
Best Regards,
Du, Changbin
Quoted 6 lines · Changbin Du
Quoted 6 lines · Felipe Balbi
Quoted 4 lines
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_sizeis true, then we avoidmemcpy()at the UDC driver level.
Quoted 7 lines
Quoted 3 lines
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_fsdriver.
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.
Quoted 7 lines
Quoted 7 lines
f_fs’s code is becoming, I’m now leaning to letting to
f_fsdo the same
thing: if user space makes an unaligned read,f_fsaligns the buffer and then drops excess data.Any arguments for
f_fsto not drop the data apply to UDC, so they should behave identically.
Quoted 8 lines
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?
Quoted 3 lines
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).
Signature · 3 lines
Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving»
Hi,
Quoted 5 lines
Quoted 5 lines
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.
Quoted 7 lines
Quoted 3 lines
So, now we have 3 choices:
1) buffer the excess data
2) fail the transferYou 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.
Quoted 10 lines
Quoted 3 lines
3) drop the excess data, then print an warning message
Which one do you prefer?
I think
f_fsshould 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.
Quoted 4 lines
--
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
Best Regards,
Du, Changbin
Quoted 15 lines
Quoted 2 lines
Quoted 2 lines
thanks Alan Stern and Michal.
Here just have a comment - the buffered data need be dropped whenthe
Quoted 11 lines
Quoted 1 line
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.
Quoted 5 lines
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.
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?
Signature · 3 lines
Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving»
Quoted 2 lines · Michal Nazarewicz
So I’ve been looking at AIO handling in
f_fsand 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:).
Quoted 3 lines · Michal Nazarewicz
Quoted 2 lines
So I’ve been looking at AIO handling in
f_fsand either I’m stupid or the code is broken.
Quoted 2 lines · Lars-Peter Clausen
The code was broken. Fixed in commit
332a5b446b791("usb: gadget:f_fs: Fix EFAULT generation for async read operations:).
Ah, now I remember, thanks!
Signature · 3 lines
Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving»
Hi Michal,
Quoted 18 lines · Michal Nazarewicz
So I’ve been looking at AIO handling in
f_fsand either I’m stupid or the code is broken. Here’s part offfs_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_iterinvocation be:if (copy_to_iter(io_data->buf, ret, &io_data->data)) ret = -EFAULT;
Quoted 7 lines
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,
Signature · 3 lines
Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics