Re: [PATCH v2] sparc64: Add support for Application Data Integrity (ADI)
From: Khalid Aziz
Date: Thu Mar 03 2016 - 12:29:46 EST
On 03/02/2016 05:48 PM, Julian Calaby wrote:
Hi Khalid,
On Thu, Mar 3, 2016 at 11:25 AM, Khalid Aziz <khalid.aziz@xxxxxxxxxx> wrote:
Thanks, Julian! I really appreciate your feedback.
No problem!
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@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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.
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.
Hi Julian,
My goal in making CONFIG_SPARC_ADI auto-selected was to not add yet
another config option that end user has to understand and figure out
what to do with, and make the kernel self-configuring where ADI simply
becomes available if platform supports it. Kernel auto-detecting
platform features is especially useful for distro kernels. I do see your
point in being able to build a minimal kernel when building a custom
kernel. Both options of making CONFIG_SPARC_ADI auto-selected or not,
have pros and cons. I don't have a strong feeling about it one way or
the other and can go either way.
Thanks,
Khalid