Re: usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
On 06/29/2018 08:32 AM, Felipe Balbi wrote: > > Hi, > > Alan Stern writes: >> On Thu, 21 Jun 2018, Roger Quadros wrote: >> > Can we avoid the spin_lock() and the work-queue and call usb_ep_dequeue() > directly from here? >> What is the purpose of the spin_lock()? I agree that the lock doesn't seem to be necessary. But I believe the whole thing is already running in non-sleeping context, even before the spinlock is taken. So this wouldn't help much. Even the io_cancel() syscall takes a spinlock before invoking the cancel function. So this issue is not exclusive to program termination. Are there any documented guidelines on which context usb_ep_dequeue() should be able to be called in? The sleep in the dwc3 driver seems to be a recent addition. >>> >>> drivers/usb/udc/gadget/core.c has the only documentation, but context is >>> not mentioned there. >>> Felipe, what do you suggest? >> >> As far as I remember, usb_ep_dequeue() is supposed to be more or less >> analogous to usb_ep_queue(); drivers should be allowed to call either >> routine in an atomic context. > > Hmm, that's what I remember, but we don't have that documented and dwc3 > has a sleep in its dequeue, which I need to remove for other reasons > anyway. > > Can we get a patch updating documentation to make it clear that both > queue and dequeue should be callable from any context? > Felipe, for the DWC3 do you see an issue with moving the giveback and the cleanup of the TRBs simply into END_TRANSFER interrupt? That seems to work for me for fixing the issue, but I don't have a full understanding of all the dependencies of the DWC3 gadget driver and don't know if it could introduce other issues.
Re: usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
On 09/25/2018 02:46 PM, Vincent Pelletier wrote: > Hello, > > On Thu, 2 Aug 2018 14:23:51 +, Vincent Pelletier > wrote: >> On Thu, 2 Aug 2018 00:45:14 +, "He, Bo" wrote: >>> Your patch fix the issue BUG: scheduling while atomic: >> >> Yes, although from my understanding of Felipe's answer, the actual bug >> is the "scheduling" part (sleeping in dwc3 UDC) rather than the >> "atomic" part. >> >> So my patch addresses, still if my understanding is correct, the wrong >> half of the problem, and even introduced the regression you identified. >> Hence my uncertainty... > > I notice that neither He's patch, nor a dwc3 change to prevent it from > scheduling inside usb_ep_dequeue are in Linus' master. Please correct > me if I missed something. > > Just in case my previous emails were not clear: > - I have no objection to He's patch on its own (and I do not know the > code nearly enough to provide a meaningful reviewed-by). > - I do not intend to work on making changes to dwc3 gadget to stop it > from scheduling in usb_ep_dequeue in the foreseeable future. > > So if there is no ongoing work on dwc3 cancel behaviour (Felipe ?), > please do resume/carry on with reviewing and integrating He's patch. > > It is only *if* dwc3 cancel stops scheduling that I believe my patch > should be reverted (here is the hash as of Linus' master): > commit d52e4d0c0c428bf2ba35074a7495cdb28e2efbae > Author: Vincent Pelletier > Date: Wed Jun 13 11:05:06 2018 + > > usb: gadget: ffs: Fix BUG when userland exits with submitted AIO > transfers > > This bug happens only when the UDC needs to sleep during usb_ep_dequeue, > as is the case for (at least) dwc3. > and, in my understanding, a consequence is that He's fix would not > be needed anymore - the bug my patch introduced disappearing in the > revert. Hi, I just started testing with your patch applied and the patch seems to contain a race condition on its own. When a aio_cancel() is called ffs_aio_cancel_worker() is queued. Now what might happen is that the URB completes on its own before the work item has a chance to run. And it seems that ffs_copy_worker() can even overtake the cancel work. In that case the data structure associated with the URB is deleted while the cancel_worker is still queued. I get the following output on the console [ 91.974151] ODEBUG: free active (active state 0) object type: work_struct hint: ffs_aio_cancel_worker+0x0/0x20 [ 92.211124] [] debug_print_object+0xac/0xc0 [ 92.216854] [] __debug_check_no_obj_freed+0x1b0/0x208 [ 92.223451] [] debug_check_no_obj_freed+0x1c/0x28 [ 92.229701] [] kfree+0x78/0xc8 [ 92.234302] [] ffs_user_copy_worker+0xd0/0x128 [ 92.240293] [] process_one_work+0x1e0/0x3c0 [ 92.246022] [] worker_thread+0x54/0x410 [ 92.251403] [] kthread+0x104/0x130 [ 92.256351] [] ret_from_fork+0x10/0x18 - Lars
Re: usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
On 06/21/2018 10:29 AM, Roger Quadros wrote:
[...]
>>> static int ffs_aio_cancel(struct kiocb *kiocb)
>>> {
>>> struct ffs_io_data *io_data = kiocb->private;
>>> - struct ffs_epfile *epfile = kiocb->ki_filp->private_data;
>>> + struct ffs_data *ffs = io_data->ffs;
>>> int value;
>>>
>>> ENTER();
>>>
>>> - spin_lock_irq(&epfile->ffs->eps_lock);
>>> -
>>> - if (likely(io_data && io_data->ep && io_data->req))
>>> - value = usb_ep_dequeue(io_data->ep, io_data->req);
>>> - else
>>> + if (likely(io_data && io_data->ep && io_data->req)) {
>>> + INIT_WORK(&io_data->cancellation_work,
>>> ffs_aio_cancel_worker);
>>> + queue_work(ffs->io_completion_wq,
>>> &io_data->cancellation_work);
>>> + value = -EINPROGRESS;
>>> + } else {
>>> value = -EINVAL;
>>> -
>>> - spin_unlock_irq(&epfile->ffs->eps_lock);
>>> + }
>
> Can we avoid the spin_lock() and the work-queue and call usb_ep_dequeue()
> directly from here?
>> What is the purpose of the spin_lock()?
I agree that the lock doesn't seem to be necessary. But I believe the whole
thing is already running in non-sleeping context, even before the spinlock
is taken. So this wouldn't help much.
Even the io_cancel() syscall takes a spinlock before invoking the cancel
function. So this issue is not exclusive to program termination.
Are there any documented guidelines on which context usb_ep_dequeue() should
be able to be called in? The sleep in the dwc3 driver seems to be a recent
addition.
>
>>>
>>> return value;
>>> }
>>> --
>>> 2.17.1
>>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] USB/PHY driver patches for 4.17-rc1
On 04/05/2018 08:31 AM, Kees Cook wrote: > On Wed, Apr 4, 2018 at 3:31 AM, Greg KH wrote: >> Lars-Peter Clausen (2): >> usb: gadget: ffs: Execute copy_to_user() with USER_DS set > > https://git.kernel.org/linus/4058ebf33cb0be88ca516f968eda24ab7b6b93e4 > > Isn't there a better way to do this without the set_fs() usage? We've > been try to eliminate it in the kernel. I thought there was a safer > way to use iters now? The problem is use_mm(). It needs to be accompanied with set_fs(DS_USER) to work reliably. This has simply been missing for this particular instance of use_mm(). Now, in my opinion, use_mm() is not the best approach here in the first place and instead of using copy_to_user() it is probably better to map the userspace pages to kernel space and then access them directly. But that's a lot more intrusive and separate from this issue. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: ffs: Let setup() return USB_GADGET_DELAYED_STATUS
On 01/12/2018 01:01 PM, Felipe Balbi wrote: This fixed random occasional failures that were previously observed on a DWC3 based system under high system load. >>> >>> I need to see tracepoint capture from the failure ;-) Care to send them >>> to me for analysis? >> >> I've attached the full trace up to the point where the failure occurs. (The >> tracing infrastructure in the DWC3 driver was very helpful in tracking this >> down btw.). > > yeah, that was the idea. I see, however, that you're using an older > kernel based on the your trace format. Just to make sure, did you try > with v4.14? I see the problem is pretty clear and we will take the > patch, but just to make sure, can you try v4.14? > I wish I could. The only SoC I have with DWC3 isn't fully supported in upstream yet and the vendor kernel is still at v4.9. I've tried it with other UDC drivers running v4.14 just to make sure there is no regression. Do you have a system with DWC3 that you can run in gadget mode? If you use the aio_simple.c from the function fs examples and put a sleep(1) (or something similar) in handle_ep0() after the display_event(&event) that should emulate the scheduling due to high system load and should be able to reproduce the issue when the patch is not applied. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: ffs: Let setup() return USB_GADGET_DELAYED_STATUS
On 01/12/2018 12:26 PM, Felipe Balbi wrote: > > Hi, > > Lars-Peter Clausen writes: >> Some UDC drivers (like the DWC3) expect that the response to a setup() > > not some, but *all*. You can only queue a response later IFF you return > USB_GADGET_DELAYED_STATUS. Yeah, but most drivers don't care. DWC3 is one of two which handles USB_GADGET_DELAYED_STATUS. > >> request is queued from within the setup function itself so that it is >> available as soon as setup() has completed. >> >> Upon receiving a setup request the function fs driver creates an event that >> is made available to userspace. And only once userspace has acknowledged >> that event the response to the setup request is queued. >> >> So it violates the requirement of those UDC drivers and random failures can >> be observed. This is basically a race condition and if userspace is able to >> read the event and queue the response fast enough all is good. But if it is >> not, for example because other processes are currently scheduled to run, >> the USB host that sent the setup request will observe an error. >> >> To avoid this the gadget framework provides the USB_GADGET_DELAYED_STATUS >> return code. If a setup() callback returns this value the UDC driver is >> aware that response is not yet available and can uses the appropriate >> methods to handle this case. >> >> Since in the case of function fs the response will never be available when >> the setup() function returns make sure that this status code is used. >> >> This fixed random occasional failures that were previously observed on a >> DWC3 based system under high system load. > > I need to see tracepoint capture from the failure ;-) Care to send them > to me for analysis? I've attached the full trace up to the point where the failure occurs. (The tracing infrastructure in the DWC3 driver was very helpful in tracking this down btw.). The important parts are around: 125.504109: dwc3_complete_trb: ep0out: ... 125.504111: dwc3_prepare_trb: ep0out: ... Then a few moments later 125.508529: dwc3_ep0: queueing request ffc8787b6d00 to ep0out ... Usually the queuing would happen before the dwc3_complete_trb and this function would remove the URB from the queue. But now it will just sit on the pending list waiting for something to happen since the original transfer is already over. When the next control request comes in and we try to queue something for EP0 the 'list_empty(&dep->pending_list)' check will fail, we return an error and the endpoint is stalled and the host will observe an error. - Lars # tracer: nop # # entries-in-buffer/entries-written: 3779/3779 #P:4 # # _-=> irqs-off # / _=> need-resched #| / _---=> hardirq/softirq #|| / _--=> preempt-depth #||| / delay # TASK-PID CPU# TIMESTAMP FUNCTION # | | | | | -0 [000] d.h. 125.495083: dwc3_readl: addr ff800948040c value 0008 -0 [000] d.h. 125.495090: dwc3_readl: addr ff8009480408 value 0100 -0 [000] d.h. 125.495093: dwc3_writel: addr ff8009480408 value 8100 irq/221-dwc3-3336 [003] d... 125.495159: dwc3_event: event (c040): ep0out: Transfer Complete irq/221-dwc3-3336 [003] d... 125.495165: dwc3_ep0: ep0out: Transfer Complete: state 'Setup Phase' irq/221-dwc3-3336 [003] d... 125.495167: dwc3_ep0: Setup Phase irq/221-dwc3-3336 [003] d... 125.495169: dwc3_ctrl_req: bRequestType 41 bRequest 00 wValue wIndex wLength 0 irq/221-dwc3-3336 [003] d... 125.499466: dwc3_writel: addr ff800948040c value 0004 irq/221-dwc3-3336 [003] d... 125.499468: dwc3_event: event (20c2): ep0in: Transfer Not Ready (Not Active) irq/221-dwc3-3336 [003] d... 125.499470: dwc3_ep0: ep0in: Transfer Not Ready (Not Active): state 'Setup Phase' irq/221-dwc3-3336 [003] d... 125.499472: dwc3_ep0: Control Status irq/221-dwc3-3336 [003] d... 125.499474: dwc3_prepare_trb: ep0in: 0/0 trb ff800944b000 buf 78079000 size 0 ctrl 0c33 (HLcs:SC:status2) irq/221-dwc3-3336 [003] d... 125.499477: dwc3_writel: addr ff8009480818 value irq/221-dwc3-3336 [003] d... 125.499480: dwc3_writel: addr ff8009480814 value 7807a000 irq/221-dwc3-3336 [003] d... 125.499482: dwc3_writel: addr ff8009480810 value irq/221-dwc3-3336 [003] d... 125.499484: dwc3_writel: addr ff800948081c value 0406 irq/221-dwc3-3336 [003] d... 125.499486: dwc3_readl: addr ff800948081c value 00010006
[PATCH] usb: gadget: ffs: Let setup() return USB_GADGET_DELAYED_STATUS
Some UDC drivers (like the DWC3) expect that the response to a setup() request is queued from within the setup function itself so that it is available as soon as setup() has completed. Upon receiving a setup request the function fs driver creates an event that is made available to userspace. And only once userspace has acknowledged that event the response to the setup request is queued. So it violates the requirement of those UDC drivers and random failures can be observed. This is basically a race condition and if userspace is able to read the event and queue the response fast enough all is good. But if it is not, for example because other processes are currently scheduled to run, the USB host that sent the setup request will observe an error. To avoid this the gadget framework provides the USB_GADGET_DELAYED_STATUS return code. If a setup() callback returns this value the UDC driver is aware that response is not yet available and can uses the appropriate methods to handle this case. Since in the case of function fs the response will never be available when the setup() function returns make sure that this status code is used. This fixed random occasional failures that were previously observed on a DWC3 based system under high system load. Signed-off-by: Lars-Peter Clausen --- drivers/usb/gadget/function/f_fs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c index 038a27a13ebc..4d1cef1d394b 100644 --- a/drivers/usb/gadget/function/f_fs.c +++ b/drivers/usb/gadget/function/f_fs.c @@ -3265,7 +3265,7 @@ static int ffs_func_setup(struct usb_function *f, __ffs_event_add(ffs, FUNCTIONFS_SETUP); spin_unlock_irqrestore(&ffs->ev.waitq.lock, flags); - return 0; + return USB_GADGET_DELAYED_STATUS; } static bool ffs_func_req_match(struct usb_function *f, -- 2.11.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: gadget: ffs: Execute copy_to_user() with USER_DS set
When using a AIO read() operation on the function FS gadget driver a URB is
submitted asynchronously and on URB completion the received data is copied
to the userspace buffer associated with the read operation.
This is done from a kernel worker thread invoking copy_to_user() (through
copy_to_iter()). And while the user space process memory is made available
to the kernel thread using use_mm(), some architecture require in addition
to this that the operation runs with USER_DS set. Otherwise the userspace
memory access will fail.
For example on ARM64 with Privileged Access Never (PAN) and User Access
Override (UAO) enabled the following crash occurs.
Internal error: Accessing user space memory with fs=KERNEL_DS: 964f
[#1] SMP
Modules linked in:
CPU: 2 PID: 1636 Comm: kworker/2:1 Not tainted
4.9.0-04081-g8ab2dfb-dirty #487
Hardware name: ZynqMP ZCU102 Rev1.0 (DT)
Workqueue: events ffs_user_copy_worker
task: ffc87afc8080 task.stack: ffc87a00c000
PC is at __arch_copy_to_user+0x190/0x220
LR is at copy_to_iter+0x78/0x3c8
[...]
[] __arch_copy_to_user+0x190/0x220
[] ffs_user_copy_worker+0x70/0x130
[] process_one_work+0x1dc/0x460
[] worker_thread+0x50/0x4b0
[] kthread+0xd8/0xf0
[] ret_from_fork+0x10/0x50
Address this by placing a set_fs(USER_DS) before of the copy operation
and revert it again once the copy operation has finished.
This patch is analogous to commit d7ffde35e31a ("vhost: use USER_DS in
vhost_worker thread") which addresses the same underlying issue.
Signed-off-by: Lars-Peter Clausen
---
drivers/usb/gadget/function/f_fs.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 038a27a13ebc..ae2b70bad111 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -758,9 +758,13 @@ static void ffs_user_copy_worker(struct work_struct *work)
bool kiocb_has_eventfd = io_data->kiocb->ki_flags & IOCB_EVENTFD;
if (io_data->read && ret > 0) {
+ mm_segment_t oldfs = get_fs();
+
+ set_fs(USER_DS);
use_mm(io_data->mm);
ret = ffs_copy_to_iter(io_data->buf, ret, &io_data->data);
unuse_mm(io_data->mm);
+ set_fs(oldfs);
}
io_data->kiocb->ki_complete(io_data->kiocb, ret, ret);
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: f_fs: report error if excess data received
On 05/16/2016 06:05 PM, Michal Nazarewicz wrote:
> So I’ve been looking at AIO handling in f_fs and either I’m stupid or
> the code is broken.
The code was broken. Fixed in commit 332a5b446b791 ("usb: gadget: f_fs: Fix
EFAULT generation for async read operations:).
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: f_fs: Fix kernel panic for SuperSpeed
On 04/22/2016 12:43 PM, Jim Lin wrote: > Android N adds os_desc_compat in v2_descriptor by init_functionfs() > (system/core/adb/usb_linux_client.cpp) to support automatic install > of MTP driver on Windows for USB device mode. > > Current __ffs_data_do_os_desc() of f_fs.c will check reserved1 field > and return -EINVAL. > This results in a second adb_write of usb_linux_client.cpp > (system/core/adb/) which doesn't have ss_descriptors filled. > Then later kernel_panic (composite.c) occurs when ss_descriptors > as a pointer with NULL is being accessed. > > Fix is to ignore the checking on reserved1 field so that first > adb_write goes successfully with v2_descriptor which has > ss_descriptors filled. That sounds like the wrong approach. The kernel should not crash if ss_descriptors is not filled. I think the right fix is to make sure that the NULL pointer deref can never happen regardless of which input is supplied by userspace. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] usb: gadget: f_fs: Fix use-after-free when completing AIO request
When using asynchronous read or write operations on the USB endpoints the
issuer of the IO request is notified by calling the ki_complete() callback
of the submitted kiocb when the URB has been completed.
Calling this ki_complete() callback will free kiocb. Make sure that the
structure is no longer accessed beyond that point, otherwise undefined
behaviour might occur.
Fixes: 2e4c7553cd6f ("usb: gadget: f_fs: add aio support") (for v3.19 and
earlier drop the eventfd part)
Signed-off-by: Lars-Peter Clausen
---
Changes since v1:
* More verbose commit message
---
drivers/usb/gadget/function/f_fs.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 2c314c1..73515d5 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -646,6 +646,7 @@ static void ffs_user_copy_worker(struct work_struct *work)
work);
int ret = io_data->req->status ? io_data->req->status :
io_data->req->actual;
+ bool kiocb_has_eventfd = io_data->kiocb->ki_flags & IOCB_EVENTFD;
if (io_data->read && ret > 0) {
use_mm(io_data->mm);
@@ -657,13 +658,11 @@ static void ffs_user_copy_worker(struct work_struct *work)
io_data->kiocb->ki_complete(io_data->kiocb, ret, ret);
- if (io_data->ffs->ffs_eventfd &&
- !(io_data->kiocb->ki_flags & IOCB_EVENTFD))
+ if (io_data->ffs->ffs_eventfd && !kiocb_has_eventfd)
eventfd_signal(io_data->ffs->ffs_eventfd, 1);
usb_ep_free_request(io_data->ep, io_data->req);
- io_data->kiocb->private = NULL;
if (io_data->read)
kfree(io_data->to_free);
kfree(io_data->buf);
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: f_fs: Fix use-after-free
On 04/19/2016 05:34 PM, Michal Nazarewicz wrote:
> On Thu, Apr 14 2016, Lars-Peter Clausen wrote:
>> Calling the ki_complete() callback will free the underlying data structure.
>> Make sure that it is no longer accessed beyond that point, otherwise
>> undefined behaviour might occur.
>>
>> Fixes: 2e4c7553cd6f ("usb: gadget: f_fs: add aio support")
[...]
> This probably also deserves
>
> Cc: # 3.14+
The Fixes tag makes sure it is picked up by the appropriate stable trees.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: f_fs: Fix use-after-free
On 04/19/2016 05:29 PM, Michal Nazarewicz wrote: > On Thu, Apr 14 2016, Lars-Peter Clausen wrote: >> Calling the ki_complete() callback will free the underlying data structure. >> Make sure that it is no longer accessed beyond that point, otherwise >> undefined behaviour might occur. > > To be honest I have trouble tracking what ki_complete is. Could you > describe the path that leads to the bug in a little more detail? It's the completion function for AIO requests. So any AIO read or write request on a EP file descriptor will trigger this. I can try to rewrite the commit message if you want to. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: gadget: f_fs: Fix use-after-free
Calling the ki_complete() callback will free the underlying data structure.
Make sure that it is no longer accessed beyond that point, otherwise
undefined behaviour might occur.
Fixes: 2e4c7553cd6f ("usb: gadget: f_fs: add aio support")
Signed-off-by: Lars-Peter Clausen
---
drivers/usb/gadget/function/f_fs.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index e21ca2bd..15b648c 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -646,6 +646,7 @@ static void ffs_user_copy_worker(struct work_struct *work)
work);
int ret = io_data->req->status ? io_data->req->status :
io_data->req->actual;
+ bool kiocb_has_eventfd = io_data->kiocb->ki_flags & IOCB_EVENTFD;
if (io_data->read && ret > 0) {
use_mm(io_data->mm);
@@ -657,13 +658,11 @@ static void ffs_user_copy_worker(struct work_struct *work)
io_data->kiocb->ki_complete(io_data->kiocb, ret, ret);
- if (io_data->ffs->ffs_eventfd &&
- !(io_data->kiocb->ki_flags & IOCB_EVENTFD))
+ if (io_data->ffs->ffs_eventfd && !kiocb_has_eventfd)
eventfd_signal(io_data->ffs->ffs_eventfd, 1);
usb_ep_free_request(io_data->ep, io_data->req);
- io_data->kiocb->private = NULL;
if (io_data->read)
kfree(io_data->to_free);
kfree(io_data->buf);
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: gadget: f_fs: Fix EFAULT generation for async read operations
In the current implementation functionfs generates a EFAULT for async read
operations if the read buffer size is larger than the URB data size. Since
a application does not necessarily know how much data the host side is
going to send it typically supplies a buffer larger than the actual data,
which will then result in a EFAULT error.
This behaviour was introduced while refactoring the code to use iov_iter
interface in commit c993c39b8639 ("gadget/function/f_fs.c: use put iov_iter
into io_data"). The original code took the minimum over the URB size and
the user buffer size and then attempted to copy that many bytes using
copy_to_user(). If copy_to_user() could not copy all data a EFAULT error
was generated. Restore the original behaviour by only generating a EFAULT
error when the number of bytes copied is not the size of the URB and the
target buffer has not been fully filled.
Commit 342f39a6c8d3 ("usb: gadget: f_fs: fix check in read operation")
already fixed the same problem for the synchronous read path.
Fixes: c993c39b8639 ("gadget/function/f_fs.c: use put iov_iter into io_data")
Signed-off-by: Lars-Peter Clausen
---
Changes since v1:
* copy_to_iter() can fail, make sure that is handled. Test this case by
mapping part of the target buffer read-only.
Signed-off-by: Lars-Peter Clausen
---
drivers/usb/gadget/function/f_fs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 8cfce10..3b27aa0 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -650,7 +650,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))
+ if (ret != io_data->req->actual &&
iov_iter_count(&io_data->data))
ret = -EFAULT;
unuse_mm(io_data->mm);
}
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: f_fs: Fix incorrect EFAULT generation for async read operations
On 03/28/2016 04:28 PM, Al Viro wrote:
> On Mon, Mar 28, 2016 at 02:42:43PM +0200, Lars-Peter Clausen wrote:
>> In the current implementation functionfs generates a EFAULT for async read
>> operations if the read buffer size is larger than the URB data size. Since
>> a application does not necessarily know how much data the host side is
>> going to send it typically supplies a buffer larger than the actual data,
>> which will then result in a EFAULT error.
>>
>> This behaviour was introduced while refactoring the code to use iov_iter
>> interface in commit c993c39b8639 ("gadget/function/f_fs.c: use put iov_iter
>> into io_data"). The original code took the minimum over the URB size and
>> the user buffer size and then attempt to copy that many bytes using
>> copy_to_user(). If copy_to_user() returned less copied bytes the code would
>> generate a EFAULT error. This has exactly the same behaviour as using
>> copy_to_iter() except that copy_to_iter() can't fail since the check
>> whether access to the user supplied buffer is OK is already done earlier
>> and copy_to_iter() wont be called in that case. Restore the original
>> behaviour by simply dropping the code that generates the EFAULT error.
>
> "The check whether access to user supplied buffer is OK" is *not* done
> before. access_ok() checked does not guarantee that copy_to_user() won't
> fail.
>
> access_ok() and its equivalents checks only one thing - that you are not
> trying to pass kernel addresses for userland ones. It does not check
> permissions you have for individual pages, it does not check if they are
> present, or, if absent, that page fault will bring them in.
>
> Both copy_to_user() and copy_to_iter() can bloody well fail halfway through
> without access_ok() having any objections.
Hm, right. I guess I got confused on the copy_to_iter() case since it
can't fail when copying into kernel buffers. It would have be nice if
the semantics of all those iov iter functions had been documented.
> E.g. do read() into an mmapped buffer, with one page somewhere in the
> middle munmapped. Or the first page munmapped, for that matter...
Yeah, I actually wondered how that could possibly work...
>
> I see where the commit in question is buggy, but your patch doesn't really
> fix it; original behaviour is not restored.
>
> - ret = min_t(size_t, ret,
> io_data->len);
> -
> - if
> (unlikely(copy_to_user(io_data->buf,
> - data, ret)))
> + ret = copy_to_iter(data, ret,
> &io_data->data);
> + if
> (unlikely(iov_iter_count(&io_data->data)))
> ret = -EFAULT;
>
> was wrong; what it should've done to preserve the original behaviour is
> something like
> copied = copy_to_iter(data, ret, &io_data->data)
> if (unlikely(copied != ret))
> ret = iov_iter_count(&io_data->data)
> ? EFAULT : copied;
>
> After copy_to_iter(data, len, target) we could have three outcomes:
> * everything copied; return value is equal to len
> * copying stopped at the end of iterator; return value is less than len,
> iov_iter_count(target) is zero.
> * copying stopped at unmapped page/page that was readonly/etc;
> return value is less than len, iov_iter_count(target) is *not* zero.
Yes. It's a bit of a shame that the copy_{to,from}_iter() functions
don't allow you to easily distinguish between target buffer is to small
and a fault occurred mid way through. This can't be the only place which
wants to know this.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: gadget: f_fs: Fix incorrect EFAULT generation for async read operations
In the current implementation functionfs generates a EFAULT for async read
operations if the read buffer size is larger than the URB data size. Since
a application does not necessarily know how much data the host side is
going to send it typically supplies a buffer larger than the actual data,
which will then result in a EFAULT error.
This behaviour was introduced while refactoring the code to use iov_iter
interface in commit c993c39b8639 ("gadget/function/f_fs.c: use put iov_iter
into io_data"). The original code took the minimum over the URB size and
the user buffer size and then attempt to copy that many bytes using
copy_to_user(). If copy_to_user() returned less copied bytes the code would
generate a EFAULT error. This has exactly the same behaviour as using
copy_to_iter() except that copy_to_iter() can't fail since the check
whether access to the user supplied buffer is OK is already done earlier
and copy_to_iter() wont be called in that case. Restore the original
behaviour by simply dropping the code that generates the EFAULT error.
Commit 342f39a6c8d3 ("usb: gadget: f_fs: fix check in read operation")
already fixed the same problem for the synchronous read path.
Fixes: c993c39b8639 ("gadget/function/f_fs.c: use put iov_iter into io_data")
Signed-off-by: Lars-Peter Clausen
---
drivers/usb/gadget/function/f_fs.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 8cfce10..e219379 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -650,8 +650,6 @@ 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;
unuse_mm(io_data->mm);
}
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] uwb: Remove umc bus legacy suspend/resume support
On 04/03/2015 03:44 PM, Greg Kroah-Hartman wrote: On Sun, Mar 15, 2015 at 06:03:00PM +0100, Lars-Peter Clausen wrote: There are currently no umc drivers implementing suspend/resume, so remove the legacy suspend/resume support from the framework. If a umc driver ever wants to implement suspend/resume they can use dev_pm_ops, which works out of the box without any additional support necessary from the bus itself. Signed-off-by: Lars-Peter Clausen --- drivers/uwb/umc-bus.c | 34 -- include/linux/uwb/umc.h | 2 -- 2 files changed, 36 deletions(-) This patch is already in my tree :( Yea, you applied it two weeks ago :) -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] uwb: Remove umc bus legacy suspend/resume support
There are currently no umc drivers implementing suspend/resume, so remove
the legacy suspend/resume support from the framework. If a umc driver ever
wants to implement suspend/resume they can use dev_pm_ops, which works out
of the box without any additional support necessary from the bus itself.
Signed-off-by: Lars-Peter Clausen
---
drivers/uwb/umc-bus.c | 34 --
include/linux/uwb/umc.h | 2 --
2 files changed, 36 deletions(-)
diff --git a/drivers/uwb/umc-bus.c b/drivers/uwb/umc-bus.c
index 88a290f..c857140 100644
--- a/drivers/uwb/umc-bus.c
+++ b/drivers/uwb/umc-bus.c
@@ -163,38 +163,6 @@ static int umc_device_remove(struct device *dev)
return 0;
}
-static int umc_device_suspend(struct device *dev, pm_message_t state)
-{
- struct umc_dev *umc;
- struct umc_driver *umc_driver;
- int err = 0;
-
- umc = to_umc_dev(dev);
-
- if (dev->driver) {
- umc_driver = to_umc_driver(dev->driver);
- if (umc_driver->suspend)
- err = umc_driver->suspend(umc, state);
- }
- return err;
-}
-
-static int umc_device_resume(struct device *dev)
-{
- struct umc_dev *umc;
- struct umc_driver *umc_driver;
- int err = 0;
-
- umc = to_umc_dev(dev);
-
- if (dev->driver) {
- umc_driver = to_umc_driver(dev->driver);
- if (umc_driver->resume)
- err = umc_driver->resume(umc);
- }
- return err;
-}
-
static ssize_t capability_id_show(struct device *dev, struct device_attribute
*attr, char *buf)
{
struct umc_dev *umc = to_umc_dev(dev);
@@ -223,8 +191,6 @@ struct bus_type umc_bus_type = {
.match = umc_bus_match,
.probe = umc_device_probe,
.remove = umc_device_remove,
- .suspend= umc_device_suspend,
- .resume = umc_device_resume,
.dev_groups = umc_dev_groups,
};
EXPORT_SYMBOL_GPL(umc_bus_type);
diff --git a/include/linux/uwb/umc.h b/include/linux/uwb/umc.h
index ba82f03..0211229 100644
--- a/include/linux/uwb/umc.h
+++ b/include/linux/uwb/umc.h
@@ -87,8 +87,6 @@ struct umc_driver {
int (*probe)(struct umc_dev *);
void (*remove)(struct umc_dev *);
- int (*suspend)(struct umc_dev *, pm_message_t state);
- int (*resume)(struct umc_dev *);
int (*pre_reset)(struct umc_dev *);
int (*post_reset)(struct umc_dev *);
--
1.8.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH_V4 1/3] dt: usb: jz4740: Add DT binding document for OHCI
On 02/03/2015 11:53 AM, Zubair Lutfullah Kakakhel wrote: On 03/02/15 10:32, Lars-Peter Clausen wrote: On 02/03/2015 11:17 AM, Zubair Lutfullah Kakakhel wrote: [...] V4 Changes Removed clock binding because of pending work in clock tree. Will add binding later. Rather than introduce a bad binding now and change later. But this patch is introducing a bad binding. The part needs the clock to work. The binding you are specifying right now says that it works fine without any clocks. Facing a chicken and egg problem with these patches here.. When the new clock driver gets in we can add the correct clock binding and replace devm_clk_get(&pdev->dev, "uhc"); with devm_clk_get(&pdev->dev, NULL); Specifying the current binding got NAKed by Arnd who mentioned that clock names shouldn't be needed as required properties. And even if needed, then it shouldn't be "uhc" and more close to what other ohci drivers have. It's not a chicken and egg problem. The order is very clear, first convert the clock driver to the CCF, then update the driver to pass NULL instead of "uhc", then add the devicetree bindings. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH_V4 1/3] dt: usb: jz4740: Add DT binding document for OHCI
On 02/03/2015 11:17 AM, Zubair Lutfullah Kakakhel wrote:
[...]
V4 Changes
Removed clock binding because of pending work in clock tree. Will add
binding later. Rather than introduce a bad binding now and change later.
But this patch is introducing a bad binding. The part needs the clock to
work. The binding you are specifying right now says that it works fine
without any clocks.
> [...]
+Example for jz4740:
+
+/ {
+ ohci: jz4780-ohci@0x134a {
s/jz4780/jz4740
+ compatible = "ingenic,jz4740-ohci";
+ reg = <0x134a 0x1>;
+
+ interrupts = <5>;
+ };
+};
+
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/8] usb: musb: Change end point selection to use new IO access
On 11/25/2014 12:52 AM, Tony Lindgren wrote: * Apelete Seketeli [141124 15:40]: Hi Tony, Thanks for the patch. On Mon, Nov-24-2014 at 11:05:03 AM -0800, Tony Lindgren wrote: This allows the endpoints to work when multiple MUSB glue layers are built in. Applied on top of 3.18-rc6 mainline and tested successfully on JZ4740. Been able to use ethernet-over-usb to access the internet on device. No issue as far as I'm concerned. Great that's good to hear and thanks for testing. Doing the DMA patches here right now.. For the DMA, I've set up JZ4740 to use the MUSB_DMA_INVENTRA option by default, is that OK or do you have some other DMA hardware on it? If MUSB_DMA_INVENTRA does not work, and you don't have other DMA hardware on it, we can pass MUSB_DMA_INVENTRA and leave the DMA function pointers empty, and then the driver will bail out during init unless the option for CONFIG_MUSB_PIO_ONLY is set. Yea... so according to the datasheet there is no DMA support, or at least it is not documented in the datasheet's description of the USB core. There is a vendor driver for the core which has ifdefs to enable DMA which looks like MUSB_DMA_INVENTRA, but I think we never really go that to work too well. So the current configuration is to use only PIO. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] USB: ohci-jz4740: Miscellaneous fixes
On 04/14/2014 12:08 PM, Laurent Pinchart wrote: Hello, I've noticed two issues with teh ohci-jz4740 driver while reading the source code. Here are two patches to fix them. Both are untested as I don't have access to the hardware. I unfortunately don't have the hardware handy for testing at the moment, but the patches look good. Acked-by: Lars-Peter Clausen Thanks, - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] usb: musb: add support for JZ4740 usb device controller
On 01/04/2014 12:11 PM, Richard Weinberger wrote: Am Samstag, 4. Januar 2014, 12:06:22 schrieb Apelete Seketeli: On 04-Jan-14, Richard Weinberger wrote: On Thu, Dec 19, 2013 at 9:42 PM, Apelete Seketeli wrote: Add support for Ingenic JZ4740 USB Device Controller through a specific musb glue layer. JZ4740 UDC not being OTG compatible and missing some hardware registers, this musb glue layer is written from scratch to be used in gadget mode only and take silicon design specifics into account. Signed-off-by: Apelete Seketeli Signed-off-by: Lars-Peter Clausen --- drivers/usb/musb/Kconfig |8 +- drivers/usb/musb/Makefile |1 + drivers/usb/musb/jz4740.c | 201 + 3 files changed, 209 insertions(+), 1 deletion(-) create mode 100644 drivers/usb/musb/jz4740.c diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig index 57dfc0c..14d7e72 100644 --- a/drivers/usb/musb/Kconfig +++ b/drivers/usb/musb/Kconfig @@ -93,6 +93,12 @@ config USB_MUSB_BLACKFIN config USB_MUSB_UX500 tristate "Ux500 platforms" +config USB_MUSB_JZ4740 + tristate "JZ4740" + depends on MACH_JZ4740 || COMPILE_TEST + depends on USB_MUSB_GADGET + depends on USB_OTG_BLACKLIST_HUB + endchoice config USB_MUSB_AM335X_CHILD @@ -100,7 +106,7 @@ config USB_MUSB_AM335X_CHILD choice prompt 'MUSB DMA mode' - default MUSB_PIO_ONLY if ARCH_MULTIPLATFORM + default MUSB_PIO_ONLY if ARCH_MULTIPLATFORM || USB_MUSB_JZ4740 Just out of curiosity, why can't we use DMA? We wrote the musb glue layer for the Ben Nanonote handheld computer only to provide ethernet-over-usb functionality. I was thinking about adding DMA support later, but Lars-Peter Clausen did experiment with DMA, and found out that all ethernet packets were unaligned, while the DMA only works if they are aligned. As a consequence, the DMA was bypassed all the time, making it useless at least for the ethernet gadget we are interested in. That's why DMA didn't make it into the glue layer. DMA support would be nice to have. Especially to make the USB storage gadget work well with the Ben NanoNote. It is not clear at this point how well the DMA works. While there seems support for DMA, there is no documentation about it in the datasheet, which might indicate that it is broken. So we first need to make sure that it works reliable before we can enable it. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/16] dmaengine: add transfered member to dma_async_tx_descriptor
On 07/25/2013 05:12 PM, Sebastian Andrzej Siewior wrote: > On 07/25/2013 04:57 PM, Lars-Peter Clausen wrote: >> I'm not sure that this will work. Once you've submitted the descriptor it is >> owned by the dmaengine driver and you are not allowed to dereference it >> anymore since the dmaengine driver might free the memory at any time. You >> should only reference the descriptor by the cookie returned by submit(). > > I see. But it can't be reused before calling the callback if it is > going to call the callback, right? > So if this is a no-no, I'm left with an additional argument to the > complete callback? Hm, maybe using dmaengine_tx_status() and checking the residue field of the state struct. transferred is basically len - residue. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/16] dmaengine: add transfered member to dma_async_tx_descriptor
On 07/22/2013 08:10 PM, Sebastian Andrzej Siewior wrote: > In USB RX path it is possible that the we receive less bytes than > requested. Take the following example: > The driver for USB-to-UART submits an URB with 256 bytes in size and the > dmaengine driver in turn programs a transfer of 256 bytes. The transfer > is programmed and the dma engine waits for the data to arrive. Once data > is sent on the UART the dma engine begins to move data. If there was > only one data byte in the USB packet received then the DMA engine will > only move one byte due to USB restrictions / rules. The real size of the > transfer has to be notified to the user / caller so he set this to the > caller. > This patch adds the transfered member to the dma_async_tx_descriptor > where the caller can obtain the final size. I'm not sure that this will work. Once you've submitted the descriptor it is owned by the dmaengine driver and you are not allowed to dereference it anymore since the dmaengine driver might free the memory at any time. You should only reference the descriptor by the cookie returned by submit(). - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
