From: Dominique MARTINET <dominique.martinet@atmark-techno.com> To: Jianxiong Gao <jxgao@google.com> Cc: "Christoph Hellwig" <hch@lst.de>, "Konrad Rzeszutek Wilk" <konrad@darnok.org>, "Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>, "Linus Torvalds" <torvalds@linux-foundation.org>, "Horia Geantă" <horia.geanta@nxp.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>, "Marc Orr" <marcorr@google.com>, "Erdem Aktas" <erdemaktas@google.com>, "Peter Gonda" <pgonda@google.com>, "Chanho Park" <chanho61.park@samsung.com> Subject: Re: swiotlb/caamjr regression (Was: [GIT PULL] (swiotlb) stable/for-linus-5.12) Date: Mon, 21 Jun 2021 11:03:07 +0900 [thread overview] Message-ID: <YM/zWyZlk1bzHWgI@atmark-techno.com> (raw) In-Reply-To: <CAMGD6P0=9RE1-q1WHkwR1jymK5jyvN6QgypQ2KgdvBQn0CUTHw@mail.gmail.com> [-- Attachment #1: Type: text/plain, Size: 1450 bytes --] Jianxiong Gao wrote on Fri, Jun 18, 2021 at 11:01:59AM -0700: > > Jianxiong Gao, before spending more time on this, could you also try > > Chanho Park's patch? > > https://lore.kernel.org/linux-iommu/20210510091816.GA2084@lst.de/T/#m0d0df6490350a08dcc24c9086c8edc165b402d6f > > > I have tested Chanho Parks's patch and it works for us. > The NVMe driver performs correctly with the patch. > > I have teste the patch on 06af8679449d Thanks! (a bit late, but added Chanho Park in Cc...) I can confirm it also works for our caam problem, as Horia said. I've also come to term with the use of swiotlb_align_offset() through testing, or rather many devices seem to have a 0 mask so it will almost always be cancelled out, so if it works for Jianxiong then it's probably good enough and I'll just assume that's how the orig_addr has been designed... I think it's missing a couple of checks like the one Linus had in his patch, and would be comfortable with something like the attached patch (in practice for me exactly the same as the original patch, except I've added two checks: offsets smaller than orig addr offset are refused as well as offsets bigger than the mapping size) I'm sorry Jianxiong but would you be willing to take the time to test again just to make sure there were no such offsets in your case? If we're good with that I'll send it as an official v2 keeping Chanho's from, unless he wants to. Thanks everyone, -- Dominique [-- Attachment #2: swiotlb.patch --] [-- Type: text/x-diff, Size: 2103 bytes --] diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index 8ca7d505d61c..23f8d0b168c5 100644 --- a/kernel/dma/swiotlb.c +++ b/kernel/dma/swiotlb.c @@ -334,6 +334,14 @@ void __init swiotlb_exit(void) io_tlb_default_mem = NULL; } +/* + * Return the offset into a iotlb slot required to keep the device happy. + */ +static unsigned int swiotlb_align_offset(struct device *dev, u64 addr) +{ + return addr & dma_get_min_align_mask(dev) & (IO_TLB_SIZE - 1); +} + /* * Bounce: copy the swiotlb buffer from or back to the original dma location */ @@ -346,10 +354,31 @@ static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t size size_t alloc_size = mem->slots[index].alloc_size; unsigned long pfn = PFN_DOWN(orig_addr); unsigned char *vaddr = phys_to_virt(tlb_addr); + unsigned int tlb_offset, orig_addr_offset; if (orig_addr == INVALID_PHYS_ADDR) return; + tlb_offset = tlb_addr & (IO_TLB_SIZE - 1); + orig_addr_offset = swiotlb_align_offset(dev, orig_addr); + if (tlb_offset < orig_addr_offset) { + dev_WARN_ONCE(dev, 1, + "Access before mapping start detected. orig offset %u, requested offset %u.\n", + orig_addr_offset, tlb_offset); + return; + } + + tlb_offset -= orig_addr_offset; + if (tlb_offset > alloc_size) { + dev_WARN_ONCE(dev, 1, + "Buffer overflow detected. Allocation size: %zu. Mapping size: %zu+%u.\n", + alloc_size, size, tlb_offset); + return; + } + + orig_addr += tlb_offset; + alloc_size -= tlb_offset; + if (size > alloc_size) { dev_WARN_ONCE(dev, 1, "Buffer overflow detected. Allocation size: %zu. Mapping size: %zu.\n", @@ -390,14 +419,6 @@ static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t size #define slot_addr(start, idx) ((start) + ((idx) << IO_TLB_SHIFT)) -/* - * Return the offset into a iotlb slot required to keep the device happy. - */ -static unsigned int swiotlb_align_offset(struct device *dev, u64 addr) -{ - return addr & dma_get_min_align_mask(dev) & (IO_TLB_SIZE - 1); -} - /* * Carefully handle integer overflow which can occur when boundary_mask == ~0UL. */
WARNING: multiple messages have this Message-ID (diff)
From: Dominique MARTINET <dominique.martinet@atmark-techno.com> To: Jianxiong Gao <jxgao@google.com> Cc: "Aymen Sghaier" <aymen.sghaier@nxp.com>, "Herbert Xu" <herbert@gondor.apana.org.au>, "Horia Geantă" <horia.geanta@nxp.com>, "Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>, "Marc Orr" <marcorr@google.com>, "Lukas Hartmann" <lukas@mntmn.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "David S. Miller" <davem@davemloft.net>, "iommu@lists.linux-foundation.org" <iommu@lists.linux-foundation.org>, "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>, "Peter Gonda" <pgonda@google.com>, "Konrad Rzeszutek Wilk" <konrad@darnok.org>, "Chanho Park" <chanho61.park@samsung.com>, "Linus Torvalds" <torvalds@linux-foundation.org>, "Christoph Hellwig" <hch@lst.de> Subject: Re: swiotlb/caamjr regression (Was: [GIT PULL] (swiotlb) stable/for-linus-5.12) Date: Mon, 21 Jun 2021 11:03:07 +0900 [thread overview] Message-ID: <YM/zWyZlk1bzHWgI@atmark-techno.com> (raw) In-Reply-To: <CAMGD6P0=9RE1-q1WHkwR1jymK5jyvN6QgypQ2KgdvBQn0CUTHw@mail.gmail.com> [-- Attachment #1: Type: text/plain, Size: 1450 bytes --] Jianxiong Gao wrote on Fri, Jun 18, 2021 at 11:01:59AM -0700: > > Jianxiong Gao, before spending more time on this, could you also try > > Chanho Park's patch? > > https://lore.kernel.org/linux-iommu/20210510091816.GA2084@lst.de/T/#m0d0df6490350a08dcc24c9086c8edc165b402d6f > > > I have tested Chanho Parks's patch and it works for us. > The NVMe driver performs correctly with the patch. > > I have teste the patch on 06af8679449d Thanks! (a bit late, but added Chanho Park in Cc...) I can confirm it also works for our caam problem, as Horia said. I've also come to term with the use of swiotlb_align_offset() through testing, or rather many devices seem to have a 0 mask so it will almost always be cancelled out, so if it works for Jianxiong then it's probably good enough and I'll just assume that's how the orig_addr has been designed... I think it's missing a couple of checks like the one Linus had in his patch, and would be comfortable with something like the attached patch (in practice for me exactly the same as the original patch, except I've added two checks: offsets smaller than orig addr offset are refused as well as offsets bigger than the mapping size) I'm sorry Jianxiong but would you be willing to take the time to test again just to make sure there were no such offsets in your case? If we're good with that I'll send it as an official v2 keeping Chanho's from, unless he wants to. Thanks everyone, -- Dominique [-- Attachment #2: swiotlb.patch --] [-- Type: text/x-diff, Size: 2103 bytes --] diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index 8ca7d505d61c..23f8d0b168c5 100644 --- a/kernel/dma/swiotlb.c +++ b/kernel/dma/swiotlb.c @@ -334,6 +334,14 @@ void __init swiotlb_exit(void) io_tlb_default_mem = NULL; } +/* + * Return the offset into a iotlb slot required to keep the device happy. + */ +static unsigned int swiotlb_align_offset(struct device *dev, u64 addr) +{ + return addr & dma_get_min_align_mask(dev) & (IO_TLB_SIZE - 1); +} + /* * Bounce: copy the swiotlb buffer from or back to the original dma location */ @@ -346,10 +354,31 @@ static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t size size_t alloc_size = mem->slots[index].alloc_size; unsigned long pfn = PFN_DOWN(orig_addr); unsigned char *vaddr = phys_to_virt(tlb_addr); + unsigned int tlb_offset, orig_addr_offset; if (orig_addr == INVALID_PHYS_ADDR) return; + tlb_offset = tlb_addr & (IO_TLB_SIZE - 1); + orig_addr_offset = swiotlb_align_offset(dev, orig_addr); + if (tlb_offset < orig_addr_offset) { + dev_WARN_ONCE(dev, 1, + "Access before mapping start detected. orig offset %u, requested offset %u.\n", + orig_addr_offset, tlb_offset); + return; + } + + tlb_offset -= orig_addr_offset; + if (tlb_offset > alloc_size) { + dev_WARN_ONCE(dev, 1, + "Buffer overflow detected. Allocation size: %zu. Mapping size: %zu+%u.\n", + alloc_size, size, tlb_offset); + return; + } + + orig_addr += tlb_offset; + alloc_size -= tlb_offset; + if (size > alloc_size) { dev_WARN_ONCE(dev, 1, "Buffer overflow detected. Allocation size: %zu. Mapping size: %zu.\n", @@ -390,14 +419,6 @@ static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t size #define slot_addr(start, idx) ((start) + ((idx) << IO_TLB_SHIFT)) -/* - * Return the offset into a iotlb slot required to keep the device happy. - */ -static unsigned int swiotlb_align_offset(struct device *dev, u64 addr) -{ - return addr & dma_get_min_align_mask(dev) & (IO_TLB_SIZE - 1); -} - /* * Carefully handle integer overflow which can occur when boundary_mask == ~0UL. */ [-- 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-21 2:13 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 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 [this message] 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=YM/zWyZlk1bzHWgI@atmark-techno.com \ --to=dominique.martinet@atmark-techno.com \ --cc=aymen.sghaier@nxp.com \ --cc=chanho61.park@samsung.com \ --cc=davem@davemloft.net \ --cc=erdemaktas@google.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=konrad@darnok.org \ --cc=linux-crypto@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=lukas@mntmn.com \ --cc=marcorr@google.com \ --cc=pgonda@google.com \ --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.