| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <cd2a9ccec0d88821fb0c7580f7ae934f2de71ddf.camel@intel.com>
Date: Wed, 11 Jan 2023 20:46:38 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "rafael@...nel.org" <rafael@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"peterz@...radead.org" <peterz@...radead.org>,
"rppt@...nel.org" <rppt@...nel.org>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
"pavel@....cz" <pavel@....cz>, "x86@...nel.org" <x86@...nel.org>,
"hpa@...or.com" <hpa@...or.com>,
"mingo@...hat.com" <mingo@...hat.com>,
"Lutomirski, Andy" <luto@...nel.org>,
"bp@...en8.de" <bp@...en8.de>, "Brown, Len" <len.brown@...el.com>
Subject: Re: [PATCH v2] x86/hibernate: Use fixmap for saving unmapped pages
On Wed, 2023-01-11 at 21:35 +0100, Rafael J. Wysocki wrote:
> On Tue, Jan 10, 2023 at 10:19 PM Rick Edgecombe
> <rick.p.edgecombe@...el.com> wrote:
> >
> > Hibernate uses the direct map to read memory it saves to disk.
> > Since
> > sometimes pages are not accessible on the direct map ("not present"
> > on
> > x86), it has special case logic to temporarily make a page present.
> > On x86
> > these direct map addresses can be mapped at various page sizes, but
> > the
> > logic works ok as long as the not present pages are always mapped
> > as
> > PAGE_SIZE such that they don't require a split to map the region as
> > present. If the address was mapped not present by a larger page
> > size, the
> > split may fail and hibernate would then try to read an address
> > mapped not
> > present.
> >
> > Today on x86 there are no known cases of this (huge not present
> > pages on
> > the direct map), but it has come up from time to time when
> > developing
> > things that operate on the direct map. It blocked making
> > VM_FLUSH_RESET_PERMS support huge vmalloc when that came up, and
> > also
> > has been a complication for various direct map protection efforts.
> >
> > This dependency is also pretty hidden and easily missed by people
> > poking at
> > the direct map. For this reason, there are warnings in place to
> > complain
> > but not handle this scenario.
> >
> > One way to make this more robust would be to create some new CPA
> > functionality that can know to map and reset the whole huge page in
> > the
> > case of trying to map a subpage. But for simplicity and smaller
> > code, just
> > make x86 hibernate have its own fixmap PTE that it can use to point
> > to 4k pages when it encounters an unmapped direct map page.
> >
> > Move do_copy_page() to a header such that it can be used in an arch
> > breakout. Rename it hib_copy_page() to be more hibernate specific
> > since
> > it could appear in other files.
> >
> > Use __weak for the arch breakout because there is not a suitable
> > arch
> > specific header to use the #define method.
> >
> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@...el.com>
> > ---
> >
> > Rebase to v6.2-rc3 (original still applied) and resending per:
> >
https://lore.kernel.org/lkml/CAJZ5v0i6cxGD+V6G+q-Y_Lp-ov51_zmkZr8ZGpCtqWV-e=BsLg@mail.gmail.com/
> >
> > arch/x86/include/asm/fixmap.h | 3 +++
> > arch/x86/power/hibernate.c | 10 ++++++++++
> > include/linux/suspend.h | 13 +++++++++++++
> > kernel/power/snapshot.c | 21 +++++++--------------
> > 4 files changed, 33 insertions(+), 14 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/fixmap.h
> > b/arch/x86/include/asm/fixmap.h
> > index d0dcefb5cc59..0fceed9a4152 100644
> > --- a/arch/x86/include/asm/fixmap.h
> > +++ b/arch/x86/include/asm/fixmap.h
> > @@ -108,6 +108,9 @@ enum fixed_addresses {
> > #ifdef CONFIG_PARAVIRT_XXL
> > FIX_PARAVIRT_BOOTMAP,
> > #endif
> > +#ifdef CONFIG_HIBERNATION
> > + FIX_HIBERNATE,
> > +#endif
> >
> > #ifdef CONFIG_ACPI_APEI_GHES
> > /* Used for GHES mapping from assorted contexts */
> > diff --git a/arch/x86/power/hibernate.c
> > b/arch/x86/power/hibernate.c
> > index 6f955eb1e163..473b6b5f6b7e 100644
> > --- a/arch/x86/power/hibernate.c
> > +++ b/arch/x86/power/hibernate.c
> > @@ -147,6 +147,16 @@ int arch_hibernation_header_restore(void
> > *addr)
> > return 0;
> > }
> >
> > +void copy_unmapped_page(void *dst, struct page *page)
> > +{
> > + WARN_ON(!preempt_count());
>
> I don't think the above is needed. The code using this function
> cannot be preempted anyway AFAICS.
The reason I thought it was useful was because this function is now
defined in a header. Someone else might decide to use it. Does it seem
more useful?
>
> > +
> > + set_fixmap(FIX_HIBERNATE, page_to_phys(page));
> > + __flush_tlb_all();
>
> So do TLBs need to be flushed before copying every single page?
> Basically, they are all copied in one loop.
It is only one fixmap entry so it needs to be flushed after changing
the PTE to point to a different page. But this is only for the case of
unmapped pages, the more common mapped pages are copied from the direct
map like usual.
>
> > + hib_copy_page(dst, (void *)fix_to_virt(FIX_HIBERNATE));
> > + clear_fixmap(FIX_HIBERNATE);
> > +}
> > +
> > int relocate_restore_code(void)
> > {
> > pgd_t *pgd;
> > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > index cfe19a028918..0b19b910526e 100644
> > --- a/include/linux/suspend.h
> > +++ b/include/linux/suspend.h
> > @@ -447,6 +447,19 @@ extern bool hibernation_available(void);
> > asmlinkage int swsusp_save(void);
> > extern struct pbe *restore_pblist;
> > int pfn_is_nosave(unsigned long pfn);
> > +void copy_unmapped_page(void *dst, struct page *page);
> > +
> > +/*
> > + * This is needed, because copy_page and memcpy are not usable for
> > copying
> > + * task structs.
> > + */
> > +static inline void hib_copy_page(long *dst, long *src)
> > +{
> > + int n;
> > +
> > + for (n = PAGE_SIZE / sizeof(long); n; n--)
> > + *dst++ = *src++;
> > +}
> >
> > int hibernate_quiet_exec(int (*func)(void *data), void *data);
> > #else /* CONFIG_HIBERNATION */
> > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> > index cd8b7b35f1e8..344c071f29d3 100644
> > --- a/kernel/power/snapshot.c
> > +++ b/kernel/power/snapshot.c
> > @@ -1369,16 +1369,11 @@ static unsigned int count_data_pages(void)
> > return n;
> > }
> >
> > -/*
> > - * This is needed, because copy_page and memcpy are not usable for
> > copying
> > - * task structs.
> > - */
> > -static inline void do_copy_page(long *dst, long *src)
> > +void __weak copy_unmapped_page(void *dst, struct page *page)
> > {
> > - int n;
> > -
> > - for (n = PAGE_SIZE / sizeof(long); n; n--)
> > - *dst++ = *src++;
> > + hibernate_map_page(page);
> > + hib_copy_page(dst, page_address(page));
> > + hibernate_unmap_page(page);
> > }
> >
> > /**
> > @@ -1392,11 +1387,9 @@ static inline void do_copy_page(long *dst,
> > long *src)
> > static void safe_copy_page(void *dst, struct page *s_page)
> > {
> > if (kernel_page_present(s_page)) {
> > - do_copy_page(dst, page_address(s_page));
> > + hib_copy_page(dst, page_address(s_page));
> > } else {
> > - hibernate_map_page(s_page);
> > - do_copy_page(dst, page_address(s_page));
> > - hibernate_unmap_page(s_page);
> > + copy_unmapped_page(dst, s_page);
> > }
> > }
> >
> > @@ -1417,7 +1410,7 @@ static void copy_data_page(unsigned long
> > dst_pfn, unsigned long src_pfn)
> > if (PageHighMem(s_page)) {
> > src = kmap_atomic(s_page);
> > dst = kmap_atomic(d_page);
> > - do_copy_page(dst, src);
> > + hib_copy_page(dst, src);
> > kunmap_atomic(dst);
> > kunmap_atomic(src);
> > } else {
> > --
> > 2.17.1
> >
Powered by blists - more mailing lists