Messages in this thread | | | Subject | Re: [PATCH v2] sparc64: Add support for Application Data Integrity (ADI) | From | Khalid Aziz <> | Date | Wed, 2 Mar 2016 17:25:21 -0700 |
| |
Thanks, Julian! I really appreciate your feedback.
My comments below.
On 03/02/2016 04:08 PM, Julian Calaby wrote: > Hi Khalid, > > On Thu, Mar 3, 2016 at 7:39 AM, Khalid Aziz <khalid.aziz@oracle.com> wrote: >> >> Enable Application Data Integrity (ADI) support in the sparc >> kernel for applications to use ADI in userspace. ADI is a new >> feature supported on sparc M7 and newer processors. ADI is supported >> for data fetches only and not instruction fetches. This patch adds >> prctl commands to enable and disable ADI (TSTATE.mcde), return ADI >> parameters to userspace, enable/disable MCD (Memory Corruption >> Detection) on selected memory ranges and enable TTE.mcd in PTEs. It >> also adds handlers for all traps related to MCD. ADI is not enabled >> by default for any task and a task must explicitly enable ADI >> (TSTATE.mcde), turn MCD on on a memory range and set version tag >> for ADI to be effective for the task. This patch adds support for >> ADI for hugepages only. Addresses passed into system calls must be >> non-ADI tagged addresses. > > I can't comment on the actual functionality here, but I do see a few > minor style issues in your patch. > > My big concern is that you're defining a lot of new code that is ADI > specific but isn't inside a CONFIG_SPARC_ADI ifdef. (That said, > handling ADI specific traps if ADI isn't enabled looks like a good > idea to me, however most of the other stuff is just dead code if > CONFIG_SPARC_ADI isn't enabled.)
Some of the code will be executed when CONFIG_SPARC_ADI is not enabled, for instance init_adi() which will parse machine description to determine if platform supports ADI. On the other hand, it might still make sense to enclose this code in #ifdef. More on that below.
> >> Signed-off-by: Khalid Aziz <khalid.aziz@oracle.com> >> --- >> NOTES: ADI is a new feature added to M7 processor to allow hardware >> to catch rogue accesses to memory. An app can enable ADI on >> its data pages, set version tags on them and use versioned >> addresses (bits 63-60 of the address contain a version tag) >> to access the data pages. If a rogue app attempts to access >> ADI enabled data pages, its access is blocked and processor >> generates an exception. Enabling this functionality for all >> data pages of an app requires adding infrastructure to save >> version tags for any data pages that get swapped out and >> restoring those tags when pages are swapped back in. In this >> first implementation I am enabling ADI for hugepages only >> since these pages are locked in memory and hence avoid the >> issue of saving and restoring tags. Once this core functionality >> is stable, ADI for other memory pages can be enabled more >> easily. >> >> v2: >> - Fixed a build error >> >> Documentation/prctl/sparc_adi.txt | 62 ++++++++++ >> Documentation/sparc/adi.txt | 206 +++++++++++++++++++++++++++++++ >> arch/sparc/Kconfig | 12 ++ >> arch/sparc/include/asm/hugetlb.h | 14 +++ >> arch/sparc/include/asm/hypervisor.h | 2 + >> arch/sparc/include/asm/mmu_64.h | 1 + >> arch/sparc/include/asm/pgtable_64.h | 15 +++ >> arch/sparc/include/asm/processor_64.h | 19 +++ >> arch/sparc/include/asm/ttable.h | 10 ++ >> arch/sparc/include/uapi/asm/asi.h | 3 + >> arch/sparc/include/uapi/asm/pstate.h | 10 ++ >> arch/sparc/kernel/entry.h | 3 + >> arch/sparc/kernel/head_64.S | 1 + >> arch/sparc/kernel/mdesc.c | 81 +++++++++++++ >> arch/sparc/kernel/process_64.c | 222 ++++++++++++++++++++++++++++++++++ >> arch/sparc/kernel/sun4v_mcd.S | 16 +++ >> arch/sparc/kernel/traps_64.c | 96 ++++++++++++++- >> arch/sparc/kernel/ttable_64.S | 6 +- >> include/linux/mm.h | 2 + >> include/uapi/asm-generic/siginfo.h | 5 +- >> include/uapi/linux/prctl.h | 16 +++ >> kernel/sys.c | 30 +++++ >> 22 files changed, 826 insertions(+), 6 deletions(-) >> create mode 100644 Documentation/prctl/sparc_adi.txt >> create mode 100644 Documentation/sparc/adi.txt >> create mode 100644 arch/sparc/kernel/sun4v_mcd.S > > I must admit that I'm slightly impressed that the documentation is > over a quarter of the lines added. =) > >> diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig >> index 56442d2..0aac0ae 100644 >> --- a/arch/sparc/Kconfig >> +++ b/arch/sparc/Kconfig >> @@ -80,6 +80,7 @@ config SPARC64 >> select NO_BOOTMEM >> select HAVE_ARCH_AUDITSYSCALL >> select ARCH_SUPPORTS_ATOMIC_RMW >> + select SPARC_ADI > > This doesn't look right. > >> config ARCH_DEFCONFIG >> string >> @@ -314,6 +315,17 @@ if SPARC64 >> source "kernel/power/Kconfig" >> endif >> >> +config SPARC_ADI >> + bool "Application Data Integrity support" >> + def_bool y if SPARC64 > > def_bool is for config options without names (i.e. "this is a boolean > value and it's default is...") > > So if you want people to be able to disable this option, then you > should remove the select above and just have: > > bool "Application Data Integrity support" > default y if SPARC64 > > If you don't want people disabling it, then there's no point in having > a separate Kconfig symbol. >
Ah, I see. I do not want people disabling it. I will make changes.
>> + help >> + Support for Application Data Integrity (ADI). ADI feature allows >> + a process to tag memory blocks with version tags. Once ADI is >> + enabled and version tag is set on a memory block, any access to >> + it is allowed only if the correct version tag is presented by >> + a process. This feature is meant to help catch rogue accesses >> + to memory. >> + > > You should probably mention that it's only available on newer > processors and recommend that it's enabled on them.
Good point.
> > This code won't break anything on older processors, right? I haven't > looked very closely, but I don't see anything that specifically > disables the code if it's run on, say, a UltraSparc I.
Right, this code does not break anything on older processors and has been tested on older machines. init_adi() will detect the platform does not support ADI when it parses machine description and will leave ADI disabled in that case (adi_state.enabled=false).
> >> config SCHED_SMT >> bool "SMT (Hyperthreading) scheduler support" >> depends on SPARC64 && SMP >> diff --git a/arch/sparc/include/asm/processor_64.h b/arch/sparc/include/asm/processor_64.h >> index 6924bde..9a71701 100644 >> --- a/arch/sparc/include/asm/processor_64.h >> +++ b/arch/sparc/include/asm/processor_64.h >> @@ -97,6 +97,25 @@ struct thread_struct { >> struct task_struct; >> unsigned long thread_saved_pc(struct task_struct *); >> >> +#ifdef CONFIG_SPARC_ADI >> +extern struct adi_caps *get_adi_caps(void); >> +extern long get_sparc_adicaps(unsigned long); >> +extern long set_sparc_pstate_mcde(unsigned long); >> +extern long enable_sparc_adi(unsigned long, unsigned long); >> +extern long disable_sparc_adi(unsigned long, unsigned long); >> +extern long get_sparc_adi_status(unsigned long); >> +extern bool adi_capable(void); >> + >> +#define GET_SPARC_ADICAPS(a) get_sparc_adicaps(a) >> +#define SET_SPARC_MCDE(a) set_sparc_pstate_mcde(a) >> +#define ENABLE_SPARC_ADI(a, b) enable_sparc_adi(a, b) >> +#define DISABLE_SPARC_ADI(a, b) disable_sparc_adi(a, b) >> +#define GET_SPARC_ADI_STATUS(a) get_sparc_adi_status(a) >> +#define ADI_CAPABLE() adi_capable() > > Get rid of the ADI_CAPABLE macro, the usual pattern here is to define > a static inline function for the entire API when the symbol is > disabled, i.e. > > #ifdef CONFIG_SPARC_ADI > ... > extern bool adi_capable(void); > #else > ... > static inline bool adi_capable(void) { > return false; > } > #endif > > That way you get type checking on the arguments even if the option is > disabled and modern compilers are smart enough to optimise all the > no-op code away. (Not that the type checking is needed here.) > > Also, in all but one place you use the ADI_CAPABLE() macro when the > adi_capable() function is defined and available.
I defined ADI_CAPABLE() 0 for the case when CONFIG_SPARC_ADI is not set to help compiler optimize sun4v_mem_corrupt_detect_precise(). Since sun4v_mem_corrupt_detect_precise() is exception handler, optimizing it can be good for performance but perhaps compiler is smart enough to do that any way if adi_capable() is defined inline as you show above? I do like that doing it this way retains type checking.
> >> +#else >> +#define ADI_CAPABLE() 0 >> +#endif >> + >> /* On Uniprocessor, even in RMO processes see TSO semantics */ >> #ifdef CONFIG_SMP >> #define TSTATE_INITIAL_MM TSTATE_TSO >> diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c >> index 6f80936..79f981c 100644 >> --- a/arch/sparc/kernel/mdesc.c >> +++ b/arch/sparc/kernel/mdesc.c >> @@ -1007,6 +1013,80 @@ static int mdesc_open(struct inode *inode, struct file *file) >> return 0; >> } >> >> +bool adi_capable(void) >> +{ >> + return adi_state.enabled; >> +} >> + >> +struct adi_caps *get_adi_caps(void) >> +{ >> + return &adi_state.caps; >> +} >> + >> +void __init >> +init_adi(void) >> +{ >> + struct mdesc_handle *hp = mdesc_grab(); >> + const char *prop; >> + u64 pn, *val; >> + int len; >> + >> + adi_state.enabled = false; >> + >> + if (!hp) >> + return; >> + >> + pn = mdesc_node_by_name(hp, MDESC_NODE_NULL, "cpu"); >> + if (pn == MDESC_NODE_NULL) >> + goto out; >> + >> + prop = mdesc_get_property(hp, pn, "hwcap-list", &len); >> + if (!prop) >> + goto out; >> + >> + /* >> + * Look for "adp" keyword in hwcap-list which would indicate >> + * ADI support >> + */ >> + while (len) { >> + int plen; >> + >> + if (!strcmp(prop, "adp")) { >> + adi_state.enabled = true; >> + break; >> + } >> + >> + plen = strlen(prop) + 1; >> + prop += plen; >> + len -= plen; >> + } >> + >> + if (!adi_state.enabled) >> + goto out; >> + >> + pn = mdesc_node_by_name(hp, MDESC_NODE_NULL, "platform"); >> + if (pn == MDESC_NODE_NULL) >> + goto out; >> + >> + val = (u64 *) mdesc_get_property(hp, pn, "adp-blksz", &len); >> + if (!val) >> + goto out; >> + adi_state.caps.blksz = *val; >> + >> + val = (u64 *) mdesc_get_property(hp, pn, "adp-nbits", &len); >> + if (!val) >> + goto out; >> + adi_state.caps.nbits = *val; >> + >> + val = (u64 *) mdesc_get_property(hp, pn, "ue-on-adp", &len); >> + if (!val) >> + goto out; >> + adi_state.caps.ue_on_adi = *val; >> + >> +out: >> + mdesc_release(hp); >> +} >> + > > Should all the ADI related functions above be within a #ifdef CONFIG_SPARC_ADI? >
CONFIG_SPARC_ADI is selected for 64-bit kernels only since M7 is 64-bit only. init_adi() will do the right thing whether CONFIG_SPARC_ADI is enabled or not. It will parse machine description on 32-bit kernels, detect ADI is not supported by the platform and leave adi_state.enabled=false. I was considering adding something like /proc/sys/vm/sparc_adi_available at later point which would get its data from what init_adi() detects. On the other hand, since 32-bit processors do not support ADI, why have even this much code run on them. I can enclose this code as well inside #ifdef.
>> static ssize_t mdesc_read(struct file *file, char __user *buf, >> size_t len, loff_t *offp) >> { >> diff --git a/arch/sparc/kernel/traps_64.c b/arch/sparc/kernel/traps_64.c >> index d21cd62..29db583 100644 >> --- a/arch/sparc/kernel/traps_64.c >> +++ b/arch/sparc/kernel/traps_64.c >> @@ -2531,6 +2589,38 @@ void sun4v_do_mna(struct pt_regs *regs, unsigned long addr, unsigned long type_c >> force_sig_info(SIGBUS, &info, current); >> } >> >> +void sun4v_mem_corrupt_detect_precise(struct pt_regs *regs, unsigned long addr, >> + unsigned long context) >> +{ >> + siginfo_t info; >> + >> + if (!ADI_CAPABLE()) { >> + bad_trap(regs, 0x1a); >> + return; >> + } >> + >> + if (notify_die(DIE_TRAP, "memory corruption precise exception", regs, >> + 0, 0x8, SIGSEGV) == NOTIFY_STOP) >> + return; >> + >> + if (regs->tstate & TSTATE_PRIV) { >> + pr_emerg("sun4v_mem_corrupt_detect_precise: ADDR[%016lx] " >> + "CTX[%lx], going.\n", addr, context); >> + die_if_kernel("MCD precise", regs); >> + } >> + >> + if (test_thread_flag(TIF_32BIT)) { >> + regs->tpc &= 0xffffffff; >> + regs->tnpc &= 0xffffffff; >> + } >> + info.si_signo = SIGSEGV; >> + info.si_code = SEGV_ADIPERR; >> + info.si_errno = 0; >> + info.si_addr = (void __user *) addr; >> + info.si_trapno = 0; >> + force_sig_info(SIGSEGV, &info, current); >> +} >> + > > Should this be ifdef'd too?
I would prefer to leave exception handlers in place any way unless there are strong objections.
> >> void do_privop(struct pt_regs *regs) >> { >> enum ctx_state prev_state = exception_enter(); >> diff --git a/kernel/sys.c b/kernel/sys.c >> index 6af9212..fa7b5d9 100644 >> --- a/kernel/sys.c >> +++ b/kernel/sys.c >> @@ -103,6 +103,21 @@ >> #ifndef SET_FP_MODE >> # define SET_FP_MODE(a,b) (-EINVAL) >> #endif >> +#ifndef GET_SPARC_ADICAPS >> +# define GET_SPARC_ADICAPS(a) (-EINVAL) >> +#endif >> +#ifndef SET_SPARC_MCDE >> +# define SET_SPARC_MCDE(a) (-EINVAL) >> +#endif >> +#ifndef ENABLE_SPARC_ADI >> +# define ENABLE_SPARC_ADI(a, b) (-EINVAL) >> +#endif >> +#ifndef DISABLE_SPARC_ADI >> +# define DISABLE_SPARC_ADI(a, b) (-EINVAL) >> +#endif >> +#ifndef GET_SPARC_ADI_STATUS >> +# define GET_SPARC_ADI_STATUS(a) (-EINVAL) >> +#endif > > Ah, I was wondering why you were defining macros in processor_64.h. > >> /* >> * this is where the system-wide overflow UID and GID are defined, for > > I've got a couple more comments, I'll send another email with them shortly. > > Thanks, >
Thanks, Khalid
| |