MAX_DMA_ADDRESS overflow with non-zero arm_dma_zone_size and VMSPLIT_3G

[Geert Uytterhoeven]

Hi Florian,

On Wed, Mar 23, 2022 at 8:52 PM Florian Fainelli <f.fainelli at gmail.com> wrote:
> On 3/23/22 09:53, Geert Uytterhoeven wrote:
> > On Wed, Mar 23, 2022 at 4:02 PM Linus Walleij <linus.walleij at linaro.org> wrote:
> >> On Mon, Mar 21, 2022 at 4:46 AM Florian Fainelli <f.fainelli at gmail.com> wrote:
> >>> All of the virt_to_phys() and related functions either take a pointer
> >>> size argument (const volatile void *) or an unsigned long argument and
> >>> these are virtual addresses so unable to go over 32-bit anyway.
> >>
> >> Oh I ran into that too, in some different context that I since forgot.
> >> A macro that works the same on pointers and unsigned long but with
> >> slightly different semantics :P
> >>
> >> I don't know what is the proper thing to do here. Let's involve Arnd
> >> and Ard and Geert!
> >>
> >>> Since MAX_DMA_ADDRESS is intended to be "This is the maximum virtual
> >>> address which can be DMA'd from.", should we make sure that we clamp it
> >>> below 32-bit in case it overflows?
> >>
> >> Hmmmm.... I don't know what that would mean in practice?
> >
> >>> While debugging numerous KASAN splats with CONFIG_DEBUG_VIRTUAL on ARM
> >>> 32-bit with a Raspberry Pi 4B 4GB, it finally clicked that the problem
> >>> is with the use of __virt_to_phys(MAX_DMA_ADDRESS). Since that platform
> >>> has CONFIG_ZONE_DMA enabled, we end-up with:
> >>>
> >>>     #define MAX_DMA_ADDRESS ({ \
> >>>             extern phys_addr_t arm_dma_zone_size; \
> >>>             arm_dma_zone_size && arm_dma_zone_size < (0x10000000 -
> >>> PAGE_OFFSET) ? \
> >>>                     (PAGE_OFFSET + arm_dma_zone_size) : 0xffffffffUL; })
> >
> > I guess that should be "PAGE_OFFSET + (arm_dma_zone_size - 1)"?
>
> Yes, we are definitively off by one here, so this is a good catch and
> this will work for bcm2711 and any platform whereby PAGE_OFFSET +
> arm_dma_zone_size < 0xffff_ffff.
>
> There are a few that will still overflow that quantity:
>
> arch/arm/mach-highbank/highbank.c:      .dma_zone_size  = (4ULL * SZ_1G),
> arch/arm/mach-keystone/keystone.c:      .dma_zone_size  = SZ_2G,
> arch/arm/mach-omap2/board-generic.c:    .dma_zone_size  = SZ_2G,
> arch/arm/mach-omap2/board-generic.c:    .dma_zone_size  = SZ_2G,
> arch/arm/mach-omap2/board-generic.c:    .dma_zone_size  = SZ_2G,

Better show some context:

    $ git grep -W "\<dma_zone_size.*SZ_.G" -- arch/arm
    arch/arm/mach-bcm/bcm2711.c=DT_MACHINE_START(BCM2711, "BCM2711")
    arch/arm/mach-bcm/bcm2711.c-#ifdef CONFIG_ZONE_DMA
    arch/arm/mach-bcm/bcm2711.c:    .dma_zone_size  = SZ_1G,

So this is the problematic one?

    arch/arm/mach-bcm/bcm2711.c-#endif
    arch/arm/mach-bcm/bcm2711.c-    .dt_compat = bcm2711_compat,
    arch/arm/mach-bcm/bcm2711.c-    .smp = smp_ops(bcm2836_smp_ops),
    --
    arch/arm/mach-highbank/highbank.c=DT_MACHINE_START(HIGHBANK, "Highbank")
    arch/arm/mach-highbank/highbank.c-#if defined(CONFIG_ZONE_DMA) &&
defined(CONFIG_ARM_LPAE)
    arch/arm/mach-highbank/highbank.c:      .dma_zone_size  = (4ULL * SZ_1G),

This is fine, as LPAE implies physaddr_t is 64-bit.

    arch/arm/mach-highbank/highbank.c-#endif

The omap ones are fine for the same reason (LPAE).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds