linux-kernel - Re: USB: free coherent buffer into atomic context

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] [day] [month] [year] [list]

Date:	Wed, 22 Sep 2010 10:23:19 +0800
From:	Ming Lei <tom.leiming@...il.com>
To:	Wolfgang Mües <wolfgang@...w-muees.de>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: USB: free coherent buffer into atomic context

2010/9/20 Wolfgang Mües <wolfgang@...w-muees.de>:
> Hi Ming,
>
> Am Samstag, 18. September 2010 10:50:20 schrieb Ming Lei:
>> IMO, it is not necessary for a coherent buffer to be kept for
>> the whole time a driver operates on a device.
>
> From DMA-mapping.txt:
>>There are two types of DMA mappings:
>>
>>- Consistent DMA mappings which are usually mapped at driver
>>  initialization, unmapped at the end and for which the hardware should
>>  guarantee that the device and the CPU can access the data
>>  in parallel and will see updates made by each other without any
>>  explicit software flushing.
>
>>- Streaming DMA mappings which are usually mapped for one DMA transfer,
>>  unmapped right after it (unless you use pci_dma_sync_* below) and for which
>>  hardware can optimize for sequential accesses.
>
> You wrote:
>> Also, coherent buffer
>> is very limited in some arch(such as ARM), maybe we should use it
>> more dynamically, instead of keeping it for long time.
>
> No, this is against the design (see above).
>
> If you want to be dynamic, please use streaming DMA mapping.

The dynamic here is that allocate and free dynamically, not map and unmap
dynamically. As I said, some arch may have very limited coherent buffer
space, so allocate and free coherent memory for one driver does make sense.

>
> If a DMA buffer is needed often and you want to avoid the overhead of streaming
> dma mapping, use a coherent buffer.

This is nothing to do with the topic.

>
>> irqs_disabled() doesn't mean the buffer is freed in an interrupt context.
>
> I am very sure that the writer of this code has used irqs_disabled() as an
> indication of interrupt context. Maybe in_interrupt() will be more precise.

You mean WARN_ON(irqs_disables) is wrong?  Anyway, irqs_disabled is very
different with in_interrupt, isn't it?

You still not reply the root cause of WARN_ON(irqs_disables) is added
into dma_free_coherent. I want to know there is any catastrophic or
side effect if dma_free_coherent is called with irq disabled, seems
you just said it is against the initial design.

>
>> If it is only a misuse, seems WARN_ON can be removed.
>
> How on earth will you detect the misuse if there is no visible indication at
> runtime?

In fact, some usb host controller only can DMA data using its internal sram,
which is generally declared as coherent memory using
dma_declare_coherent_memory. When submiting urb to usbcore, the
internal coherent buffer is allocated as bounce buffer to do dma transfer
, and after the transfer is over the bounce buffer will be deallocated using
dma_free_coherent, which can cause the warning since urb->complete always
run in irq context.

So I want to know the root cause of the warning.  If there is very little side
effect to call dma_free_coherent in irq context, we can keep the current usb
implementation in this case and just remove the WARN_ON from
dma_free_coherent.

thanks,
-- 
Lei Ming
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/