On Mon, Oct 19, 2020 at 09:59:57PM +1100, Michael Ellerman wrote:
> Hi Ganesh,
>
> Some comments below ...
>
> Ganesh Goudar <ganeshgr at linux.ibm.com> writes:
> > To check machine check handling, add support to inject slb
> > multihit errors.
> >
> > Cc: Kees Cook <keescook at chromium.org>
> > Reviewed-by: Michal Suchánek <msuchanek at suse.de>
> > Co-developed-by: Mahesh Salgaonkar <mahesh at linux.ibm.com>
> > Signed-off-by: Mahesh Salgaonkar <mahesh at linux.ibm.com>
> > Signed-off-by: Ganesh Goudar <ganeshgr at linux.ibm.com>
> > ---
> > drivers/misc/lkdtm/Makefile | 1 +
> > drivers/misc/lkdtm/core.c | 3 +
> > drivers/misc/lkdtm/lkdtm.h | 3 +
> > drivers/misc/lkdtm/powerpc.c | 156 ++++++++++++++++++++++++
> > tools/testing/selftests/lkdtm/tests.txt | 1 +
> > 5 files changed, 164 insertions(+)
> > create mode 100644 drivers/misc/lkdtm/powerpc.c
> >
> ..
> > diff --git a/drivers/misc/lkdtm/powerpc.c b/drivers/misc/lkdtm/powerpc.c
> > new file mode 100644
> > index 000000000000..f388b53dccba
> > --- /dev/null
> > +++ b/drivers/misc/lkdtm/powerpc.c
> > @@ -0,0 +1,156 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include "lkdtm.h"
> > +#include <linux/slab.h>
> > +#include <linux/vmalloc.h>
>
> Usual style is to include the linux headers first and then the local header.
>
> > +
> > +/* Gets index for new slb entry */
> > +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;
> > +}
>
> I'm not sure we need that really?
>
> We can just always insert at SLB_MUM_BOLTED and SLB_NUM_BOLTED + 1.
>
> Or we could allocate from the top down using mmu_slb_size - 1, and
> mmu_slb_size - 2.
>
>
> > +#define slb_esid_mask(ssize) \
> > + (((ssize) == MMU_SEGSIZE_256M) ? ESID_MASK : ESID_MASK_1T)
> > +
> > +/* Form the operand for slbmte */
> > +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)
> > +
> > +/* Form the operand for slbmte */
> > +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);
> > +}
>
> I realise it's not much code, but I'd rather those were in a header,
> rather than copied from slb.c. That way they can never skew vs the
> versions in slb.c
>
> Best place I think would be arch/powerpc/include/asm/book3s/64/mmu-hash.h
>
>
> > +
> > +/* Inserts new slb entry */
>
> It inserts two.
>
> > +static void insert_slb_entry(char *p, int ssize)
> > +{
> > + unsigned long flags, entry;
> > +
> > + flags = SLB_VSID_KERNEL | mmu_psize_defs[MMU_PAGE_64K].sllp;
>
> That won't work if the kernel is built for 4K pages. Or at least it
> won't work the way we want it to.
>
> You should use mmu_linear_psize.
>
> But for vmalloc you should use mmu_vmalloc_psize, so it will need to be
> a parameter.
>
> > + 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();
> > + /*
> > + * This triggers exception, If handled correctly we must recover
> > + * from this error.
> > + */
> > + p[0] = '!';
>
> That doesn't belong in here, it should be done by the caller.
>
> That would also mean p could be unsigned long in here, so you wouldn't
> have to cast it four times.
>
> > +}
> > +
> > +/* Inject slb multihit on vmalloc-ed address i.e 0xD00... */
> > +static void inject_vmalloc_slb_multihit(void)
> > +{
> > + char *p;
> > +
> > + p = vmalloc(2048);
>
> vmalloc() allocates whole pages, so it may as well be vmalloc(PAGE_SIZE).
>
> > + if (!p)
> > + return;
>
> That's unlikely, but it should be an error that's propagated up to the caller.
>
> > +
> > + insert_slb_entry(p, MMU_SEGSIZE_1T);
> > + vfree(p);
> > +}
> > +
> > +/* Inject slb multihit on kmalloc-ed address i.e 0xC00... */
> > +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);
> > +}
> > +
> > +/*
> > + * Few initial SLB entries are bolted. Add a test to inject
> > + * multihit in bolted entry 0.
> > + */
> > +static void insert_dup_slb_entry_0(void)
> > +{
> > + unsigned long test_address = 0xC000000000000000;
>
> Should use PAGE_OFFSET;
>
> > + volatile unsigned long *test_ptr;
>
> Does it need to be a volatile?
> The slbmte should act as a compiler barrier (it has a memory clobber)
> and a CPU barrier as well?
>
> > + unsigned long entry, i = 0;
> > + unsigned long esid, vsid;
>
> Please group your variables:
>
> unsigned long esid, vsid, entry, test_address, i;
> volatile unsigned long *test_ptr;
>
> And then initialise them as appropriate.
>
> > + 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));
>
> Why do we need to read them out of the SLB rather than just computing
> the values?
It ensures that the entry is perfect duplicate without copying even more
code from other parts of the kernel, doesn't it?
Especially when inserting only one duplicate as suggested later it
ensures that the test really does what it should.
>
> > + entry = get_slb_index();
> > +
> > + /* for i !=0 we would need to mask out the old entry number */
>
> Or you could just compute esid and then it wouldn't be an issue.
>
> > + asm volatile("slbmte %0,%1" :
> > + : "r" (vsid),
> > + "r" (esid | entry)
> > + : "memory");
>
> At this point we've just inserted a duplicate of entry 0. So you don't
> need to insert a third entry do you?
This code was obviously adapted from the previous one which needed two
entries in case there was none for the memory region to start with.
Addin only one duplicate should suffice and it can be easily tested that
it still generates a MCE.
>
> > + 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("%s accessing test address 0x%lx: 0x%lx\n",
> > + __func__, test_address, *test_ptr);
>
> This prints the first two instructions of the kernel. I happen to know
> what values they should have, but most people won't understand what
> they're seeing. A better test would be to read the value at the top of
> the function and then load it again here and check we got the right
> thing.
It does not really matter what we read back so long as the compiler does
not optimize out the read. The point here is to access an address in the
range covered by the SLB entry 0. The failure case is that the system
crashes and the test never finishes.
Thanks
Michal