Re: [PATCH 1/4] x86/mm/pat: Handle no-GBPAGES case correctly in populate_pud

[Ard Biesheuvel]

On Tue, 3 Mar 2020 at 21:54, Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote:
>
> Commit d367cef0a7f0 ("x86/mm/pat: Fix boot crash when 1GB pages are not
> supported by the CPU") added checking for CPU support for 1G pages
> before using them.
>
> However, when support is not present, nothing is done to map the
> intermediate 1G regions and we go directly to the code that normally
> maps the remainder after 1G mappings have been done. This code can only
> handle mappings that fit inside a single PUD entry, but there is no
> check, and it instead silently produces a corrupted mapping to the end
> of the PUD entry, and no mapping beyond it, but still returns success.
>
> This bug is encountered on EFI machines in mixed mode (32-bit firmware
> with 64-bit kernel), with RAM beyond 2G. The EFI support code
> direct-maps all the RAM, so a memory range from below 1G to above 2G
> triggers the bug and results in no mapping above 2G, and an incorrect
> mapping in the 1G-2G range. If the kernel resides in the 1G-2G range, a
> firmware call does not return correctly, and if it resides above 2G, we
> end up passing addresses that are not mapped in the EFI pagetable.
>
> Fix this by mapping the 1G regions using 2M pages when 1G page support
> is not available.
>
> Signed-off-by: Arvind Sankar <nivedita@xxxxxxxxxxxx>

I was trying to test these patches, and while they seem fine from a
regression point of view, I can't seem to reproduce this issue and
make it go away again by applying this patch.

Do you have any detailed instructions how to reproduce this?

> ---
>  arch/x86/mm/pat/set_memory.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> index c4aedd00c1ba..d0b7b06253a5 100644
> --- a/arch/x86/mm/pat/set_memory.c
> +++ b/arch/x86/mm/pat/set_memory.c
> @@ -1370,12 +1370,22 @@ static int populate_pud(struct cpa_data *cpa, unsigned long start, p4d_t *p4d,
>         /*
>          * Map everything starting from the Gb boundary, possibly with 1G pages
>          */
> -       while (boot_cpu_has(X86_FEATURE_GBPAGES) && end - start >= PUD_SIZE) {
> -               set_pud(pud, pud_mkhuge(pfn_pud(cpa->pfn,
> -                                  canon_pgprot(pud_pgprot))));
> +       while (end - start >= PUD_SIZE) {
> +               if (boot_cpu_has(X86_FEATURE_GBPAGES)) {
> +                       set_pud(pud, pud_mkhuge(pfn_pud(cpa->pfn,
> +                                          canon_pgprot(pud_pgprot))));
> +                       cpa->pfn += PUD_SIZE >> PAGE_SHIFT;
> +               } else {
> +                       if (pud_none(*pud))
> +                               if (alloc_pmd_page(pud))
> +                                       return -1;
> +                       if (populate_pmd(cpa, start, start + PUD_SIZE,
> +                                        PUD_SIZE >> PAGE_SHIFT,
> +                                        pud, pgprot) < 0)
> +                               return cur_pages;
> +               }
>
>                 start     += PUD_SIZE;
> -               cpa->pfn  += PUD_SIZE >> PAGE_SHIFT;
>                 cur_pages += PUD_SIZE >> PAGE_SHIFT;
>                 pud++;
>         }
> --
> 2.24.1
>