From: Linus Torvalds <torvalds@linux-foundation.org> To: "Horia Geantă" <horia.geanta@nxp.com>, "Christoph Hellwig" <hch@lst.de>, "Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com> Cc: Dominique MARTINET <dominique.martinet@atmark-techno.com>, Jianxiong Gao <jxgao@google.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Lukas Hartmann <lukas@mntmn.com>, Aymen Sghaier <aymen.sghaier@nxp.com>, Herbert Xu <herbert@gondor.apana.org.au>, "David S. Miller" <davem@davemloft.net>, "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>, "iommu@lists.linux-foundation.org" <iommu@lists.linux-foundation.org> Subject: Re: swiotlb/caamjr regression (Was: [GIT PULL] (swiotlb) stable/for-linus-5.12) Date: Thu, 10 Jun 2021 12:41:26 -0700 [thread overview] Message-ID: <CAHk-=wiKAdWpSav4+qYT4_LDSQm=7pO8RqKEoQoJsyDVtTCk3Q@mail.gmail.com> (raw) In-Reply-To: <2e899de2-4b69-c4b6-33a6-09fb8949d2fd@nxp.com> [-- Attachment #1: Type: text/plain, Size: 1249 bytes --] On Thu, Jun 10, 2021 at 7:52 AM Horia Geantă <horia.geanta@nxp.com> wrote: > > Documentation/core-api/dma-api.rst explicitly allows for partial syncs: > Synchronise a single contiguous or scatter/gather mapping for the CPU > and device. With the sync_sg API, all the parameters must be the same > as those passed into the single mapping API. With the sync_single API, > you can use dma_handle and size parameters that aren't identical to > those passed into the single mapping API to do a partial sync. > > AFAICS commit 16fc3cef33a0 ("swiotlb: don't modify orig_addr in swiotlb_tbl_sync_single") > is breaking this functionality. How about a patch like the attached? Does that fix things for you. Christoph? Comments - that commit removed the offset calculation entirely, because the old (unsigned long)tlb_addr & (IO_TLB_SIZE - 1) was wrong, but instead of removing it, I think it should have just fixed it to be (tlb_addr - mem->start) & (IO_TLB_SIZE - 1); instead. That way the slot offset always matches the slot index calculation. I also made it then take the offset into account for the alloc_size checks. Does this UNTESTED patch perhaps do the right thing? Linus [-- Attachment #2: patch.diff --] [-- Type: text/x-patch, Size: 1130 bytes --] kernel/dma/swiotlb.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index 8ca7d505d61c..f63d15e94d35 100644 --- a/kernel/dma/swiotlb.c +++ b/kernel/dma/swiotlb.c @@ -342,6 +342,7 @@ static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t size { struct io_tlb_mem *mem = io_tlb_default_mem; int index = (tlb_addr - mem->start) >> IO_TLB_SHIFT; + unsigned long offset = (tlb_addr - mem->start) & (IO_TLB_SIZE - 1); phys_addr_t orig_addr = mem->slots[index].orig_addr; size_t alloc_size = mem->slots[index].alloc_size; unsigned long pfn = PFN_DOWN(orig_addr); @@ -350,6 +351,14 @@ static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t size if (orig_addr == INVALID_PHYS_ADDR) return; + if (offset > alloc_size) { + dev_WARN_ONCE(dev, 1, + "Buffer overflow detected. Offset: %lu. Mapping size: %zu.\n", + offset, size); + return; + } + alloc_size -= offset; + orig_addr += offset; if (size > alloc_size) { dev_WARN_ONCE(dev, 1, "Buffer overflow detected. Allocation size: %zu. Mapping size: %zu.\n",
WARNING: multiple messages have this Message-ID (diff)
From: Linus Torvalds <torvalds@linux-foundation.org> To: "Horia Geantă" <horia.geanta@nxp.com>, "Christoph Hellwig" <hch@lst.de>, "Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com> Cc: Dominique MARTINET <dominique.martinet@atmark-techno.com>, Aymen Sghaier <aymen.sghaier@nxp.com>, Herbert Xu <herbert@gondor.apana.org.au>, Lukas Hartmann <lukas@mntmn.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "iommu@lists.linux-foundation.org" <iommu@lists.linux-foundation.org>, "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>, "David S. Miller" <davem@davemloft.net>, Jianxiong Gao <jxgao@google.com> Subject: Re: swiotlb/caamjr regression (Was: [GIT PULL] (swiotlb) stable/for-linus-5.12) Date: Thu, 10 Jun 2021 12:41:26 -0700 [thread overview] Message-ID: <CAHk-=wiKAdWpSav4+qYT4_LDSQm=7pO8RqKEoQoJsyDVtTCk3Q@mail.gmail.com> (raw) In-Reply-To: <2e899de2-4b69-c4b6-33a6-09fb8949d2fd@nxp.com> [-- Attachment #1: Type: text/plain, Size: 1249 bytes --] On Thu, Jun 10, 2021 at 7:52 AM Horia Geantă <horia.geanta@nxp.com> wrote: > > Documentation/core-api/dma-api.rst explicitly allows for partial syncs: > Synchronise a single contiguous or scatter/gather mapping for the CPU > and device. With the sync_sg API, all the parameters must be the same > as those passed into the single mapping API. With the sync_single API, > you can use dma_handle and size parameters that aren't identical to > those passed into the single mapping API to do a partial sync. > > AFAICS commit 16fc3cef33a0 ("swiotlb: don't modify orig_addr in swiotlb_tbl_sync_single") > is breaking this functionality. How about a patch like the attached? Does that fix things for you. Christoph? Comments - that commit removed the offset calculation entirely, because the old (unsigned long)tlb_addr & (IO_TLB_SIZE - 1) was wrong, but instead of removing it, I think it should have just fixed it to be (tlb_addr - mem->start) & (IO_TLB_SIZE - 1); instead. That way the slot offset always matches the slot index calculation. I also made it then take the offset into account for the alloc_size checks. Does this UNTESTED patch perhaps do the right thing? Linus [-- Attachment #2: patch.diff --] [-- Type: text/x-patch, Size: 1130 bytes --] kernel/dma/swiotlb.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index 8ca7d505d61c..f63d15e94d35 100644 --- a/kernel/dma/swiotlb.c +++ b/kernel/dma/swiotlb.c @@ -342,6 +342,7 @@ static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t size { struct io_tlb_mem *mem = io_tlb_default_mem; int index = (tlb_addr - mem->start) >> IO_TLB_SHIFT; + unsigned long offset = (tlb_addr - mem->start) & (IO_TLB_SIZE - 1); phys_addr_t orig_addr = mem->slots[index].orig_addr; size_t alloc_size = mem->slots[index].alloc_size; unsigned long pfn = PFN_DOWN(orig_addr); @@ -350,6 +351,14 @@ static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t size if (orig_addr == INVALID_PHYS_ADDR) return; + if (offset > alloc_size) { + dev_WARN_ONCE(dev, 1, + "Buffer overflow detected. Offset: %lu. Mapping size: %zu.\n", + offset, size); + return; + } + alloc_size -= offset; + orig_addr += offset; if (size > alloc_size) { dev_WARN_ONCE(dev, 1, "Buffer overflow detected. Allocation size: %zu. Mapping size: %zu.\n", [-- Attachment #3: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2021-06-10 19:42 UTC|newest] Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-26 16:00 [GIT PULL] (swiotlb) stable/for-linus-5.12 Konrad Rzeszutek Wilk 2021-02-26 22:24 ` pr-tracker-bot 2021-06-08 2:35 ` swiotlb/caamjr regression (Was: [GIT PULL] (swiotlb) stable/for-linus-5.12) Dominique MARTINET 2021-06-10 14:52 ` Horia Geantă 2021-06-10 14:52 ` Horia Geantă 2021-06-10 19:41 ` Linus Torvalds [this message] 2021-06-10 19:41 ` Linus Torvalds 2021-06-10 23:20 ` Horia Geantă 2021-06-10 23:20 ` Horia Geantă 2021-06-11 6:21 ` Christoph Hellwig 2021-06-11 6:21 ` Christoph Hellwig 2021-06-11 10:34 ` Konrad Rzeszutek Wilk 2021-06-11 10:34 ` Konrad Rzeszutek Wilk 2021-06-11 10:59 ` Horia Geantă 2021-06-11 10:59 ` Horia Geantă 2021-06-11 16:21 ` Linus Torvalds 2021-06-11 16:21 ` Linus Torvalds 2021-06-16 20:49 ` Jianxiong Gao 2021-06-16 20:49 ` Jianxiong Gao via iommu 2021-06-17 0:27 ` Konrad Rzeszutek Wilk 2021-06-17 0:27 ` Konrad Rzeszutek Wilk 2021-06-17 0:39 ` Dominique MARTINET 2021-06-17 0:39 ` Dominique MARTINET 2021-06-17 5:12 ` Christoph Hellwig 2021-06-17 5:12 ` Christoph Hellwig 2021-06-17 5:36 ` Dominique MARTINET 2021-06-17 5:36 ` Dominique MARTINET 2021-06-18 18:01 ` Jianxiong Gao via iommu 2021-06-18 18:01 ` Jianxiong Gao 2021-06-21 2:03 ` Dominique MARTINET 2021-06-21 2:03 ` Dominique MARTINET 2021-06-21 2:55 ` Chanho Park 2021-06-21 2:55 ` Chanho Park 2021-06-21 4:14 ` 'Dominique MARTINET' 2021-06-21 4:14 ` 'Dominique MARTINET' 2021-06-21 13:16 ` Konrad Rzeszutek Wilk 2021-06-21 13:16 ` Konrad Rzeszutek Wilk 2021-06-22 7:48 ` 'Dominique MARTINET' 2021-06-22 7:48 ` 'Dominique MARTINET' 2021-06-22 21:58 ` Konrad Rzeszutek Wilk 2021-06-22 21:58 ` Konrad Rzeszutek Wilk 2021-06-22 23:04 ` 'Dominique MARTINET' 2021-06-22 23:04 ` 'Dominique MARTINET' 2021-06-17 11:33 ` Christoph Hellwig 2021-06-17 11:33 ` Christoph Hellwig 2021-06-11 16:01 ` Linus Torvalds 2021-06-11 16:01 ` Linus Torvalds
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='CAHk-=wiKAdWpSav4+qYT4_LDSQm=7pO8RqKEoQoJsyDVtTCk3Q@mail.gmail.com' \ --to=torvalds@linux-foundation.org \ --cc=aymen.sghaier@nxp.com \ --cc=davem@davemloft.net \ --cc=dominique.martinet@atmark-techno.com \ --cc=hch@lst.de \ --cc=herbert@gondor.apana.org.au \ --cc=horia.geanta@nxp.com \ --cc=iommu@lists.linux-foundation.org \ --cc=jxgao@google.com \ --cc=konrad.wilk@oracle.com \ --cc=linux-crypto@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=lukas@mntmn.com \ /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.