Messages in this thread | | | Date | Mon, 7 Aug 2023 23:46:05 +0100 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH] arm: dma-mapping: fix potential endless loop in __dma_page_dev_to_cpu() |
| |
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!
| |