Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754572AbcCBXIt (ORCPT ); Wed, 2 Mar 2016 18:08:49 -0500 Received: from mail-io0-f193.google.com ([209.85.223.193]:34683 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751725AbcCBXIp (ORCPT ); Wed, 2 Mar 2016 18:08:45 -0500 MIME-Version: 1.0 In-Reply-To: <1456951177-23579-1-git-send-email-khalid.aziz@oracle.com> References: <1456951177-23579-1-git-send-email-khalid.aziz@oracle.com> From: Julian Calaby Date: Thu, 3 Mar 2016 10:08:19 +1100 Message-ID: Subject: Re: [PATCH v2] sparc64: Add support for Application Data Integrity (ADI) To: Khalid Aziz Cc: "David S. Miller" , corbet@lwn.net, Andrew Morton , dingel@linux.vnet.ibm.com, zhenzhang.zhang@huawei.com, bob.picco@oracle.com, kirill.shutemov@linux.intel.com, aneesh.kumar@linux.vnet.ibm.com, aarcange@redhat.com, Arnd Bergmann , sparclinux , rob.gardner@oracle.com, mhocko@suse.cz, chris.hyser@oracle.com, richard@nod.at, vbabka@suse.cz, koct9i@gmail.com, oleg@redhat.com, gthelen@google.com, jack@suse.cz, xiexiuqi@huawei.com, Vineet.Gupta1@synopsys.com, Andy Lutomirski , ebiederm@xmission.com, bsegall@google.com, Geert Uytterhoeven , dave@stgolabs.net, Alexey Dobriyan , linux-doc@vger.kernel.org, "linux-kernel@vger.kernel.org" , linux-mm@kvack.org, linux-arch@vger.kernel.org, linux-api@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12551 Lines: 355 Hi Khalid, On Thu, Mar 3, 2016 at 7:39 AM, Khalid Aziz 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.) > Signed-off-by: Khalid Aziz > --- > 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. > + 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. 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. > 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. > +#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? > 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? > 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, -- Julian Calaby Email: julian.calaby@gmail.com Profile: http://www.google.com/profiles/julian.calaby/