Message-ID: <CAHVXubgtOoHQBED-aF7PS8Cq1h6X-M2tou532=qk3ZmAVm73gQ@mail.gmail.com>
Date: Tue, 20 Feb 2024 21:50:38 +0100
From: Alexandre Ghiti <alexghiti@...osinc.com>
To: dvlachos <dvlachos@....forth.gr>
Cc: linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	clameter@....com, akpm@...ux-foundation.org, rppt@...nel.org, arnd@...db.de, 
	paul.walmsley@...ive.com, palmer@...belt.com, mick@....forth.gr, 
	csd4492@....uoc.gr
Subject: Re: [PATCH] [RFC] sparsemem: warn on out-of-bounds initialization

Hi Dimitris,

On Mon, Feb 19, 2024 at 3:59 PM dvlachos <dvlachos@....forth.gr> wrote:
>
> Alexandre,
>
> Yes, you understood correctly, I indeed proposed to change
> pfn_to_page()/page_to_pfn() but your solution appears to solve the
> problem without risking any modifications to the conversion macros. In
> addition, your solution seems to be valid for any phys_ram_base/pfn
> value inside the limits of physical memory.
>
> However, I wanted to note that if the pfn is large enough, vmemmap will
> not be a valid SV39/48 address unless a pfn offset is applied to it. I
> can't think of a possible scenario where vmemmap would be used without
> an offset. I would like to know your opinion on that, does it concern you?

No, I don't see when that could happen, that would be a mistake and
then it would be easy to catch with this patch :)

>
> Finally, do you want me to send a patch or will you do it?
>

I'd be happy to help if you need, and if it's too much of a pain for
you, I'll do it, let me know.

Thanks,

Alex

> Dimitris
>
> On 2/18/2024 10:58 PM, Dimitris Vlachos wrote:
> >
> >
> >
> >
> > -------- Forwarded Message --------
> > Subject:      Re: [PATCH] [RFC] sparsemem: warn on out-of-bounds
> > initialization
> > Date:         Tue, 6 Feb 2024 22:17:44 +0100
> > From:         Alexandre Ghiti <alexghiti@...osinc.com>
> > To:   Dimitris Vlachos <csd4492@....uoc.gr>
> > CC:   linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
> > clameter@....com, akpm@...ux-foundation.org, rppt@...nel.org,
> > arnd@...db.de, paul.walmsley@...ive.com, palmer@...belt.com,
> > mick@....forth.gr
> >
> >
> >
> > Hi Dimitris,
> >
> > On Fri, Feb 2, 2024 at 2:59 PM Alexandre Ghiti
> > <alexghiti@...osinc.com> wrote:
> >> Hi Dimitris,
> >>
> >> On Fri, Feb 2, 2024 at 2:50 PM Dimitris Vlachos<csd4492@....uoc.gr>  wrote:
> >>> From: Dimitris Vlachos<dvlachos@....forth.gr>
> >>>
> >>> Hello all
> >>>
> >>> I am sending this email with regards to a bug that I discovered in the Sparse Memory Model configuration and more specifically, the Virtual Memory Map optimization. Moreover, I would like to inquire about possible ways of fixing it.
> >>>
> >>> I work as a pre-graduate research assistant at ICS-FORTH in the Computer Architecture and VLSI Systems laboratory.
> >>> We were running some tests in our prototype hardware (RISC-V), where we noticed that the Kernel crashes early in the boot process with the following setup:
> >>>
> >>> We are using the default Kconfig configurations (defconfig) that includes Sparse Memory + Virtual Memory Map.
> >>> The DRAM base address of our system is : 0x800000000000
> >>> A 3-level page table is used (Sv39).
> >>>
> >>> When the vmemmap optimization is enabled the macro pfn_to_page() is called, which offsets the vmemmap with the pfn argument to acquire a struct page pointer.
> >>>
> >>> As our DRAM starts at 0x800000000000, the initial pfn will be 0x800000000000 divided by PAGE_SIZE. The calculation result will be:
> >>> 0xffffffcf00000000 (vmemmap start) +  (0x800000000 (pfn) * 64 (sizeof(struct page))
> >>>
> >>> This causes an overflow as the number is very large, the resulting address is 0x1c700000000 which is not a valid Sv39 address (all bits above bit 38 should be set) and does not belong to the kernel’s virtual address space.
> >>>
> >>> The same will happen with all valid pfns as the memory is being initialized. When the Kernel will try to access a page using pfn_to_page, a page fault will occur crashing the system.
> >>> It should be noted that when the vmemmap is disabled the system boots normally.
> >>>
> >>> However, I would like to emphasize another important detail. When the DRAM base is small enough to avoid an overflow, the virtual memory map mappings will be initialized out of bounds with regard to the vmemmap address space which is 4GiB in Sv39.
> >>> The system will not crash, but the address space used for this purpose will be outside of the designated range.
> >>>
> >>> Everything mentioned above holds true when Sv48 is used as well. Given a high enough DRAM base the system will either crash or perform false initializations. Given the fact that virtual memory map is not only used by RISC-V systems, this could be an issue for other architectures as well.
> >>>
> >>> The kernel already contains mminit_validate_memmodel_limits() that checks memory limits.
> >>> However, it only checks physical memory ranges. I added a few checks, provided that virtual memory map is enabled, to determine whether offset start and offset end fall inside the range of virtual memory map. Otherwise, a warning will be printed.
> >>>
> >>> Finally, I would like to ask you for any information/advice that could help me fix/prevent the issues that I mentioned (if it’s possible) or recover from them at runtime by disabling the optimization and reverting back to the non-vmemmap mappings.
> >>>
> >>> Thank you in advance.
> >>> Best regards,
> >>> Dimitris Vlachos
> >>> ---
> >>>   mm/sparse.c | 20 ++++++++++++++++++++
> >>>   1 file changed, 20 insertions(+)
> >>>
> >>> diff --git a/mm/sparse.c b/mm/sparse.c
> >>> index 338cf946d..33b57758e 100644
> >>> --- a/mm/sparse.c
> >>> +++ b/mm/sparse.c
> >>> @@ -149,6 +149,26 @@ static void __meminit mminit_validate_memmodel_limits(unsigned long *start_pfn,
> >>>                  WARN_ON_ONCE(1);
> >>>                  *end_pfn = max_sparsemem_pfn;
> >>>          }
> >>> +
> >>> +       /*check vmemmap limits*/
> >>> +       #ifdef CONFIG_SPARSEMEM_VMEMMAP
> >>> +
> >>> +       unsigned long vmemmap_offset_start = (unsigned long) pfn_to_page(*start_pfn);
> >>> +       unsigned long vmemmap_offset_end   = (unsigned long) pfn_to_page(*end_pfn);
> >>> +
> >>> +       if (vmemmap_offset_start < VMEMMAP_START) {
> >>> +               mminit_dprintk(MMINIT_WARNING, "pfnvalidation",
> >>> +                       "Start of range %lu -> %lu exceeds bounds of SPARSEMEM virtual memory map address range %lu -> %lu\n",
> >>> +                       vmemmap_offset_start, vmemmap_offset_end, VMEMMAP_START,VMEMMAP_END);
> >>> +               WARN_ON_ONCE(1);
> >>> +
> >>> +       } else if (vmemmap_offset_end > VMEMMAP_END ) {
> >>> +               mminit_dprintk(MMINIT_WARNING, "pfnvalidation",
> >>> +                       "End of range %lu -> %lu exceeds bounds of SPARSEMEM virtual memory map address range %lu -> %lu\n",
> >>> +                       vmemmap_offset_start, vmemmap_offset_end, VMEMMAP_START,VMEMMAP_END);
> >>> +               WARN_ON_ONCE(1);
> >>> +       }
> >>> +       #endif
> >>>   }
> >>>
> >>>   /*
> >>> --
> >>> 2.39.2
> >>>
> >>
> >> Since struct pages are only available for memory,  we could offset
> >> vmemmap so that VMEMMAP_START actually points to the first pfn?
> >
> >> Hello Alexandre,
> >> My first thought was to use a "better" offset for vmemmap as well.Since pfn is essential for pfn_to_page and page_to_pfn i think that maybe we could get a smaller number out of pfn to offset the vmemmap and ensure that it's  bounds are respected for pfn_to_page operation.
> >> If we use only a part of the pfn for the offset we could get the  pfn from page_to_pfn by adding PAGE-VMEMMAP_START
> >> to the DRAM base/PAGE_SIZE which is the first pfn.
> >
> > Let's keep the discussion here.
> >
> > IIUC you propose to change pfn_to_page()/page_to_pfn() but what I
> > proposed was to directly offset vmemmap like this:
> >
> > #define vmemmap ((struct page *)VMEMMAP_START - phys_ram_base
> >>> PAGE_SHIFT)
> >
> > So that the first page in vmemmap corresponds to the first page of
> > physical memory, that avoids the burden of changing the conversion
> > macros, WDYT?
> >
> > Thanks,
> >
> > Alex

Powered by blists - more mailing lists