linux-kernel - Re: [REGRESSION] Recent swiotlb DMA_FROM

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: <20220327051502.63fde20a.pasic@linux.ibm.com>
Date:   Sun, 27 Mar 2022 05:15:02 +0200
From:   Halil Pasic <pasic@...ux.ibm.com>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Maxime Bizon <mbizon@...ebox.fr>,
        Toke Høiland-Jørgensen <toke@...e.dk>,
        Robin Murphy <robin.murphy@....com>,
        Christoph Hellwig <hch@....de>,
        Oleksandr Natalenko <oleksandr@...alenko.name>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Kalle Valo <kvalo@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Olha Cherevyk <olha.cherevyk@...il.com>,
        iommu <iommu@...ts.linux-foundation.org>,
        linux-wireless <linux-wireless@...r.kernel.org>,
        Netdev <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        stable <stable@...r.kernel.org>,
        Halil Pasic <pasic@...ux.ibm.com>
Subject: Re: [REGRESSION] Recent swiotlb DMA_FROM_DEVICE fixes break
 ath9k-based AP

On Fri, 25 Mar 2022 22:13:08 +0100
Johannes Berg <johannes@...solutions.net> wrote:

> > > Then, however, we need to define what happens if you pass
> > > DMA_BIDIRECTIONAL to the sync_for_cpu() and sync_for_device() functions,
> > > which adds two more cases? Or maybe we eventually just think that's not
> > > valid at all, since you have to specify how you're (currently?) using
> > > the buffer, which can't be DMA_BIDIRECTIONAL?  
> > 
> > Ugh. Do we actually have cases that do it?  
> 
> Yes, a few.
> 
> > That sounds really odd for
> > a "sync" operation. It sounds very reasonable for _allocating_ DMA,
> > but for syncing I'm left scratching my head what the semantics would
> > be.  
> 
> I agree.
> 
> > But yes, if we do and people come up with semantics for it, those
> > semantics should be clearly documented.  
> 
> I'm not sure? I'm wondering if this isn't just because - like me
> initially - people misunderstood the direction argument, or didn't
> understand it well enough, and then just passed the same value as for
> the map()/unmap()?

I don't think you misunderstood the direction argument and its usage. I
didn't finish thinking about the proposal of Linus, I'm pretty confident
about my understanding of the current semantics of the direction.
According to the documentation, you do have to pass in the very same
direction, that was specified when the mapping was created. A qoute
from the documentation.

"""
        void
        dma_sync_single_for_cpu(struct device *dev, dma_addr_t dma_handle,
                                size_t size,
                                enum dma_data_direction direction)

        void
        dma_sync_single_for_device(struct device *dev, dma_addr_t dma_handle,
                                   size_t size,
                                   enum dma_data_direction direction)

        void
        dma_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,
                            int nents,
                            enum dma_data_direction direction)

        void
        dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
                               int nents,
                               enum dma_data_direction direction)

Synchronise a single contiguous or scatter/gather mapping for the CPU
and device. With the sync_sg API, all the parameters must be the same
as those passed into the single mapping API. With the sync_single API,
you can use dma_handle and size parameters that aren't identical to
those passed into the single mapping API to do a partial sync.
"""
(Documentation/core-api/dma-api.rst)

The key here is "sync_sg API, all the parameters must be the same
as those passed into the single mapping API", but I have to admit,
I don't understand the *single* in here. The intended meaning of the
last sentence is that one can do partial sync by choose 
dma_hande_sync, size_sync such that dma_handle_mapping <= dma_handle_sync
< dma_handle_mapping + size_mapping and dma_handle_sync + size_sync <=
dma_handle_mapping + size_mapping. But the direction has to remain the
same.


BTW, the current documented definition of the direction is about the
data transfer direction between memory and the device, and how the CPU
is interacting with the memory is not in scope. A quote form the
documentation.

"""
======================= =============================================
DMA_NONE                no direction (used for debugging)
DMA_TO_DEVICE           data is going from the memory to the device
DMA_FROM_DEVICE         data is coming from the device to the memory
DMA_BIDIRECTIONAL       direction isn't known
======================= =============================================
"""
(Documentation/core-api/dma-api.rst)

My feeling is, that re-defining the dma direction is not a good idea. But
I don't think my opinion has much weight here.

@Christoph, Robin: What do you think?

Regards,
Halil