From: Johannes Berg <johannes@sipsolutions.net> To: Linus Torvalds <torvalds@linux-foundation.org>, Maxime Bizon <mbizon@freebox.fr> Cc: "Toke Høiland-Jørgensen" <toke@toke.dk>, "Robin Murphy" <robin.murphy@arm.com>, "Christoph Hellwig" <hch@lst.de>, "Oleksandr Natalenko" <oleksandr@natalenko.name>, "Halil Pasic" <pasic@linux.ibm.com>, "Marek Szyprowski" <m.szyprowski@samsung.com>, "Kalle Valo" <kvalo@kernel.org>, "David S. Miller" <davem@davemloft.net>, "Jakub Kicinski" <kuba@kernel.org>, "Paolo Abeni" <pabeni@redhat.com>, "Olha Cherevyk" <olha.cherevyk@gmail.com>, iommu <iommu@lists.linux-foundation.org>, linux-wireless <linux-wireless@vger.kernel.org>, Netdev <netdev@vger.kernel.org>, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, stable <stable@vger.kernel.org> Subject: Re: [REGRESSION] Recent swiotlb DMA_FROM_DEVICE fixes break ath9k-based AP Date: Fri, 25 Mar 2022 21:37:35 +0100 [thread overview] Message-ID: <298f4f9ccad7c3308d3a1fd8b4b4740571305204.camel@sipsolutions.net> (raw) In-Reply-To: <CAHk-=wippum+MksdY7ixMfa3i1sZ+nxYPWLLpVMNyXCgmiHbBQ@mail.gmail.com> So I've been watching this from the sidelines mostly, and discussing a bit with Toke, but: On Fri, 2022-03-25 at 11:30 -0700, Linus Torvalds wrote: > > (2) The CPU now wants to see any state written by the device since > the last sync > > This is "dma_sync_single_for_cpu(DMA_FROM_DEVICE)". > > A bounce-buffer implementation needs to copy *from* the bounce buffer. > > A cache-coherent implementation needs to do nothing. > > A non-coherent implementation maybe needs to do nothing (ie it > assumes that previous ops have flushed the cache, and just accessing > the data will bring the rigth thing back into it). Or it could just > flush the cache. Doesn't that just need to *invalidate* the cache, rather than *flush* it? The cache is somewhat similar to the bounce buffer, and here you're copying _from_ the bounce buffer (which is where the device is accessing), so shouldn't it be the same for the cache, i.e. you invalidate it so you read again from the real memory? > (3) The CPU has seen the state, but wants to leave it to the device > > This is "dma_sync_single_for_device(DMA_FROM_DEVICE)". > > A bounce buffer implementation needs to NOT DO ANYTHING (this is > the current ath9k bug - copying to the bounce buffer is wrong) > > A cache coherent implementation needs to do nothing > > A non-coherent implementation needs to flush the cache again, bot > not necessarily do a writeback-flush if there is some cheaper form > (assuming it does nothing in the "CPU now wants to see any state" case > because it depends on the data not having been in the caches) And similarly here, it would seem that the implementation can't _flush_ the cache as the device might be writing concurrently (which it does in fact do in the ath9k case), but it must invalidate the cache? I'm not sure about the (2) case, but here it seems fairly clear cut that if you have a cache, don't expect the CPU to write to the buffer (as evidenced by DMA_FROM_DEVICE), you wouldn't want to write out the cache to DRAM? I'll also note independently that ath9k actually maps the buffers as DMA_BIDIRECTIONAL, but the flush operations happen with DMA_FROM_DEVICE, at least after the setup is done. I must admit that I was scratching my head about this, I had sort of expected one should be passing the same DMA direction to all different APIs for the same buffer, but clearly, as we can see in your list of cases here, that's _not_ true. 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? > (4) There is a fourth case: dma_sync_single_for_cpu(DMA_TO_DEVICE) > which maybe should generate a warning because it seems to make no > sense? I can't think of a case where this would be an issue - the data > is specifically for the device, but it's synced "for the CPU"? I'd tend to agree with that, that's fairly much useless, since if only the CPU wrote to it, then you wouldn't care about any caching or bounce buffers, so no need to sync back. > In other words, I think commit aa6f8dcbab47 ("swiotlb: rework 'fix > info leak with DMA_FROM_DEVICE'") is fundamentally wrong. It doesn't > just break ath9k, it fundamentally break that "case 3" above. It's > doing a DMA_TO_DEVICE copy, even though it was a DMA_FROM_DEVICE sync. > > So I really think that "revert aa6f8dcbab47" is not only inevitable > because of practical worries about what it breaks, but because that > commit was just entirely and utterly WRONG. Honestly, I was scratching my head about this too - sadly it just says "what was agreed", without a pointer to how that was derived, but it seemed that the original issue was: "we're leaking old bounce buffer data to the device" or was it not? In which case doing any copies during map should've been sufficient, since then later no more data leaks could occur? johannes
WARNING: multiple messages have this Message-ID (diff)
From: Johannes Berg <johannes@sipsolutions.net> To: Linus Torvalds <torvalds@linux-foundation.org>, Maxime Bizon <mbizon@freebox.fr> Cc: "Toke Høiland-Jørgensen" <toke@toke.dk>, Netdev <netdev@vger.kernel.org>, "Kalle Valo" <kvalo@kernel.org>, linux-wireless <linux-wireless@vger.kernel.org>, "Oleksandr Natalenko" <oleksandr@natalenko.name>, stable <stable@vger.kernel.org>, "David S. Miller" <davem@davemloft.net>, "Halil Pasic" <pasic@linux.ibm.com>, iommu <iommu@lists.linux-foundation.org>, "Olha Cherevyk" <olha.cherevyk@gmail.com>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Jakub Kicinski" <kuba@kernel.org>, "Paolo Abeni" <pabeni@redhat.com>, "Robin Murphy" <robin.murphy@arm.com>, "Christoph Hellwig" <hch@lst.de>, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org> Subject: Re: [REGRESSION] Recent swiotlb DMA_FROM_DEVICE fixes break ath9k-based AP Date: Fri, 25 Mar 2022 21:37:35 +0100 [thread overview] Message-ID: <298f4f9ccad7c3308d3a1fd8b4b4740571305204.camel@sipsolutions.net> (raw) In-Reply-To: <CAHk-=wippum+MksdY7ixMfa3i1sZ+nxYPWLLpVMNyXCgmiHbBQ@mail.gmail.com> So I've been watching this from the sidelines mostly, and discussing a bit with Toke, but: On Fri, 2022-03-25 at 11:30 -0700, Linus Torvalds wrote: > > (2) The CPU now wants to see any state written by the device since > the last sync > > This is "dma_sync_single_for_cpu(DMA_FROM_DEVICE)". > > A bounce-buffer implementation needs to copy *from* the bounce buffer. > > A cache-coherent implementation needs to do nothing. > > A non-coherent implementation maybe needs to do nothing (ie it > assumes that previous ops have flushed the cache, and just accessing > the data will bring the rigth thing back into it). Or it could just > flush the cache. Doesn't that just need to *invalidate* the cache, rather than *flush* it? The cache is somewhat similar to the bounce buffer, and here you're copying _from_ the bounce buffer (which is where the device is accessing), so shouldn't it be the same for the cache, i.e. you invalidate it so you read again from the real memory? > (3) The CPU has seen the state, but wants to leave it to the device > > This is "dma_sync_single_for_device(DMA_FROM_DEVICE)". > > A bounce buffer implementation needs to NOT DO ANYTHING (this is > the current ath9k bug - copying to the bounce buffer is wrong) > > A cache coherent implementation needs to do nothing > > A non-coherent implementation needs to flush the cache again, bot > not necessarily do a writeback-flush if there is some cheaper form > (assuming it does nothing in the "CPU now wants to see any state" case > because it depends on the data not having been in the caches) And similarly here, it would seem that the implementation can't _flush_ the cache as the device might be writing concurrently (which it does in fact do in the ath9k case), but it must invalidate the cache? I'm not sure about the (2) case, but here it seems fairly clear cut that if you have a cache, don't expect the CPU to write to the buffer (as evidenced by DMA_FROM_DEVICE), you wouldn't want to write out the cache to DRAM? I'll also note independently that ath9k actually maps the buffers as DMA_BIDIRECTIONAL, but the flush operations happen with DMA_FROM_DEVICE, at least after the setup is done. I must admit that I was scratching my head about this, I had sort of expected one should be passing the same DMA direction to all different APIs for the same buffer, but clearly, as we can see in your list of cases here, that's _not_ true. 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? > (4) There is a fourth case: dma_sync_single_for_cpu(DMA_TO_DEVICE) > which maybe should generate a warning because it seems to make no > sense? I can't think of a case where this would be an issue - the data > is specifically for the device, but it's synced "for the CPU"? I'd tend to agree with that, that's fairly much useless, since if only the CPU wrote to it, then you wouldn't care about any caching or bounce buffers, so no need to sync back. > In other words, I think commit aa6f8dcbab47 ("swiotlb: rework 'fix > info leak with DMA_FROM_DEVICE'") is fundamentally wrong. It doesn't > just break ath9k, it fundamentally break that "case 3" above. It's > doing a DMA_TO_DEVICE copy, even though it was a DMA_FROM_DEVICE sync. > > So I really think that "revert aa6f8dcbab47" is not only inevitable > because of practical worries about what it breaks, but because that > commit was just entirely and utterly WRONG. Honestly, I was scratching my head about this too - sadly it just says "what was agreed", without a pointer to how that was derived, but it seemed that the original issue was: "we're leaking old bounce buffer data to the device" or was it not? In which case doing any copies during map should've been sufficient, since then later no more data leaks could occur? johannes _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2022-03-25 20:38 UTC|newest] Thread overview: 139+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-23 7:19 [REGRESSION] Recent swiotlb DMA_FROM_DEVICE fixes break ath9k-based AP Oleksandr Natalenko 2022-03-23 7:19 ` Oleksandr Natalenko via iommu 2022-03-23 7:28 ` Kalle Valo 2022-03-23 7:28 ` Kalle Valo 2022-03-23 17:27 ` Linus Torvalds 2022-03-23 17:27 ` Linus Torvalds 2022-03-23 19:06 ` Robin Murphy 2022-03-23 19:06 ` Robin Murphy 2022-03-23 19:16 ` Linus Torvalds 2022-03-23 19:16 ` Linus Torvalds 2022-03-23 20:54 ` Robin Murphy 2022-03-23 20:54 ` Robin Murphy 2022-03-24 5:57 ` Christoph Hellwig 2022-03-24 5:57 ` Christoph Hellwig 2022-03-24 10:25 ` Oleksandr Natalenko 2022-03-24 10:25 ` Oleksandr Natalenko via iommu 2022-03-24 11:05 ` Robin Murphy 2022-03-24 11:05 ` Robin Murphy 2022-03-24 14:27 ` Toke Høiland-Jørgensen 2022-03-24 14:27 ` Toke Høiland-Jørgensen via iommu 2022-03-24 16:29 ` Maxime Bizon 2022-03-24 16:29 ` Maxime Bizon 2022-03-24 16:31 ` Christoph Hellwig 2022-03-24 16:31 ` Christoph Hellwig 2022-03-24 16:52 ` Robin Murphy 2022-03-24 16:52 ` Robin Murphy 2022-03-24 17:07 ` Toke Høiland-Jørgensen 2022-03-24 17:07 ` Toke Høiland-Jørgensen via iommu 2022-03-24 19:26 ` Linus Torvalds 2022-03-24 19:26 ` Linus Torvalds 2022-03-24 21:14 ` Toke Høiland-Jørgensen 2022-03-24 21:14 ` Toke Høiland-Jørgensen via iommu 2022-03-25 10:25 ` Maxime Bizon 2022-03-25 10:25 ` Maxime Bizon 2022-03-25 11:27 ` Robin Murphy 2022-03-25 11:27 ` Robin Murphy 2022-03-25 23:38 ` Halil Pasic 2022-03-25 23:38 ` Halil Pasic 2022-03-26 16:05 ` Toke Høiland-Jørgensen 2022-03-26 16:05 ` Toke Høiland-Jørgensen via iommu 2022-03-26 18:38 ` Linus Torvalds 2022-03-26 18:38 ` Linus Torvalds 2022-03-26 22:38 ` David Laight 2022-03-26 22:38 ` David Laight 2022-03-26 22:41 ` Linus Torvalds 2022-03-26 22:41 ` Linus Torvalds 2022-03-25 16:25 ` Toke Høiland-Jørgensen 2022-03-25 16:25 ` Toke Høiland-Jørgensen via iommu 2022-03-25 16:45 ` Robin Murphy 2022-03-25 16:45 ` Robin Murphy 2022-03-25 18:13 ` Toke Høiland-Jørgensen via iommu 2022-03-25 18:13 ` Toke Høiland-Jørgensen 2022-03-25 18:30 ` Linus Torvalds 2022-03-25 18:30 ` Linus Torvalds 2022-03-25 19:14 ` Robin Murphy 2022-03-25 19:14 ` Robin Murphy 2022-03-25 19:21 ` Linus Torvalds 2022-03-25 19:21 ` Linus Torvalds 2022-03-25 19:26 ` Oleksandr Natalenko via iommu 2022-03-25 19:26 ` Oleksandr Natalenko 2022-03-25 19:27 ` Linus Torvalds 2022-03-25 19:27 ` Linus Torvalds 2022-03-25 19:35 ` Oleksandr Natalenko via iommu 2022-03-25 19:35 ` Oleksandr Natalenko 2022-03-25 20:37 ` Johannes Berg [this message] 2022-03-25 20:37 ` Johannes Berg 2022-03-25 20:47 ` Linus Torvalds 2022-03-25 20:47 ` Linus Torvalds 2022-03-25 21:13 ` Johannes Berg 2022-03-25 21:13 ` Johannes Berg 2022-03-25 21:40 ` David Laight 2022-03-25 21:40 ` David Laight 2022-03-25 21:56 ` Linus Torvalds 2022-03-25 21:56 ` Linus Torvalds 2022-03-25 22:41 ` David Laight 2022-03-25 22:41 ` David Laight 2022-03-27 3:15 ` Halil Pasic 2022-03-27 3:15 ` Halil Pasic 2022-03-28 9:48 ` Johannes Berg 2022-03-28 9:48 ` Johannes Berg 2022-03-28 9:50 ` Johannes Berg 2022-03-28 9:50 ` Johannes Berg 2022-03-28 9:57 ` Johannes Berg 2022-03-28 9:57 ` Johannes Berg 2022-03-27 3:48 ` Halil Pasic 2022-03-27 3:48 ` Halil Pasic 2022-03-27 5:06 ` Linus Torvalds 2022-03-27 5:06 ` Linus Torvalds 2022-03-27 5:21 ` Linus Torvalds 2022-03-27 5:21 ` Linus Torvalds 2022-03-27 15:24 ` David Laight 2022-03-27 15:24 ` David Laight 2022-03-27 19:23 ` Linus Torvalds 2022-03-27 19:23 ` Linus Torvalds 2022-03-27 20:04 ` Linus Torvalds 2022-03-27 20:04 ` Linus Torvalds 2022-03-27 23:52 ` Halil Pasic 2022-03-27 23:52 ` Halil Pasic 2022-03-28 0:30 ` Linus Torvalds 2022-03-28 0:30 ` Linus Torvalds 2022-03-28 12:02 ` Halil Pasic 2022-03-28 12:02 ` Halil Pasic 2022-03-27 23:37 ` Halil Pasic 2022-03-27 23:37 ` Halil Pasic 2022-03-28 0:37 ` Linus Torvalds 2022-03-28 0:37 ` Linus Torvalds 2022-03-25 7:12 ` Oleksandr Natalenko 2022-03-25 7:12 ` Oleksandr Natalenko via iommu 2022-03-25 9:21 ` Thorsten Leemhuis 2022-03-25 9:21 ` Thorsten Leemhuis 2022-03-24 18:31 ` Halil Pasic 2022-03-24 18:31 ` Halil Pasic 2022-03-25 16:31 ` Christoph Hellwig 2022-03-25 16:31 ` Christoph Hellwig 2022-03-24 18:02 ` Halil Pasic 2022-03-24 18:02 ` Halil Pasic 2022-03-25 15:25 ` Halil Pasic 2022-03-25 15:25 ` Halil Pasic 2022-03-25 16:23 ` Robin Murphy 2022-03-25 16:23 ` Robin Murphy 2022-03-25 16:32 ` Christoph Hellwig 2022-03-25 16:32 ` Christoph Hellwig 2022-03-25 18:15 ` Toke Høiland-Jørgensen via iommu 2022-03-25 18:15 ` Toke Høiland-Jørgensen 2022-03-25 18:42 ` Robin Murphy 2022-03-25 18:42 ` Robin Murphy 2022-03-25 18:46 ` Linus Torvalds 2022-03-25 18:46 ` Linus Torvalds 2022-03-28 6:37 ` Christoph Hellwig 2022-03-28 6:37 ` Christoph Hellwig 2022-03-28 8:15 ` David Laight 2022-03-28 8:15 ` David Laight 2022-03-30 12:11 ` Halil Pasic 2022-03-30 12:11 ` Halil Pasic 2022-03-24 8:55 ` Oleksandr Natalenko 2022-03-24 8:55 ` Oleksandr Natalenko via iommu 2022-03-24 12:32 ` [REGRESSION] Recent swiotlb DMA_FROM_DEVICE fixes break ath9k-based AP #forregzbot Thorsten Leemhuis 2022-03-25 9:24 ` Thorsten Leemhuis 2022-03-27 9:00 ` Thorsten Leemhuis
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=298f4f9ccad7c3308d3a1fd8b4b4740571305204.camel@sipsolutions.net \ --to=johannes@sipsolutions.net \ --cc=davem@davemloft.net \ --cc=gregkh@linuxfoundation.org \ --cc=hch@lst.de \ --cc=iommu@lists.linux-foundation.org \ --cc=kuba@kernel.org \ --cc=kvalo@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-wireless@vger.kernel.org \ --cc=m.szyprowski@samsung.com \ --cc=mbizon@freebox.fr \ --cc=netdev@vger.kernel.org \ --cc=oleksandr@natalenko.name \ --cc=olha.cherevyk@gmail.com \ --cc=pabeni@redhat.com \ --cc=pasic@linux.ibm.com \ --cc=robin.murphy@arm.com \ --cc=stable@vger.kernel.org \ --cc=toke@toke.dk \ --cc=torvalds@linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.