| Message ID | 20190129003422.9328-14-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Merge text_poke fixes and executable lockdowns | expand |
On Mon, Jan 28, 2019 at 04:34:15PM -0800, Rick Edgecombe wrote: > This adds two new functions set_alias_default_noflush and s/This adds/Add/ > set_alias_nv_noflush for setting the alias mapping for the page to its Please end function names with parentheses, below too. > default valid permissions and to an invalid state that cannot be cached in > a TLB, respectively. These functions to not flush the TLB. s/to/do/ Also, pls put that description as comments over the functions in the code. Otherwise that "nv" as part of the name doesn't really explain what it does. Actually, you could just as well call the function set_alias_invalid_noflush() All the other words are written in full, no need to have "nv" there. Thx.
On Mon, 2019-02-11 at 20:09 +0100, Borislav Petkov wrote: > On Mon, Jan 28, 2019 at 04:34:15PM -0800, Rick Edgecombe wrote: > > This adds two new functions set_alias_default_noflush and > > s/This adds/Add/ > > > set_alias_nv_noflush for setting the alias mapping for the page to its > > Please end function names with parentheses, below too. Ok. > > default valid permissions and to an invalid state that cannot be cached in > > a TLB, respectively. These functions to not flush the TLB. > > s/to/do/ > Argh, thanks. > Also, pls put that description as comments over the functions in the > code. Otherwise that "nv" as part of the name doesn't really explain > what it does. > > Actually, you could just as well call the function > > set_alias_invalid_noflush() > > All the other words are written in full, no need to have "nv" there. > > Thx. Yes, that seems better. Thanks, Rick
On Mon, Feb 11, 2019 at 11:09 AM Borislav Petkov <bp@alien8.de> wrote: > > On Mon, Jan 28, 2019 at 04:34:15PM -0800, Rick Edgecombe wrote: > > This adds two new functions set_alias_default_noflush and > > s/This adds/Add/ > > > set_alias_nv_noflush for setting the alias mapping for the page to its > > Please end function names with parentheses, below too. > > > default valid permissions and to an invalid state that cannot be cached in > > a TLB, respectively. These functions to not flush the TLB. > > s/to/do/ > > Also, pls put that description as comments over the functions in the > code. Otherwise that "nv" as part of the name doesn't really explain > what it does. > > Actually, you could just as well call the function > > set_alias_invalid_noflush() > > All the other words are written in full, no need to have "nv" there. Why are you calling this an "alias"? You're modifying the direct map. Your patches are thinking of the direct map as an alias of the vmap mapping, but that does seem a bit backwards. How about set_direct_map_invalid_noflush(), etc? --Andy
On Mon, 2019-02-11 at 14:59 -0800, Andy Lutomirski wrote: > On Mon, Feb 11, 2019 at 11:09 AM Borislav Petkov <bp@alien8.de> wrote: > > > > On Mon, Jan 28, 2019 at 04:34:15PM -0800, Rick Edgecombe wrote: > > > This adds two new functions set_alias_default_noflush and > > > > s/This adds/Add/ > > > > > set_alias_nv_noflush for setting the alias mapping for the page to its > > > > Please end function names with parentheses, below too. > > > > > default valid permissions and to an invalid state that cannot be cached in > > > a TLB, respectively. These functions to not flush the TLB. > > > > s/to/do/ > > > > Also, pls put that description as comments over the functions in the > > code. Otherwise that "nv" as part of the name doesn't really explain > > what it does. > > > > Actually, you could just as well call the function > > > > set_alias_invalid_noflush() > > > > All the other words are written in full, no need to have "nv" there. > > Why are you calling this an "alias"? You're modifying the direct map. > Your patches are thinking of the direct map as an alias of the vmap > mapping, but that does seem a bit backwards. How about > set_direct_map_invalid_noflush(), etc? > I picked it up from some of the names in arch/x86/mm/pageattr.c: CPA_NO_CHECK_ALIAS, set_memory_np_noalias(), etc. In that file the directmap address seems to be the "alias". For 32 bit with highmem though, this would also set permissions for a kmap mapping as well (if one existed), since that address will be returned from page_address(). Yea, in vmalloc, vm_unmap_aliases talks about the vmap address "alias". So I guess calling it "alias" is ambiguous. But does set_direct_map_invalid_noflush make sense in the highmem case? I couldn't think of any names that I loved, which is why I ran the set_alias_*_noflush names by people in an earlier version, although looking back only Ard chimed in on that. "set_direct_map_invalid_noflush" is fine with me if nobody objects. Thanks, Rick
diff --git a/arch/Kconfig b/arch/Kconfig index 4cfb6de48f79..4ef9db190f2d 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -249,6 +249,10 @@ config ARCH_HAS_FORTIFY_SOURCE config ARCH_HAS_SET_MEMORY bool +# Select if arch has all set_alias_nv/default() functions +config ARCH_HAS_SET_ALIAS + bool + # Select if arch init_task must go in the __init_task_data section config ARCH_TASK_STRUCT_ON_STACK bool diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 26387c7bf305..42bb1df4ea94 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -66,6 +66,7 @@ config X86 select ARCH_HAS_UACCESS_FLUSHCACHE if X86_64 select ARCH_HAS_UACCESS_MCSAFE if X86_64 && X86_MCE select ARCH_HAS_SET_MEMORY + select ARCH_HAS_SET_ALIAS select ARCH_HAS_STRICT_KERNEL_RWX select ARCH_HAS_STRICT_MODULE_RWX select ARCH_HAS_SYNC_CORE_BEFORE_USERMODE diff --git a/arch/x86/include/asm/set_memory.h b/arch/x86/include/asm/set_memory.h index 07a25753e85c..2ef4e4222df1 100644 --- a/arch/x86/include/asm/set_memory.h +++ b/arch/x86/include/asm/set_memory.h @@ -85,6 +85,9 @@ int set_pages_nx(struct page *page, int numpages); int set_pages_ro(struct page *page, int numpages); int set_pages_rw(struct page *page, int numpages); +int set_alias_nv_noflush(struct page *page); +int set_alias_default_noflush(struct page *page); + extern int kernel_set_to_readonly; void set_kernel_text_rw(void); void set_kernel_text_ro(void); diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c index 4f8972311a77..3a51915a1410 100644 --- a/arch/x86/mm/pageattr.c +++ b/arch/x86/mm/pageattr.c @@ -2209,8 +2209,6 @@ int set_pages_rw(struct page *page, int numpages) return set_memory_rw(addr, numpages); } -#ifdef CONFIG_DEBUG_PAGEALLOC - static int __set_pages_p(struct page *page, int numpages) { unsigned long tempaddr = (unsigned long) page_address(page); @@ -2249,6 +2247,17 @@ static int __set_pages_np(struct page *page, int numpages) return __change_page_attr_set_clr(&cpa, 0); } +int set_alias_nv_noflush(struct page *page) +{ + return __set_pages_np(page, 1); +} + +int set_alias_default_noflush(struct page *page) +{ + return __set_pages_p(page, 1); +} + +#ifdef CONFIG_DEBUG_PAGEALLOC void __kernel_map_pages(struct page *page, int numpages, int enable) { if (PageHighMem(page)) @@ -2282,7 +2291,6 @@ void __kernel_map_pages(struct page *page, int numpages, int enable) } #ifdef CONFIG_HIBERNATION - bool kernel_page_present(struct page *page) { unsigned int level; diff --git a/include/linux/set_memory.h b/include/linux/set_memory.h index 2a986d282a97..d19481ac6a8f 100644 --- a/include/linux/set_memory.h +++ b/include/linux/set_memory.h @@ -10,6 +10,16 @@ #ifdef CONFIG_ARCH_HAS_SET_MEMORY #include <asm/set_memory.h> +#ifndef CONFIG_ARCH_HAS_SET_ALIAS +static inline int set_alias_nv_noflush(struct page *page) +{ + return 0; +} +static inline int set_alias_default_noflush(struct page *page) +{ + return 0; +} +#endif #else static inline int set_memory_ro(unsigned long addr, int numpages) { return 0; } static inline int set_memory_rw(unsigned long addr, int numpages) { return 0; }
This adds two new functions set_alias_default_noflush and set_alias_nv_noflush for setting the alias mapping for the page to its default valid permissions and to an invalid state that cannot be cached in a TLB, respectively. These functions to not flush the TLB. Note, __kernel_map_pages does something similar but flushes the TLB and doesn't reset the permission bits to default on all architectures. There is also an ARCH config ARCH_HAS_SET_ALIAS for specifying whether these have an actual implementation or a default empty one. Cc: Dave Hansen <dave.hansen@linux.intel.com> Cc: Andy Lutomirski <luto@kernel.org> Cc: Peter Zijlstra <peterz@infradead.org> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> --- arch/Kconfig | 4 ++++ arch/x86/Kconfig | 1 + arch/x86/include/asm/set_memory.h | 3 +++ arch/x86/mm/pageattr.c | 14 +++++++++++--- include/linux/set_memory.h | 10 ++++++++++ 5 files changed, 29 insertions(+), 3 deletions(-)