[PATCH v2 0/3] powerpc/mce: Fix mce handler and add selftest

[PATCH v2 0/3] powerpc/mce: Fix mce handler and add selftest

[Ganesh Goudar]

This patch series fixes mce handling for pseries, Adds LKDTM test
for SLB multihit recovery and enables selftest for the same,
basically to test MCE handling on pseries/powernv machines running
in hash mmu mode.

v2:
* Remove in_nmi check before calling nmi_enter/exit,
  as nesting is supported.
* Fix build errors and remove unused variables.
* Integrate error injection code into LKDTM.
* Add support to inject multihit in paca.

Ganesh Goudar (3):
  powerpc/mce: remove nmi_enter/exit from real mode handler
  lkdtm/powerpc: Add SLB multihit test
  selftests/lkdtm: Enable selftest for SLB multihit

 arch/powerpc/kernel/mce.c               |  10 +-
 drivers/misc/lkdtm/Makefile             |   4 +
 drivers/misc/lkdtm/core.c               |   3 +
 drivers/misc/lkdtm/lkdtm.h              |   3 +
 drivers/misc/lkdtm/powerpc.c            | 132 ++++++++++++++++++++++++
 tools/testing/selftests/lkdtm/tests.txt |   1 +
 6 files changed, 149 insertions(+), 4 deletions(-)
 create mode 100644 drivers/misc/lkdtm/powerpc.c

-- 
2.26.2

[PATCH v2 1/3] powerpc/mce: remove nmi_enter/exit from real mode handler

[Ganesh Goudar]

Use of nmi_enter/exit in real mode handler causes the kernel to panic
and reboot on injecting slb mutihit on pseries machine running in hash
mmu mode, As these calls try to accesses memory outside RMO region in
real mode handler where translation is disabled.

Add check to not to use these calls on pseries machine running in hash
mmu mode.

Fixes: 116ac378bb3f ("powerpc/64s: machine check interrupt update NMI accounting")
Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
---
 arch/powerpc/kernel/mce.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/mce.c b/arch/powerpc/kernel/mce.c
index ada59f6c4298..3bf39dd5dd43 100644
--- a/arch/powerpc/kernel/mce.c
+++ b/arch/powerpc/kernel/mce.c
@@ -591,12 +591,14 @@ EXPORT_SYMBOL_GPL(machine_check_print_event_info);
 long notrace machine_check_early(struct pt_regs *regs)
 {
 	long handled = 0;
-	bool nested = in_nmi();
+	bool is_pseries_hpt_guest;
 	u8 ftrace_enabled = this_cpu_get_ftrace_enabled();
 
 	this_cpu_set_ftrace_enabled(0);
-
-	if (!nested)
+	is_pseries_hpt_guest = machine_is(pseries) &&
+			       mmu_has_feature(MMU_FTR_HPTE_TABLE);
+	/* Do not use nmi_enter/exit for pseries hpte guest */
+	if (!is_pseries_hpt_guest)
 		nmi_enter();
 
 	hv_nmi_check_nonrecoverable(regs);
@@ -607,7 +609,7 @@ long notrace machine_check_early(struct pt_regs *regs)
 	if (ppc_md.machine_check_early)
 		handled = ppc_md.machine_check_early(regs);
 
-	if (!nested)
+	if (!is_pseries_hpt_guest)
 		nmi_exit();
 
 	this_cpu_set_ftrace_enabled(ftrace_enabled);
-- 
2.26.2

[PATCH v2 2/3] lkdtm/powerpc: Add SLB multihit test

[Ganesh Goudar]

Add support to inject slb multihit errors, to test machine
check handling.

Based on work by Mahesh Salgaonkar and Michal Suchánek.

Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com>
Cc: Michal Suchánek <msuchanek@suse.de>
Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
---
 drivers/misc/lkdtm/Makefile  |   4 ++
 drivers/misc/lkdtm/core.c    |   3 +
 drivers/misc/lkdtm/lkdtm.h   |   3 +
 drivers/misc/lkdtm/powerpc.c | 132 +++++++++++++++++++++++++++++++++++
 4 files changed, 142 insertions(+)
 create mode 100644 drivers/misc/lkdtm/powerpc.c

diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile
index c70b3822013f..6a82f407fbcd 100644
--- a/drivers/misc/lkdtm/Makefile
+++ b/drivers/misc/lkdtm/Makefile
@@ -11,6 +11,10 @@ lkdtm-$(CONFIG_LKDTM)		+= usercopy.o
 lkdtm-$(CONFIG_LKDTM)		+= stackleak.o
 lkdtm-$(CONFIG_LKDTM)		+= cfi.o
 
+ifeq ($(CONFIG_PPC64),y)
+lkdtm-$(CONFIG_LKDTM)		+= powerpc.o
+endif
+
 KASAN_SANITIZE_stackleak.o	:= n
 KCOV_INSTRUMENT_rodata.o	:= n
 
diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
index a5e344df9166..8d5db42baa90 100644
--- a/drivers/misc/lkdtm/core.c
+++ b/drivers/misc/lkdtm/core.c
@@ -178,6 +178,9 @@ static const struct crashtype crashtypes[] = {
 #ifdef CONFIG_X86_32
 	CRASHTYPE(DOUBLE_FAULT),
 #endif
+#ifdef CONFIG_PPC64
+	CRASHTYPE(PPC_SLB_MULTIHIT),
+#endif
 };
 
 
diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
index 8878538b2c13..b305bd511ee5 100644
--- a/drivers/misc/lkdtm/lkdtm.h
+++ b/drivers/misc/lkdtm/lkdtm.h
@@ -104,4 +104,7 @@ void lkdtm_STACKLEAK_ERASING(void);
 /* cfi.c */
 void lkdtm_CFI_FORWARD_PROTO(void);
 
+/* powerpc.c */
+void lkdtm_PPC_SLB_MULTIHIT(void);
+
 #endif
diff --git a/drivers/misc/lkdtm/powerpc.c b/drivers/misc/lkdtm/powerpc.c
new file mode 100644
index 000000000000..d6db18444757
--- /dev/null
+++ b/drivers/misc/lkdtm/powerpc.c
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/slab.h>
+#include <linux/vmalloc.h>
+
+static inline unsigned long get_slb_index(void)
+{
+	unsigned long index;
+
+	index = get_paca()->stab_rr;
+
+	/*
+	 * simple round-robin replacement of slb starting at SLB_NUM_BOLTED.
+	 */
+	if (index < (mmu_slb_size - 1))
+		index++;
+	else
+		index = SLB_NUM_BOLTED;
+	get_paca()->stab_rr = index;
+	return index;
+}
+
+#define slb_esid_mask(ssize)	\
+	(((ssize) == MMU_SEGSIZE_256M) ? ESID_MASK : ESID_MASK_1T)
+
+static inline unsigned long mk_esid_data(unsigned long ea, int ssize,
+					 unsigned long slot)
+{
+	return (ea & slb_esid_mask(ssize)) | SLB_ESID_V | slot;
+}
+
+#define slb_vsid_shift(ssize)	\
+	((ssize) == MMU_SEGSIZE_256M ? SLB_VSID_SHIFT : SLB_VSID_SHIFT_1T)
+
+static inline unsigned long mk_vsid_data(unsigned long ea, int ssize,
+					 unsigned long flags)
+{
+	return (get_kernel_vsid(ea, ssize) << slb_vsid_shift(ssize)) | flags |
+		((unsigned long)ssize << SLB_VSID_SSIZE_SHIFT);
+}
+
+static void insert_slb_entry(char *p, int ssize)
+{
+	unsigned long flags, entry;
+
+	flags = SLB_VSID_KERNEL | mmu_psize_defs[MMU_PAGE_64K].sllp;
+	preempt_disable();
+
+	entry = get_slb_index();
+	asm volatile("slbmte %0,%1" :
+			: "r" (mk_vsid_data((unsigned long)p, ssize, flags)),
+			  "r" (mk_esid_data((unsigned long)p, ssize, entry))
+			: "memory");
+
+	entry = get_slb_index();
+	asm volatile("slbmte %0,%1" :
+			: "r" (mk_vsid_data((unsigned long)p, ssize, flags)),
+			  "r" (mk_esid_data((unsigned long)p, ssize, entry))
+			: "memory");
+	preempt_enable();
+	p[0] = '!';
+}
+
+static void inject_vmalloc_slb_multihit(void)
+{
+	char *p;
+
+	p = vmalloc(2048);
+	if (!p)
+		return;
+
+	insert_slb_entry(p, MMU_SEGSIZE_1T);
+	vfree(p);
+}
+
+static void inject_kmalloc_slb_multihit(void)
+{
+	char *p;
+
+	p = kmalloc(2048, GFP_KERNEL);
+	if (!p)
+		return;
+
+	insert_slb_entry(p, MMU_SEGSIZE_1T);
+	kfree(p);
+}
+
+static void insert_dup_slb_entry_0(void)
+{
+	unsigned long test_address = 0xC000000000000000;
+	volatile unsigned long *test_ptr;
+	unsigned long entry, i = 0;
+	unsigned long esid, vsid;
+
+	test_ptr = (unsigned long *)test_address;
+	preempt_disable();
+
+	asm volatile("slbmfee  %0,%1" : "=r" (esid) : "r" (i));
+	asm volatile("slbmfev  %0,%1" : "=r" (vsid) : "r" (i));
+	entry = get_slb_index();
+
+	/* for i !=0 we would need to mask out the old entry number */
+	asm volatile("slbmte %0,%1" :
+			: "r" (vsid),
+			  "r" (esid | entry)
+			: "memory");
+
+	asm volatile("slbmfee  %0,%1" : "=r" (esid) : "r" (i));
+	asm volatile("slbmfev  %0,%1" : "=r" (vsid) : "r" (i));
+	entry = get_slb_index();
+
+	/* for i !=0 we would need to mask out the old entry number */
+	asm volatile("slbmte %0,%1" :
+			: "r" (vsid),
+			  "r" (esid | entry)
+			: "memory");
+
+	pr_info("lkdtm: %s accessing test address 0x%lx: 0x%lx\n",
+		__func__, test_address, *test_ptr);
+
+	preempt_enable();
+}
+
+void lkdtm_PPC_SLB_MULTIHIT(void)
+{
+	if (mmu_has_feature(MMU_FTR_HPTE_TABLE)) {
+		inject_vmalloc_slb_multihit();
+		inject_kmalloc_slb_multihit();
+		insert_dup_slb_entry_0();
+	}
+	pr_info("Recovered from SLB multihit. (Ignore this message on non HPTE machines)");
+}
-- 
2.26.2

Re: [PATCH v2 2/3] lkdtm/powerpc: Add SLB multihit test

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1950 bytes --]

Hi Ganesh,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on powerpc/next v5.9-rc6 next-20200925]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Ganesh-Goudar/powerpc-mce-Fix-mce-handler-and-add-selftest/20200925-184048
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 9eb29f2ed95edda511ce28651b1d7cdef3614c12
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/c066c175c2699a6aec1b0a25f6b95746590d802a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ganesh-Goudar/powerpc-mce-Fix-mce-handler-and-add-selftest/20200925-184048
        git checkout c066c175c2699a6aec1b0a25f6b95746590d802a
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/misc/lkdtm/powerpc.c:124:6: warning: no previous prototype for 'lkdtm_PPC_SLB_MULTIHIT' [-Wmissing-prototypes]
     124 | void lkdtm_PPC_SLB_MULTIHIT(void)
         |      ^~~~~~~~~~~~~~~~~~~~~~

vim +/lkdtm_PPC_SLB_MULTIHIT +124 drivers/misc/lkdtm/powerpc.c

   123	
 > 124	void lkdtm_PPC_SLB_MULTIHIT(void)

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 70393 bytes --]

Re: [PATCH v2 2/3] lkdtm/powerpc: Add SLB multihit test

[Kees Cook]

On Fri, Sep 25, 2020 at 04:01:22PM +0530, Ganesh Goudar wrote:
> Add support to inject slb multihit errors, to test machine
> check handling.

Thank you for more tests in here!

> 
> Based on work by Mahesh Salgaonkar and Michal Suchánek.
> 
> Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com>
> Cc: Michal Suchánek <msuchanek@suse.de>

Should these be Co-developed-by: with S-o-b?

> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
> ---
>  drivers/misc/lkdtm/Makefile  |   4 ++
>  drivers/misc/lkdtm/core.c    |   3 +
>  drivers/misc/lkdtm/lkdtm.h   |   3 +
>  drivers/misc/lkdtm/powerpc.c | 132 +++++++++++++++++++++++++++++++++++
>  4 files changed, 142 insertions(+)
>  create mode 100644 drivers/misc/lkdtm/powerpc.c
> 
> diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile
> index c70b3822013f..6a82f407fbcd 100644
> --- a/drivers/misc/lkdtm/Makefile
> +++ b/drivers/misc/lkdtm/Makefile
> @@ -11,6 +11,10 @@ lkdtm-$(CONFIG_LKDTM)		+= usercopy.o
>  lkdtm-$(CONFIG_LKDTM)		+= stackleak.o
>  lkdtm-$(CONFIG_LKDTM)		+= cfi.o
>  
> +ifeq ($(CONFIG_PPC64),y)
> +lkdtm-$(CONFIG_LKDTM)		+= powerpc.o
> +endif

This can just be:

lkdtm-$(CONFIG_PPC64)		+= powerpc.o

> +
>  KASAN_SANITIZE_stackleak.o	:= n
>  KCOV_INSTRUMENT_rodata.o	:= n
>  
> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
> index a5e344df9166..8d5db42baa90 100644
> --- a/drivers/misc/lkdtm/core.c
> +++ b/drivers/misc/lkdtm/core.c
> @@ -178,6 +178,9 @@ static const struct crashtype crashtypes[] = {
>  #ifdef CONFIG_X86_32
>  	CRASHTYPE(DOUBLE_FAULT),
>  #endif
> +#ifdef CONFIG_PPC64
> +	CRASHTYPE(PPC_SLB_MULTIHIT),
> +#endif
>  };
>  
>  
> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> index 8878538b2c13..b305bd511ee5 100644
> --- a/drivers/misc/lkdtm/lkdtm.h
> +++ b/drivers/misc/lkdtm/lkdtm.h
> @@ -104,4 +104,7 @@ void lkdtm_STACKLEAK_ERASING(void);
>  /* cfi.c */
>  void lkdtm_CFI_FORWARD_PROTO(void);
>  
> +/* powerpc.c */
> +void lkdtm_PPC_SLB_MULTIHIT(void);
> +
>  #endif
> diff --git a/drivers/misc/lkdtm/powerpc.c b/drivers/misc/lkdtm/powerpc.c
> new file mode 100644
> index 000000000000..d6db18444757
> --- /dev/null
> +++ b/drivers/misc/lkdtm/powerpc.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0
> +

Please #include "lkdtm.h" here to get the correct pr_fmt heading (and
any future header adjustments).

> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +
> +static inline unsigned long get_slb_index(void)
> +{
> +	unsigned long index;
> +
> +	index = get_paca()->stab_rr;
> +
> +	/*
> +	 * simple round-robin replacement of slb starting at SLB_NUM_BOLTED.
> +	 */
> +	if (index < (mmu_slb_size - 1))
> +		index++;
> +	else
> +		index = SLB_NUM_BOLTED;
> +	get_paca()->stab_rr = index;
> +	return index;
> +}
> +
> +#define slb_esid_mask(ssize)	\
> +	(((ssize) == MMU_SEGSIZE_256M) ? ESID_MASK : ESID_MASK_1T)
> +
> +static inline unsigned long mk_esid_data(unsigned long ea, int ssize,
> +					 unsigned long slot)
> +{
> +	return (ea & slb_esid_mask(ssize)) | SLB_ESID_V | slot;
> +}
> +
> +#define slb_vsid_shift(ssize)	\
> +	((ssize) == MMU_SEGSIZE_256M ? SLB_VSID_SHIFT : SLB_VSID_SHIFT_1T)
> +
> +static inline unsigned long mk_vsid_data(unsigned long ea, int ssize,
> +					 unsigned long flags)
> +{
> +	return (get_kernel_vsid(ea, ssize) << slb_vsid_shift(ssize)) | flags |
> +		((unsigned long)ssize << SLB_VSID_SSIZE_SHIFT);
> +}
> +
> +static void insert_slb_entry(char *p, int ssize)
> +{
> +	unsigned long flags, entry;
> +
> +	flags = SLB_VSID_KERNEL | mmu_psize_defs[MMU_PAGE_64K].sllp;
> +	preempt_disable();
> +
> +	entry = get_slb_index();
> +	asm volatile("slbmte %0,%1" :
> +			: "r" (mk_vsid_data((unsigned long)p, ssize, flags)),
> +			  "r" (mk_esid_data((unsigned long)p, ssize, entry))
> +			: "memory");
> +
> +	entry = get_slb_index();
> +	asm volatile("slbmte %0,%1" :
> +			: "r" (mk_vsid_data((unsigned long)p, ssize, flags)),
> +			  "r" (mk_esid_data((unsigned long)p, ssize, entry))
> +			: "memory");
> +	preempt_enable();
> +	p[0] = '!';
> +}

Can you add some comments to these helpers? It'll help people (me) with
understanding what is actually being done here (and more importantly,
what is _expected_ to happen).

> +
> +static void inject_vmalloc_slb_multihit(void)
> +{
> +	char *p;
> +
> +	p = vmalloc(2048);
> +	if (!p)
> +		return;
> +
> +	insert_slb_entry(p, MMU_SEGSIZE_1T);
> +	vfree(p);
> +}
> +
> +static void inject_kmalloc_slb_multihit(void)
> +{
> +	char *p;
> +
> +	p = kmalloc(2048, GFP_KERNEL);
> +	if (!p)
> +		return;
> +
> +	insert_slb_entry(p, MMU_SEGSIZE_1T);
> +	kfree(p);
> +}

It looks like the expected failure injection is actually the "p[0] = '!'" line in the
"insert" helper? I would expect pr_info/pr_err wrappers, etc, as in
other lkdtm tests.

If this is the negative test, what does the positive test look like?
e.g. in other lkdtm tests, first a positive test is done, then a
negative: first run a legit function, then run a function from a bad
location.

> +
> +static void insert_dup_slb_entry_0(void)
> +{
> +	unsigned long test_address = 0xC000000000000000;
> +	volatile unsigned long *test_ptr;
> +	unsigned long entry, i = 0;
> +	unsigned long esid, vsid;
> +
> +	test_ptr = (unsigned long *)test_address;
> +	preempt_disable();
> +
> +	asm volatile("slbmfee  %0,%1" : "=r" (esid) : "r" (i));
> +	asm volatile("slbmfev  %0,%1" : "=r" (vsid) : "r" (i));
> +	entry = get_slb_index();
> +
> +	/* for i !=0 we would need to mask out the old entry number */
> +	asm volatile("slbmte %0,%1" :
> +			: "r" (vsid),
> +			  "r" (esid | entry)
> +			: "memory");
> +
> +	asm volatile("slbmfee  %0,%1" : "=r" (esid) : "r" (i));
> +	asm volatile("slbmfev  %0,%1" : "=r" (vsid) : "r" (i));
> +	entry = get_slb_index();
> +
> +	/* for i !=0 we would need to mask out the old entry number */
> +	asm volatile("slbmte %0,%1" :
> +			: "r" (vsid),
> +			  "r" (esid | entry)
> +			: "memory");
> +
> +	pr_info("lkdtm: %s accessing test address 0x%lx: 0x%lx\n",
> +		__func__, test_address, *test_ptr);
> +
> +	preempt_enable();
> +}

What does this do?

> +
> +void lkdtm_PPC_SLB_MULTIHIT(void)
> +{
> +	if (mmu_has_feature(MMU_FTR_HPTE_TABLE)) {
> +		inject_vmalloc_slb_multihit();
> +		inject_kmalloc_slb_multihit();
> +		insert_dup_slb_entry_0();
> +	}
> +	pr_info("Recovered from SLB multihit. (Ignore this message on non HPTE machines)");

Is this bad? If so, I'd expect pr_err("FAIL: ...") Can HPTE machines be
detected so that an XFAIL can be emitted instead?

Since there are three (two?) distinct regions being tested, should these
be separate tests? Right now there is no way to separate a vmalloc
failure from a kmalloc failure, and no way to fail the first without
hiding the result from the latter (or maybe the machine cannot survive
this test? ... which should also be a comment.)

And finally, assuming a successful test (or testing from a separate
thread later), so there any state that needs to be restored (or cleaned
up before doing the "insert" calls)?

Thanks!

-- 
Kees Cook

Re: [PATCH v2 2/3] lkdtm/powerpc: Add SLB multihit test

[Ganesh]

On 9/26/20 1:27 AM, Kees Cook wrote:
> On Fri, Sep 25, 2020 at 04:01:22PM +0530, Ganesh Goudar wrote:
>> Add support to inject slb multihit errors, to test machine
>> check handling.
> Thank you for more tests in here!
>
>> Based on work by Mahesh Salgaonkar and Michal Suchánek.
>>
>> Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com>
>> Cc: Michal Suchánek <msuchanek@suse.de>
> Should these be Co-developed-by: with S-o-b?
Sure, ill add.
>> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
>> ---
>>   drivers/misc/lkdtm/Makefile  |   4 ++
>>   drivers/misc/lkdtm/core.c    |   3 +
>>   drivers/misc/lkdtm/lkdtm.h   |   3 +
>>   drivers/misc/lkdtm/powerpc.c | 132 +++++++++++++++++++++++++++++++++++
>>   4 files changed, 142 insertions(+)
>>   create mode 100644 drivers/misc/lkdtm/powerpc.c
>>
>> diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile
>> index c70b3822013f..6a82f407fbcd 100644
>> --- a/drivers/misc/lkdtm/Makefile
>> +++ b/drivers/misc/lkdtm/Makefile
>> @@ -11,6 +11,10 @@ lkdtm-$(CONFIG_LKDTM)		+= usercopy.o
>>   lkdtm-$(CONFIG_LKDTM)		+= stackleak.o
>>   lkdtm-$(CONFIG_LKDTM)		+= cfi.o
>>   
>> +ifeq ($(CONFIG_PPC64),y)
>> +lkdtm-$(CONFIG_LKDTM)		+= powerpc.o
>> +endif
> This can just be:
>
> lkdtm-$(CONFIG_PPC64)		+= powerpc.o
ok
>> +
>>   KASAN_SANITIZE_stackleak.o	:= n
>>   KCOV_INSTRUMENT_rodata.o	:= n
>>   
>> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
>> index a5e344df9166..8d5db42baa90 100644
>> --- a/drivers/misc/lkdtm/core.c
>> +++ b/drivers/misc/lkdtm/core.c
>> @@ -178,6 +178,9 @@ static const struct crashtype crashtypes[] = {
>>   #ifdef CONFIG_X86_32
>>   	CRASHTYPE(DOUBLE_FAULT),
>>   #endif
>> +#ifdef CONFIG_PPC64
>> +	CRASHTYPE(PPC_SLB_MULTIHIT),
>> +#endif
>>   };
>>   
>>   
>> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
>> index 8878538b2c13..b305bd511ee5 100644
>> --- a/drivers/misc/lkdtm/lkdtm.h
>> +++ b/drivers/misc/lkdtm/lkdtm.h
>> @@ -104,4 +104,7 @@ void lkdtm_STACKLEAK_ERASING(void);
>>   /* cfi.c */
>>   void lkdtm_CFI_FORWARD_PROTO(void);
>>   
>> +/* powerpc.c */
>> +void lkdtm_PPC_SLB_MULTIHIT(void);
>> +
>>   #endif
>> diff --git a/drivers/misc/lkdtm/powerpc.c b/drivers/misc/lkdtm/powerpc.c
>> new file mode 100644
>> index 000000000000..d6db18444757
>> --- /dev/null
>> +++ b/drivers/misc/lkdtm/powerpc.c
>> @@ -0,0 +1,132 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
> Please #include "lkdtm.h" here to get the correct pr_fmt heading (and
> any future header adjustments).
Sure
>> +#include <linux/slab.h>
>> +#include <linux/vmalloc.h>
>> +
>> +static inline unsigned long get_slb_index(void)
>> +{
>> +	unsigned long index;
>> +
>> +	index = get_paca()->stab_rr;
>> +
>> +	/*
>> +	 * simple round-robin replacement of slb starting at SLB_NUM_BOLTED.
>> +	 */
>> +	if (index < (mmu_slb_size - 1))
>> +		index++;
>> +	else
>> +		index = SLB_NUM_BOLTED;
>> +	get_paca()->stab_rr = index;
>> +	return index;
>> +}
>> +
>> +#define slb_esid_mask(ssize)	\
>> +	(((ssize) == MMU_SEGSIZE_256M) ? ESID_MASK : ESID_MASK_1T)
>> +
>> +static inline unsigned long mk_esid_data(unsigned long ea, int ssize,
>> +					 unsigned long slot)
>> +{
>> +	return (ea & slb_esid_mask(ssize)) | SLB_ESID_V | slot;
>> +}
>> +
>> +#define slb_vsid_shift(ssize)	\
>> +	((ssize) == MMU_SEGSIZE_256M ? SLB_VSID_SHIFT : SLB_VSID_SHIFT_1T)
>> +
>> +static inline unsigned long mk_vsid_data(unsigned long ea, int ssize,
>> +					 unsigned long flags)
>> +{
>> +	return (get_kernel_vsid(ea, ssize) << slb_vsid_shift(ssize)) | flags |
>> +		((unsigned long)ssize << SLB_VSID_SSIZE_SHIFT);
>> +}
>> +
>> +static void insert_slb_entry(char *p, int ssize)
>> +{
>> +	unsigned long flags, entry;
>> +
>> +	flags = SLB_VSID_KERNEL | mmu_psize_defs[MMU_PAGE_64K].sllp;
>> +	preempt_disable();
>> +
>> +	entry = get_slb_index();
>> +	asm volatile("slbmte %0,%1" :
>> +			: "r" (mk_vsid_data((unsigned long)p, ssize, flags)),
>> +			  "r" (mk_esid_data((unsigned long)p, ssize, entry))
>> +			: "memory");
>> +
>> +	entry = get_slb_index();
>> +	asm volatile("slbmte %0,%1" :
>> +			: "r" (mk_vsid_data((unsigned long)p, ssize, flags)),
>> +			  "r" (mk_esid_data((unsigned long)p, ssize, entry))
>> +			: "memory");
>> +	preempt_enable();
>> +	p[0] = '!';
>> +}
> Can you add some comments to these helpers? It'll help people (me) with
> understanding what is actually being done here (and more importantly,
> what is _expected_ to happen).
Sure, ill add comments.
>> +
>> +static void inject_vmalloc_slb_multihit(void)
>> +{
>> +	char *p;
>> +
>> +	p = vmalloc(2048);
>> +	if (!p)
>> +		return;
>> +
>> +	insert_slb_entry(p, MMU_SEGSIZE_1T);
>> +	vfree(p);
>> +}
>> +
>> +static void inject_kmalloc_slb_multihit(void)
>> +{
>> +	char *p;
>> +
>> +	p = kmalloc(2048, GFP_KERNEL);
>> +	if (!p)
>> +		return;
>> +
>> +	insert_slb_entry(p, MMU_SEGSIZE_1T);
>> +	kfree(p);
>> +}
> It looks like the expected failure injection is actually the "p[0] = '!'" line in the
> "insert" helper? I would expect pr_info/pr_err wrappers, etc, as in
> other lkdtm tests.
Yes "p[0] = '!'" is the final step in this error injection, ill add 
comments.
> If this is the negative test, what does the positive test look like?
> e.g. in other lkdtm tests, first a positive test is done, then a
> negative: first run a legit function, then run a function from a bad
> location.

