Re: [PATCH 1/1] x86/mm: Use proper mask when setting PUD mapping

[Linus Torvalds]

On Thu, Aug 18, 2022 at 7:30 PM Aaron Lu <aaron.lu@xxxxxxxxx> wrote:
>
> -                       prot = __pgprot(pgprot_val(prot) | __PAGE_KERNEL_LARGE);
> +                       prot = __pgprot_mask(pgprot_val(prot) | __PAGE_KERNEL_LARGE);

The patch looks "ObviouslyCorrect(tm)" to me, but I have to admit that
I absolutely hate how we use the pte helpers for all the levels.

It gets even worse when we do that

                        set_pte_init((pte_t *)pud,
                                     pfn_pte((paddr & PUD_MASK) >> PAGE_SHIFT,
                                             prot),
                                     init);

on the next lines, and I don't understand why this doesn't use
"set_pud_init()" here.

It's probably something obvious, like "using set_pud_init() would mean
that we'd have to cast the *second* argument instead, because we don't
have a pfd_pud() function".

But it all makes me go a bit eww, and also makes me suspect I am
missing something else too, and that my "this looks
ObviouslyCorrect(tm)" is thus worthless.

Also, I don't understand why we use that __PAGE_KERNEL_LARGE at all.
We already have a valid set of protection bits, that had gotten
properly masked previously.

Isn't the only bit we actually want to set "_PAGE_PSE"?

IOW, I get the feeling that that patch should instead just be

-                       prot = __pgprot(pgprot_val(prot) | __PAGE_KERNEL_LARGE);
+                       prot = __pgprot(pgprot_val(prot) | _PAGE_PSE);

and we should never need to mask anything off at all with
__pgprot_mask() - the bug was really that we set way too many bits.

But again, I *also* have the feeling that I'm missing something important.

Ingo, Thomas, any others who know this code better than me by now - comments?

                Linus