On Tue, 23 Jul 2024 at 16:52, Will Deacon <will@xxxxxxxxxx> wrote:
>
> 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.
>
Indeed. I failed to spot from the context (which is there in the diff)
that this is in the else branch.
> > 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.
>
Indeed.
> > > +{
> >
> > 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.
The 5-level case is never handled here.
There is the 3-level case, where the runtime PUD folding needs the
actual address in order to recalculate the descriptor address using
the correct shift. In this case, we don't dereference the pointer
anyway so the 'lockless' thing doesn't matter (afaict)
In the 4-level case, we want to preserve the original behavior, where
pgd is not reloaded from pgdp. Setting pgdp to &pgd achieves that.
> 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.
>
I don't understand what you are saying here. Why is that fine?
> > > + 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?
>
Good point. I did some boot tests with this but I'm not sure if it is
100% safe with the fixmap.
