Re: [PATCH] arm: dma-mapping: fix potential endless loop in __dma_page_dev_to_cpu()

[Matthew Wilcox]

On Mon, Aug 07, 2023 at 11:46:05PM +0100, Russell King (Oracle) wrote:
> 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.

I'd really like it if gcc did notice that folio_size() could be CSE.
Unfortunately, I don't think it can.

+long rmk(struct folio *folio, long size)
+{
+       while (size >= folio_size(folio)) {
+               size -= folio_size(folio);
+               folio_set_workingset(folio);
+               if (size < 0)
+                       return size;
+               folio = folio_next(folio);
+       }
+
+       return size;
+}

000039d4 <rmk>:
    39d4:       e92d41f0        push    {r4, r5, r6, r7, r8, lr}
    39d8:       e1a04000        mov     r4, r0
    39dc:       e1a05001        mov     r5, r1
    39e0:       e3a06a01        mov     r6, #4096       @ 0x1000
    39e4:       e3a07020        mov     r7, #32
    39e8:       ea000010        b       3a30 <rmk+0x5c>
    39ec:       e5943000        ldr     r3, [r4]
    39f0:       e1a01004        mov     r1, r4
    39f4:       e3a00009        mov     r0, #9
    39f8:       e3130040        tst     r3, #64 @ 0x40
    39fc:       03a03a01        moveq   r3, #4096       @ 0x1000
    3a00:       15d43020        ldrbne  r3, [r4, #32]
    3a04:       11a03316        lslne   r3, r6, r3
    3a08:       e0455003        sub     r5, r5, r3
    3a0c:       ebfffffe        bl      0 <_set_bit>
                        3a0c: R_ARM_CALL        _set_bit
    3a10:       e3550000        cmp     r5, #0
    3a14:       ba00000c        blt     3a4c <rmk+0x78>
    3a18:       e5943000        ldr     r3, [r4]
    3a1c:       e3130040        tst     r3, #64 @ 0x40
    3a20:       03a03020        moveq   r3, #32
    3a24:       15d43020        ldrbne  r3, [r4, #32]
    3a28:       11a03317        lslne   r3, r7, r3
    3a2c:       e0844003        add     r4, r4, r3
    3a30:       e5943000        ldr     r3, [r4]
    3a34:       e3130040        tst     r3, #64 @ 0x40
    3a38:       03a03a01        moveq   r3, #4096       @ 0x1000
    3a3c:       15d43020        ldrbne  r3, [r4, #32]
    3a40:       11a03316        lslne   r3, r6, r3
    3a44:       e1550003        cmp     r5, r3
    3a48:       2affffe7        bcs     39ec <rmk+0x18>
    3a4c:       e1a00005        mov     r0, r5
    3a50:       e8bd81f0        pop     {r4, r5, r6, r7, r8, pc}

Certainly seems to me like it's calculating folio_size() twice.
And actually it's redone the ordering to put the calculation
after the call to set_bit!

> 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?

Because earlier we did:

+       if (offset) {
+               left -= folio_size(folio) - offset;
+               folio = folio_next(folio);
+       }

so left might now be negative.  If we did an unsigned comparison,
we'd go round this loop.

Er.  And the fix from Marek didn't accommodate this problem.  So we need
a fix-fix:

	if (offset) {
		left -= folio_size(folio) - offset;
+		if (left <= 0)
+			return;
		folio = folio_next(folio);
	}

Marek, can you do the honours here?

> 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!