Date:   Sat, 26 Mar 2022 11:38:59 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Toke Høiland-Jørgensen <toke@...e.dk>
Cc:     Halil Pasic <pasic@...ux.ibm.com>,
        Robin Murphy <robin.murphy@....com>,
        Maxime Bizon <mbizon@...ebox.fr>,
        Netdev <netdev@...r.kernel.org>, Kalle Valo <kvalo@...nel.org>,
        linux-wireless <linux-wireless@...r.kernel.org>,
        Oleksandr Natalenko <oleksandr@...alenko.name>,
        stable <stable@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        iommu <iommu@...ts.linux-foundation.org>,
        Olha Cherevyk <olha.cherevyk@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Christoph Hellwig <hch@....de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [REGRESSION] Recent swiotlb DMA_FROM_DEVICE fixes break
 ath9k-based AP

On Sat, Mar 26, 2022 at 9:06 AM Toke Høiland-Jørgensen <toke@...e.dk> wrote:
>
> I was also toying with the idea of having a copy-based peek helper like:
>
> u32 data = dma_peek_word(buf, offset)

I really don't think you can or want to have a word-based one.

That said, I like the *name* of that thing.

I think a lot of confusion comes from the very subtle naming of
fundamentally having a lot of odd conditions with

 - two different "directions of the sync" - ie who it is that cares:

       dma_sync_single_for_{cpu,device}

 - three different "direction of the data" - ie who it is that writes the data:

        DMA_FROM_DEVICE / DMA_TO_DEVICE / DMA_BIDIRECTIONAL

so you have six possible combinations, three of which seem insane and
not useful, and of the three that are actually possible, some are very
unusual (it exactly that "device is the one writing, but we want to
sync the dma area for the device").

I do not think it helps that not only do we have this combinatorial
naming, we also use _different_ names. We say "for device" and "for
cpu", but then when we specify who does the writing, we don't say "cpu
vs device", we just specify the direction instead (FROM_DEVICE means
the device did the writing, TO_DEVICE means that the CPU did the
writing).

Anyway, I spent a lot of time looking at this, and I am now personally
convinced that commit aa6f8dcbab47 (swiotlb: rework "fix info leak
with DMA_FROM_DEVICE") was just completely buggy, and was buggy
exactly becasue it was fundamentally confused even about which
direction the bounce was happening.

I have reverted it in my tree, and I tried to write a comprehensive
summary about why it was wrong.

What I *didn't* do in that commit was to argue against the naming, and
try to enumerate all the different valid cases.

Because I think naming matters, and I think the current dma_sync()
interfaces are horribly confusing exactly due to those naming
combinatorials.

But I think "peek" is a good name, not because I think reading one
work is a valid thing (you want to often peek more than that), but
because it seems much more intuitive than
"dma_sync_for_cpu(DMA_FROM_DEVICE)".

Similarly, I would think that "flush" is a much better word for
"dma_sync_for_device(DMA_FROM_CPU)".

I don't know what a good word for
"dma_sync_for_device(DMA_FROM_DEVICE)" is, but maybe "forget" would
come closest - we want the CPU to "forget" what it peeked.

Anyway, I have reverted that commit, and I think it was wrong both in
spirit and in implementation, and I'll ask Greg to remove it from
stable.

And I think the security argument was entirely bogus, because the
whole security argument was based on an incorrect understanding of the
direction of the data.

But hey, I may currently be convinced that revert is the right thing
to do, BUT I've been wrong before, and I'll happily change my mind if
somebody makes a really cogent argument

               Linus

Powered by blists - more mailing lists