On Sat, Mar 26, 2022 at 9:06 AM Toke Høiland-Jørgensen toke@toke.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