Messages in this thread Patch in this message | | | From | Dan Williams <> | Date | Fri, 4 Feb 2022 13:07:10 -0800 | Subject | Re: [PATCH V8 41/44] kmap: Ensure kmap works for devmap pages |
| |
On Thu, Jan 27, 2022 at 9:55 AM <ira.weiny@intel.com> wrote: > > From: Ira Weiny <ira.weiny@intel.com> > > Users of devmap pages should not have to know that the pages they are > operating on are special.
How about get straight to the point without any ambiguous references:
Today, kmap_{local_page,atomic} handles granting access to HIGHMEM pages without the caller needing to know if the page is HIGHMEM, or not. Use that existing infrastructure to grant access to PKS/PGMAP access protected pages.
> Co-opt the kmap_{local_page,atomic}() to mediate access to PKS protected > pages via the devmap facility. kmap_{local_page,atomic}() are both > thread local mappings so they work well with the thread specific > protections available. > > kmap(), on the other hand, allows for global mappings to be established, > Which is incompatible with the underlying PKS facility.
Why is kmap incompatible with PKS? I know why, but this is a claim without evidence. If you documented that in a previous patch, there's no harm and copying and pasting into this one. A future git log user will thank you for not making them go to lore to try to find the one patch with the details. Extra credit for creating a PKS theory of operation document with this detail, unless I missed that?
> For this reason > kmap() is not supported. Rather than leave the kmap mappings to fault > at random times when users may access them,
Is that a problem? This instrumentation is also insufficient for legitimate usages of page_address(). Might as well rely on the kernel developer community being able to debug PKS WARN() splats back to the source because that will need to be done regardless, given kmap() is not the only source of false positive access violations.
> call > pgmap_protection_flag_invalid() to show kmap() users the call stack of > where mapping was created. This allows better debugging. > > This behavior is safe because neither of the 2 current DAX-capable > filesystems (ext4 and xfs) perform such global mappings. And known > device drivers that would handle devmap pages are not using kmap(). Any > future filesystems that gain DAX support, or device drivers wanting to > support devmap protected pages will need to use kmap_local_page(). > > Direct-map exposure is already mitigated by default on HIGHMEM systems > because by definition HIGHMEM systems do not have large capacities of > memory in the direct map. And using kmap in those systems actually > creates a separate mapping. Therefore, to reduce complexity HIGHMEM > systems are not supported.
It was only at the end of this paragraph did I understand why I was reading this paragraph. The change in topic was buried. I.e.
---
Note: HIGHMEM support is mutually exclusive with PGMAP protection. The rationale is mainly to reduce complexity, but also because direct-map exposure is already mitigated by default on HIGHMEM systems because by definition HIGHMEM systems do not have large capacities of memory in the direct map...
---
That note and related change should probably go in the same patch that introduces CONFIG_DEVMAP_ACCESS_PROTECTION in the first place. It's an unrelated change to instrumenting kmap() to fail early, which again I don't think is strictly necessary.
> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Dave Hansen <dave.hansen@intel.com> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > Changes for V8 > Reword commit message > --- > include/linux/highmem-internal.h | 5 +++++ > mm/Kconfig | 1 + > 2 files changed, 6 insertions(+) > > diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h > index 0a0b2b09b1b8..1a006558734c 100644 > --- a/include/linux/highmem-internal.h > +++ b/include/linux/highmem-internal.h > @@ -159,6 +159,7 @@ static inline struct page *kmap_to_page(void *addr) > static inline void *kmap(struct page *page) > { > might_sleep(); > + pgmap_protection_flag_invalid(page); > return page_address(page); > } > > @@ -174,6 +175,7 @@ static inline void kunmap(struct page *page) > > static inline void *kmap_local_page(struct page *page) > { > + pgmap_mk_readwrite(page); > return page_address(page); > } > > @@ -197,6 +199,7 @@ static inline void __kunmap_local(void *addr) > #ifdef ARCH_HAS_FLUSH_ON_KUNMAP > kunmap_flush_on_unmap(addr); > #endif > + pgmap_mk_noaccess(kmap_to_page(addr)); > } > > static inline void *kmap_atomic(struct page *page) > @@ -206,6 +209,7 @@ static inline void *kmap_atomic(struct page *page) > else > preempt_disable(); > pagefault_disable(); > + pgmap_mk_readwrite(page); > return page_address(page); > } > > @@ -224,6 +228,7 @@ static inline void __kunmap_atomic(void *addr) > #ifdef ARCH_HAS_FLUSH_ON_KUNMAP > kunmap_flush_on_unmap(addr); > #endif > + pgmap_mk_noaccess(kmap_to_page(addr)); > pagefault_enable(); > if (IS_ENABLED(CONFIG_PREEMPT_RT)) > migrate_enable(); > diff --git a/mm/Kconfig b/mm/Kconfig > index 67e0264acf7d..d537679448ae 100644 > --- a/mm/Kconfig > +++ b/mm/Kconfig > @@ -779,6 +779,7 @@ config ZONE_DEVICE > config DEVMAP_ACCESS_PROTECTION > bool "Access protection for memremap_pages()" > depends on NVDIMM_PFN > + depends on !HIGHMEM > depends on ARCH_HAS_SUPERVISOR_PKEYS > select ARCH_ENABLE_SUPERVISOR_PKEYS > default y > -- > 2.31.1 >
| |