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.
>>> + 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).
Awesome, I just wanted to check =)
>>> 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.
>>> +#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.
>>> 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.
>>> void do_privop(struct pt_regs *regs)
>>> {
>>> enum ctx_state prev_state = exception_enter();
Thanks,
--
Julian Calaby
Email: julian.calaby@xxxxxxxxx
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
