Hi Khalid,

On Thu, Mar 3, 2016 at 11:25 AM, Khalid Aziz [off-list ref] wrote:

Thanks, Julian! I really appreciate your feedback.

No problem!

My comments below.

On 03/02/2016 04:08 PM, Julian Calaby wrote:

quoted

Hi Khalid,

On Thu, Mar 3, 2016 at 7:39 AM, Khalid Aziz [off-list ref]
wrote:

quoted

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.

quoted

quoted

Signed-off-by: Khalid Aziz <redacted>
---
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. =)

quoted

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.

quoted

  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.

Why don't you want people disabling it? I must acknowledge that it's
not a lot of code, but I can see people wanting to build "minimal"
kernels for processors without ADI or to run some specific thing that
doesn't use ADI. Providing the kernel responds appropriately if
there's an unexpected ADI fault I don't see why the code would be
needed if it'll never be used.

quoted

quoted

+       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.

quoted

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).

Awesome, I just wanted to check =)

quoted

quoted

  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.

Inlines can usually be treated as if they're macros from an optimising
perspective.

quoted

quoted

+#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.

Precisely.

quoted

quoted

  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.

Thinking over it again, you're right. It's possible, however unlikely,
that this trap could occur without ADI being enabled anywhere, so we
should handle it unconditionally.

quoted

quoted

  void do_privop(struct pt_regs *regs)
  {
         enum ctx_state prev_state = exception_enter();

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>