Yes, this is negative test, As SLB search is very fundamental thing in 
address translation and

used all the time, no positive test required here.

>> +
>> +static void insert_dup_slb_entry_0(void)
>> +{
>> +	unsigned long test_address = 0xC000000000000000;
>> +	volatile unsigned long *test_ptr;
>> +	unsigned long entry, i = 0;
>> +	unsigned long esid, vsid;
>> +
>> +	test_ptr = (unsigned long *)test_address;
>> +	preempt_disable();
>> +
>> +	asm volatile("slbmfee  %0,%1" : "=r" (esid) : "r" (i));
>> +	asm volatile("slbmfev  %0,%1" : "=r" (vsid) : "r" (i));
>> +	entry = get_slb_index();
>> +
>> +	/* for i !=0 we would need to mask out the old entry number */
>> +	asm volatile("slbmte %0,%1" :
>> +			: "r" (vsid),
>> +			  "r" (esid | entry)
>> +			: "memory");
>> +
>> +	asm volatile("slbmfee  %0,%1" : "=r" (esid) : "r" (i));
>> +	asm volatile("slbmfev  %0,%1" : "=r" (vsid) : "r" (i));
>> +	entry = get_slb_index();
>> +
>> +	/* for i !=0 we would need to mask out the old entry number */
>> +	asm volatile("slbmte %0,%1" :
>> +			: "r" (vsid),
>> +			  "r" (esid | entry)
>> +			: "memory");
>> +
>> +	pr_info("lkdtm: %s accessing test address 0x%lx: 0x%lx\n",
>> +		__func__, test_address, *test_ptr);
>> +
>> +	preempt_enable();
>> +}
> What does this do?

It tries to inject error in address range where most important kernel 
data structure may fall.

Mahesh and Michal Suchánek correct me if I am wrong.

