On Wed, 10 May 2023, Peter Zijlstra wrote:
> On Tue, May 09, 2023 at 10:08:37PM -0700, Hugh Dickins wrote:
> > In rare transient cases, not yet made possible, pte_offset_map() and
> > pte_offset_map_lock() may not find a page table: handle appropriately.
> >
> > Signed-off-by: Hugh Dickins <hughd@xxxxxxxxxx>
> > ---
> > arch/x86/kernel/ldt.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/ldt.c b/arch/x86/kernel/ldt.c
> > index 525876e7b9f4..eb844549cd83 100644
> > --- a/arch/x86/kernel/ldt.c
> > +++ b/arch/x86/kernel/ldt.c
> > @@ -367,8 +367,10 @@ static void unmap_ldt_struct(struct mm_struct *mm, struct ldt_struct *ldt)
> >
> > va = (unsigned long)ldt_slot_va(ldt->slot) + offset;
> > ptep = get_locked_pte(mm, va, &ptl);
> > - pte_clear(mm, va, ptep);
> > - pte_unmap_unlock(ptep, ptl);
> > + if (ptep) {
> > + pte_clear(mm, va, ptep);
> > + pte_unmap_unlock(ptep, ptl);
> > + }
> > }
>
> Ow geez, now I have to go remember how the whole PTI/LDT crud worked :/
I apologize for sending you back there!
>
> At first glance this seems wrong; we can't just not unmap the LDT if we
> can't find it in a hurry. Also, IIRC this isn't in fact a regular user
> mapping, so it should not be subject to THP induced seizures.
>
> ... memory bubbles back ... for PTI kernels we need to map this in the
> user and kernel page-tables because obviously userspace needs to be able
> to have access to the LDT. But it is not directly acessible by
> userspace. It lives in the cpu_entry_area as a virtual map of the real
> kernel allocation, and this virtual address is used for LLDT.
> Modification is done through sys_modify_ldt().
And there must be a user-style page table backing that cpu_entry_area,
because the use of get_locked_pte() and pte_unmap_unlock() implies
that there's a user page table (struct page containing spinlock if
config says so) rather than just a kernel page table mapping it.
>
> I think I would feel much better if this were something like:
>
> if (!WARN_ON_ONCE(!ptep))
>
> This really shouldn't fail and if it does, simply skipping it isn't the
> right thing either.
Sure, I'll gladly make that change when I respin - not immediately, let's
get more feedback on this arch series first, but maybe in a week's time.
Thanks for looking so quickly, Peter: I didn't Cc you on this particular
series, but shall certainly be doing so on the ones that follow, because
a few of those patches go into interesting pmdp_get_lockless() territory.
Hugh
