| Message ID | eba2b72f-2180-498b-c8bd-ce8f717fc78a@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arch: allow pte_offset_map[_lock]() to fail | expand |
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@google.com> > --- > 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 :/ 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(). 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.
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@google.com> > > --- > > 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
On Wed, May 10, 2023 at 08:16:34PM -0700, Hugh Dickins wrote: > 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. I'm in the x86@ catch-all, which is how I saw this as quickly :-) A direct copy won't hurt ofc, the mail system will sort it out.
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); + } } va = (unsigned long)ldt_slot_va(ldt->slot);
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@google.com> --- arch/x86/kernel/ldt.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)