>> +
>> +void lkdtm_PPC_SLB_MULTIHIT(void)
>> +{
>> +	if (mmu_has_feature(MMU_FTR_HPTE_TABLE)) {
>> +		inject_vmalloc_slb_multihit();
>> +		inject_kmalloc_slb_multihit();
>> +		insert_dup_slb_entry_0();
>> +	}
>> +	pr_info("Recovered from SLB multihit. (Ignore this message on non HPTE machines)");
> Is this bad? If so, I'd expect pr_err("FAIL: ...") Can HPTE machines be
> detected so that an XFAIL can be emitted instead?

Sure, ill add XFAIL print in non HPTE case. And in case of failure we 
will not return

here, so if we hit this print line we are good.

> Since there are three (two?) distinct regions being tested, should these
> be separate tests? Right now there is no way to separate a vmalloc
> failure from a kmalloc failure, and no way to fail the first without
> hiding the result from the latter (or maybe the machine cannot survive
> this test? ... which should also be a comment.)
Sure, ill comment, And yes machine cannot survive these tests in case of 
failure to handle.
> And finally, assuming a successful test (or testing from a separate
> thread later), so there any state that needs to be restored (or cleaned
> up before doing the "insert" calls)?
No, there is nothing to be restored here, Thanks!
> Thanks!
>

Re: [PATCH v2 2/3] lkdtm/powerpc: Add SLB multihit test

[Michal Suchánek]

Hello,

On Fri, Sep 25, 2020 at 12:57:33PM -0700, Kees Cook wrote:
> On Fri, Sep 25, 2020 at 04:01:22PM +0530, Ganesh Goudar wrote:
> > Add support to inject slb multihit errors, to test machine
> > check handling.
> 
> Thank you for more tests in here!
Thanks for working on integrating this.
> 
> > 
> > Based on work by Mahesh Salgaonkar and Michal Suchánek.
> > 
> > Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com>
> > Cc: Michal Suchánek <msuchanek@suse.de>
> 
> Should these be Co-developed-by: with S-o-b?

I don't think I wrote any of this code. I packaged it for SUSE and maybe
changed some constants based on test result discussion.

I compared this code to my saved snapshots of past versions of the test
module and this covers all the test cases I have. The only difference is that
the development modules have verbose prints showing what's going on.

It is true that without the verbose prints some explanatory comments
could be helpful.

Reviewed-by: Michal Suchánek <msuchanek@suse.de>
> 
> > Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
> > ---
> >  drivers/misc/lkdtm/Makefile  |   4 ++
> >  drivers/misc/lkdtm/core.c    |   3 +
> >  drivers/misc/lkdtm/lkdtm.h   |   3 +
> >  drivers/misc/lkdtm/powerpc.c | 132 +++++++++++++++++++++++++++++++++++
> >  4 files changed, 142 insertions(+)
> >  create mode 100644 drivers/misc/lkdtm/powerpc.c
> > 
> > diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile
> > index c70b3822013f..6a82f407fbcd 100644
> > --- a/drivers/misc/lkdtm/Makefile
> > +++ b/drivers/misc/lkdtm/Makefile
> > @@ -11,6 +11,10 @@ lkdtm-$(CONFIG_LKDTM)		+= usercopy.o
> >  lkdtm-$(CONFIG_LKDTM)		+= stackleak.o
> >  lkdtm-$(CONFIG_LKDTM)		+= cfi.o
> >  
> > +ifeq ($(CONFIG_PPC64),y)
> > +lkdtm-$(CONFIG_LKDTM)		+= powerpc.o
> > +endif
> 
> This can just be:
> 
> lkdtm-$(CONFIG_PPC64)		+= powerpc.o
> 
> > +
> >  KASAN_SANITIZE_stackleak.o	:= n
> >  KCOV_INSTRUMENT_rodata.o	:= n
> >  
> > diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
> > index a5e344df9166..8d5db42baa90 100644
> > --- a/drivers/misc/lkdtm/core.c
> > +++ b/drivers/misc/lkdtm/core.c
> > @@ -178,6 +178,9 @@ static const struct crashtype crashtypes[] = {
> >  #ifdef CONFIG_X86_32
> >  	CRASHTYPE(DOUBLE_FAULT),
> >  #endif
> > +#ifdef CONFIG_PPC64
> > +	CRASHTYPE(PPC_SLB_MULTIHIT),
> > +#endif
> >  };
> >  
> >  
> > diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> > index 8878538b2c13..b305bd511ee5 100644
> > --- a/drivers/misc/lkdtm/lkdtm.h
> > +++ b/drivers/misc/lkdtm/lkdtm.h
> > @@ -104,4 +104,7 @@ void lkdtm_STACKLEAK_ERASING(void);
> >  /* cfi.c */
> >  void lkdtm_CFI_FORWARD_PROTO(void);
> >  
> > +/* powerpc.c */
> > +void lkdtm_PPC_SLB_MULTIHIT(void);
> > +
> >  #endif
> > diff --git a/drivers/misc/lkdtm/powerpc.c b/drivers/misc/lkdtm/powerpc.c
> > new file mode 100644
> > index 000000000000..d6db18444757
> > --- /dev/null
> > +++ b/drivers/misc/lkdtm/powerpc.c
> > @@ -0,0 +1,132 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> 
> Please #include "lkdtm.h" here to get the correct pr_fmt heading (and
> any future header adjustments).
> 
> > +#include <linux/slab.h>
> > +#include <linux/vmalloc.h>
> > +
> > +static inline unsigned long get_slb_index(void)
> > +{
> > +	unsigned long index;
> > +
> > +	index = get_paca()->stab_rr;
> > +
> > +	/*
> > +	 * simple round-robin replacement of slb starting at SLB_NUM_BOLTED.
> > +	 */
> > +	if (index < (mmu_slb_size - 1))
> > +		index++;
> > +	else
> > +		index = SLB_NUM_BOLTED;
> > +	get_paca()->stab_rr = index;
> > +	return index;
> > +}
> > +
> > +#define slb_esid_mask(ssize)	\
> > +	(((ssize) == MMU_SEGSIZE_256M) ? ESID_MASK : ESID_MASK_1T)
> > +
> > +static inline unsigned long mk_esid_data(unsigned long ea, int ssize,
> > +					 unsigned long slot)
> > +{
> > +	return (ea & slb_esid_mask(ssize)) | SLB_ESID_V | slot;
> > +}
> > +
> > +#define slb_vsid_shift(ssize)	\
> > +	((ssize) == MMU_SEGSIZE_256M ? SLB_VSID_SHIFT : SLB_VSID_SHIFT_1T)
> > +
> > +static inline unsigned long mk_vsid_data(unsigned long ea, int ssize,
> > +					 unsigned long flags)
> > +{
> > +	return (get_kernel_vsid(ea, ssize) << slb_vsid_shift(ssize)) | flags |
> > +		((unsigned long)ssize << SLB_VSID_SSIZE_SHIFT);
> > +}
> > +
> > +static void insert_slb_entry(char *p, int ssize)
> > +{
> > +	unsigned long flags, entry;
> > +
> > +	flags = SLB_VSID_KERNEL | mmu_psize_defs[MMU_PAGE_64K].sllp;
> > +	preempt_disable();
> > +
> > +	entry = get_slb_index();
> > +	asm volatile("slbmte %0,%1" :
> > +			: "r" (mk_vsid_data((unsigned long)p, ssize, flags)),
> > +			  "r" (mk_esid_data((unsigned long)p, ssize, entry))
> > +			: "memory");
> > +
> > +	entry = get_slb_index();
> > +	asm volatile("slbmte %0,%1" :
> > +			: "r" (mk_vsid_data((unsigned long)p, ssize, flags)),
> > +			  "r" (mk_esid_data((unsigned long)p, ssize, entry))
> > +			: "memory");
> > +	preempt_enable();
> > +	p[0] = '!';
> > +}
> 
> Can you add some comments to these helpers? It'll help people (me) with
> understanding what is actually being done here (and more importantly,
> what is _expected_ to happen).
> 
> > +
> > +static void inject_vmalloc_slb_multihit(void)
> > +{
> > +	char *p;
> > +
> > +	p = vmalloc(2048);
> > +	if (!p)
> > +		return;
> > +
> > +	insert_slb_entry(p, MMU_SEGSIZE_1T);
> > +	vfree(p);
> > +}
> > +
> > +static void inject_kmalloc_slb_multihit(void)
> > +{
> > +	char *p;
> > +
> > +	p = kmalloc(2048, GFP_KERNEL);
> > +	if (!p)
> > +		return;
> > +
> > +	insert_slb_entry(p, MMU_SEGSIZE_1T);
> > +	kfree(p);
> > +}
> 
> It looks like the expected failure injection is actually the "p[0] = '!'" line in the
> "insert" helper? I would expect pr_info/pr_err wrappers, etc, as in
> other lkdtm tests.
> 
> If this is the negative test, what does the positive test look like?
> e.g. in other lkdtm tests, first a positive test is done, then a
> negative: first run a legit function, then run a function from a bad
> location.
> 
> > +
> > +static void insert_dup_slb_entry_0(void)
> > +{
> > +	unsigned long test_address = 0xC000000000000000;
> > +	volatile unsigned long *test_ptr;
> > +	unsigned long entry, i = 0;
> > +	unsigned long esid, vsid;
> > +
> > +	test_ptr = (unsigned long *)test_address;
> > +	preempt_disable();
> > +
> > +	asm volatile("slbmfee  %0,%1" : "=r" (esid) : "r" (i));
> > +	asm volatile("slbmfev  %0,%1" : "=r" (vsid) : "r" (i));
> > +	entry = get_slb_index();
> > +
> > +	/* for i !=0 we would need to mask out the old entry number */
> > +	asm volatile("slbmte %0,%1" :
> > +			: "r" (vsid),
> > +			  "r" (esid | entry)
> > +			: "memory");
> > +
> > +	asm volatile("slbmfee  %0,%1" : "=r" (esid) : "r" (i));
> > +	asm volatile("slbmfev  %0,%1" : "=r" (vsid) : "r" (i));
> > +	entry = get_slb_index();
> > +
> > +	/* for i !=0 we would need to mask out the old entry number */
> > +	asm volatile("slbmte %0,%1" :
> > +			: "r" (vsid),
> > +			  "r" (esid | entry)
> > +			: "memory");
> > +
> > +	pr_info("lkdtm: %s accessing test address 0x%lx: 0x%lx\n",
> > +		__func__, test_address, *test_ptr);
> > +
> > +	preempt_enable();
> > +}
> 
> What does this do?
> 
> > +
> > +void lkdtm_PPC_SLB_MULTIHIT(void)
> > +{
> > +	if (mmu_has_feature(MMU_FTR_HPTE_TABLE)) {
> > +		inject_vmalloc_slb_multihit();
> > +		inject_kmalloc_slb_multihit();
> > +		insert_dup_slb_entry_0();
> > +	}
> > +	pr_info("Recovered from SLB multihit. (Ignore this message on non HPTE machines)");
> 
> Is this bad? If so, I'd expect pr_err("FAIL: ...") Can HPTE machines be
> detected so that an XFAIL can be emitted instead?
> 
> Since there are three (two?) distinct regions being tested, should these
> be separate tests? Right now there is no way to separate a vmalloc
> failure from a kmalloc failure, and no way to fail the first without
> hiding the result from the latter (or maybe the machine cannot survive
> this test? ... which should also be a comment.)
> 
> And finally, assuming a successful test (or testing from a separate
> thread later), so there any state that needs to be restored (or cleaned
> up before doing the "insert" calls)?
> 
> Thanks!
> 
> -- 
> Kees Cook

[PATCH v2 3/3] selftests/lkdtm: Enable selftest for SLB multihit

[Ganesh Goudar]

Add PPC_SLB_MULTIHIT to lkdtm selftest framework.

Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
---
 tools/testing/selftests/lkdtm/tests.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt
index 9d266e79c6a2..7eb3cf91c89e 100644
--- a/tools/testing/selftests/lkdtm/tests.txt
+++ b/tools/testing/selftests/lkdtm/tests.txt
@@ -70,3 +70,4 @@ USERCOPY_KERNEL
 USERCOPY_KERNEL_DS
 STACKLEAK_ERASING OK: the rest of the thread stack is properly erased
 CFI_FORWARD_PROTO
+PPC_SLB_MULTIHIT Recovered
-- 
2.26.2

Re: [PATCH v2 3/3] selftests/lkdtm: Enable selftest for SLB multihit

[Kees Cook]

On Fri, Sep 25, 2020 at 04:01:23PM +0530, Ganesh Goudar wrote:
> Add PPC_SLB_MULTIHIT to lkdtm selftest framework.
> 
> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
> ---
>  tools/testing/selftests/lkdtm/tests.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt
> index 9d266e79c6a2..7eb3cf91c89e 100644
> --- a/tools/testing/selftests/lkdtm/tests.txt
> +++ b/tools/testing/selftests/lkdtm/tests.txt
> @@ -70,3 +70,4 @@ USERCOPY_KERNEL
>  USERCOPY_KERNEL_DS
>  STACKLEAK_ERASING OK: the rest of the thread stack is properly erased
>  CFI_FORWARD_PROTO
> +PPC_SLB_MULTIHIT Recovered

Please squash this into the lkdtm patch -- I'd like test implementation
and kselftest awareness to go in together.

-- 
Kees Cook

Re: [PATCH v2 3/3] selftests/lkdtm: Enable selftest for SLB multihit

[Ganesh]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]


On 9/26/20 1:29 AM, Kees Cook wrote:
> On Fri, Sep 25, 2020 at 04:01:23PM +0530, Ganesh Goudar wrote:
>> Add PPC_SLB_MULTIHIT to lkdtm selftest framework.
>>
>> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
>> ---
>>   tools/testing/selftests/lkdtm/tests.txt | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt
>> index 9d266e79c6a2..7eb3cf91c89e 100644
>> --- a/tools/testing/selftests/lkdtm/tests.txt
>> +++ b/tools/testing/selftests/lkdtm/tests.txt
>> @@ -70,3 +70,4 @@ USERCOPY_KERNEL
>>   USERCOPY_KERNEL_DS
>>   STACKLEAK_ERASING OK: the rest of the thread stack is properly erased
>>   CFI_FORWARD_PROTO
>> +PPC_SLB_MULTIHIT Recovered
> Please squash this into the lkdtm patch -- I'd like test implementation
> and kselftest awareness to go in together.

Sure, Thanks.

>

[-- Attachment #2: Type: text/html, Size: 1532 bytes --]