Messages in this thread Patch in this message | | | From | Dan Williams <> | Date | Fri, 4 Feb 2022 11:01:55 -0800 | Subject | Re: [PATCH V8 39/44] memremap_pages: Add memremap.pks_fault_mode |
| |
On Thu, Jan 27, 2022 at 9:55 AM <ira.weiny@intel.com> wrote: > > From: Ira Weiny <ira.weiny@intel.com> > > Some systems may be using pmem in unanticipated ways. As such, it is > possible an foreseen code path to violate the restrictions of the PMEM > PKS protections.
These sentences do not parse for me. How about:
"When PKS protections for PMEM are enabled the kernel may capture stray writes, or it may capture false positive access violations. An example of a false positive access violation is a code path that neglects to call kmap_{atomic,local_page}, but is otherwise a valid access. In the false positive scenario there is no actual risk to data integrity, but the kernel still needs to make a decision as to whether to report the access violation and continue, or treat the violation as fatal. That policy decision is captured in a new pks_fault_mode kernel parameter."
> > In order to provide a more seamless integration of the PMEM PKS feature
Not sure what "seamless integration" means in this context?
> provide a pks_fault_mode that allows for a relaxed mode should a > previously working feature fault on the PKS protected PMEM. > > 2 modes are available: > > 'relaxed' (default) -- WARN_ONCE, removed the protections, and > continuing to operate. > > 'strict' -- BUG_ON/or fault indicating the error. This is the > most protective of the PMEM memory but may be undesirable in > some configurations. > > NOTE: The typedef of pks_fault_modes is required to allow > param_check_pks_fault() to work automatically for us. So the typedef > checkpatch warning is ignored.
This doesn't parse for me, why is a typedef needed for a simple toggle? Who is "us"?
> > NOTE: There was some debate about if a 3rd mode called 'silent' should > be available. 'silent' would be the same as 'relaxed' but not print any > output. While 'silent' is nice for admins to reduce console/log output > it would result in less motivation to fix invalid access to the > protected pmem pages. Therefore, 'silent' is left out. > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > Changes for V8 > Use pks_update_exception() instead of abandoning the pkey. > Split out pgmap_protection_flag_invalid() into a separate patch > for clarity. > From Rick Edgecombe > Fix sysfs_streq() checks > From Randy Dunlap > Fix Documentation closing parans > > Changes for V7 > Leverage Rick Edgecombe's fault callback infrastructure to relax invalid > uses and prevent crashes > From Dan Williams > Use sysfs_* calls for parameter > Make pgmap_disable_protection inline > Remove pfn from warn output > Remove silent parameter option > --- > .../admin-guide/kernel-parameters.txt | 14 ++++ > arch/x86/mm/pkeys.c | 4 ++ > include/linux/mm.h | 3 + > mm/memremap.c | 67 +++++++++++++++++++ > 4 files changed, 88 insertions(+) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index f5a27f067db9..3e70a6194831 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -4158,6 +4158,20 @@ > pirq= [SMP,APIC] Manual mp-table setup > See Documentation/x86/i386/IO-APIC.rst. > > + memremap.pks_fault_mode= [X86] Control the behavior of page map > + protection violations. Violations may not be an actual > + use of the memory but simply an attempt to map it in an > + incompatible way. > + (depends on CONFIG_DEVMAP_ACCESS_PROTECTION) > + > + Format: { relaxed | strict } > + > + relaxed - Print a warning, disable the protection and > + continue execution. > + strict - Stop kernel execution via BUG_ON or fault > + > + default: relaxed > + > plip= [PPT,NET] Parallel port network link > Format: { parport<nr> | timid | 0 } > See also Documentation/admin-guide/parport.rst. > diff --git a/arch/x86/mm/pkeys.c b/arch/x86/mm/pkeys.c > index fa71037c1dd0..e864a9b7828a 100644 > --- a/arch/x86/mm/pkeys.c > +++ b/arch/x86/mm/pkeys.c > @@ -6,6 +6,7 @@ > #include <linux/debugfs.h> /* debugfs_create_u32() */ > #include <linux/mm_types.h> /* mm_struct, vma, etc... */ > #include <linux/pkeys.h> /* PKEY_* */ > +#include <linux/mm.h> /* fault callback */ > #include <uapi/asm-generic/mman-common.h> > > #include <asm/cpufeature.h> /* boot_cpu_has, ... */ > @@ -243,6 +244,9 @@ static const pks_key_callback pks_key_callbacks[PKS_KEY_NR_CONSUMERS] = { > #ifdef CONFIG_PKS_TEST > [PKS_KEY_TEST] = pks_test_fault_callback, > #endif > +#ifdef CONFIG_DEVMAP_ACCESS_PROTECTION > + [PKS_KEY_PGMAP_PROTECTION] = pgmap_pks_fault_callback, > +#endif > }; > > static bool pks_call_fault_callback(struct pt_regs *regs, unsigned long address, > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 60044de77c54..e900df563437 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1193,6 +1193,9 @@ static inline void pgmap_mk_noaccess(struct page *page) > > bool pgmap_protection_available(void); > > +bool pgmap_pks_fault_callback(struct pt_regs *regs, unsigned long address, > + bool write); > + > #else > > static inline void __pgmap_mk_readwrite(struct dev_pagemap *pgmap) { } > diff --git a/mm/memremap.c b/mm/memremap.c > index b75c4f778c59..783b1cd4bb42 100644 > --- a/mm/memremap.c > +++ b/mm/memremap.c > @@ -96,6 +96,73 @@ static void devmap_protection_disable(void) > static_branch_dec(&dev_pgmap_protection_static_key); > } > > +/* > + * Ignore the checkpatch warning because the typedef allows
Why document forever in perpetuity to ignore a checkpatch warning for something that is no longer a patch once it is upstream?
> + * param_check_pks_fault_modes to automatically check the passed value. > + */ > +typedef enum { > + PKS_MODE_STRICT = 0, > + PKS_MODE_RELAXED = 1, > +} pks_fault_modes; > + > +pks_fault_modes pks_fault_mode = PKS_MODE_RELAXED; > + > +static int param_set_pks_fault_mode(const char *val, const struct kernel_param *kp) > +{ > + int ret = -EINVAL; > + > + if (sysfs_streq(val, "relaxed")) { > + pks_fault_mode = PKS_MODE_RELAXED; > + ret = 0; > + } else if (sysfs_streq(val, "strict")) { > + pks_fault_mode = PKS_MODE_STRICT; > + ret = 0; > + } > + > + return ret; > +} > + > +static int param_get_pks_fault_mode(char *buffer, const struct kernel_param *kp) > +{ > + int ret = 0; > + > + switch (pks_fault_mode) { > + case PKS_MODE_STRICT: > + ret = sysfs_emit(buffer, "strict\n"); > + break; > + case PKS_MODE_RELAXED: > + ret = sysfs_emit(buffer, "relaxed\n"); > + break; > + default: > + ret = sysfs_emit(buffer, "<unknown>\n"); > + break; > + } > + > + return ret; > +} > + > +static const struct kernel_param_ops param_ops_pks_fault_modes = { > + .set = param_set_pks_fault_mode, > + .get = param_get_pks_fault_mode, > +}; > + > +#define param_check_pks_fault_modes(name, p) \ > + __param_check(name, p, pks_fault_modes) > +module_param(pks_fault_mode, pks_fault_modes, 0644);
Is the complexity to change this at runtime necessary? It seems sufficient to make this read-only via sysfs and only rely on command line toggles to override the default policy.
> + > +bool pgmap_pks_fault_callback(struct pt_regs *regs, unsigned long address, > + bool write) > +{ > + /* In strict mode just let the fault handler oops */ > + if (pks_fault_mode == PKS_MODE_STRICT) > + return false; > + > + WARN_ONCE(1, "Page map protection being disabled"); > + pks_update_exception(regs, PKS_KEY_PGMAP_PROTECTION, 0); > + return true; > +} > +EXPORT_SYMBOL_GPL(pgmap_pks_fault_callback); > + > void __pgmap_mk_readwrite(struct dev_pagemap *pgmap) > { > if (!current->pgmap_prot_count++) > -- > 2.31.1 >
| |