Message-ID: <CAHk-=whUJ=tMEgP3KiWwk0pzmHn+1QORUu50syE+zOGk4UnFog@mail.gmail.com>
Date:   Sat, 26 Mar 2022 22:06:15 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Halil Pasic <pasic@...ux.ibm.com>
Cc:     Toke Høiland-Jørgensen <toke@...e.dk>,
        Robin Murphy <robin.murphy@....com>,
        Christoph Hellwig <hch@....de>,
        Maxime Bizon <mbizon@...ebox.fr>,
        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>
Subject: Re: [REGRESSION] Recent swiotlb DMA_FROM_DEVICE fixes break
 ath9k-based AP

On Sat, Mar 26, 2022 at 8:49 PM Halil Pasic <pasic@...ux.ibm.com> wrote:
>
> I agree it CPU modified buffers *concurrently* with DMA can never work,
> and I believe the ownership model was conceived to prevent this
> situation.

But that just means that the "ownership" model is garbage, and cannot
handle this REAL LIFE situation.

Here's the deal: if somebody makes a model that is counter-factual,
you have exactly two choices:

 - fix the damn model

 - live in a la-la-land that isn't reality

Which choice do you pick?

And I'll be brutally honest: if you pick the la-la-land one, I don't
think we can really discuss this any more.

> But a CPU can modify the buffer *after* DMA has written to
> it, while the mapping is still alive.

Yes.

But you are making ALL your arguments based on that "ownership" model
that we now know is garbage.

If you make your arguments based on garbage, the end result _might_
work just by happenstance, but the arguments sure aren't very
convincing, are they?

So let's make it really clear that the arguments must not be based on
some "ownership" model that you just admitted cannot handle the very
real case of real and common hardware.

Ok?

>  For example one could do one
> partial read from the device, *after* the DMA is done,
> sync_for_cpu(DMA_FROM_DEVICE), examine, then zero out the entire buffer,
> sync_for_device(DMA_FROM_DEVICE)

So the result you want to get to I can believe in, but your sequence
of getting there is untenable, since it depends on breaking other
cases that are more important than your utterly broken hardware that
you don't even know how much data it filled.

And I fundamentally also happen to think that since the CPU just wrote
that buffer, and *that* write is what you want to sync with the
device, then that DMA_FROM_DEVICE was just pure fantasy to begin with.

So that code sequence you quote is wrong. You are literally trying to
re-introduce the semantics that we already know broke the ath9k
driver.

Instead, let me give you a couple of alternative scenarios.

Alternative 1:

 - ok, so the CPU wrote zeroes to the area, and we want to tell the
DMA mapping code that it needs to make that CPU write visible to the
device.

 - Ahh, you mean "sync_for_device(DMA_TO_DEVICE)"?

 - Yes.

   The "for_device()" tells us that afterwards, the device can access
the memory (we synced it for the device).

   And the DMA_TO_DEVICE tells us that we're transferring the zeroes
we wrote on the CPU to the device bounce buffer.

   Now the device got those zeroes, and it can overwrite them
(partially), and everything is fine

 - And then we need to use "sync_for_cpu(DMA_FROM_DEVICE)" when we
want to see the result once it's all done?

 - Yes.

 - Splendid. Except I don't like how you mix DMA_TO_DEVICE and
DMA_FROM_DEVICE and you made the dma_alloc() not use DMA_BIDIRECTIONAL

 - Yeah, that's ugly, but it matches reality, and it would "just work" today.

Alternative 2:

 - Ok, so the CPU doesn't even want to write to the area AT ALL, but
we know we have a broken device that might not fill all of the bounce
buffer, and we don't want to see old stale bounce buffer contents that
could come from some other operation entirely and leak sensitive data
that wasn't for us.

 - Ahh, so you really want to just clear the bounce buffer before IO?

 - Yes. So let's introduce a "clear_bounce_buffer()" operation before
starting DMA. Let's call it "sync_for_device(DMA_NONE)" and teach the
non-bounce-buffer dmas mapping entities to just ignore it, because
they don't have a bounce buffer that can contain stale data.

 - Sounds good.

Alternative 3:

 - Ok, you have me stumped. I can't come up with something else sane.

Anyway, notice what's common about these alternatives? They are based
not on some "we have a broken model", but on trying to solve the
actual real-life problem case.

I'm more than happy to hear other alternatives.

But the alternative I am _not_ willing to entertain is "Yeah, we have
a model of ownership, and that can't handle ath9k because that one
wants to do CPU reads while DMA is possibly active, so ath9k is
broken".

Can we please agree on that?

                          Linus

Powered by blists - more mailing lists