Hey Ard,
On Fri, Jul 19, 2024 at 11:02:29AM -0700, Ard Biesheuvel wrote:
> Thanks for the cc, and thanks to Lina for the excellent diagnosis -
> this is really helpful.
>
> > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> > index f8efbc128446..3afe624a39e1 100644
> > --- a/arch/arm64/include/asm/pgtable.h
> > +++ b/arch/arm64/include/asm/pgtable.h
> > @@ -1065,6 +1065,13 @@ static inline bool pgtable_l5_enabled(void) { return false; }
> >
> > #define p4d_offset_kimg(dir,addr) ((p4d_t *)dir)
> >
> > +static inline
> > +p4d_t *p4d_offset_lockless(pgd_t *pgdp, pgd_t pgd, unsigned long addr)
>
> This is in the wrong place, I think - we already define this for the
> 5-level case (around line 1760).
Hmm, I'm a bit confused. In my tree, we have one definition at line 1012,
which is for the 5-level case (i.e. guarded by
'#if CONFIG_PGTABLE_LEVELS > 4'). I'm adding a new one at line 1065,
which puts it in the '#else' block and means we use an override instead
of the problematic generic version when we're folding.
> We'll need to introduce another version for the 4-level case, so
> perhaps, to reduce the risk of confusion, we might define it as
>
> static inline
> p4d_t *p4d_offset_lockless_folded(pgd_t *pgdp, pgd_t pgd, unsigned long addr)
> {
> ...
> }
> #ifdef __PAGETABLE_P4D_FOLDED
> #define p4d_offset_lockless p4d_offset_lockless_folded
> #endif
Renaming will definitely make this easier on the eye, so I'll do that.
I don't think I need the 'ifdef' though.
> > +{
>
> We might add
>
> if (pgtable_l4_enabled())
> pgdp = &pgd;
>
> here to preserve the existing 'lockless' behavior when PUDs are not
> folded.
The code still needs to be 'lockless' for the 5-level case, so I don't
think this is necessary. Yes, we'll load the same entry multiple times,
but it should be fine because they're in the context of a different
(albeit folded) level.
> > + return p4d_offset(pgdp, addr);
> > +}
> > +#define p4d_offset_lockless p4d_offset_lockless
> > +
> > #endif /* CONFIG_PGTABLE_LEVELS > 4 */
> >
>
> I suggest we also add something like the below so we can catch these
> issues more easily
>
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -874,9 +874,26 @@ static inline phys_addr_t p4d_page_paddr(p4d_t p4d)
>
> static inline pud_t *p4d_to_folded_pud(p4d_t *p4dp, unsigned long addr)
> {
> + /*
> + * The transformation below does not work correctly for descriptors
> + * copied to the stack.
> + */
> + VM_WARN_ON((u64)p4dp >= VMALLOC_START && !__is_kernel((u64)p4dp));
Hmm, this is a bit coarse. Does it work properly with the fixmap?
Will
