Re: [REGRESSION] Recent swiotlb DMA_FROM_DEVICE fixes break ath9k-based AP

[Halil Pasic <pasic@linux.ibm.com>]

On Sat, 26 Mar 2022 22:06:15 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, Mar 26, 2022 at 8:49 PM Halil Pasic <pasic@linux.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?

I pix "fix the dam model". This email of mine was supposed to discuss how
the model can be fixed:
https://www.spinics.net/lists/linux-wireless/msg222442.html

> 
> 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.

I completely agree. Never intended to pick the la-la-land one.

> 
> > 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.

Sorry, I'm not very knowledgeable when it comes all the different
hardware out there.

> 
> 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?

No it is not. I have to admit, I did see some value in talking about the
model that is described by the current documentation for two reasons:
1) Not everybody has great knowledge of all the hardware out there, and
there might be people other than me, who based their work on that broken
model. And thus wrote code that is correct within the broken model,
but not correct within the fixed mode. Does not see to be the case here,
but I was not able to tell.
2) To fix the model, we have to understand both reality and the model.
Or we have to replace it with an entirely new one.

> 
> 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?

Sure. It was never my intention to base my argument on false assumptions.

> 
> >  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.
> 

I agree, and that is is the very reason I said, I'm not against the
partial revert (see
https://www.spinics.net/lists/linux-wireless/msg222326.html)

Hey, I don't even know if there is a single usage of what I described.
In fact I asked the community is this even something legit. What I know,
is that the current (broken) model does allow the scenario.

> 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.
> 

Not sync with the device, but with the memory. And it is supposed to
happen after the DMA has completed, and not while the DMA is ongoing.

But I'm clearly not knowledgeable enough to have this discussion. I'm
afraid if I continue, I will just drag the community down.

> 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.

Not make visible to the device but make actually it RAM instead of 
remaining in cache. My most basic mental model is:

+----------------+
|                |        +---------+         +--------+
|     +--------+ |        |         |   DMA   |        |
| CPU | CACHE  | |  <---> |   RAM   |  <----> | 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.
> 

It is certainly an option. The tricky part is that one is supposed to use
DMA_TO_DEVICE even if the device does not read RAM but only writes it,
and thus the direction of the data flow of the direct memory access (DMA)
is from the device to the memory (RAM).

> 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.
> 

It is an option.

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

I have tired to in
https://www.spinics.net/lists/linux-wireless/msg222442.html

> 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.

Yes.
> 
> 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?
> 

Yes.

AFAIU the main reason we have should postulate the ath9k code is correct,
is that it would be prohibitively expensive not to do so. Because we
can comparatively easily change ath9k, but we can't be sure about other
uses of sync_for_device unless we audit all of them. That does make
perfect sense for me.

IMHO that also means that the whole "ownership" concept is beyond saving,
and that we should rewrite the corresponding parts of the documentation.
Thus my effort at saving it was misguided.

Give how this unfolded, I intend to let the more knowledgeable people
hash out the new model, and avoiding dragging down the community by
being too vocal about my opinion.

For the record, I believe that the partial revert proposed here
https://www.spinics.net/lists/linux-wireless/msg222300.html
would have been a wiser choice, than a complete revert of commit
aa6f8dcbab47 ("swiotlb: rework "fix info leak with DMA_FROM_DEVICE"").
The reason why is in the commit message, and was also pointed out by
Robin.

Regards,
Halil