linux-kernel - Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem rework

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5fa97254-3038-6fe7-a7e7-a267262049c6@nxp.com>
Date:   Fri, 17 May 2019 10:52:54 +0000
From:   Laurentiu Tudor <laurentiu.tudor@....com>
To:     Fredrik Noring <noring@...rew.org>
CC:     Robin Murphy <robin.murphy@....com>, "hch@....de" <hch@....de>,
        "stern@...land.harvard.edu" <stern@...land.harvard.edu>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "marex@...x.de" <marex@...x.de>,
        "JuergenUrban@....de" <JuergenUrban@....de>,
        Leo Li <leoyang.li@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem
 rework

Hi Fredrik,

On 16.05.2019 18:15, Fredrik Noring wrote:
> Hi Laurentiu,
> 
>> I took your code, added the missing mapping and placed it in a patch.
>> Please see attached (compile tested only).
> 
> Thanks! Unfortunately, the OHCI fails with errors such as
> 
> 	usb 1-1: device descriptor read/64, error -12

Alright, at least the crash went away. :-)

> that I tracked down to the calls
> 
> 	   hub_port_init
> 	-> usb_control_msg
> 	-> usb_internal_control_msg
> 	-> usb_start_wait_urb
> 	-> usb_submit_urb
> 	-> usb_hcd_submit_urb
> 	-> hcd->driver->urb_enqueue
> 	-> ohci_urb_enqueue
> 	-> ed_get
> 	-> ed_alloc
> 	-> dma_pool_zalloc
> 	-> dma_pool_alloc
> 	-> pool_alloc_page,
> 
> which returns NULL. Then I noticed
> 
> 	/* pool_alloc_page() might sleep, so temporarily drop &pool->lock */
> that might be a problem considering that the HCD handles pool memory in
> IRQ handlers, for instance:
> 
> 	   do_IRQ
> 	-> generic_handle_irq
> 	-> handle_level_irq
> 	-> handle_irq_event
> 	-> handle_irq_event_percpu
> 	-> __handle_irq_event_percpu
> 	-> usb_hcd_irq
> 	-> ohci_irq
> 	-> ohci_work
> 	-> finish_urb
> 	-> __usb_hcd_giveback_urb
> 	-> usb_hcd_unmap_urb_for_dma
> 	-> hcd_buffer_free


That looks indeed worrisome but I'd expect that it's not related to 
these genalloc changes that I'm working on. I'll try to look into it but 
as I'm not familiar with the area maybe others will comment.

> Also, DMA_BUFFER_SIZE in ohci-ps2.c is only 256 KiB in total. Is the new
> pool implementation at least as efficient as the previous one?

I don't see any obvious reasons for genalloc to be less efficient.

One thing I noticed between genalloc and the original implementation is 
that previously the device memory was mapped with memremap() while with 
genalloc I'm doing a devm_ioremap(). I can't think of a relevant 
difference except that memremap() maps with WC buth maybe there are 
other arch specific subtleties. I've attached a new version of the 
ohci-ps2 patch replacing the devm_ioremap() with devm_memremap() to 
better match the original implementation. Please test if you find some time.

---
Thanks & Best Regards, Laurentiu

View attachment "v2-0001-usb-host-ohci-ps2-init-genalloc-for-local-memory.patch" of type "text/x-patch" (3447 bytes)