From: "Russell King (Oracle)" <linux@armlinux.org.uk> To: Matthew Wilcox <willy@infradead.org> Cc: Marek Szyprowski <m.szyprowski@samsung.com>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Andrew Morton <akpm@linux-foundation.org>, Mike Rapoport <rppt@kernel.org> Subject: Re: [PATCH] arm: dma-mapping: fix potential endless loop in __dma_page_dev_to_cpu() Date: Mon, 7 Aug 2023 23:46:05 +0100 [thread overview] Message-ID: <ZNF0LUrtoCp+aS5U@shell.armlinux.org.uk> (raw) In-Reply-To: <ZNFstXmztIriaKOX@casper.infradead.org> On Mon, Aug 07, 2023 at 11:14:13PM +0100, Matthew Wilcox wrote: > On Mon, Aug 07, 2023 at 05:26:57PM +0200, Marek Szyprowski wrote: > > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > > index 70cb7e63a9a5..02250106e5ed 100644 > > --- a/arch/arm/mm/dma-mapping.c > > +++ b/arch/arm/mm/dma-mapping.c > > @@ -718,7 +718,7 @@ static void __dma_page_dev_to_cpu(struct page *page, unsigned long off, > > folio = folio_next(folio); > > } > > > > - while (left >= (ssize_t)folio_size(folio)) { > > + while (left && left >= (ssize_t)folio_size(folio)) { > > set_bit(PG_dcache_clean, &folio->flags); > > left -= folio_size(folio); > > folio = folio_next(folio); > > I've been thinking about this and I think this is the right fix for the > wrong reason. I don't understand how it can produce the failure you > saw, but we shouldn't be calling folio_next() if left is zero, let alone > calling folio_size() on it. So I'd rather see this fix: > > while (left >= (ssize_t)folio_size(folio)) { > set_bit(PG_dcache_clean, &folio->flags); > left -= folio_size(folio); > + if (!left) > + break; Given that set_bit() involves atomics, wouldn't it be better if this had been written as: while (left >= folio_size(folio)) { left -= folio_size(folio); set_bit(PG_dcache_clean, &folio->flags); if (!left) break; > folio = folio_next(folio); > } That likely means that folio_size() will only be evaluated once per loop rather than twice. I may be wrong though, I didn't check the generated code. Also, I'm wondering what that ssize_t cast is doing there - "left" is a size_t, which is unsigned. folio_size() returns a size_t, so is also unsigned. Why convert folio_size() to a signed number to then be compared with an unsigned number? Or did "left" get converted to ssize_t along with the folio conversion? Even if it did, how could "left" be negative (except through casting a large positive number as "size" that in 2's complement would be negative after casting to "left") ? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
WARNING: multiple messages have this Message-ID (diff)
From: "Russell King (Oracle)" <linux@armlinux.org.uk> To: Matthew Wilcox <willy@infradead.org> Cc: Marek Szyprowski <m.szyprowski@samsung.com>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Andrew Morton <akpm@linux-foundation.org>, Mike Rapoport <rppt@kernel.org> Subject: Re: [PATCH] arm: dma-mapping: fix potential endless loop in __dma_page_dev_to_cpu() Date: Mon, 7 Aug 2023 23:46:05 +0100 [thread overview] Message-ID: <ZNF0LUrtoCp+aS5U@shell.armlinux.org.uk> (raw) In-Reply-To: <ZNFstXmztIriaKOX@casper.infradead.org> On Mon, Aug 07, 2023 at 11:14:13PM +0100, Matthew Wilcox wrote: > On Mon, Aug 07, 2023 at 05:26:57PM +0200, Marek Szyprowski wrote: > > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > > index 70cb7e63a9a5..02250106e5ed 100644 > > --- a/arch/arm/mm/dma-mapping.c > > +++ b/arch/arm/mm/dma-mapping.c > > @@ -718,7 +718,7 @@ static void __dma_page_dev_to_cpu(struct page *page, unsigned long off, > > folio = folio_next(folio); > > } > > > > - while (left >= (ssize_t)folio_size(folio)) { > > + while (left && left >= (ssize_t)folio_size(folio)) { > > set_bit(PG_dcache_clean, &folio->flags); > > left -= folio_size(folio); > > folio = folio_next(folio); > > I've been thinking about this and I think this is the right fix for the > wrong reason. I don't understand how it can produce the failure you > saw, but we shouldn't be calling folio_next() if left is zero, let alone > calling folio_size() on it. So I'd rather see this fix: > > while (left >= (ssize_t)folio_size(folio)) { > set_bit(PG_dcache_clean, &folio->flags); > left -= folio_size(folio); > + if (!left) > + break; Given that set_bit() involves atomics, wouldn't it be better if this had been written as: while (left >= folio_size(folio)) { left -= folio_size(folio); set_bit(PG_dcache_clean, &folio->flags); if (!left) break; > folio = folio_next(folio); > } That likely means that folio_size() will only be evaluated once per loop rather than twice. I may be wrong though, I didn't check the generated code. Also, I'm wondering what that ssize_t cast is doing there - "left" is a size_t, which is unsigned. folio_size() returns a size_t, so is also unsigned. Why convert folio_size() to a signed number to then be compared with an unsigned number? Or did "left" get converted to ssize_t along with the folio conversion? Even if it did, how could "left" be negative (except through casting a large positive number as "size" that in 2's complement would be negative after casting to "left") ? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-08-07 22:46 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <CGME20230807152704eucas1p1bbe08af4559a7d2984198fe8ba487a2e@eucas1p1.samsung.com> 2023-08-07 15:26 ` [PATCH] arm: dma-mapping: fix potential endless loop in __dma_page_dev_to_cpu() Marek Szyprowski 2023-08-07 15:26 ` Marek Szyprowski 2023-08-07 16:23 ` Matthew Wilcox 2023-08-07 16:23 ` Matthew Wilcox 2023-08-09 17:05 ` Marek Szyprowski 2023-08-09 17:05 ` Marek Szyprowski 2023-08-07 22:14 ` Matthew Wilcox 2023-08-07 22:14 ` Matthew Wilcox 2023-08-07 22:46 ` Russell King (Oracle) [this message] 2023-08-07 22:46 ` Russell King (Oracle) 2023-08-09 19:38 ` Matthew Wilcox 2023-08-09 19:38 ` Matthew Wilcox
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=ZNF0LUrtoCp+aS5U@shell.armlinux.org.uk \ --to=linux@armlinux.org.uk \ --cc=akpm@linux-foundation.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=m.szyprowski@samsung.com \ --cc=rppt@kernel.org \ --cc=willy@infradead.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.