Re: [PATCH 2/3] powernv/pci: Use common code in pnv_ioda_pick_m64_pe()

[Alexey Kardashevskiy]

On 18/10/17 02:39, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Tue, 17 Oct 2017 17:07:54 +0200
> 
> Add a jump target so that a bit of code can be better reused
> at the end of this function.


Rather than moving bits around, I'd rather allocate pe_alloc on stack and
ditch kfree() at all. I'll make a patch for this.

> 
> Signed-off-by: Markus Elfring 
> ---
>  arch/powerpc/platforms/powernv/pci-ioda.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c 
> b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 17c0330bb059..98d9435240f4 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -364,21 +364,20 @@ static struct pnv_ioda_pe *pnv_ioda_pick_m64_pe(struct 
> pci_bus *bus, bool all)
>   /* Figure out reserved PE numbers by the PE */
>   pnv_ioda_reserve_m64_pe(bus, pe_alloc, all);
>  
> + master_pe = NULL;
> +
>   /*
>* the current bus might not own M64 window and that's all
>* contributed by its child buses. For the case, we needn't
>* pick M64 dependent PE#.
>*/
> - if (bitmap_empty(pe_alloc, phb->ioda.total_pe_num)) {
> - kfree(pe_alloc);
> - return NULL;
> - }
> + if (bitmap_empty(pe_alloc, phb->ioda.total_pe_num))
> + goto free_pe;
>  
>   /*
>* Figure out the master PE and put all slave PEs to master
>* PE's list to form compound PE.
>*/
> - master_pe = NULL;
>   i = -1;
>   while ((i = find_next_bit(pe_alloc, phb->ioda.total_pe_num, i + 1)) <
>   phb->ioda.total_pe_num) {
> @@ -416,6 +415,7 @@ static struct pnv_ioda_pe *pnv_ioda_pick_m64_pe(struct 
> pci_bus *bus, bool all)
>   }
>   }
>  
> +free_pe:
>   kfree(pe_alloc);
>   return master_pe;
>  }
> 



-- 
Alexey

Re: [PATCH 3/3] powernv/pci: Improve a size determination in pnv_pci_init_ioda_phb()

[Alexey Kardashevskiy]

On 18/10/17 02:40, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Tue, 17 Oct 2017 17:18:10 +0200
> 
> Replace the specification of a data structure by a pointer dereference
> as the parameter for the operator "sizeof" to make the corresponding size
> determination a bit safer according to the Linux coding style convention.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring 
> ---
>  arch/powerpc/platforms/powernv/pci-ioda.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c 
> b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 98d9435240f4..2febdf06a237 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -3802,7 +3802,7 @@ static void __init pnv_pci_init_ioda_phb(struct 
> device_node *np,
>   phb_id = be64_to_cpup(prop64);
>   pr_debug("  PHB-ID  : 0x%016llx\n", phb_id);
>  
> - phb = memblock_virt_alloc(sizeof(struct pnv_phb), 0);
> + phb = memblock_virt_alloc(sizeof(*phb), 0);
>  
>   /* Allocate PCI controller */
>   phb->hose = hose = pcibios_alloc_controller(np);
> 

Reviewed-by: Alexey Kardashevskiy 



-- 
Alexey

Re: [PATCH 1/3] powernv/pci: Delete an error message for a failed memory allocation in pnv_ioda_pick_m64_pe()

[Alexey Kardashevskiy]

On 18/10/17 02:37, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Tue, 17 Oct 2017 16:52:43 +0200
> 
> Omit an extra message for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring 
> ---
>  arch/powerpc/platforms/powernv/pci-ioda.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c 
> b/arch/powerpc/platforms/powernv/pci-ioda.c
> index fb5cd7511189..17c0330bb059 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -358,11 +358,8 @@ static struct pnv_ioda_pe *pnv_ioda_pick_m64_pe(struct 
> pci_bus *bus, bool all)
>   /* Allocate bitmap */
>   size = _ALIGN_UP(phb->ioda.total_pe_num / 8, sizeof(unsigned long));
>   pe_alloc = kzalloc(size, GFP_KERNEL);
> - if (!pe_alloc) {
> - pr_warn("%s: Out of memory !\n",
> - __func__);
> + if (!pe_alloc)
>   return NULL;
> - }
>  
>   /* Figure out reserved PE numbers by the PE */
>   pnv_ioda_reserve_m64_pe(bus, pe_alloc, all);
> 

Reviewed-by: Alexey Kardashevskiy 



-- 
Alexey

Re: [PATCH 09/25] powerpc: ability to create execute-disabled pkeys

[Ram Pai]

On Wed, Oct 18, 2017 at 02:42:56PM +1100, Balbir Singh wrote:
> On Fri,  8 Sep 2017 15:44:57 -0700
> Ram Pai  wrote:
> 
> > powerpc has hardware support to disable execute on a pkey.
> > This patch enables the ability to create execute-disabled
> > keys.
> > 
> > Signed-off-by: Ram Pai 
> > ---
> >  arch/powerpc/include/uapi/asm/mman.h |6 ++
> >  arch/powerpc/mm/pkeys.c  |   16 
> >  2 files changed, 22 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/uapi/asm/mman.h 
> > b/arch/powerpc/include/uapi/asm/mman.h
> > index ab45cc2..f272b09 100644
> > --- a/arch/powerpc/include/uapi/asm/mman.h
> > +++ b/arch/powerpc/include/uapi/asm/mman.h
> > @@ -45,4 +45,10 @@
> >  #define MAP_HUGE_1GB   (30 << MAP_HUGE_SHIFT)  /* 1GB   HugeTLB Page */
> >  #define MAP_HUGE_16GB  (34 << MAP_HUGE_SHIFT)  /* 16GB  HugeTLB Page */
> >  
> > +/* override any generic PKEY Permission defines */
> > +#define PKEY_DISABLE_EXECUTE   0x4
> > +#undef PKEY_ACCESS_MASK
> > +#define PKEY_ACCESS_MASK   (PKEY_DISABLE_ACCESS |\
> > +   PKEY_DISABLE_WRITE  |\
> > +   PKEY_DISABLE_EXECUTE)
> >  #endif /* _UAPI_ASM_POWERPC_MMAN_H */
> > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> > index cc5be6a..2282864 100644
> > --- a/arch/powerpc/mm/pkeys.c
> > +++ b/arch/powerpc/mm/pkeys.c
> > @@ -24,6 +24,14 @@ void __init pkey_initialize(void)
> >  {
> > int os_reserved, i;
> >  
> > +   /*
> > +* we define PKEY_DISABLE_EXECUTE in addition to the arch-neutral
> > +* generic defines for PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE.
> > +* Ensure that the bits a distinct.
> > +*/
> > +   BUILD_BUG_ON(PKEY_DISABLE_EXECUTE &
> > +(PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));
> 
> Will these values every change? It's good to have I guess.
> 
> > +
> > /* disable the pkey system till everything
> >  * is in place. A patch further down the
> >  * line will enable it.
> > @@ -120,10 +128,18 @@ int __arch_set_user_pkey_access(struct task_struct 
> > *tsk, int pkey,
> > unsigned long init_val)
> >  {
> > u64 new_amr_bits = 0x0ul;
> > +   u64 new_iamr_bits = 0x0ul;
> >  
> > if (!is_pkey_enabled(pkey))
> > return -EINVAL;
> >  
> > +   if ((init_val & PKEY_DISABLE_EXECUTE)) {
> > +   if (!pkey_execute_disable_support)
> > +   return -EINVAL;
> > +   new_iamr_bits |= IAMR_EX_BIT;
> > +   }
> > +   init_iamr(pkey, new_iamr_bits);
> > +
> 
> Where do we check the reserved keys?

The main gate keeper against spurious keys are the system calls.
sys_pkey_mprotect(), sys_pkey_free() and sys_pkey_modify() are the one
that will check against reserved and unallocated keys.  Once it has
passed the check, all other internal functions trust the key values
provided to them. I can put in additional checks but that will
unnecessarily chew a few cpu cycles.

Agree?

BTW: you raise a good point though, I may have missed guarding against
unallocated or reserved keys in sys_pkey_modify(). That was a power
specific system call that I have introduced to change the permissions on
a key.

RP

Re: [PATCH v3] KVM: PPC: Book3S PR: only install valid SLBs during KVM_SET_SREGS

[David Gibson]

On Mon, Oct 16, 2017 at 12:29:44PM +0200, Greg Kurz wrote:
> Userland passes an array of 64 SLB descriptors to KVM_SET_SREGS,
> some of which are valid (ie, SLB_ESID_V is set) and the rest are
> likely all-zeroes (with QEMU at least).
> 
> Each of them is then passed to kvmppc_mmu_book3s_64_slbmte(), which
> assumes to find the SLB index in the 3 lower bits of its rb argument.
> When passed zeroed arguments, it happily overwrites the 0th SLB entry
> with zeroes. This is exactly what happens while doing live migration
> with QEMU when the destination pushes the incoming SLB descriptors to
> KVM PR. When reloading the SLBs at the next synchronization, QEMU first
> clears its SLB array and only restore valid ones, but the 0th one is
> now gone and we cannot access the corresponding memory anymore:
> 
> (qemu) x/x $pc
> c00b742c: Cannot access memory
> 
> To avoid this, let's filter out non-valid SLB entries. While here, we
> also force a full SLB flush before installing new entries. Since SLB
> is for 64-bit only, we now build this path conditionally to avoid a
> build break on 32-bit, which doesn't define SLB_ESID_V.
> 
> Signed-off-by: Greg Kurz 

Reviewed-by: David Gibson 

> ---
> v3: - build SLB path for 64-bit only [*]
> - add blank line after declaration of rb and rs to silence checkpatch
> 
> v2: - flush SLB before installing new entries
> 
> [*] I'm wondering if other users of BOOK3S_HFLAG_SLB in this file
> shouldn't be conditionally built as well, for consistency.
> ---
>  arch/powerpc/kvm/book3s_pr.c |   16 +---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
> index 3beb4ff469d1..68c1f8bc17c4 100644
> --- a/arch/powerpc/kvm/book3s_pr.c
> +++ b/arch/powerpc/kvm/book3s_pr.c
> @@ -1326,12 +1326,22 @@ static int kvm_arch_vcpu_ioctl_set_sregs_pr(struct 
> kvm_vcpu *vcpu,
>   kvmppc_set_pvr_pr(vcpu, sregs->pvr);
>  
>   vcpu3s->sdr1 = sregs->u.s.sdr1;
> +#ifdef CONFIG_PPC_BOOK3S_64
>   if (vcpu->arch.hflags & BOOK3S_HFLAG_SLB) {
> + /* Flush all SLB entries */
> + vcpu->arch.mmu.slbmte(vcpu, 0, 0);
> + vcpu->arch.mmu.slbia(vcpu);
> +
>   for (i = 0; i < 64; i++) {
> - vcpu->arch.mmu.slbmte(vcpu, 
> sregs->u.s.ppc64.slb[i].slbv,
> - 
> sregs->u.s.ppc64.slb[i].slbe);
> + u64 rb = sregs->u.s.ppc64.slb[i].slbe;
> + u64 rs = sregs->u.s.ppc64.slb[i].slbv;
> +
> + if (rb & SLB_ESID_V)
> + vcpu->arch.mmu.slbmte(vcpu, rs, rb);
>   }
> - } else {
> + } else
> +#endif
> + {
>   for (i = 0; i < 16; i++) {
>   vcpu->arch.mmu.mtsrin(vcpu, i, sregs->u.s.ppc32.sr[i]);
>   }
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH 17/25] powerpc: helper to validate key-access permissions of a pte

[Balbir Singh]

On Fri,  8 Sep 2017 15:45:05 -0700
Ram Pai  wrote:

> helper function that checks if the read/write/execute is allowed
> on the pte.
> 
> Signed-off-by: Ram Pai 
> ---
>  arch/powerpc/include/asm/book3s/64/pgtable.h |4 +++
>  arch/powerpc/include/asm/pkeys.h |   12 +++
>  arch/powerpc/mm/pkeys.c  |   28 
> ++
>  3 files changed, 44 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h 
> b/arch/powerpc/include/asm/book3s/64/pgtable.h
> index 5935d4e..bd244b3 100644
> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> @@ -492,6 +492,10 @@ static inline void write_uamor(u64 value)
>   mtspr(SPRN_UAMOR, value);
>  }
>  
> +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> +extern bool arch_pte_access_permitted(u64 pte, bool write, bool execute);
> +#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
> +
>  #define __HAVE_ARCH_PTEP_GET_AND_CLEAR
>  static inline pte_t ptep_get_and_clear(struct mm_struct *mm,
>  unsigned long addr, pte_t *ptep)
> diff --git a/arch/powerpc/include/asm/pkeys.h 
> b/arch/powerpc/include/asm/pkeys.h
> index cd3924c..50522a0 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -80,6 +80,18 @@ static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
>   ((pteflags & H_PAGE_PKEY_BIT4) ? HPTE_R_KEY_BIT4 : 0x0UL));
>  }
>  
> +static inline u16 pte_to_pkey_bits(u64 pteflags)
> +{
> + if (!pkey_inited)
> + return 0x0UL;
> +
> + return (((pteflags & H_PAGE_PKEY_BIT0) ? 0x10 : 0x0UL) |
> + ((pteflags & H_PAGE_PKEY_BIT1) ? 0x8 : 0x0UL) |
> + ((pteflags & H_PAGE_PKEY_BIT2) ? 0x4 : 0x0UL) |
> + ((pteflags & H_PAGE_PKEY_BIT3) ? 0x2 : 0x0UL) |
> + ((pteflags & H_PAGE_PKEY_BIT4) ? 0x1 : 0x0UL));
> +}
> +
>  #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
>   VM_PKEY_BIT3 | VM_PKEY_BIT4)
>  #define AMR_BITS_PER_PKEY 2
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index fb1a76a..24589d9 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -292,3 +292,31 @@ int __arch_override_mprotect_pkey(struct vm_area_struct 
> *vma, int prot,
>*/
>   return vma_pkey(vma);
>  }
> +
> +static bool pkey_access_permitted(int pkey, bool write, bool execute)
> +{
> + int pkey_shift;
> + u64 amr;
> +
> + if (!pkey)
> + return true;

Why would we have pkey set to 0, it's reserved. Why do we return true?

> +
> + pkey_shift = pkeyshift(pkey);
> + if (!(read_uamor() & (0x3UL << pkey_shift)))
> + return true;
> +
> + if (execute && !(read_iamr() & (IAMR_EX_BIT << pkey_shift)))
> + return true;
> +
> + amr = read_amr(); /* delay reading amr uptil absolutely needed*/
> + return ((!write && !(amr & (AMR_RD_BIT << pkey_shift))) ||
> + (write &&  !(amr & (AMR_WR_BIT << pkey_shift;
> +}
> +
> +bool arch_pte_access_permitted(u64 pte, bool write, bool execute)
> +{
> + if (!pkey_inited)
> + return true;

Again, don't like the pkey_inited bits :)

> + return pkey_access_permitted(pte_to_pkey_bits(pte),
> + write, execute);
> +}

Balbir Singh

Re: [PATCH 16/25] powerpc: Program HPTE key protection bits

[Balbir Singh]

On Fri,  8 Sep 2017 15:45:04 -0700
Ram Pai  wrote:

> Map the PTE protection key bits to the HPTE key protection bits,
> while creating HPTE  entries.
> 
> Signed-off-by: Ram Pai 
> ---
>  arch/powerpc/include/asm/book3s/64/mmu-hash.h |5 +
>  arch/powerpc/include/asm/mmu_context.h|6 ++
>  arch/powerpc/include/asm/pkeys.h  |   13 +
>  arch/powerpc/mm/hash_utils_64.c   |1 +
>  4 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h 
> b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> index 508275b..2e22357 100644
> --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> @@ -90,6 +90,8 @@
>  #define HPTE_R_PP0   ASM_CONST(0x8000)
>  #define HPTE_R_TSASM_CONST(0x4000)
>  #define HPTE_R_KEY_HIASM_CONST(0x3000)
> +#define HPTE_R_KEY_BIT0  ASM_CONST(0x2000)
> +#define HPTE_R_KEY_BIT1  ASM_CONST(0x1000)
>  #define HPTE_R_RPN_SHIFT 12
>  #define HPTE_R_RPN   ASM_CONST(0x0000)
>  #define HPTE_R_RPN_3_0   ASM_CONST(0x01fff000)
> @@ -104,6 +106,9 @@
>  #define HPTE_R_C ASM_CONST(0x0080)
>  #define HPTE_R_R ASM_CONST(0x0100)
>  #define HPTE_R_KEY_LOASM_CONST(0x0e00)
> +#define HPTE_R_KEY_BIT2  ASM_CONST(0x0800)
> +#define HPTE_R_KEY_BIT3  ASM_CONST(0x0400)
> +#define HPTE_R_KEY_BIT4  ASM_CONST(0x0200)
>  #define HPTE_R_KEY   (HPTE_R_KEY_LO | HPTE_R_KEY_HI)
>  
>  #define HPTE_V_1TB_SEG   ASM_CONST(0x4000)
> diff --git a/arch/powerpc/include/asm/mmu_context.h 
> b/arch/powerpc/include/asm/mmu_context.h
> index 8e5a87e..04e9221 100644
> --- a/arch/powerpc/include/asm/mmu_context.h
> +++ b/arch/powerpc/include/asm/mmu_context.h
> @@ -150,6 +150,12 @@ static inline int vma_pkey(struct vm_area_struct *vma)
>  {
>   return 0;
>  }
> +
> +static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
> +{
> + return 0x0UL;
> +}
> +
>  #endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
>  
>  #endif /* __KERNEL__ */
> diff --git a/arch/powerpc/include/asm/pkeys.h 
> b/arch/powerpc/include/asm/pkeys.h
> index 0d2488a..cd3924c 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -67,6 +67,19 @@ static inline int vma_pkey(struct vm_area_struct *vma)
>  #define AMR_RD_BIT 0x1UL
>  #define AMR_WR_BIT 0x2UL
>  #define IAMR_EX_BIT 0x1UL
> +
> +static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
> +{
> + if (!pkey_inited)
> + return 0x0UL;
> +
> + return (((pteflags & H_PAGE_PKEY_BIT0) ? HPTE_R_KEY_BIT0 : 0x0UL) |
> + ((pteflags & H_PAGE_PKEY_BIT1) ? HPTE_R_KEY_BIT1 : 0x0UL) |
> + ((pteflags & H_PAGE_PKEY_BIT2) ? HPTE_R_KEY_BIT2 : 0x0UL) |
> + ((pteflags & H_PAGE_PKEY_BIT3) ? HPTE_R_KEY_BIT3 : 0x0UL) |
> + ((pteflags & H_PAGE_PKEY_BIT4) ? HPTE_R_KEY_BIT4 : 0x0UL));
> +}
> +
>  #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
>   VM_PKEY_BIT3 | VM_PKEY_BIT4)
>  #define AMR_BITS_PER_PKEY 2
> diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
> index 67f62b5..a739a2d 100644
> --- a/arch/powerpc/mm/hash_utils_64.c
> +++ b/arch/powerpc/mm/hash_utils_64.c
> @@ -232,6 +232,7 @@ unsigned long htab_convert_pte_flags(unsigned long 
> pteflags)
>*/
>   rflags |= HPTE_R_M;
>  
> + rflags |= pte_to_hpte_pkey_bits(pteflags);
>   return rflags;
>  }
>  


Looks good!

Acked-by: Balbir Singh 

Re: [PATCH 14/25] powerpc: map vma key-protection bits to pte key bits.

[Balbir Singh]

On Fri,  8 Sep 2017 15:45:02 -0700
Ram Pai  wrote:

> map  the  key  protection  bits of the vma to the pkey bits in
> the PTE.
> 
> The Pte  bits used  for pkey  are  3,4,5,6  and 57. The  first
> four bits are the same four bits that were freed up  initially
> in this patch series. remember? :-) Without those four bits
> this patch would'nt be possible.
> 
> BUT, On 4k kernel, bit 3, and 4 could not be freed up. remember?
> Hence we have to be satisfied with 5,6 and 7.
> 
> Signed-off-by: Ram Pai 
> ---
>  arch/powerpc/include/asm/book3s/64/pgtable.h |   25 -
>  arch/powerpc/include/asm/mman.h  |8 
>  arch/powerpc/include/asm/pkeys.h |   12 
>  3 files changed, 44 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h 
> b/arch/powerpc/include/asm/book3s/64/pgtable.h
> index 73ed52c..5935d4e 100644
> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> @@ -38,6 +38,7 @@
>  #define _RPAGE_RSV2  0x0800UL
>  #define _RPAGE_RSV3  0x0400UL
>  #define _RPAGE_RSV4  0x0200UL
> +#define _RPAGE_RSV5  0x00040UL
>  
>  #define _PAGE_PTE0x4000UL/* distinguishes PTEs 
> from pointers */
>  #define _PAGE_PRESENT0x8000UL/* pte contains 
> a translation */
> @@ -57,6 +58,25 @@
>  /* Max physical address bit as per radix table */
>  #define _RPAGE_PA_MAX57
>  
> +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> +#ifdef CONFIG_PPC_64K_PAGES
> +#define H_PAGE_PKEY_BIT0 _RPAGE_RSV1
> +#define H_PAGE_PKEY_BIT1 _RPAGE_RSV2
> +#else /* CONFIG_PPC_64K_PAGES */
> +#define H_PAGE_PKEY_BIT0 0 /* _RPAGE_RSV1 is not available */
> +#define H_PAGE_PKEY_BIT1 0 /* _RPAGE_RSV2 is not available */
> +#endif /* CONFIG_PPC_64K_PAGES */
> +#define H_PAGE_PKEY_BIT2 _RPAGE_RSV3
> +#define H_PAGE_PKEY_BIT3 _RPAGE_RSV4
> +#define H_PAGE_PKEY_BIT4 _RPAGE_RSV5
> +#else /*  CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
> +#define H_PAGE_PKEY_BIT0 0
> +#define H_PAGE_PKEY_BIT1 0
> +#define H_PAGE_PKEY_BIT2 0
> +#define H_PAGE_PKEY_BIT3 0
> +#define H_PAGE_PKEY_BIT4 0
> +#endif /*  CONFIG_PPC64_MEMORY_PROTECTION_KEYS */

H_PTE_PKEY_BITX?

> +
>  /*
>   * Max physical address bit we will use for now.
>   *
> @@ -120,13 +140,16 @@
>  #define _PAGE_CHG_MASK   (PTE_RPN_MASK | _PAGE_HPTEFLAGS | _PAGE_DIRTY | 
> \
>_PAGE_ACCESSED | _PAGE_SPECIAL | _PAGE_PTE |   \
>_PAGE_SOFT_DIRTY)
> +
> +#define H_PAGE_PKEY  (H_PAGE_PKEY_BIT0 | H_PAGE_PKEY_BIT1 | H_PAGE_PKEY_BIT2 
> | \
> + H_PAGE_PKEY_BIT3 | H_PAGE_PKEY_BIT4)
>  /*
>   * Mask of bits returned by pte_pgprot()
>   */
>  #define PAGE_PROT_BITS  (_PAGE_SAO | _PAGE_NON_IDEMPOTENT | _PAGE_TOLERANT | 
> \
>H_PAGE_4K_PFN | _PAGE_PRIVILEGED | _PAGE_ACCESSED | \
>_PAGE_READ | _PAGE_WRITE |  _PAGE_DIRTY | _PAGE_EXEC | 
> \
> -  _PAGE_SOFT_DIRTY)
> +  _PAGE_SOFT_DIRTY | H_PAGE_PKEY)
>  /*
>   * We define 2 sets of base prot bits, one for basic pages (ie,
>   * cacheable kernel and user pages) and one for non cacheable
> diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
> index 067eec2..3f7220f 100644
> --- a/arch/powerpc/include/asm/mman.h
> +++ b/arch/powerpc/include/asm/mman.h
> @@ -32,12 +32,20 @@ static inline unsigned long 
> arch_calc_vm_prot_bits(unsigned long prot,
>  }
>  #define arch_calc_vm_prot_bits(prot, pkey) arch_calc_vm_prot_bits(prot, pkey)
>  
> +
>  static inline pgprot_t arch_vm_get_page_prot(unsigned long vm_flags)
>  {
> +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> + return (vm_flags & VM_SAO) ?
> + __pgprot(_PAGE_SAO | vmflag_to_page_pkey_bits(vm_flags)) :
> + __pgprot(0 | vmflag_to_page_pkey_bits(vm_flags));
> +#else
>   return (vm_flags & VM_SAO) ? __pgprot(_PAGE_SAO) : __pgprot(0);
> +#endif
>  }
>  #define arch_vm_get_page_prot(vm_flags) arch_vm_get_page_prot(vm_flags)
>  
> +
>  static inline bool arch_validate_prot(unsigned long prot)
>  {
>   if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_SAO))
> diff --git a/arch/powerpc/include/asm/pkeys.h 
> b/arch/powerpc/include/asm/pkeys.h
> index d2fffef..0d2488a 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -41,6 +41,18 @@ static inline u64 pkey_to_vmflag_bits(u16 pkey)
>   ((pkey & 0x10UL) ? VM_PKEY_BIT4 : 0x0UL));
>  }
>  
> +static inline u64 vmflag_to_page_pkey_bits(u64 vm_flags)

vmflag_to_pte_pkey_bits?

> +{
> + if (!pkey_inited)
> + return 0x0UL;
> +
> + return (((vm_flags & VM_PKEY_BIT0) ? H_PAGE_PKEY_BIT4 : 0x0UL) |
> + 

Re: [PATCH 13/25] powerpc: implementation for arch_override_mprotect_pkey()

[Balbir Singh]

On Fri,  8 Sep 2017 15:45:01 -0700
Ram Pai  wrote:

> arch independent code calls arch_override_mprotect_pkey()
> to return a pkey that best matches the requested protection.
> 
> This patch provides the implementation.
> 
> Signed-off-by: Ram Pai 
> ---
>  arch/powerpc/include/asm/mmu_context.h |5 +++
>  arch/powerpc/include/asm/pkeys.h   |   17 ++-
>  arch/powerpc/mm/pkeys.c|   47 
> 
>  3 files changed, 67 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/mmu_context.h 
> b/arch/powerpc/include/asm/mmu_context.h
> index c705a5d..8e5a87e 100644
> --- a/arch/powerpc/include/asm/mmu_context.h
> +++ b/arch/powerpc/include/asm/mmu_context.h
> @@ -145,6 +145,11 @@ static inline bool arch_vma_access_permitted(struct 
> vm_area_struct *vma,
>  #ifndef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
>  #define pkey_initialize()
>  #define pkey_mm_init(mm)
> +
> +static inline int vma_pkey(struct vm_area_struct *vma)
> +{
> + return 0;
> +}
>  #endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
>  
>  #endif /* __KERNEL__ */
> diff --git a/arch/powerpc/include/asm/pkeys.h 
> b/arch/powerpc/include/asm/pkeys.h
> index f13e913..d2fffef 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -41,6 +41,16 @@ static inline u64 pkey_to_vmflag_bits(u16 pkey)
>   ((pkey & 0x10UL) ? VM_PKEY_BIT4 : 0x0UL));
>  }
>  
> +#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
> + VM_PKEY_BIT3 | VM_PKEY_BIT4)
> +
> +static inline int vma_pkey(struct vm_area_struct *vma)
> +{
> + if (!pkey_inited)
> + return 0;

We don't want pkey_inited to be present in all functions, why do we need
a conditional branch for all functions. Even if we do, it should be a jump
label. I would rather we just removed !pkey_inited unless really really
required.

> + return (vma->vm_flags & ARCH_VM_PKEY_FLAGS) >> VM_PKEY_SHIFT;
> +}
> +
>  #define arch_max_pkey()  pkeys_total
>  #define AMR_RD_BIT 0x1UL
>  #define AMR_WR_BIT 0x2UL
> @@ -142,11 +152,14 @@ static inline int execute_only_pkey(struct mm_struct 
> *mm)
>   return __execute_only_pkey(mm);
>  }
>  
> -
> +extern int __arch_override_mprotect_pkey(struct vm_area_struct *vma,
> + int prot, int pkey);
>  static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
>   int prot, int pkey)
>  {
> - return 0;
> + if (!pkey_inited)
> + return 0;
> + return __arch_override_mprotect_pkey(vma, prot, pkey);
>  }
>  
>  extern int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index 8a24983..fb1a76a 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -245,3 +245,50 @@ int __execute_only_pkey(struct mm_struct *mm)
>   mm->context.execute_only_pkey = execute_only_pkey;
>   return execute_only_pkey;
>  }
> +
> +static inline bool vma_is_pkey_exec_only(struct vm_area_struct *vma)
> +{
> + /* Do this check first since the vm_flags should be hot */
> + if ((vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC)) != VM_EXEC)
> + return false;
> +
> + return (vma_pkey(vma) == vma->vm_mm->context.execute_only_pkey);
> +}
> +
> +/*
> + * This should only be called for *plain* mprotect calls.

What's a plain mprotect call?

> + */
> +int __arch_override_mprotect_pkey(struct vm_area_struct *vma, int prot,
> + int pkey)
> +{
> + /*
> +  * Is this an mprotect_pkey() call?  If so, never
> +  * override the value that came from the user.
> +  */
> + if (pkey != -1)
> + return pkey;

If the user specified a key, we always use it? Presumably the user
got it from pkey_alloc(), in other cases, the user was lazy and used
-1 in the mprotect call?

> +
> + /*
> +  * If the currently associated pkey is execute-only,
> +  * but the requested protection requires read or write,
> +  * move it back to the default pkey.
> +  */
> + if (vma_is_pkey_exec_only(vma) &&
> + (prot & (PROT_READ|PROT_WRITE)))
> + return 0;
> +
> + /*
> +  * the requested protection is execute-only. Hence
> +  * lets use a execute-only pkey.
> +  */
> + if (prot == PROT_EXEC) {
> + pkey = execute_only_pkey(vma->vm_mm);
> + if (pkey > 0)
> + return pkey;
> + }
> +
> + /*
> +  * nothing to override.
> +  */
> + return vma_pkey(vma);
> +}

Balbir Singh.

Re: [PATCH 12/25] powerpc: ability to associate pkey to a vma

[Balbir Singh]

On Fri,  8 Sep 2017 15:45:00 -0700
Ram Pai  wrote:

> arch-independent code expects the arch to  map
> a  pkey  into the vma's protection bit setting.
> The patch provides that ability.
> 
> Signed-off-by: Ram Pai 
> ---
>  arch/powerpc/include/asm/mman.h  |8 +++-
>  arch/powerpc/include/asm/pkeys.h |   18 ++
>  2 files changed, 25 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
> index 30922f6..067eec2 100644
> --- a/arch/powerpc/include/asm/mman.h
> +++ b/arch/powerpc/include/asm/mman.h
> @@ -13,6 +13,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  /*
> @@ -22,7 +23,12 @@
>  static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
>   unsigned long pkey)
>  {
> - return (prot & PROT_SAO) ? VM_SAO : 0;
> +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> + return (((prot & PROT_SAO) ? VM_SAO : 0) |
> + pkey_to_vmflag_bits(pkey));
> +#else
> + return ((prot & PROT_SAO) ? VM_SAO : 0);
> +#endif
>  }
>  #define arch_calc_vm_prot_bits(prot, pkey) arch_calc_vm_prot_bits(prot, pkey)
>  
> diff --git a/arch/powerpc/include/asm/pkeys.h 
> b/arch/powerpc/include/asm/pkeys.h
> index 0cf115f..f13e913 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -23,6 +23,24 @@
>  #define VM_PKEY_BIT4 VM_HIGH_ARCH_4
>  #endif
>  
> +/* override any generic PKEY Permission defines */
> +#define PKEY_DISABLE_EXECUTE   0x4
> +#define PKEY_ACCESS_MASK   (PKEY_DISABLE_ACCESS |\
> + PKEY_DISABLE_WRITE  |\
> + PKEY_DISABLE_EXECUTE)
> +
> +static inline u64 pkey_to_vmflag_bits(u16 pkey)
> +{
> + if (!pkey_inited)
> + return 0x0UL;
> +
> + return (((pkey & 0x1UL) ? VM_PKEY_BIT0 : 0x0UL) |
> + ((pkey & 0x2UL) ? VM_PKEY_BIT1 : 0x0UL) |
> + ((pkey & 0x4UL) ? VM_PKEY_BIT2 : 0x0UL) |
> + ((pkey & 0x8UL) ? VM_PKEY_BIT3 : 0x0UL) |
> + ((pkey & 0x10UL) ? VM_PKEY_BIT4 : 0x0UL));
> +}

Assuming that there is a linear order between VM_PKEY_BIT4 to
VM_PKEY_BIT0, the conditional checks can be removed

(pkey & 0x1fUL) << VM_PKEY_BIT0?


Balbir Singh

Re: [PATCH 11/25] powerpc: introduce execute-only pkey

[Balbir Singh]

On Fri,  8 Sep 2017 15:44:59 -0700
Ram Pai  wrote:

> This patch provides the implementation of execute-only pkey.
> The architecture-independent layer expects the arch-dependent
> layer, to support the ability to create and enable a special
> key which has execute-only permission.
> 
> Signed-off-by: Ram Pai 
> ---
>  arch/powerpc/include/asm/book3s/64/mmu.h |1 +
>  arch/powerpc/include/asm/pkeys.h |9 -
>  arch/powerpc/mm/pkeys.c  |   57 
> ++
>  3 files changed, 66 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/book3s/64/mmu.h 
> b/arch/powerpc/include/asm/book3s/64/mmu.h
> index 55950f4..ee18ba0 100644
> --- a/arch/powerpc/include/asm/book3s/64/mmu.h
> +++ b/arch/powerpc/include/asm/book3s/64/mmu.h
> @@ -115,6 +115,7 @@ struct patb_entry {
>* bit unset -> key available for allocation
>*/
>   u32 pkey_allocation_map;
> + s16 execute_only_pkey; /* key holding execute-only protection */
>  #endif
>  } mm_context_t;
>  
> diff --git a/arch/powerpc/include/asm/pkeys.h 
> b/arch/powerpc/include/asm/pkeys.h
> index 78c5362..0cf115f 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -115,11 +115,16 @@ static inline int mm_pkey_free(struct mm_struct *mm, 
> int pkey)
>   * Try to dedicate one of the protection keys to be used as an
>   * execute-only protection key.
>   */
> +extern int __execute_only_pkey(struct mm_struct *mm);
>  static inline int execute_only_pkey(struct mm_struct *mm)
>  {
> - return 0;
> + if (!pkey_inited || !pkey_execute_disable_support)
> + return -1;
> +
> + return __execute_only_pkey(mm);
>  }
>  
> +
>  static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
>   int prot, int pkey)
>  {
> @@ -141,6 +146,8 @@ static inline void pkey_mm_init(struct mm_struct *mm)
>   if (!pkey_inited)
>   return;
>   mm_pkey_allocation_map(mm) = initial_allocation_mask;
> + /* -1 means unallocated or invalid */
> + mm->context.execute_only_pkey = -1;
>  }
>  
>  extern void thread_pkey_regs_save(struct thread_struct *thread);
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index 7cd1be4..8a24983 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -188,3 +188,60 @@ void thread_pkey_regs_init(struct thread_struct *thread)
>   write_iamr(0x0ul);
>   write_uamor(0x0ul);
>  }
> +
> +static inline bool pkey_allows_readwrite(int pkey)
> +{
> + int pkey_shift = pkeyshift(pkey);
> +
> + if (!(read_uamor() & (0x3UL << pkey_shift)))
> + return true;

If uamor for key 0 is 0x10 for example or 0x01 it's a bug.
The above check might miss it.

> +
> + return !(read_amr() & ((AMR_RD_BIT|AMR_WR_BIT) << pkey_shift));
> +}
> +
> +int __execute_only_pkey(struct mm_struct *mm)
> +{
> + bool need_to_set_mm_pkey = false;
> + int execute_only_pkey = mm->context.execute_only_pkey;
> + int ret;
> +
> + /* Do we need to assign a pkey for mm's execute-only maps? */
> + if (execute_only_pkey == -1) {
> + /* Go allocate one to use, which might fail */
> + execute_only_pkey = mm_pkey_alloc(mm);
> + if (execute_only_pkey < 0)
> + return -1;
> + need_to_set_mm_pkey = true;
> + }
> +
> + /*
> +  * We do not want to go through the relatively costly
> +  * dance to set AMR if we do not need to.  Check it
> +  * first and assume that if the execute-only pkey is
> +  * readwrite-disabled than we do not have to set it
> +  * ourselves.
> +  */
> + if (!need_to_set_mm_pkey &&
> + !pkey_allows_readwrite(execute_only_pkey))
> + return execute_only_pkey;
> +
> + /*
> +  * Set up AMR so that it denies access for everything
> +  * other than execution.
> +  */
> + ret = __arch_set_user_pkey_access(current, execute_only_pkey,
> + (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));
> + /*
> +  * If the AMR-set operation failed somehow, just return
> +  * 0 and effectively disable execute-only support.
> +  */
> + if (ret) {
> + mm_set_pkey_free(mm, execute_only_pkey);
> + return -1;
> + }
> +
> + /* We got one, store it and use it from here on out */
> + if (need_to_set_mm_pkey)
> + mm->context.execute_only_pkey = execute_only_pkey;
> + return execute_only_pkey;
> +}

Looks good otherwise

Acked-by: Balbir Singh 

Re: [PATCH 10/25] powerpc: store and restore the pkey state across context switches

[Balbir Singh]

On Fri,  8 Sep 2017 15:44:58 -0700
Ram Pai  wrote:

> Store and restore the AMR, IAMR and UAMOR register state of the task
> before scheduling out and after scheduling in, respectively.
> 
> Signed-off-by: Ram Pai 
> ---
>  arch/powerpc/include/asm/pkeys.h |4 +++
>  arch/powerpc/include/asm/processor.h |5 
>  arch/powerpc/kernel/process.c|   10 
>  arch/powerpc/mm/pkeys.c  |   39 
> ++
>  4 files changed, 58 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/pkeys.h 
> b/arch/powerpc/include/asm/pkeys.h
> index 7fd48a4..78c5362 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -143,5 +143,9 @@ static inline void pkey_mm_init(struct mm_struct *mm)
>   mm_pkey_allocation_map(mm) = initial_allocation_mask;
>  }
>  
> +extern void thread_pkey_regs_save(struct thread_struct *thread);
> +extern void thread_pkey_regs_restore(struct thread_struct *new_thread,
> + struct thread_struct *old_thread);
> +extern void thread_pkey_regs_init(struct thread_struct *thread);
>  extern void pkey_initialize(void);
>  #endif /*_ASM_PPC64_PKEYS_H */
> diff --git a/arch/powerpc/include/asm/processor.h 
> b/arch/powerpc/include/asm/processor.h
> index fab7ff8..de9d9ba 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -309,6 +309,11 @@ struct thread_struct {
>   struct thread_vr_state ckvr_state; /* Checkpointed VR state */
>   unsigned long   ckvrsave; /* Checkpointed VRSAVE */
>  #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
> +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> + unsigned long   amr;
> + unsigned long   iamr;
> + unsigned long   uamor;
> +#endif
>  #ifdef CONFIG_KVM_BOOK3S_32_HANDLER
>   void*   kvm_shadow_vcpu; /* KVM internal data */
>  #endif /* CONFIG_KVM_BOOK3S_32_HANDLER */
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index a0c74bb..ba80002 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -42,6 +42,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -1085,6 +1086,9 @@ static inline void save_sprs(struct thread_struct *t)
>   t->tar = mfspr(SPRN_TAR);
>   }
>  #endif
> +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> + thread_pkey_regs_save(t);
> +#endif

Just define two variants of thread_pkey_regs_save() based on
CONFIG_PPC64_MEMORY_PROTECTION_KEYS and remove the #ifdefs from process.c
Ditto for the lines below

>  }
>  
>  static inline void restore_sprs(struct thread_struct *old_thread,
> @@ -1120,6 +1124,9 @@ static inline void restore_sprs(struct thread_struct 
> *old_thread,
>   mtspr(SPRN_TAR, new_thread->tar);
>   }
>  #endif
> +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> + thread_pkey_regs_restore(new_thread, old_thread);
> +#endif
>  }
>  
>  #ifdef CONFIG_PPC_BOOK3S_64
> @@ -1705,6 +1712,9 @@ void start_thread(struct pt_regs *regs, unsigned long 
> start, unsigned long sp)
>   current->thread.tm_tfiar = 0;
>   current->thread.load_tm = 0;
>  #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
> +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> + thread_pkey_regs_init(>thread);
> +#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
>  }
>  EXPORT_SYMBOL(start_thread);
>  
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index 2282864..7cd1be4 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -149,3 +149,42 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, 
> int pkey,
>   init_amr(pkey, new_amr_bits);
>   return 0;
>  }
> +
> +void thread_pkey_regs_save(struct thread_struct *thread)
> +{
> + if (!pkey_inited)
> + return;
> +
> + /* @TODO skip saving any registers if the thread
> +  * has not used any keys yet.
> +  */

Comment style is broken

> +
> + thread->amr = read_amr();
> + thread->iamr = read_iamr();
> + thread->uamor = read_uamor();
> +}
> +
> +void thread_pkey_regs_restore(struct thread_struct *new_thread,
> + struct thread_struct *old_thread)
> +{
> + if (!pkey_inited)
> + return;
> +
> + /* @TODO just reset uamor to zero if the new_thread
> +  * has not used any keys yet.
> +  */

Comment style is broken.

> +
> + if (old_thread->amr != new_thread->amr)
> + write_amr(new_thread->amr);
> + if (old_thread->iamr != new_thread->iamr)
> + write_iamr(new_thread->iamr);
> + if (old_thread->uamor != new_thread->uamor)
> + write_uamor(new_thread->uamor);

Is this order correct? Ideally, You want to write the uamor first
but since we are in supervisor state, I think we can get away
with this order. Do we want to expose the uamor to user space
for it to modify the AMR 

Re: [PATCH 09/25] powerpc: ability to create execute-disabled pkeys

[Balbir Singh]

On Fri,  8 Sep 2017 15:44:57 -0700
Ram Pai  wrote:

> powerpc has hardware support to disable execute on a pkey.
> This patch enables the ability to create execute-disabled
> keys.
> 
> Signed-off-by: Ram Pai 
> ---
>  arch/powerpc/include/uapi/asm/mman.h |6 ++
>  arch/powerpc/mm/pkeys.c  |   16 
>  2 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/include/uapi/asm/mman.h 
> b/arch/powerpc/include/uapi/asm/mman.h
> index ab45cc2..f272b09 100644
> --- a/arch/powerpc/include/uapi/asm/mman.h
> +++ b/arch/powerpc/include/uapi/asm/mman.h
> @@ -45,4 +45,10 @@
>  #define MAP_HUGE_1GB (30 << MAP_HUGE_SHIFT)  /* 1GB   HugeTLB Page */
>  #define MAP_HUGE_16GB(34 << MAP_HUGE_SHIFT)  /* 16GB  HugeTLB Page */
>  
> +/* override any generic PKEY Permission defines */
> +#define PKEY_DISABLE_EXECUTE   0x4
> +#undef PKEY_ACCESS_MASK
> +#define PKEY_ACCESS_MASK   (PKEY_DISABLE_ACCESS |\
> + PKEY_DISABLE_WRITE  |\
> + PKEY_DISABLE_EXECUTE)
>  #endif /* _UAPI_ASM_POWERPC_MMAN_H */
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index cc5be6a..2282864 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -24,6 +24,14 @@ void __init pkey_initialize(void)
>  {
>   int os_reserved, i;
>  
> + /*
> +  * we define PKEY_DISABLE_EXECUTE in addition to the arch-neutral
> +  * generic defines for PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE.
> +  * Ensure that the bits a distinct.
> +  */
> + BUILD_BUG_ON(PKEY_DISABLE_EXECUTE &
> +  (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));

Will these values every change? It's good to have I guess.

> +
>   /* disable the pkey system till everything
>* is in place. A patch further down the
>* line will enable it.
> @@ -120,10 +128,18 @@ int __arch_set_user_pkey_access(struct task_struct 
> *tsk, int pkey,
>   unsigned long init_val)
>  {
>   u64 new_amr_bits = 0x0ul;
> + u64 new_iamr_bits = 0x0ul;
>  
>   if (!is_pkey_enabled(pkey))
>   return -EINVAL;
>  
> + if ((init_val & PKEY_DISABLE_EXECUTE)) {
> + if (!pkey_execute_disable_support)
> + return -EINVAL;
> + new_iamr_bits |= IAMR_EX_BIT;
> + }
> + init_iamr(pkey, new_iamr_bits);
> +

Where do we check the reserved keys?

Balbir Singh.

Re: [PATCH 04/25] powerpc: helper function to read,write AMR,IAMR,UAMOR registers

[Ram Pai]

On Wed, Oct 18, 2017 at 02:17:35PM +1100, Balbir Singh wrote:
> On Fri,  8 Sep 2017 15:44:52 -0700
> Ram Pai  wrote:
> 
> > Implements helper functions to read and write the key related
> > registers; AMR, IAMR, UAMOR.
> > 
> > AMR register tracks the read,write permission of a key
> > IAMR register tracks the execute permission of a key
> > UAMOR register enables and disables a key
> > 
> > Signed-off-by: Ram Pai 
> > ---
> >  arch/powerpc/include/asm/book3s/64/pgtable.h |   31 
> > ++
> >  1 files changed, 31 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h 
> > b/arch/powerpc/include/asm/book3s/64/pgtable.h
> > index b9aff51..73ed52c 100644
> > --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> > +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> > @@ -438,6 +438,37 @@ static inline void huge_ptep_set_wrprotect(struct 
> > mm_struct *mm,
> > pte_update(mm, addr, ptep, 0, _PAGE_PRIVILEGED, 1);
> >  }
> >  
> > +#include 
> > +static inline u64 read_amr(void)
> > +{
> > +   return mfspr(SPRN_AMR);
> > +}
> > +static inline void write_amr(u64 value)
> > +{
> > +   mtspr(SPRN_AMR, value);
> > +}
> 
> Do we care to validate values or is that in
> the caller


No. Caller is expected to validate the values.

> 
> > +extern bool pkey_execute_disable_support;
> > +static inline u64 read_iamr(void)
> > +{
> > +   if (pkey_execute_disable_support)
> > +   return mfspr(SPRN_IAMR);
> > +   else
> > +   return 0x0UL;
> > +}
> > +static inline void write_iamr(u64 value)
> > +{
> > +   if (pkey_execute_disable_support)
> > +   mtspr(SPRN_IAMR, value);
> > +}
> > +static inline u64 read_uamor(void)
> > +{
> > +   return mfspr(SPRN_UAMOR);
> > +}
> > +static inline void write_uamor(u64 value)
> > +{
> > +   mtspr(SPRN_UAMOR, value);
> > +}
> > +
> >  #define __HAVE_ARCH_PTEP_GET_AND_CLEAR
> >  static inline pte_t ptep_get_and_clear(struct mm_struct *mm,
> >unsigned long addr, pte_t *ptep)
> 
> Looks reasonable otherwise

Thanks!
RP


> Acked-by: Balbir Singh 

-- 
Ram Pai

Re: [RFC v7 02/25] powerpc: track allocation status of all pkeys

[Ram Pai]

On Wed, Oct 18, 2017 at 01:42:49PM +1100, Balbir Singh wrote:
> On Sun, 30 Jul 2017 17:12:03 -0700
> Ram Pai  wrote:
> 
> > Total 32 keys are available on power7 and above. However
> > pkey 0,1 are reserved. So effectively we  have  30 pkeys.
> > 
> > On 4K kernels, we do not  have  5  bits  in  the  PTE to
> > represent  all the keys; we only have 3bits.Two of those
> > keys are reserved; pkey 0 and pkey 1. So effectively  we
> > have 6 pkeys.
> > 
> > This patch keeps track of reserved keys, allocated  keys
> > and keys that are currently free.
> > 
> > Also it  adds  skeletal  functions  and macros, that the
> > architecture-independent code expects to be available.
> > 
> > Signed-off-by: Ram Pai 
> > ---
> >  arch/powerpc/include/asm/book3s/64/mmu.h |9 +++
> >  arch/powerpc/include/asm/mmu_context.h   |1 +
> >  arch/powerpc/include/asm/pkeys.h |   98 
> > -
> >  arch/powerpc/mm/mmu_context_book3s64.c   |2 +
> >  arch/powerpc/mm/pkeys.c  |2 +
> >  5 files changed, 108 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/book3s/64/mmu.h 
> > b/arch/powerpc/include/asm/book3s/64/mmu.h
> > index 77529a3..104ad72 100644
> > --- a/arch/powerpc/include/asm/book3s/64/mmu.h
> > +++ b/arch/powerpc/include/asm/book3s/64/mmu.h
> > @@ -108,6 +108,15 @@ struct patb_entry {
> >  #ifdef CONFIG_SPAPR_TCE_IOMMU
> > struct list_head iommu_group_mem_list;
> >  #endif
> > +
> > +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> > +   /*
> > +* Each bit represents one protection key.
> > +* bit set   -> key allocated
> > +* bit unset -> key available for allocation
> > +*/
> > +   u32 pkey_allocation_map;
> > +#endif
> >  } mm_context_t;
> >  
> >  /*
> > diff --git a/arch/powerpc/include/asm/mmu_context.h 
> > b/arch/powerpc/include/asm/mmu_context.h
> > index 4b93547..4705dab 100644
> > --- a/arch/powerpc/include/asm/mmu_context.h
> > +++ b/arch/powerpc/include/asm/mmu_context.h
> > @@ -184,6 +184,7 @@ static inline bool arch_vma_access_permitted(struct 
> > vm_area_struct *vma,
> >  
> >  #ifndef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> >  #define pkey_initialize()
> > +#define pkey_mm_init(mm)
> >  #endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
> >  
> >  #endif /* __KERNEL__ */
> > diff --git a/arch/powerpc/include/asm/pkeys.h 
> > b/arch/powerpc/include/asm/pkeys.h
> > index 4ccb8f5..def385f 100644
> > --- a/arch/powerpc/include/asm/pkeys.h
> > +++ b/arch/powerpc/include/asm/pkeys.h
> > @@ -2,6 +2,8 @@
> >  #define _ASM_PPC64_PKEYS_H
> >  
> >  extern bool pkey_inited;
> > +extern int pkeys_total; /* total pkeys as per device tree */
> > +extern u32 initial_allocation_mask;/* bits set for reserved keys */
> >  
> >  /*
> >   * powerpc needs an additional vma bit to support 32 keys.
> > @@ -20,21 +22,76 @@
> >  #define VM_PKEY_BIT4   VM_HIGH_ARCH_4
> >  #endif
> >  
> > -#define ARCH_VM_PKEY_FLAGS 0
> > +#define arch_max_pkey()  pkeys_total
> > +#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
> > +   VM_PKEY_BIT3 | VM_PKEY_BIT4)
> > +
> > +#define pkey_alloc_mask(pkey) (0x1 << pkey)
> > +
> > +#define mm_pkey_allocation_map(mm) (mm->context.pkey_allocation_map)
> > +
> > +#define mm_set_pkey_allocated(mm, pkey) {  \
> > +   mm_pkey_allocation_map(mm) |= pkey_alloc_mask(pkey); \
> > +}
> > +
> > +#define mm_set_pkey_free(mm, pkey) {   \
> > +   mm_pkey_allocation_map(mm) &= ~pkey_alloc_mask(pkey);   \
> > +}
> > +
> > +#define mm_set_pkey_is_allocated(mm, pkey) \
> > +   (mm_pkey_allocation_map(mm) & pkey_alloc_mask(pkey))
> > +
> > +#define mm_set_pkey_is_reserved(mm, pkey) (initial_allocation_mask & \
> > +   pkey_alloc_mask(pkey))
> >  
> >  static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
> >  {
> > -   return (pkey == 0);
> > +   /* a reserved key is never considered as 'explicitly allocated' */
> > +   return ((pkey < arch_max_pkey()) &&
> > +   !mm_set_pkey_is_reserved(mm, pkey) &&
> > +   mm_set_pkey_is_allocated(mm, pkey));
> 
> Sounds like this should be called mm_pkey_is_free()

hmm. why?

> 
> >  }
> >  
> > +/*
> > + * Returns a positive, 5-bit key on success, or -1 on failure.
> > + */
> >  static inline int mm_pkey_alloc(struct mm_struct *mm)
> >  {
> > -   return -1;
> > +   /*
> > +* Note: this is the one and only place we make sure
> > +* that the pkey is valid as far as the hardware is
> > +* concerned.  The rest of the kernel trusts that
> > +* only good, valid pkeys come out of here.
> > +*/
> > +   u32 all_pkeys_mask = (u32)(~(0x0));
> > +   int ret;
> > +
> > +   if (!pkey_inited)
> > +   return -1;
> > +   /*
> > +* Are we out of pkeys?  We must handle this specially
> > +* because ffz() behavior is undefined if there are no
> > +* zeros.
> > +*/
> Point A
> 
> > +   if 

Re: [PATCH 06/25] powerpc: cleaup AMR,iAMR when a key is allocated or freed

[Balbir Singh]

On Fri,  8 Sep 2017 15:44:54 -0700
Ram Pai  wrote:

> cleanup the bits corresponding to a key in the AMR, and IAMR
> register, when the key is newly allocated/activated or is freed.
> We dont want some residual bits cause the hardware enforce
> unintended behavior when the key is activated or freed.
> 
> Signed-off-by: Ram Pai 
> ---
>  arch/powerpc/include/asm/pkeys.h |   12 
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/pkeys.h 
> b/arch/powerpc/include/asm/pkeys.h
> index 5a83ed7..53bf13b 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -54,6 +54,8 @@ static inline bool mm_pkey_is_allocated(struct mm_struct 
> *mm, int pkey)
>   mm_set_pkey_is_allocated(mm, pkey));
>  }
>  
> +extern void __arch_activate_pkey(int pkey);
> +extern void __arch_deactivate_pkey(int pkey);
>  /*
>   * Returns a positive, 5-bit key on success, or -1 on failure.
>   */
> @@ -80,6 +82,12 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
>  
>   ret = ffz((u32)mm_pkey_allocation_map(mm));
>   mm_set_pkey_allocated(mm, ret);
> +
> + /*
> +  * enable the key in the hardware
> +  */
> + if (ret > 0)
> + __arch_activate_pkey(ret);
>   return ret;
>  }
>  
> @@ -91,6 +99,10 @@ static inline int mm_pkey_free(struct mm_struct *mm, int 
> pkey)
>   if (!mm_pkey_is_allocated(mm, pkey))
>   return -EINVAL;
>  
> + /*
> +  * Disable the key in the hardware
> +  */
> + __arch_deactivate_pkey(pkey);
>   mm_set_pkey_free(mm, pkey);
>  
>   return 0;

I think some of these patches can be merged, too much fine granularity
is hurting my ability to see the larger function/implementation.

Balbir Singh.

Re: [PATCH 05/25] powerpc: helper functions to initialize AMR, IAMR and UAMOR registers

[Balbir Singh]

On Fri,  8 Sep 2017 15:44:53 -0700
Ram Pai  wrote:

> Introduce  helper functions that can initialize the bits in the AMR,
> IAMR and UAMOR register; the bits that correspond to the given pkey.
> 
> Signed-off-by: Ram Pai 
> ---
>  arch/powerpc/include/asm/pkeys.h |1 +
>  arch/powerpc/mm/pkeys.c  |   46 
> ++
>  2 files changed, 47 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/pkeys.h 
> b/arch/powerpc/include/asm/pkeys.h
> index 133f8c4..5a83ed7 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -26,6 +26,7 @@
>  #define arch_max_pkey()  pkeys_total
>  #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
>   VM_PKEY_BIT3 | VM_PKEY_BIT4)
> +#define AMR_BITS_PER_PKEY 2
>  
>  #define pkey_alloc_mask(pkey) (0x1 << pkey)
>  
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index ebc9e84..178aa33 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -59,3 +59,49 @@ void __init pkey_initialize(void)
>   for (i = 2; i < (pkeys_total - os_reserved); i++)
>   initial_allocation_mask &= ~(0x1<  }
> +
> +#define PKEY_REG_BITS (sizeof(u64)*8)
> +#define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
> +
> +static inline void init_amr(int pkey, u8 init_bits)
> +{
> + u64 new_amr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey));
> + u64 old_amr = read_amr() & ~((u64)(0x3ul) << pkeyshift(pkey));
> +

Do we need to check for reserved keys or that is at a layer above?

> + write_amr(old_amr | new_amr_bits);
> +}
> +
> +static inline void init_iamr(int pkey, u8 init_bits)
> +{
> + u64 new_iamr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey));
> + u64 old_iamr = read_iamr() & ~((u64)(0x3ul) << pkeyshift(pkey));
> +
> + write_iamr(old_iamr | new_iamr_bits);

Do we need to check for reserved keys here?

> +}
> +
> +static void pkey_status_change(int pkey, bool enable)
> +{
> + u64 old_uamor;
> +
> + /* reset the AMR and IAMR bits for this key */
> + init_amr(pkey, 0x0);
> + init_iamr(pkey, 0x0);
> +
> + /* enable/disable key */
> + old_uamor = read_uamor();
> + if (enable)
> + old_uamor |= (0x3ul << pkeyshift(pkey));
> + else
> + old_uamor &= ~(0x3ul << pkeyshift(pkey));
> + write_uamor(old_uamor);
> +}
> +
> +void __arch_activate_pkey(int pkey)
> +{
> + pkey_status_change(pkey, true);
> +}
> +
> +void __arch_deactivate_pkey(int pkey)
> +{
> + pkey_status_change(pkey, false);
> +}


Balbir Singh.

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Michael Ellerman]

Mimi Zohar  writes:
> On Tue, 2017-10-17 at 12:11 +0200, Julia Lawall wrote:
>> On Tue, 17 Oct 2017, Dan Carpenter wrote:
>> > On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote:
>> > > On Tue, 17 Oct 2017, Dan Carpenter wrote:
>> > > > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
>> > > > >
>> > > > > A minor complaint: all commits are missing "Fixes:" tag.
>> > > > >
>> > > >
>> > > > Fixes is only for bug fixes.  These don't fix any bugs.
>> > >
>> > > 0-day seems to put Fixes for everything.  Should they be removed when the
>> > > old code is undesirable but doesn't actually cause a crash, eg out of 
>> > > date
>> > > API.
>> >
>> > Yeah, I feel like Fixes tags don't belong for API updates and cleanups.
>> 
>> OK, I will remove them from the patches that go through me where they
>> don't seem appropriate.
>
> The "Fixes" tag is an indication that the patch should be backported.

No it's not that strong. It's an indication that the patch fixes another
commit, which may or may not mean it should be backported depending on
the preferences of the backporter. If it *does* need backporting then
the Fixes tag helps identify where it should go.

The doco is actually pretty well worded IMO:

  
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n183

  If your patch fixes a bug in a specific commit, e.g. you found an issue using
  ``git bisect``, please use the 'Fixes:' tag with the first 12 characters of
  the SHA-1 ID, and the one line summary.

and:

  
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n602

  A Fixes: tag indicates that the patch fixes an issue in a previous commit. It
  is used to make it easy to determine where a bug originated, which can help
  review a bug fix. This tag also assists the stable kernel team in determining
  which stable kernel versions should receive your fix. This is the preferred
  method for indicating a bug fixed by the patch. See :ref:`describe_changes`
  for more details.


cheers

Re: [PATCH 04/25] powerpc: helper function to read,write AMR,IAMR,UAMOR registers

[Balbir Singh]

On Fri,  8 Sep 2017 15:44:52 -0700
Ram Pai  wrote:

> Implements helper functions to read and write the key related
> registers; AMR, IAMR, UAMOR.
> 
> AMR register tracks the read,write permission of a key
> IAMR register tracks the execute permission of a key
> UAMOR register enables and disables a key
> 
> Signed-off-by: Ram Pai 
> ---
>  arch/powerpc/include/asm/book3s/64/pgtable.h |   31 
> ++
>  1 files changed, 31 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h 
> b/arch/powerpc/include/asm/book3s/64/pgtable.h
> index b9aff51..73ed52c 100644
> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> @@ -438,6 +438,37 @@ static inline void huge_ptep_set_wrprotect(struct 
> mm_struct *mm,
>   pte_update(mm, addr, ptep, 0, _PAGE_PRIVILEGED, 1);
>  }
>  
> +#include 
> +static inline u64 read_amr(void)
> +{
> + return mfspr(SPRN_AMR);
> +}
> +static inline void write_amr(u64 value)
> +{
> + mtspr(SPRN_AMR, value);
> +}

Do we care to validate values or is that in
the caller

> +extern bool pkey_execute_disable_support;
> +static inline u64 read_iamr(void)
> +{
> + if (pkey_execute_disable_support)
> + return mfspr(SPRN_IAMR);
> + else
> + return 0x0UL;
> +}
> +static inline void write_iamr(u64 value)
> +{
> + if (pkey_execute_disable_support)
> + mtspr(SPRN_IAMR, value);
> +}
> +static inline u64 read_uamor(void)
> +{
> + return mfspr(SPRN_UAMOR);
> +}
> +static inline void write_uamor(u64 value)
> +{
> + mtspr(SPRN_UAMOR, value);
> +}
> +
>  #define __HAVE_ARCH_PTEP_GET_AND_CLEAR
>  static inline pte_t ptep_get_and_clear(struct mm_struct *mm,
>  unsigned long addr, pte_t *ptep)

Looks reasonable otherwise

Acked-by: Balbir Singh 

Re: [RFC v7 24/25] powerpc: Deliver SEGV signal on pkey violation

[Ram Pai]

On Wed, Oct 18, 2017 at 01:25:48PM +1100, Balbir Singh wrote:
> On Fri, 18 Aug 2017 15:36:55 -0700
> Ram Pai  wrote:
> 
> > On Sat, Aug 19, 2017 at 07:54:20AM +1000, Benjamin Herrenschmidt wrote:
> > > On Fri, 2017-08-18 at 10:04 -0700, Ram Pai wrote:  
> > > > Assume two threads of a task.
> > > > 
> > > > T1:  mprotect_key(foo, PAGE_SIZE, pkey=4);
> > > > T1:  set AMR to disable access for pkey 4;
> > > > T1:  key fault
> > > > T2: set AMR to enable access to pkey 4;
> > > > T1:  fault handler called.
> > > > This fault handler will see the new AMR and not the
> > > > one at the time of the fault.  
> > > 
> > > You aren't context switching AMR with the threads ? Ugh... something is
> > > very wrong then.  
> > 
> > I do store and restore AMR accross context switch. So nevermind; the
> > above problem cannot happen.
> >
> 
> I think the assumption is that pkey_alloc() will do the right thing
> while allocating keys across threads

It does.  A key allocated to a thread will never be allocated to another
thread.

RP

Re: [PATCH 03/25] powerpc: track allocation status of all pkeys

[Balbir Singh]

On Fri,  8 Sep 2017 15:44:51 -0700
Ram Pai  wrote:

> Total 32 keys are available on power7 and above. However
> pkey 0,1 are reserved. So effectively we  have  30 pkeys.
> 
> On 4K kernels, we do not  have  5  bits  in  the  PTE to
> represent  all the keys; we only have 3bits.Two of those
> keys are reserved; pkey 0 and pkey 1. So effectively  we
> have 6 pkeys.
> 
> This patch keeps track of reserved keys, allocated  keys
> and keys that are currently free.
> 
> Also it  adds  skeletal  functions  and macros, that the
> architecture-independent code expects to be available.
> 
> Signed-off-by: Ram Pai 

I ended up reviewing v7 of the patch. Is this v8?
I think the comments still apply to this revision

Balbir Singh.

Re: [PATCH] powerpc/mm/radix: Drop unneeded NULL check

[Aneesh Kumar K.V]

On 10/17/2017 05:24 PM, Nicholas Piggin wrote:

On Mon, 16 Oct 2017 12:41:00 +0530
"Aneesh Kumar K.V"  wrote:


@@ -175,10 +175,9 @@ void radix__local_flush_tlb_page(struct vm_area_struct 
*vma, unsigned long vmadd
  #ifdef CONFIG_HUGETLB_PAGE
/* need the return fix for nohash.c */
if (vma && is_vm_hugetlb_page(vma))
-   return __local_flush_hugetlb_page(vma, vmaddr);
+   return radix__local_flush_hugetlb_page(vma, vmaddr);
  #endif
-   radix__local_flush_tlb_page_psize(vma ? vma->vm_mm : NULL, vmaddr,
- mmu_virtual_psize);
+   radix__local_flush_tlb_page_psize(vma->vm_mm, vmaddr, 
mmu_virtual_psize);
  }
  EXPORT_SYMBOL(radix__local_flush_tlb_page);
  


Missed the other NULL pointer check in this hunk.


@@ -247,10 +246,9 @@ void radix__flush_tlb_page(struct vm_area_struct *vma, 
unsigned long vmaddr)
  {
  #ifdef CONFIG_HUGETLB_PAGE
if (vma && is_vm_hugetlb_page(vma))
-   return flush_hugetlb_page(vma, vmaddr);
+   return radix__flush_hugetlb_page(vma, vmaddr);
  #endif
-   radix__flush_tlb_page_psize(vma ? vma->vm_mm : NULL, vmaddr,
-   mmu_virtual_psize);
+   radix__flush_tlb_page_psize(vma->vm_mm, vmaddr, mmu_virtual_psize);
  }
  EXPORT_SYMBOL(radix__flush_tlb_page);
  


And another one.



Didn't get that. We should not find vma NULL here. Hence i removed that 
vma == NULL check. Are you suggesting that this is not correct?


-aneesh

Re: [RFC v7 02/25] powerpc: track allocation status of all pkeys

[Balbir Singh]

On Sun, 30 Jul 2017 17:12:03 -0700
Ram Pai  wrote:

> Total 32 keys are available on power7 and above. However
> pkey 0,1 are reserved. So effectively we  have  30 pkeys.
> 
> On 4K kernels, we do not  have  5  bits  in  the  PTE to
> represent  all the keys; we only have 3bits.Two of those
> keys are reserved; pkey 0 and pkey 1. So effectively  we
> have 6 pkeys.
> 
> This patch keeps track of reserved keys, allocated  keys
> and keys that are currently free.
> 
> Also it  adds  skeletal  functions  and macros, that the
> architecture-independent code expects to be available.
> 
> Signed-off-by: Ram Pai 
> ---
>  arch/powerpc/include/asm/book3s/64/mmu.h |9 +++
>  arch/powerpc/include/asm/mmu_context.h   |1 +
>  arch/powerpc/include/asm/pkeys.h |   98 -
>  arch/powerpc/mm/mmu_context_book3s64.c   |2 +
>  arch/powerpc/mm/pkeys.c  |2 +
>  5 files changed, 108 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/book3s/64/mmu.h 
> b/arch/powerpc/include/asm/book3s/64/mmu.h
> index 77529a3..104ad72 100644
> --- a/arch/powerpc/include/asm/book3s/64/mmu.h
> +++ b/arch/powerpc/include/asm/book3s/64/mmu.h
> @@ -108,6 +108,15 @@ struct patb_entry {
>  #ifdef CONFIG_SPAPR_TCE_IOMMU
>   struct list_head iommu_group_mem_list;
>  #endif
> +
> +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> + /*
> +  * Each bit represents one protection key.
> +  * bit set   -> key allocated
> +  * bit unset -> key available for allocation
> +  */
> + u32 pkey_allocation_map;
> +#endif
>  } mm_context_t;
>  
>  /*
> diff --git a/arch/powerpc/include/asm/mmu_context.h 
> b/arch/powerpc/include/asm/mmu_context.h
> index 4b93547..4705dab 100644
> --- a/arch/powerpc/include/asm/mmu_context.h
> +++ b/arch/powerpc/include/asm/mmu_context.h
> @@ -184,6 +184,7 @@ static inline bool arch_vma_access_permitted(struct 
> vm_area_struct *vma,
>  
>  #ifndef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
>  #define pkey_initialize()
> +#define pkey_mm_init(mm)
>  #endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
>  
>  #endif /* __KERNEL__ */
> diff --git a/arch/powerpc/include/asm/pkeys.h 
> b/arch/powerpc/include/asm/pkeys.h
> index 4ccb8f5..def385f 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -2,6 +2,8 @@
>  #define _ASM_PPC64_PKEYS_H
>  
>  extern bool pkey_inited;
> +extern int pkeys_total; /* total pkeys as per device tree */
> +extern u32 initial_allocation_mask;/* bits set for reserved keys */
>  
>  /*
>   * powerpc needs an additional vma bit to support 32 keys.
> @@ -20,21 +22,76 @@
>  #define VM_PKEY_BIT4 VM_HIGH_ARCH_4
>  #endif
>  
> -#define ARCH_VM_PKEY_FLAGS 0
> +#define arch_max_pkey()  pkeys_total
> +#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
> + VM_PKEY_BIT3 | VM_PKEY_BIT4)
> +
> +#define pkey_alloc_mask(pkey) (0x1 << pkey)
> +
> +#define mm_pkey_allocation_map(mm)   (mm->context.pkey_allocation_map)
> +
> +#define mm_set_pkey_allocated(mm, pkey) {\
> + mm_pkey_allocation_map(mm) |= pkey_alloc_mask(pkey); \
> +}
> +
> +#define mm_set_pkey_free(mm, pkey) { \
> + mm_pkey_allocation_map(mm) &= ~pkey_alloc_mask(pkey);   \
> +}
> +
> +#define mm_set_pkey_is_allocated(mm, pkey)   \
> + (mm_pkey_allocation_map(mm) & pkey_alloc_mask(pkey))
> +
> +#define mm_set_pkey_is_reserved(mm, pkey) (initial_allocation_mask & \
> + pkey_alloc_mask(pkey))
>  
>  static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
>  {
> - return (pkey == 0);
> + /* a reserved key is never considered as 'explicitly allocated' */
> + return ((pkey < arch_max_pkey()) &&
> + !mm_set_pkey_is_reserved(mm, pkey) &&
> + mm_set_pkey_is_allocated(mm, pkey));

Sounds like this should be called mm_pkey_is_free()

>  }
>  
> +/*
> + * Returns a positive, 5-bit key on success, or -1 on failure.
> + */
>  static inline int mm_pkey_alloc(struct mm_struct *mm)
>  {
> - return -1;
> + /*
> +  * Note: this is the one and only place we make sure
> +  * that the pkey is valid as far as the hardware is
> +  * concerned.  The rest of the kernel trusts that
> +  * only good, valid pkeys come out of here.
> +  */
> + u32 all_pkeys_mask = (u32)(~(0x0));
> + int ret;
> +
> + if (!pkey_inited)
> + return -1;
> + /*
> +  * Are we out of pkeys?  We must handle this specially
> +  * because ffz() behavior is undefined if there are no
> +  * zeros.
> +  */
Point A

> + if (mm_pkey_allocation_map(mm) == all_pkeys_mask)
> + return -1;
> +
> + ret = ffz((u32)mm_pkey_allocation_map(mm));

Point B

So the allocation occurs from MSB to LSB?  I also think you need
a preempt disable between points A, B.

Is this code reentrant?

> + mm_set_pkey_allocated(mm, 

Re: [RFC v7 24/25] powerpc: Deliver SEGV signal on pkey violation

[Balbir Singh]

On Fri, 18 Aug 2017 15:36:55 -0700
Ram Pai  wrote:

> On Sat, Aug 19, 2017 at 07:54:20AM +1000, Benjamin Herrenschmidt wrote:
> > On Fri, 2017-08-18 at 10:04 -0700, Ram Pai wrote:  
> > > Assume two threads of a task.
> > > 
> > > T1:  mprotect_key(foo, PAGE_SIZE, pkey=4);
> > > T1:  set AMR to disable access for pkey 4;
> > > T1:  key fault
> > > T2: set AMR to enable access to pkey 4;
> > > T1:  fault handler called.
> > > This fault handler will see the new AMR and not the
> > > one at the time of the fault.  
> > 
> > You aren't context switching AMR with the threads ? Ugh... something is
> > very wrong then.  
> 
> I do store and restore AMR accross context switch. So nevermind; the
> above problem cannot happen.
>

I think the assumption is that pkey_alloc() will do the right thing
while allocating keys across threads

Balbir Singh.

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Alexey Kardashevskiy]

On 18/10/17 12:01, Alexey Kardashevskiy wrote:
> On 18/10/17 01:11, Bryant G. Ly wrote:
>> Adding Juan back into the cc: jjalv...@linux.vnet.ibm.com
>>
>>
>> On 10/16/17 10:38 PM, Michael Ellerman wrote:
>>> "Bryant G. Ly"  writes:
 On 10/12/17 1:29 PM, Bjorn Helgaas wrote:
>>> ...
> If that's the case, how to you ever bind a driver to these VFs?  The
> changelog says you don't want VF drivers to load *immediately*, so I
> assume you do want them to load eventually.
>
 The VF's that get dynamically created within the configure SR-IOV call, 
 on the Pseries Platform, wont be matched with a driver. - We do not
 want it to match.

 The Power Hypervisor will load the VFs. The VF's will get assigned(by 
 the user) > via the HMC or Novalink in this environment which will
 then trigger PHYP to load the VF device node to the device tree.
>>> What about the other "Power Hypervisor"? ie. KVM running on Power.
>> This path is only exercised when configuring SR-IOV for Pseries LPAR,
>> therefore it will not affect PowerNV or KVM(opal).
> 
> PowerNV KVM guest is a pseries machine so this code will execute there.
> 
>> Which is the reason for
>> the separation of calls to the machine dependent stuff.
>>> We also use the pseries platform when running under KVM.
>>>
>>> cheers
>>>
>> If anyone plans to enable SR-IOV on Pseries platform, firmware must provide 
>> the
>> resources to enable the VFs and map them with system resources.
> 
> This is what the PowerNV platform does.
> 
>> A new version
>> of the PAPR Document will be added to document these system resources.
> 
> The guest simply gets yet another PCI device, how is IOV different here?
> 
> In regard of EEH, the API does not change afaik, it is up to the hypervisor
> (KVM+QEMU) to handle IOV case correctly.
> 
> 
>> Lastly,
>> we were not aware that there is an intention to enable SR-IOV in adapters 
>> assigned
>> to a VM with Pseries running on KVM.
> 
> There is no any special enablement of IOV for a VM on powernv, once
> configured in the powernv host, we can just pass VFs to QEMU and therefore
> to a pseries guest, it is just a normal PCI device.
> 
> Do you assign a PF to a VM and create VFs from inside the VM? Or only pHyp
> is allowed to do that? Sorry, I know nothing about pHyp on this matter.
> 

Never mind this last question, I've just read the entire thread about this
hosting partition thing.



-- 
Alexey

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Alexey Kardashevskiy]

On 18/10/17 01:11, Bryant G. Ly wrote:
> Adding Juan back into the cc: jjalv...@linux.vnet.ibm.com
> 
> 
> On 10/16/17 10:38 PM, Michael Ellerman wrote:
>> "Bryant G. Ly"  writes:
>>> On 10/12/17 1:29 PM, Bjorn Helgaas wrote:
>> ...
 If that's the case, how to you ever bind a driver to these VFs?  The
 changelog says you don't want VF drivers to load *immediately*, so I
 assume you do want them to load eventually.

>>> The VF's that get dynamically created within the configure SR-IOV call, 
>>> on the Pseries Platform, wont be matched with a driver. - We do not
>>> want it to match.
>>>
>>> The Power Hypervisor will load the VFs. The VF's will get assigned(by 
>>> the user) > via the HMC or Novalink in this environment which will
>>> then trigger PHYP to load the VF device node to the device tree.
>> What about the other "Power Hypervisor"? ie. KVM running on Power.
> This path is only exercised when configuring SR-IOV for Pseries LPAR,
> therefore it will not affect PowerNV or KVM(opal).

PowerNV KVM guest is a pseries machine so this code will execute there.

> Which is the reason for
> the separation of calls to the machine dependent stuff.
>> We also use the pseries platform when running under KVM.
>>
>> cheers
>>
> If anyone plans to enable SR-IOV on Pseries platform, firmware must provide 
> the
> resources to enable the VFs and map them with system resources.

This is what the PowerNV platform does.

> A new version
> of the PAPR Document will be added to document these system resources.

The guest simply gets yet another PCI device, how is IOV different here?

In regard of EEH, the API does not change afaik, it is up to the hypervisor
(KVM+QEMU) to handle IOV case correctly.


> Lastly,
> we were not aware that there is an intention to enable SR-IOV in adapters 
> assigned
> to a VM with Pseries running on KVM.

There is no any special enablement of IOV for a VM on powernv, once
configured in the powernv host, we can just pass VFs to QEMU and therefore
to a pseries guest, it is just a normal PCI device.

Do you assign a PF to a VM and create VFs from inside the VM? Or only pHyp
is allowed to do that? Sorry, I know nothing about pHyp on this matter.


> Furthermore, this could be left as a todo
> for the future if this type of configuration is needed.




-- 
Alexey

[PATCH v5 18/18] ima: Write modsig to the measurement list

[Thiago Jung Bauermann]

Add modsig support for templates which require the contents of the file
signature to be included in the measurement list.

Suggested-by: Mimi Zohar 
Signed-off-by: Thiago Jung Bauermann 
---
 security/integrity/ima/ima.h  |  8 
 security/integrity/ima/ima_api.c  |  8 +++-
 security/integrity/ima/ima_main.c | 30 +-
 security/integrity/ima/ima_modsig.c   | 20 +++-
 security/integrity/ima/ima_template.c | 12 
 security/integrity/ima/ima_template_lib.c | 17 +++--
 6 files changed, 90 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index b082138461b3..68f471666151 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -148,6 +148,7 @@ int ima_init_crypto(void);
 void ima_putc(struct seq_file *m, void *data, int datalen);
 void ima_print_digest(struct seq_file *m, u8 *digest, u32 size);
 struct ima_template_desc *ima_template_desc_current(void);
+bool ima_current_template_has_sig(void);
 int ima_restore_measurement_entry(struct ima_template_entry *entry);
 int ima_restore_measurement_list(loff_t bufsize, void *buf);
 int ima_measurements_show(struct seq_file *m, void *v);
@@ -264,6 +265,7 @@ int ima_read_modsig(enum ima_hooks func, const void *buf, 
loff_t buf_len,
int *xattr_len);
 int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr, enum hash_algo *algo,
void const **hash, u8 *len);
+int ima_modsig_serialize_data(struct evm_ima_xattr_data **data, int *data_len);
 int ima_modsig_verify(const unsigned int keyring_id,
  struct evm_ima_xattr_data *hdr);
 void ima_free_xattr_data(struct evm_ima_xattr_data *hdr);
@@ -334,6 +336,12 @@ static inline int ima_get_modsig_hash(struct 
evm_ima_xattr_data *hdr,
return -ENOTSUPP;
 }
 
+static inline int ima_modsig_serialize_data(struct evm_ima_xattr_data **data,
+   int *data_len)
+{
+   return -ENOTSUPP;
+}
+
 static inline int ima_modsig_verify(const unsigned int keyring_id,
struct evm_ima_xattr_data *hdr)
 {
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c6d346e9f708..59a5b044b48b 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -284,7 +284,13 @@ void ima_store_measurement(struct integrity_iint_cache 
*iint,
xattr_len, NULL};
int violation = 0;
 
-   if (iint->measured_pcrs & (0x1 << pcr))
+   /*
+* We still need to store the measurement in the case of MODSIG because
+* we only have its contents to put in the list at the time of
+* appraisal. See comment in process_measurement for more details.
+*/
+   if ((iint->measured_pcrs & (0x1 << pcr)) &&
+   (!xattr_value || xattr_value->type != IMA_MODSIG))
return;
 
result = ima_alloc_init_template(_data, );
diff --git a/security/integrity/ima/ima_main.c 
b/security/integrity/ima/ima_main.c
index 6a2d960fbd92..0d3390de7432 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -246,7 +246,35 @@ static int process_measurement(struct file *file, char 
*buf, loff_t size,
rc = ima_appraise_measurement(func, iint, file, buf, size,
  pathname, _value,
  _len, opened);
-   if (action & IMA_MEASURE)
+
+   /*
+* MODSIG has one corner case we need to deal with here:
+*
+* Suppose the policy has one measure rule for one hook and an appraise
+* rule for a different hook. Suppose also that the template requires
+* the signature to be stored in the measurement list.
+*
+* If a file containing a MODSIG is measured by the first hook before
+* being appraised by the second one, the corresponding entry in the
+* measurement list will not contain the MODSIG because we only fetch it
+* for IMA_APPRAISAL. We don't fetch it earlier because if the file has
+* both a DIGSIG and a MODSIG it's not possible to know which one will
+* be valid without actually doing the appraisal.
+*
+* Therefore, after appraisal of a MODSIG signature we need to store the
+* measurement again if the current template requires storing the
+* signature.
+*
+* With the opposite ordering (the appraise rule triggering before the
+* measurement rule) there is the same problem but it's not possible to
+* do anything about it because at the time we are appraising the
+* signature it's impossible to know whether a measurement will ever
+* need to be stored for 

[PATCH v5 17/18] ima: Implement support for module-style appended signatures

[Thiago Jung Bauermann]

This patch actually implements the appraise_type=modsig option, allowing
IMA to read and verify modsig signatures

Signed-off-by: Thiago Jung Bauermann 
---
 security/integrity/ima/ima.h  |  17 +++--
 security/integrity/ima/ima_appraise.c | 119 --
 security/integrity/ima/ima_main.c |   7 +-
 3 files changed, 128 insertions(+), 15 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index eb58af06566f..b082138461b3 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -157,7 +157,8 @@ void ima_init_template_list(void);
 
 static inline bool is_ima_sig(const struct evm_ima_xattr_data *xattr_value)
 {
-   return xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG;
+   return xattr_value && (xattr_value->type == EVM_IMA_XATTR_DIGSIG ||
+  xattr_value->type == IMA_MODSIG);
 }
 
 /*
@@ -243,9 +244,10 @@ int ima_policy_show(struct seq_file *m, void *v);
 #ifdef CONFIG_IMA_APPRAISE
 int ima_appraise_measurement(enum ima_hooks func,
 struct integrity_iint_cache *iint,
-struct file *file, const unsigned char *filename,
-struct evm_ima_xattr_data *xattr_value,
-int xattr_len, int opened);
+struct file *file, const void *buf, loff_t size,
+const unsigned char *filename,
+struct evm_ima_xattr_data **xattr_value,
+int *xattr_len, int opened);
 int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func);
 void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file);
 enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
@@ -270,10 +272,11 @@ void ima_free_xattr_data(struct evm_ima_xattr_data *hdr);
 #else
 static inline int ima_appraise_measurement(enum ima_hooks func,
   struct integrity_iint_cache *iint,
-  struct file *file,
+  struct file *file, const void *buf,
+  loff_t size,
   const unsigned char *filename,
-  struct evm_ima_xattr_data 
*xattr_value,
-  int xattr_len, int opened)
+  struct evm_ima_xattr_data 
**xattr_value,
+  int *xattr_len, int opened)
 {
return INTEGRITY_UNKNOWN;
 }
diff --git a/security/integrity/ima/ima_appraise.c 
b/security/integrity/ima/ima_appraise.c
index 58e147049e98..108690741c1a 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -190,6 +190,45 @@ int ima_read_xattr(struct dentry *dentry,
return ret;
 }
 
+static int appraise_modsig(struct integrity_iint_cache *iint,
+  struct evm_ima_xattr_data *xattr_value,
+  int xattr_len)
+{
+   enum hash_algo algo;
+   const void *digest;
+   void *buf;
+   int rc, len;
+   u8 dig_len;
+
+   rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA, xattr_value);
+   if (rc)
+   return rc;
+
+   /*
+* The signature is good. Now let's put the sig hash
+* into the iint cache so that it gets stored in the
+* measurement list.
+*/
+
+   rc = ima_get_modsig_hash(xattr_value, , , _len);
+   if (rc)
+   return rc;
+
+   len = sizeof(iint->ima_hash) + dig_len;
+   buf = krealloc(iint->ima_hash, len, GFP_NOFS);
+   if (!buf)
+   return -ENOMEM;
+
+   iint->ima_hash = buf;
+   iint->flags |= IMA_DIGSIG;
+   iint->ima_hash->algo = algo;
+   iint->ima_hash->length = dig_len;
+
+   memcpy(iint->ima_hash->digest, digest, dig_len);
+
+   return 0;
+}
+
 /*
  * ima_appraise_measurement - appraise file measurement
  *
@@ -200,18 +239,28 @@ int ima_read_xattr(struct dentry *dentry,
  */
 int ima_appraise_measurement(enum ima_hooks func,
 struct integrity_iint_cache *iint,
-struct file *file, const unsigned char *filename,
-struct evm_ima_xattr_data *xattr_value,
-int xattr_len, int opened)
+struct file *file, const void *buf, loff_t size,
+const unsigned char *filename,
+struct evm_ima_xattr_data **xattr_value_,
+int *xattr_len_, int opened)
 {
static const char op[] = "appraise_data";
const char *cause = "unknown";
struct dentry *dentry = file_dentry(file);
struct inode *inode 

[PATCH v5 16/18] ima: Add functions to read and verify a modsig signature

[Thiago Jung Bauermann]

This is the code needed by IMA-appraise to work with modsig signatures.
It will be used by the next patch.

Signed-off-by: Thiago Jung Bauermann 
---
 security/integrity/ima/Kconfig  |   3 +
 security/integrity/ima/ima.h|  34 +++
 security/integrity/ima/ima_modsig.c | 119 
 security/integrity/integrity.h  |   1 +
 4 files changed, 157 insertions(+)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 40c6618d00e6..55f734a6124b 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -166,6 +166,9 @@ config IMA_APPRAISE_BOOTPARAM
 config IMA_APPRAISE_MODSIG
bool "Support module-style signatures for appraisal"
depends on IMA_APPRAISE
+   depends on INTEGRITY_ASYMMETRIC_KEYS
+   select PKCS7_MESSAGE_PARSER
+   select MODULE_SIG_FORMAT
default n
help
   Adds support for signatures appended to files. The format of the
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 156ba218e0b6..eb58af06566f 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -257,6 +257,14 @@ int ima_read_xattr(struct dentry *dentry,
 
 #ifdef CONFIG_IMA_APPRAISE_MODSIG
 bool ima_hook_supports_modsig(enum ima_hooks func);
+int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
+   struct evm_ima_xattr_data **xattr_value,
+   int *xattr_len);
+int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr, enum hash_algo *algo,
+   void const **hash, u8 *len);
+int ima_modsig_verify(const unsigned int keyring_id,
+ struct evm_ima_xattr_data *hdr);
+void ima_free_xattr_data(struct evm_ima_xattr_data *hdr);
 #endif
 
 #else
@@ -307,6 +315,32 @@ static inline bool ima_hook_supports_modsig(enum ima_hooks 
func)
 {
return false;
 }
+
+static inline int ima_read_modsig(enum ima_hooks func, const void *buf,
+ loff_t buf_len,
+ struct evm_ima_xattr_data **xattr_value,
+ int *xattr_len)
+{
+   return -ENOTSUPP;
+}
+
+static inline int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr,
+ enum hash_algo *algo, void const **hash,
+ u8 *len)
+{
+   return -ENOTSUPP;
+}
+
+static inline int ima_modsig_verify(const unsigned int keyring_id,
+   struct evm_ima_xattr_data *hdr)
+{
+   return -ENOTSUPP;
+}
+
+static inline void ima_free_xattr_data(struct evm_ima_xattr_data *hdr)
+{
+   kfree(hdr);
+}
 #endif
 
 /* LSM based policy rules require audit */
diff --git a/security/integrity/ima/ima_modsig.c 
b/security/integrity/ima/ima_modsig.c
index 452ce6048a7e..2786aa97060e 100644
--- a/security/integrity/ima/ima_modsig.c
+++ b/security/integrity/ima/ima_modsig.c
@@ -16,8 +16,19 @@
  * GNU General Public License for more details.
  */
 
+#include 
+#include 
+#include 
+
 #include "ima.h"
 
+struct modsig_hdr {
+   uint8_t type;   /* Should be IMA_MODSIG. */
+   const void *data;   /* Pointer to data covered by pkcs7_msg. */
+   size_t data_len;
+   struct pkcs7_message *pkcs7_msg;
+};
+
 /**
  * ima_hook_supports_modsig - can the policy allow modsig for this hook?
  *
@@ -37,3 +48,111 @@ bool ima_hook_supports_modsig(enum ima_hooks func)
return false;
}
 }
+
+int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
+   struct evm_ima_xattr_data **xattr_value,
+   int *xattr_len)
+{
+   const size_t marker_len = sizeof(MODULE_SIG_STRING) - 1;
+   const struct module_signature *sig;
+   struct modsig_hdr *hdr;
+   size_t sig_len;
+   const void *p;
+   int rc;
+
+   /*
+* Not supposed to happen. Hooks that support modsig are
+* whitelisted when parsing the policy using
+* ima_hooks_supports_modsig.
+*/
+   if (!buf || !buf_len) {
+   WARN_ONCE(true, "%s doesn't support modsig\n",
+ func_tokens[func]);
+   return -ENOENT;
+   } else if (buf_len <= marker_len + sizeof(*sig))
+   return -ENOENT;
+
+   p = buf + buf_len - marker_len;
+   if (memcmp(p, MODULE_SIG_STRING, marker_len))
+   return -ENOENT;
+
+   buf_len -= marker_len;
+   sig = (const struct module_signature *) (p - sizeof(*sig));
+
+   rc = validate_module_sig(sig, buf_len);
+   if (rc)
+   return rc;
+
+   sig_len = be32_to_cpu(sig->sig_len);
+   buf_len -= sig_len + sizeof(*sig);
+
+   hdr = kmalloc(sizeof(*hdr), GFP_KERNEL);
+   if (!hdr)
+   return -ENOMEM;
+
+   hdr->pkcs7_msg = pkcs7_parse_message(buf + buf_len, sig_len);
+   if 

[PATCH v5 15/18] ima: Add modsig appraise_type option for module-style appended signatures

[Thiago Jung Bauermann]

This patch introduces the modsig keyword to the IMA policy syntax to
specify that a given hook should expect the file to have the IMA signature
appended to it. Here is how it can be used in a rule:

appraise func=KEXEC_KERNEL_CHECK appraise_type=modsig|imasig

With this rule, IMA will accept either an appended signature or a signature
stored in the extended attribute. In that case, it will first check whether
there is an appended signature, and if not it will read it from the
extended attribute.

For now, the rule above will behave exactly the same as if
appraise_type=imasig was specified because the actual modsig implementation
will be introduced in a separate patch.

Suggested-by: Mimi Zohar 
Signed-off-by: Thiago Jung Bauermann 
---
 Documentation/ABI/testing/ima_policy |  6 +-
 security/integrity/ima/Kconfig   | 10 +
 security/integrity/ima/Makefile  |  1 +
 security/integrity/ima/ima.h | 11 ++
 security/integrity/ima/ima_modsig.c  | 39 
 security/integrity/ima/ima_policy.c  | 12 +--
 security/integrity/integrity.h   |  3 ++-
 7 files changed, 78 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy 
b/Documentation/ABI/testing/ima_policy
index e76432b9954d..ced1e1835a30 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -36,7 +36,7 @@ Description:
euid:= decimal value
fowner:= decimal value
lsm:are LSM specific
-   option: appraise_type:= [imasig]
+   option: appraise_type:= [imasig] [modsig|imasig]
pcr:= decimal value
 
default policy:
@@ -102,3 +102,7 @@ Description:
 
measure func=KEXEC_KERNEL_CHECK pcr=4
measure func=KEXEC_INITRAMFS_CHECK pcr=5
+
+   Example of appraise rule allowing modsig appended signatures:
+
+   appraise func=KEXEC_KERNEL_CHECK 
appraise_type=modsig|imasig
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 35ef69312811..40c6618d00e6 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -163,6 +163,16 @@ config IMA_APPRAISE_BOOTPARAM
  This option enables the different "ima_appraise=" modes
  (eg. fix, log) from the boot command line.
 
+config IMA_APPRAISE_MODSIG
+   bool "Support module-style signatures for appraisal"
+   depends on IMA_APPRAISE
+   default n
+   help
+  Adds support for signatures appended to files. The format of the
+  appended signature is the same used for signed kernel modules.
+  The modsig keyword can be used in the IMA policy to allow a hook
+  to accept such signatures.
+
 config IMA_TRUSTED_KEYRING
bool "Require all keys on the .ima keyring be signed (deprecated)"
depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 29f198bde02b..c72026acecc3 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -8,5 +8,6 @@ obj-$(CONFIG_IMA) += ima.o
 ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
 ima_policy.o ima_template.o ima_template_lib.o
 ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
+ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
 ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
 obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index c9ba4362760b..156ba218e0b6 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -255,6 +255,10 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data 
*xattr_value,
 int ima_read_xattr(struct dentry *dentry,
   struct evm_ima_xattr_data **xattr_value);
 
+#ifdef CONFIG_IMA_APPRAISE_MODSIG
+bool ima_hook_supports_modsig(enum ima_hooks func);
+#endif
+
 #else
 static inline int ima_appraise_measurement(enum ima_hooks func,
   struct integrity_iint_cache *iint,
@@ -298,6 +302,13 @@ static inline int ima_read_xattr(struct dentry *dentry,
 
 #endif /* CONFIG_IMA_APPRAISE */
 
+#ifndef CONFIG_IMA_APPRAISE_MODSIG
+static inline bool ima_hook_supports_modsig(enum ima_hooks func)
+{
+   return false;
+}
+#endif
+
 /* LSM based policy rules require audit */
 #ifdef CONFIG_IMA_LSM_RULES
 
diff --git a/security/integrity/ima/ima_modsig.c 
b/security/integrity/ima/ima_modsig.c
new file mode 100644
index ..452ce6048a7e
--- /dev/null
+++ b/security/integrity/ima/ima_modsig.c
@@ -0,0 +1,39 @@
+/*
+ * IMA support for appraising module-style appended signatures.
+ *
+ * Copyright (C) 2017  IBM Corporation
+ *
+ * Author:
+ * Thiago Jung Bauermann 

[PATCH v5 14/18] integrity: Introduce integrity_keyring_from_id

[Thiago Jung Bauermann]

IMA will need to obtain the keyring used to verify file signatures so that
it can verify the module-style signature appended to files.

Signed-off-by: Thiago Jung Bauermann 
---
 security/integrity/digsig.c| 28 +++-
 security/integrity/integrity.h |  1 +
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 6f9e4ce568cd..935f1b3258e7 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -48,11 +48,10 @@ static bool init_keyring __initdata;
 #define restrict_link_to_ima restrict_link_by_builtin_trusted
 #endif
 
-int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
-   const char *digest, int digestlen)
+struct key *integrity_keyring_from_id(const unsigned int id)
 {
-   if (id >= INTEGRITY_KEYRING_MAX || siglen < 2)
-   return -EINVAL;
+   if (id >= INTEGRITY_KEYRING_MAX)
+   return ERR_PTR(-EINVAL);
 
if (!keyring[id]) {
keyring[id] =
@@ -61,18 +60,29 @@ int integrity_digsig_verify(const unsigned int id, const 
char *sig, int siglen,
int err = PTR_ERR(keyring[id]);
pr_err("no %s keyring: %d\n", keyring_name[id], err);
keyring[id] = NULL;
-   return err;
+   return ERR_PTR(err);
}
}
 
+   return keyring[id];
+}
+
+int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
+   const char *digest, int digestlen)
+{
+   struct key *keyring = integrity_keyring_from_id(id);
+
+   if (IS_ERR(keyring) || siglen < 2)
+   return PTR_ERR(keyring);
+
switch (sig[1]) {
case 1:
/* v1 API expect signature without xattr type */
-   return digsig_verify(keyring[id], sig + 1, siglen - 1,
-digest, digestlen);
+   return digsig_verify(keyring, sig + 1, siglen - 1, digest,
+digestlen);
case 2:
-   return asymmetric_verify(keyring[id], sig, siglen,
-digest, digestlen);
+   return asymmetric_verify(keyring, sig, siglen, digest,
+digestlen);
}
 
return -EOPNOTSUPP;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index dd7ac4bfc89a..657ce4558984 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -132,6 +132,7 @@ int integrity_kernel_read(struct file *file, loff_t offset,
 
 #ifdef CONFIG_INTEGRITY_SIGNATURE
 
+struct key *integrity_keyring_from_id(const unsigned int id);
 int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
const char *digest, int digestlen);
 

[PATCH v5 13/18] PKCS#7: Introduce pkcs7_get_message_sig and verify_pkcs7_message_sig

[Thiago Jung Bauermann]

IMA will need to access the digest used in the signature so that it can
verify files containing module-style appended signatures. For this purpose,
add function pkcs7_get_message_sig.

It will also need to verify an already parsed PKCS#7 message. For this
purpose, add function verify_pkcs7_message_signature which takes a struct
pkcs7_message for verification instead of the raw bytes that
verify_pkcs7_signature takes.

Signed-off-by: Thiago Jung Bauermann 
---
 certs/system_keyring.c| 60 +--
 crypto/asymmetric_keys/pkcs7_parser.c | 12 +++
 include/crypto/pkcs7.h|  2 ++
 include/linux/verification.h  | 10 ++
 4 files changed, 68 insertions(+), 16 deletions(-)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 6251d1b27f0c..6a8684959780 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -190,33 +190,26 @@ late_initcall(load_system_certificate_list);
 #ifdef CONFIG_SYSTEM_DATA_VERIFICATION
 
 /**
- * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
+ * verify_pkcs7_message_sig - Verify a PKCS#7-based signature on system data.
  * @data: The data to be verified (NULL if expecting internal data).
  * @len: Size of @data.
- * @raw_pkcs7: The PKCS#7 message that is the signature.
- * @pkcs7_len: The size of @raw_pkcs7.
+ * @pkcs7: The PKCS#7 message that is the signature.
  * @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
  * (void *)1UL for all trusted keys).
  * @usage: The use to which the key is being put.
  * @view_content: Callback to gain access to content.
  * @ctx: Context for callback.
  */
-int verify_pkcs7_signature(const void *data, size_t len,
-  const void *raw_pkcs7, size_t pkcs7_len,
-  struct key *trusted_keys,
-  enum key_being_used_for usage,
-  int (*view_content)(void *ctx,
-  const void *data, size_t len,
-  size_t asn1hdrlen),
-  void *ctx)
+int verify_pkcs7_message_sig(const void *data, size_t len,
+struct pkcs7_message *pkcs7,
+struct key *trusted_keys,
+enum key_being_used_for usage,
+int (*view_content)(void *ctx, const void *data,
+size_t len, size_t asn1hdrlen),
+void *ctx)
 {
-   struct pkcs7_message *pkcs7;
int ret;
 
-   pkcs7 = pkcs7_parse_message(raw_pkcs7, pkcs7_len);
-   if (IS_ERR(pkcs7))
-   return PTR_ERR(pkcs7);
-
/* The data should be detached - so we need to supply it. */
if (data && pkcs7_supply_detached_data(pkcs7, data, len) < 0) {
pr_err("PKCS#7 signature with non-detached data\n");
@@ -258,6 +251,41 @@ int verify_pkcs7_signature(const void *data, size_t len,
}
 
 error:
+   pr_devel("<==%s() = %d\n", __func__, ret);
+   return ret;
+}
+
+/**
+ * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
+ * @data: The data to be verified (NULL if expecting internal data).
+ * @len: Size of @data.
+ * @raw_pkcs7: The PKCS#7 message that is the signature.
+ * @pkcs7_len: The size of @raw_pkcs7.
+ * @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
+ * (void *)1UL for all trusted keys).
+ * @usage: The use to which the key is being put.
+ * @view_content: Callback to gain access to content.
+ * @ctx: Context for callback.
+ */
+int verify_pkcs7_signature(const void *data, size_t len,
+  const void *raw_pkcs7, size_t pkcs7_len,
+  struct key *trusted_keys,
+  enum key_being_used_for usage,
+  int (*view_content)(void *ctx,
+  const void *data, size_t len,
+  size_t asn1hdrlen),
+  void *ctx)
+{
+   struct pkcs7_message *pkcs7;
+   int ret;
+
+   pkcs7 = pkcs7_parse_message(raw_pkcs7, pkcs7_len);
+   if (IS_ERR(pkcs7))
+   return PTR_ERR(pkcs7);
+
+   ret = verify_pkcs7_message_sig(data, len, pkcs7, trusted_keys, usage,
+  view_content, ctx);
+
pkcs7_free_message(pkcs7);
pr_devel("<==%s() = %d\n", __func__, ret);
return ret;
diff --git a/crypto/asymmetric_keys/pkcs7_parser.c 
b/crypto/asymmetric_keys/pkcs7_parser.c
index af4cd8649117..e41beda297a8 100644
--- a/crypto/asymmetric_keys/pkcs7_parser.c
+++ b/crypto/asymmetric_keys/pkcs7_parser.c
@@ -673,3 +673,15 @@ int pkcs7_note_signed_info(void *context, size_t 

[PATCH v5 12/18] MODSIGN: Export module signature definitions

[Thiago Jung Bauermann]

IMA will use the module_signature format for append signatures, so export
the relevant definitions and factor out the code which verifies that the
appended signature trailer is valid.

Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it
and be able to use validate_module_signature without having to depend on
CONFIG_MODULE_SIG.

Signed-off-by: Thiago Jung Bauermann 
---
 include/linux/module.h   |  3 --
 include/linux/module_signature.h | 47 +
 init/Kconfig |  6 +++-
 kernel/Makefile  |  2 +-
 kernel/module.c  |  1 +
 kernel/module_signing.c  | 74 +---
 6 files changed, 85 insertions(+), 48 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index fe5aa3736707..108874af9838 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -23,9 +23,6 @@
 #include 
 #include 
 
-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
-#define MODULE_SIG_STRING "~Module signature appended~\n"
-
 /* Not Yet Implemented */
 #define MODULE_SUPPORTED_DEVICE(name)
 
diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
new file mode 100644
index ..e80728e5b86c
--- /dev/null
+++ b/include/linux/module_signature.h
@@ -0,0 +1,47 @@
+/* Module signature handling.
+ *
+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowe...@redhat.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+
+#ifndef _LINUX_MODULE_SIGNATURE_H
+#define _LINUX_MODULE_SIGNATURE_H
+
+/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
+#define MODULE_SIG_STRING "~Module signature appended~\n"
+
+enum pkey_id_type {
+   PKEY_ID_PGP,/* OpenPGP generated key ID */
+   PKEY_ID_X509,   /* X.509 arbitrary subjectKeyIdentifier */
+   PKEY_ID_PKCS7,  /* Signature in PKCS#7 message */
+};
+
+/*
+ * Module signature information block.
+ *
+ * The constituents of the signature section are, in order:
+ *
+ * - Signer's name
+ * - Key identifier
+ * - Signature data
+ * - Information block
+ */
+struct module_signature {
+   u8  algo;   /* Public-key crypto algorithm [0] */
+   u8  hash;   /* Digest algorithm [0] */
+   u8  id_type;/* Key identifier type [PKEY_ID_PKCS7] */
+   u8  signer_len; /* Length of signer's name [0] */
+   u8  key_id_len; /* Length of key identifier [0] */
+   u8  __pad[3];
+   __be32  sig_len;/* Length of signature data */
+};
+
+int validate_module_sig(const struct module_signature *ms, size_t file_len);
+int mod_verify_sig(const void *mod, unsigned long *_modlen);
+
+#endif /* _LINUX_MODULE_SIGNATURE_H */
diff --git a/init/Kconfig b/init/Kconfig
index 78cb2461012e..230e9f3aedaf 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1748,7 +1748,7 @@ config MODULE_SRCVERSION_ALL
 config MODULE_SIG
bool "Module signature verification"
depends on MODULES
-   select SYSTEM_DATA_VERIFICATION
+   select MODULE_SIG_FORMAT
help
  Check modules for valid signatures upon load: the signature
  is simply appended to the module. For more information see
@@ -1763,6 +1763,10 @@ config MODULE_SIG
  debuginfo strip done by some packagers (such as rpmbuild) and
  inclusion into an initramfs that wants the module size reduced.
 
+config MODULE_SIG_FORMAT
+   def_bool n
+   select SYSTEM_DATA_VERIFICATION
+
 config MODULE_SIG_FORCE
bool "Require modules to be validly signed"
depends on MODULE_SIG
diff --git a/kernel/Makefile b/kernel/Makefile
index ed470aac53da..20fd6d76232e 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -57,7 +57,7 @@ obj-y += up.o
 endif
 obj-$(CONFIG_UID16) += uid16.o
 obj-$(CONFIG_MODULES) += module.o
-obj-$(CONFIG_MODULE_SIG) += module_signing.o
+obj-$(CONFIG_MODULE_SIG_FORMAT) += module_signing.o
 obj-$(CONFIG_KALLSYMS) += kallsyms.o
 obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
 obj-$(CONFIG_CRASH_CORE) += crash_core.o
diff --git a/kernel/module.c b/kernel/module.c
index de66ec825992..a259e9df570d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index 937c844bee4a..204c60d4cc9f 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -11,36 +11,38 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include "module-internal.h"
 
-enum pkey_id_type {
-   PKEY_ID_PGP,/* OpenPGP 

[PATCH v5 11/18] ima: Export func_tokens

[Thiago Jung Bauermann]

ima_read_modsig will need it so that it can show an error message.

Signed-off-by: Thiago Jung Bauermann 
---
 security/integrity/ima/ima.h|  2 ++
 security/integrity/ima/ima_policy.c | 12 ++--
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 4e61d3189b72..c9ba4362760b 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -195,6 +195,8 @@ enum ima_hooks {
__ima_hooks(__ima_hook_enumify)
 };
 
+extern const char *const func_tokens[];
+
 /* LIM API function definitions */
 int ima_get_action(struct inode *inode, int mask,
   enum ima_hooks func, int *pcr);
diff --git a/security/integrity/ima/ima_policy.c 
b/security/integrity/ima/ima_policy.c
index efd8e1c60c10..7355d4ab7bf4 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -959,6 +959,12 @@ void ima_delete_rules(void)
}
 }
 
+#define __ima_hook_stringify(str)  (#str),
+
+const char *const func_tokens[] = {
+   __ima_hooks(__ima_hook_stringify)
+};
+
 #ifdef CONFIG_IMA_READ_POLICY
 enum {
mask_exec = 0, mask_write, mask_read, mask_append
@@ -971,12 +977,6 @@ static const char *const mask_tokens[] = {
"MAY_APPEND"
 };
 
-#define __ima_hook_stringify(str)  (#str),
-
-static const char *const func_tokens[] = {
-   __ima_hooks(__ima_hook_stringify)
-};
-
 void *ima_policy_start(struct seq_file *m, loff_t *pos)
 {
loff_t l = *pos;

[PATCH v5 10/18] ima: Store measurement after appraisal

[Thiago Jung Bauermann]

When module-style signatures appended at the end of files are supported for
IMA appraisal, the code will fallback to the xattr signature if the
appended one fails to verify.

The problem is that we don't know whether we need to fallback to the xattr
signature until the appraise step, and by then the measure step was already
completed and would need to be done again in case the template includes the
signature.

To avoid this problem, do the appraisal first so that the correct signature
is stored by the template in the measure step.

Suggested-by: Mimi Zohar 
Signed-off-by: Thiago Jung Bauermann 
---
 security/integrity/ima/ima_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/ima_main.c 
b/security/integrity/ima/ima_main.c
index 747a4fd9e2de..8e96450e27f5 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -242,12 +242,12 @@ static int process_measurement(struct file *file, char 
*buf, loff_t size,
if (!pathbuf)   /* ima_rdwr_violation possibly pre-fetched */
pathname = ima_d_path(>f_path, , filename);
 
-   if (action & IMA_MEASURE)
-   ima_store_measurement(iint, file, pathname,
- xattr_value, xattr_len, pcr);
if (rc == 0 && (action & IMA_APPRAISE_SUBMASK))
rc = ima_appraise_measurement(func, iint, file, pathname,
  xattr_value, xattr_len, opened);
+   if (action & IMA_MEASURE)
+   ima_store_measurement(iint, file, pathname,
+ xattr_value, xattr_len, pcr);
if (action & IMA_AUDIT)
ima_audit_measurement(iint, pathname);
 

[PATCH v5 09/18] ima: Don't pass xattr value to EVM xattr verification.

[Thiago Jung Bauermann]

The patch implementing modsig support will retry verifying the xattr
signature if the modsig verification fails, and if we have already passed
the modsig as the xattr_value we'll have problems if we pass the xattr sig
in the second call to evm_verifyxattr.

Since this is an optimization and not actually required, just don't do it.

Suggested-by: Mimi Zohar 
Signed-off-by: Thiago Jung Bauermann 
---
 security/integrity/ima/ima_appraise.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_appraise.c 
b/security/integrity/ima/ima_appraise.c
index 091977c8ec40..58e147049e98 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -229,7 +229,7 @@ int ima_appraise_measurement(enum ima_hooks func,
goto out;
}
 
-   status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
+   status = evm_verifyxattr(dentry, XATTR_NAME_IMA, NULL, 0, iint);
switch (status) {
case INTEGRITY_PASS:
case INTEGRITY_UNKNOWN:

[PATCH v5 08/18] integrity: Select CONFIG_KEYS instead of depending on it

[Thiago Jung Bauermann]

This avoids a dependency cycle in CONFIG_IMA_APPRAISE_MODSIG (introduced by
a later patch in this series): it will select CONFIG_MODULE_SIG_FORMAT
which in turn selects CONFIG_KEYS. Kconfig then complains that
CONFIG_INTEGRITY_SIGNATURE depends on CONFIG_KEYS.

Signed-off-by: Thiago Jung Bauermann 
---
 security/integrity/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index da9565891738..0d642e0317c7 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -17,8 +17,8 @@ if INTEGRITY
 
 config INTEGRITY_SIGNATURE
bool "Digital signature verification using multiple keyrings"
-   depends on KEYS
default n
+   select KEYS
select SIGNATURE
help
  This option enables digital signature verification support

[PATCH v5 07/18] integrity: Introduce struct evm_xattr

[Thiago Jung Bauermann]

Even though struct evm_ima_xattr_data includes a fixed-size array to hold a
SHA1 digest, most of the code ignores the array and uses the struct to mean
"type indicator followed by data of unspecified size" and tracks the real
size of what the struct represents in a separate length variable.

The only exception to that is the EVM code, which correctly uses the
definition of struct evm_ima_xattr_data.

This patch makes this explicit in the code by removing the length
specification from the array in struct evm_ima_xattr_data. It also changes
the name of the element from digest to data, since in most places the array
doesn't hold a digest.

A separate struct evm_xattr is introduced, with the original definition of
evm_ima_xattr_data to be used in the places that actually expect that
definition.

Signed-off-by: Thiago Jung Bauermann 
---
 security/integrity/evm/evm_crypto.c   |  4 ++--
 security/integrity/evm/evm_main.c | 10 +-
 security/integrity/ima/ima_appraise.c |  7 ---
 security/integrity/integrity.h|  5 +
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/security/integrity/evm/evm_crypto.c 
b/security/integrity/evm/evm_crypto.c
index 9c2d88c80b9d..5fddc28e8a0e 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -252,13 +252,13 @@ int evm_update_evmxattr(struct dentry *dentry, const char 
*xattr_name,
const char *xattr_value, size_t xattr_value_len)
 {
struct inode *inode = d_backing_inode(dentry);
-   struct evm_ima_xattr_data xattr_data;
+   struct evm_xattr xattr_data;
int rc = 0;
 
rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
   xattr_value_len, xattr_data.digest);
if (rc == 0) {
-   xattr_data.type = EVM_XATTR_HMAC;
+   xattr_data.data.type = EVM_XATTR_HMAC;
rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
   _data,
   sizeof(xattr_data), 0);
diff --git a/security/integrity/evm/evm_main.c 
b/security/integrity/evm/evm_main.c
index 37f062d38d5f..7f31f65b8492 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -119,7 +119,7 @@ static enum integrity_status evm_verify_hmac(struct dentry 
*dentry,
 struct integrity_iint_cache *iint)
 {
struct evm_ima_xattr_data *xattr_data = NULL;
-   struct evm_ima_xattr_data calc;
+   struct evm_xattr calc;
enum integrity_status evm_status = INTEGRITY_PASS;
int rc, xattr_len;
 
@@ -150,7 +150,7 @@ static enum integrity_status evm_verify_hmac(struct dentry 
*dentry,
/* check value type */
switch (xattr_data->type) {
case EVM_XATTR_HMAC:
-   if (xattr_len != sizeof(struct evm_ima_xattr_data)) {
+   if (xattr_len != sizeof(struct evm_xattr)) {
evm_status = INTEGRITY_FAIL;
goto out;
}
@@ -158,7 +158,7 @@ static enum integrity_status evm_verify_hmac(struct dentry 
*dentry,
   xattr_value_len, calc.digest);
if (rc)
break;
-   rc = crypto_memneq(xattr_data->digest, calc.digest,
+   rc = crypto_memneq(xattr_data->data, calc.digest,
sizeof(calc.digest));
if (rc)
rc = -EINVAL;
@@ -469,7 +469,7 @@ int evm_inode_init_security(struct inode *inode,
 const struct xattr *lsm_xattr,
 struct xattr *evm_xattr)
 {
-   struct evm_ima_xattr_data *xattr_data;
+   struct evm_xattr *xattr_data;
int rc;
 
if (!evm_initialized || !evm_protected_xattr(lsm_xattr->name))
@@ -479,7 +479,7 @@ int evm_inode_init_security(struct inode *inode,
if (!xattr_data)
return -ENOMEM;
 
-   xattr_data->type = EVM_XATTR_HMAC;
+   xattr_data->data.type = EVM_XATTR_HMAC;
rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
if (rc < 0)
goto out;
diff --git a/security/integrity/ima/ima_appraise.c 
b/security/integrity/ima/ima_appraise.c
index 2c069f47eeec..091977c8ec40 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -156,7 +156,8 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data 
*xattr_value,
return sig->hash_algo;
break;
case IMA_XATTR_DIGEST_NG:
-   ret = xattr_value->digest[0];
+   /* first byte contains algorithm id */
+   ret = xattr_value->data[0];
if (ret < HASH_ALGO__LAST)
return ret;
break;
@@ -164,7 +165,7 @@ enum hash_algo ima_get_hash_algo(struct 

[PATCH v5 06/18] ima: Improvements in ima_appraise_measurement

[Thiago Jung Bauermann]

Replace nested ifs in the EVM xattr verification logic with a switch
statement, making the code easier to understand.

Also, add comments to the if statements in the out section.

Signed-off-by: Mimi Zohar 
Signed-off-by: Thiago Jung Bauermann 
---
 security/integrity/ima/ima.h  |  5 +
 security/integrity/ima/ima_appraise.c | 33 -
 2 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d52b487ad259..4e61d3189b72 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -155,6 +155,11 @@ unsigned long ima_get_binary_runtime_size(void);
 int ima_init_template(void);
 void ima_init_template_list(void);
 
+static inline bool is_ima_sig(const struct evm_ima_xattr_data *xattr_value)
+{
+   return xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG;
+}
+
 /*
  * used to protect h_table and sha_table
  */
diff --git a/security/integrity/ima/ima_appraise.c 
b/security/integrity/ima/ima_appraise.c
index 58c6a60c7e83..2c069f47eeec 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -204,7 +204,7 @@ int ima_appraise_measurement(enum ima_hooks func,
 int xattr_len, int opened)
 {
static const char op[] = "appraise_data";
-   char *cause = "unknown";
+   const char *cause = "unknown";
struct dentry *dentry = file_dentry(file);
struct inode *inode = d_backing_inode(dentry);
enum integrity_status status = INTEGRITY_UNKNOWN;
@@ -229,11 +229,16 @@ int ima_appraise_measurement(enum ima_hooks func,
}
 
status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
-   if (status != INTEGRITY_PASS && status != INTEGRITY_UNKNOWN) {
-   if (status == INTEGRITY_NOLABEL || status == INTEGRITY_NOXATTRS)
-   cause = "missing-HMAC";
-   else if (status == INTEGRITY_FAIL)
-   cause = "invalid-HMAC";
+   switch (status) {
+   case INTEGRITY_PASS:
+   case INTEGRITY_UNKNOWN:
+   break;
+   case INTEGRITY_NOXATTRS:/* No EVM protected xattrs. */
+   case INTEGRITY_NOLABEL: /* No security.evm xattr. */
+   cause = "missing-HMAC";
+   goto out;
+   case INTEGRITY_FAIL:/* Invalid HMAC/signature. */
+   cause = "invalid-HMAC";
goto out;
}
switch (xattr_value->type) {
@@ -287,17 +292,19 @@ int ima_appraise_measurement(enum ima_hooks func,
 
 out:
if (status != INTEGRITY_PASS) {
+   /* Fix mode, but don't replace file signatures. */
if ((ima_appraise & IMA_APPRAISE_FIX) &&
-   (!xattr_value ||
-xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
+   !is_ima_sig(xattr_value)) {
if (!ima_fix_xattr(dentry, iint))
status = INTEGRITY_PASS;
-   } else if (inode->i_size == 0 &&
-  (iint->flags & IMA_NEW_FILE) &&
-  xattr_value &&
-  xattr_value->type == EVM_IMA_XATTR_DIGSIG) {
+   }
+
+   /* Permit new files with file signatures, but without data. */
+   if (inode->i_size == 0 && (iint->flags & IMA_NEW_FILE) &&
+   is_ima_sig(xattr_value)) {
status = INTEGRITY_PASS;
}
+
integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
op, cause, rc, 0);
} else {
@@ -404,7 +411,7 @@ int ima_inode_setxattr(struct dentry *dentry, const char 
*xattr_name,
if (!xattr_value_len || xvalue->type >= IMA_XATTR_LAST)
return -EINVAL;
ima_reset_appraise_flags(d_backing_inode(dentry),
-   xvalue->type == EVM_IMA_XATTR_DIGSIG);
+is_ima_sig(xvalue));
result = 0;
}
return result;

[PATCH v5 05/18] ima: Simplify ima_eventsig_init

[Thiago Jung Bauermann]

The "goto out" statement doesn't have any purpose since there's no cleanup
to be done when returning early, so remove it. This also makes the rc
variable unnecessary so remove it as well.

Also, the xattr_len and fmt variables are redundant so remove them as well.

Signed-off-by: Thiago Jung Bauermann 
---
 security/integrity/ima/ima_template_lib.c | 11 +++
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/security/integrity/ima/ima_template_lib.c 
b/security/integrity/ima/ima_template_lib.c
index d941260e979f..e8ec783b6a8d 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -378,16 +378,11 @@ int ima_eventname_ng_init(struct ima_event_data 
*event_data,
 int ima_eventsig_init(struct ima_event_data *event_data,
  struct ima_field_data *field_data)
 {
-   enum data_formats fmt = DATA_FMT_HEX;
struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
-   int xattr_len = event_data->xattr_len;
-   int rc = 0;
 
if (!xattr_value || xattr_value->type != EVM_IMA_XATTR_DIGSIG)
-   goto out;
+   return 0;
 
-   rc = ima_write_template_field_data(xattr_value, xattr_len, fmt,
-  field_data);
-out:
-   return rc;
+   return ima_write_template_field_data(xattr_value, event_data->xattr_len,
+DATA_FMT_HEX, field_data);
 }

[PATCH v5 04/18] evm, ima: Remove more superfluous parentheses

[Thiago Jung Bauermann]

This patch removes unnecessary parentheses from all EVM and IMA files
not yet cleaned up by the previous patches.

It is separate from the previous one so that it can be easily dropped if
the churn and conflict potential is deemed not worth it.

Confirmed that the patch is correct by comparing the object files from
before and after the patch. They are identical.

Signed-off-by: Thiago Jung Bauermann 
---
 security/integrity/evm/evm_posix_acl.c | 8 
 security/integrity/ima/ima_fs.c| 6 +++---
 security/integrity/ima/ima_queue.c | 6 +++---
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/security/integrity/evm/evm_posix_acl.c 
b/security/integrity/evm/evm_posix_acl.c
index 46408b9e62e8..0a32798cfc96 100644
--- a/security/integrity/evm/evm_posix_acl.c
+++ b/security/integrity/evm/evm_posix_acl.c
@@ -17,11 +17,11 @@ int posix_xattr_acl(const char *xattr)
 {
int xattr_len = strlen(xattr);
 
-   if ((strlen(XATTR_NAME_POSIX_ACL_ACCESS) == xattr_len)
-&& (strncmp(XATTR_NAME_POSIX_ACL_ACCESS, xattr, xattr_len) == 0))
+   if (strlen(XATTR_NAME_POSIX_ACL_ACCESS) == xattr_len
+   && strncmp(XATTR_NAME_POSIX_ACL_ACCESS, xattr, xattr_len) == 0)
return 1;
-   if ((strlen(XATTR_NAME_POSIX_ACL_DEFAULT) == xattr_len)
-&& (strncmp(XATTR_NAME_POSIX_ACL_DEFAULT, xattr, xattr_len) == 0))
+   if (strlen(XATTR_NAME_POSIX_ACL_DEFAULT) == xattr_len
+   && strncmp(XATTR_NAME_POSIX_ACL_DEFAULT, xattr, xattr_len) == 0)
return 1;
return 0;
 }
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 4d50b982b453..552f79fc4291 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -105,7 +105,7 @@ static void *ima_measurements_next(struct seq_file *m, void 
*v, loff_t *pos)
rcu_read_unlock();
(*pos)++;
 
-   return (>later == _measurements) ? NULL : qe;
+   return >later == _measurements ? NULL : qe;
 }
 
 static void ima_measurements_stop(struct seq_file *m, void *v)
@@ -141,7 +141,7 @@ int ima_measurements_show(struct seq_file *m, void *v)
if (e == NULL)
return -1;
 
-   template_name = (e->template_desc->name[0] != '\0') ?
+   template_name = e->template_desc->name[0] != '\0' ?
e->template_desc->name : e->template_desc->fmt;
 
/*
@@ -228,7 +228,7 @@ static int ima_ascii_measurements_show(struct seq_file *m, 
void *v)
if (e == NULL)
return -1;
 
-   template_name = (e->template_desc->name[0] != '\0') ?
+   template_name = e->template_desc->name[0] != '\0' ?
e->template_desc->name : e->template_desc->fmt;
 
/* 1st: PCR used (config option) */
diff --git a/security/integrity/ima/ima_queue.c 
b/security/integrity/ima/ima_queue.c
index a02a86d51102..2ef7d33ba1fc 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -60,7 +60,7 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 
*digest_value,
rcu_read_lock();
hlist_for_each_entry_rcu(qe, _htable.queue[key], hnext) {
rc = memcmp(qe->entry->digest, digest_value, TPM_DIGEST_SIZE);
-   if ((rc == 0) && (qe->entry->pcr == pcr)) {
+   if (rc == 0 && qe->entry->pcr == pcr) {
ret = qe;
break;
}
@@ -119,7 +119,7 @@ static int ima_add_digest_entry(struct ima_template_entry 
*entry,
int size;
 
size = get_binary_runtime_size(entry);
-   binary_runtime_size = (binary_runtime_size < ULONG_MAX - size) ?
+   binary_runtime_size = binary_runtime_size < ULONG_MAX - size ?
 binary_runtime_size + size : ULONG_MAX;
}
return 0;
@@ -132,7 +132,7 @@ static int ima_add_digest_entry(struct ima_template_entry 
*entry,
  */
 unsigned long ima_get_binary_runtime_size(void)
 {
-   if (binary_runtime_size >= (ULONG_MAX - sizeof(struct ima_kexec_hdr)))
+   if (binary_runtime_size >= ULONG_MAX - sizeof(struct ima_kexec_hdr))
return ULONG_MAX;
else
return binary_runtime_size + sizeof(struct ima_kexec_hdr);

[PATCH v5 03/18] evm, ima: Remove superfluous parentheses

[Thiago Jung Bauermann]

This patch removes unnecessary parentheses from all EVM and IMA files
touched by this patch series.

The difference from the previous patch is that it cleans up the files as a
whole, not just the lines that were already going to be modified by other
patches. It is separate from the previous one so that it can be easily
dropped if the churn and conflict potential is deemed not worth it.

Confirmed that the patch is correct by comparing the object files from
before and after the patch. They are identical.

Signed-off-by: Thiago Jung Bauermann 
---
 security/integrity/evm/evm_crypto.c   |  2 +-
 security/integrity/evm/evm_main.c | 13 +-
 security/integrity/ima/ima_api.c  |  2 +-
 security/integrity/ima/ima_appraise.c |  2 +-
 security/integrity/ima/ima_main.c | 11 +
 security/integrity/ima/ima_policy.c   | 41 ---
 security/integrity/ima/ima_template.c | 25 +--
 security/integrity/ima/ima_template_lib.c |  6 ++---
 8 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/security/integrity/evm/evm_crypto.c 
b/security/integrity/evm/evm_crypto.c
index bcd64baf8788..9c2d88c80b9d 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -199,7 +199,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 
error = -ENODATA;
for (xattrname = evm_config_xattrnames; *xattrname != NULL; 
xattrname++) {
-   if ((req_xattr_name && req_xattr_value)
+   if (req_xattr_name && req_xattr_value
&& !strcmp(*xattrname, req_xattr_name)) {
error = 0;
crypto_shash_update(desc, (const u8 *)req_xattr_value,
diff --git a/security/integrity/evm/evm_main.c 
b/security/integrity/evm/evm_main.c
index 9826c02e2db8..37f062d38d5f 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -188,7 +188,7 @@ static enum integrity_status evm_verify_hmac(struct dentry 
*dentry,
}
 
if (rc)
-   evm_status = (rc == -ENODATA) ?
+   evm_status = rc == -ENODATA ?
INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
 out:
if (iint)
@@ -205,8 +205,8 @@ static int evm_protected_xattr(const char *req_xattr_name)
 
namelen = strlen(req_xattr_name);
for (xattrname = evm_config_xattrnames; *xattrname != NULL; 
xattrname++) {
-   if ((strlen(*xattrname) == namelen)
-   && (strncmp(req_xattr_name, *xattrname, namelen) == 0)) {
+   if (strlen(*xattrname) == namelen
+   && strncmp(req_xattr_name, *xattrname, namelen) == 0) {
found = 1;
break;
}
@@ -294,8 +294,8 @@ static int evm_protect_xattr(struct dentry *dentry, const 
char *xattr_name,
if (!posix_xattr_acl(xattr_name))
return 0;
evm_status = evm_verify_current_integrity(dentry);
-   if ((evm_status == INTEGRITY_PASS) ||
-   (evm_status == INTEGRITY_NOXATTRS))
+   if (evm_status == INTEGRITY_PASS ||
+   evm_status == INTEGRITY_NOXATTRS)
return 0;
goto out;
}
@@ -434,8 +434,7 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr 
*attr)
if (!(ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)))
return 0;
evm_status = evm_verify_current_integrity(dentry);
-   if ((evm_status == INTEGRITY_PASS) ||
-   (evm_status == INTEGRITY_NOXATTRS))
+   if (evm_status == INTEGRITY_PASS || evm_status == INTEGRITY_NOXATTRS)
return 0;
integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
dentry->d_name.name, "appraise_metadata",
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c7e8db0ea4c0..c6d346e9f708 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -54,7 +54,7 @@ int ima_alloc_init_template(struct ima_event_data *event_data,
u32 len;
 
result = field->field_init(event_data,
-  &((*entry)->template_data[i]));
+  &(*entry)->template_data[i]);
if (result != 0)
goto out;
 
diff --git a/security/integrity/ima/ima_appraise.c 
b/security/integrity/ima/ima_appraise.c
index bce0b36778bd..58c6a60c7e83 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -401,7 +401,7 @@ int ima_inode_setxattr(struct dentry *dentry, const char 
*xattr_name,
result = ima_protect_xattr(dentry, xattr_name, xattr_value,
   xattr_value_len);
if (result == 1) {

[PATCH v5 02/18] ima: Remove some superfluous parentheses

[Thiago Jung Bauermann]

Superfluous parentheses just add clutter to the code, making it harder to
read and to understand.

In order to avoid churn and minimize conflicts with other patches from the
community, this patch only removes superfluous parentheses from lines that
are modified by other patches in this series.

Confirmed that the patch is correct by comparing the object files from
before and after the patch. They are identical.

Signed-off-by: Thiago Jung Bauermann 
---
 security/integrity/ima/ima_appraise.c | 11 +--
 security/integrity/ima/ima_template_lib.c |  2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c 
b/security/integrity/ima/ima_appraise.c
index ec7dfa02c051..bce0b36778bd 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -229,9 +229,8 @@ int ima_appraise_measurement(enum ima_hooks func,
}
 
status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
-   if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) {
-   if ((status == INTEGRITY_NOLABEL)
-   || (status == INTEGRITY_NOXATTRS))
+   if (status != INTEGRITY_PASS && status != INTEGRITY_UNKNOWN) {
+   if (status == INTEGRITY_NOLABEL || status == INTEGRITY_NOXATTRS)
cause = "missing-HMAC";
else if (status == INTEGRITY_FAIL)
cause = "invalid-HMAC";
@@ -293,10 +292,10 @@ int ima_appraise_measurement(enum ima_hooks func,
 xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
if (!ima_fix_xattr(dentry, iint))
status = INTEGRITY_PASS;
-   } else if ((inode->i_size == 0) &&
+   } else if (inode->i_size == 0 &&
   (iint->flags & IMA_NEW_FILE) &&
-  (xattr_value &&
-   xattr_value->type == EVM_IMA_XATTR_DIGSIG)) {
+  xattr_value &&
+  xattr_value->type == EVM_IMA_XATTR_DIGSIG) {
status = INTEGRITY_PASS;
}
integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
diff --git a/security/integrity/ima/ima_template_lib.c 
b/security/integrity/ima/ima_template_lib.c
index 28af43f63572..8bebcbb61162 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -383,7 +383,7 @@ int ima_eventsig_init(struct ima_event_data *event_data,
int xattr_len = event_data->xattr_len;
int rc = 0;
 
-   if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG))
+   if (!xattr_value || xattr_value->type != EVM_IMA_XATTR_DIGSIG)
goto out;
 
rc = ima_write_template_field_data(xattr_value, xattr_len, fmt,

[PATCH v5 01/18] ima: Remove redundant conditional operator

[Thiago Jung Bauermann]

A non-zero value is converted to 1 when assigned to a bool variable, so the
conditional operator in is_ima_appraise_enabled is redundant.

The value of a comparison operator is either 1 or 0 so the conditional
operator in ima_inode_setxattr is redundant as well.

Confirmed that the patch is correct by comparing the object file from
before and after the patch. They are identical.

Signed-off-by: Thiago Jung Bauermann 
---
 security/integrity/ima/ima_appraise.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c 
b/security/integrity/ima/ima_appraise.c
index 809ba70fbbbf..ec7dfa02c051 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -40,7 +40,7 @@ __setup("ima_appraise=", default_appraise_setup);
  */
 bool is_ima_appraise_enabled(void)
 {
-   return (ima_appraise & IMA_APPRAISE_ENFORCE) ? 1 : 0;
+   return ima_appraise & IMA_APPRAISE_ENFORCE;
 }
 
 /*
@@ -405,7 +405,7 @@ int ima_inode_setxattr(struct dentry *dentry, const char 
*xattr_name,
if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
return -EINVAL;
ima_reset_appraise_flags(d_backing_inode(dentry),
-(xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0);
+   xvalue->type == EVM_IMA_XATTR_DIGSIG);
result = 0;
}
return result;

[PATCH v5 00/18] Appended signatures support for IMA appraisal

[Thiago Jung Bauermann]

Hello,

The main highlight in this version is that it fixes a bug where the modsig
wasn't being included in the measurement list if the appraised file was
already measured by another rule. The fix is in the last patch.

Another change is that the last patch in the v4 series ("ima: Support
module-style appended signatures for appraisal") has been broken up into
smaller patches. I may have overdone it...

Finally, I have added some patches removing superfluous parentheses from
expressions. IMO these patches make it easier (and more pleasant) to read
the code, and thus easier to understand it. Since I'm not sure how welcome
the changes are, I split them in 3 "levels" in increasing potential for
conflict with patches from other people (they can be squashed together when
applied):

1. patch 2 contains the bare minimum, changing only lines that are also
   touched by other patches in the series;

2. patch 3 cleans up all the files that are touched by this patch series;

3. patch 4 cleans up all other EVM and IMA files that weren't already fixed
   by the previous patches.

If unwanted, patches 3 and 4 can be simply skipped without affecting the
rest of the patches. I have already rebased them from v4.13-rc2 to
v4.14-rc3 and now to linux-integrity/next with very few easy to resolve
conflicts, so I think they are worth keeping.

These patches apply on top of today's linux-integrity/next.

Original cover letter:

On the OpenPOWER platform, secure boot and trusted boot are being
implemented using IMA for taking measurements and verifying signatures.
Since the kernel image on Power servers is an ELF binary, kernels are
signed using the scripts/sign-file tool and thus use the same signature
format as signed kernel modules.

This patch series adds support in IMA for verifying those signatures.
It adds flexibility to OpenPOWER secure boot, because it allows it to boot
kernels with the signature appended to them as well as kernels where the
signature is stored in the IMA extended attribute.

Since modsig is only supported on some specific hooks which don't get
called often (cf. ima_hook_supports_modsig), it's possible to always check
for the presence of an appended modsig before looking for the xattr sig. In
that case, the policy doesn't need to be changed to support the modsig
keyword. Is that preferable than requiring the policy to explicitly allow a
modsig like this code does?

I tested these patches with EVM and I believe they don't break it and
things work as expected, but I'm not really familiar with EVM and its use
cases so this should be taken with a grain of salt.

I also verified that the code correctly recalculates the file hash if the
modsig verification fails and the file also has an xattr signature which
uses a different hash algorithm.

Changes since v4:
- Patch "ima: Remove redundant conditional operator"
  - New patch.

- Patch "ima: Remove some superfluous parentheses"
  - New patch.

- Patch "evm, ima: Remove superfluous parentheses"
  - New patch.

- Patch "evm, ima: Remove more superfluous parentheses"
  - New patch.

- Patch "ima: Simplify ima_eventsig_init"
  - New patch.

- Patch "ima: Improvements in ima_appraise_measurement"
  - New patch.

- Patch "ima: Don't pass xattr value to EVM xattr verification."
  - New patch.

- Patch "ima: Export func_tokens"
  - Split from patch "ima: Support module-style appended signatures for
appraisal".

- Patch "ima: Add modsig appraise_type option for module-style appended
 signatures"
  - Split from patch "ima: Support module-style appended signatures for
appraisal".
  - Mention modsig option in Documentation/ABI/testing/ima_policy
(suggested by Mimi Zohar).

- Patch "ima: Add functions to read and verify a modsig signature"
  - Split from patch "ima: Support module-style appended signatures for
appraisal".

- Patch "ima: Implement support for module-style appended signatures"
  - Split from patch "ima: Support module-style appended signatures for
appraisal".
  - In ima_appraise_measurement, change the logic of dealing with xattr
errors in case the modsig verification fails. With this,
process_xattr_error isn't needed anymore.

- Patch "ima: Write modsig to the measurement list"
  - Split from patch "ima: Support module-style appended signatures for
appraisal".
  - Added ima_current_template_has_sig function.
  - Removed hdr parameter from ima_modsig_serialize_data.
  - In ima_store_measurement, continue processing even if the given PCR
is already measured if it's for a modsig.
  - In process_measurement, add exception to store measurement even if
IMA_MEASURE is not set when appraising a modsig (suggested by
Mimi Zohar).
  - Call is_ima_sig in ima_eventsig_init.

Changes since v3:
- Patch "integrity: Introduce struct evm_hmac_xattr"
  - Renamed new struct to evm_xattr.
  - Define struct evm_xattr using struct evm_ima_xattr_data, and moved it
from evm.h to integrity.h (suggested by Mimi Zohar).
- Patch 

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Juan Alvarez]

On 10/17/17 1:52 PM, Bjorn Helgaas wrote:
> Right.  But that patch isn't really on the path to inclusion (mainly
> because it's test framework and doesn't fix a bug or add support for
> new hardware or features).
I was not aware of this decision and this will cause changes to this patch.
>
> I'm suggesting that maybe there should be a generic way to prevent
> binding to VF devices that works for everybody and doesn't require any
> arch-specific code at all.

The patch that you have suggested in kernel 4.12 is also a generic way.

https://marc.info/?l=linux-kernel=149002335105804=2

Perhaps we can use the same constructs that this patch uses at our level. 
Nonetheless,
that will take a rework to this patch and possibly an export of a function to 
set drivers_autoprobe
globally. I know that was frowned upon on first email thread. Let me know if 
this is an
acceptable solution.
>> So like existing way of enabling SRIOV we still rely on the PF driver to
>> enable VFs - but in this case the attachment phase is done via a user
>> action via a management console in our case (novalink or hmc) triggered
>> event that will essentially act like a hotplug.
>>
>> So in the fine details of that user triggered action the system
>> firmware will bind the VFs, allowing resources to be allocated to
>> the VF.  - Which essentially does all the attaching as we know it
>> today but is managed by PHYP not by the kernel.
> What exactly does "firmware binding the VFs" mean?  I guess this must
> mean assigning a VF to a partition, injecting a hotplug add event to
> that partition, and making the VF visible in config space?
 Firmware basically adds a device node to the device tree as defined
 in the (PAPR) Power Architecture Platform Requirements document.
>>> From the point of view of the kernel that consumes this device tree
>>> and owns the VF, I guess this looks like a hot-add.  
>> Correct, this is intended. We want to minimize change and only focus
>> on configure SR-IOV path in the PF on PSeries platform.
>>> It would be nice
>>> if this could be exposed to that kernel by having the firmware inject
>>> a normal PCIe hotplug interrupt, but I'm guessing it's not that
>>> simple.
>> I don't understand entirely your suggestion. However, in the case of
>> interrupts PHYP owns all the resources and will be associated accordingly
>> with a partitionable endpoint (PE).
> Assume you have a Linux kernel, and PHYP assigns a VF to it.  What
> happens in that Linux kernel? 

When  kernel boots it goes through the lists of buses in the device tree. For 
each (physical,virtual)
bus the kernel reads the list of devices. After resources are read and assigned 
it will probe the devices.

If the kernel is up and running then this will start its flow through the dlpar 
add bus code.
Which then will read the device node and same operation as boot will entail of 
going
through the list of devices under the bus.

One should note that dynamically created VFs in configure SR-IOV path will have 
is_virtfn
set and others do not, therefore pcibios_bus_add_device will only be exercised 
in certain
cases.
>  How does it discover this new device?
This is treated as a normal hot plug operation for the Operating System that
the device gets assigned which can be  IBMi, AIX or Linux in this environment.
>
> I'm suggesting that it would be cool if that kernel received a hotplug
> interrupt from the Downstream Port leading to the new VF, and the
> pciehp driver could read the Slot Status register and see that
> "Presence Detect Changed" was set.  If that happened, the pciehp
> driver should automatically enumerate the new device and there
> wouldn't be much if any powerpc-specific code in the path.
To re-state Steve's earlier comment, PHYP will enumerate the virtual pci bus 
and is not
directly correlated to the real PCI bus that the PF is on. We have no control 
of the pci
bus enumeration for the device node added to the device tree.

Juan J. Alvarez

Re: [PATCH v2 5/5] of/fdt: only store the device node basename in full_name

[Alan Tull]

On Mon, Aug 21, 2017 at 10:16 AM, Rob Herring  wrote:

Hi Rob,

> With dependencies on a statically allocated full path name converted to
> use %pOF format specifier, we can store just the basename of node, and
> the unflattening of the FDT can be simplified.
>
> This commit will affect the remaining users of full_name. After
> analyzing these users, the remaining cases should only change some print
> messages. The main users of full_name are providing a name for struct
> resource. The resource names shouldn't be important other than providing
> /proc/iomem names.
>
> We no longer distinguish between pre and post 0x10 dtb formats as either
> a full path or basename will work. However, less than 0x10 formats have
> been broken since the conversion to use libfdt (and no one has cared).
> The conversion of the unflattening code to be non-recursive also broke
> pre 0x10 formats as the populate_node function would return 0 in that
> case.
>
> Signed-off-by: Rob Herring 
> ---
> v2:
> - rebase to linux-next
>
>  drivers/of/fdt.c | 69 
> +---
>  1 file changed, 11 insertions(+), 58 deletions(-)

I've just updated to the latest next branch and am finding problems
applying overlays.   Reverting this commit alleviates things.  The
errors I get are:

[   88.498704] OF: overlay: Failed to apply prop @/__symbols__/clk_0
[   88.513447] OF: overlay: apply failed '/__symbols__'
[   88.518423] create_overlay: Failed to create overlay (err=-12)

My branch also includes Pantelis' patch that turns on symbols [1].

Alan

[1] 
https://github.com/pantoniou/linux-beagle-track-mainline/commit/486c87d450f2895a766c6097328d0c6538ec7a31

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Juan Alvarez]

On 10/17/17 11:26 AM, Bjorn Helgaas wrote:
> To pop back up to the top of the stack, I think the main point of this
> patch is to keep from binding a driver to the VFs in the kernel that
> set PCI_SRIOV_CTRL_VFE.  This patch does that by setting
> pdev->match_driver to -1 in powerpc-specific code.
Correct, to add to your comment, we will only add to the PSeries platform
 (in PowerPC)  configure SR-IOV path.
> I think all systems, not just powerpc, want to prevent this VF driver
> binding, so I hope there's a generic solution that everybody can use.
The patch that we made this change dependent on:

https://patchwork.kernel.org/patch/9882915/

Is a generic form of not binding a pci device to a device driver
and can be used in another environment if needed.
>
 So like existing way of enabling SRIOV we still rely on the PF driver to
 enable VFs - but in this case the attachment phase is done via a user
 action via a management console in our case (novalink or hmc) triggered
 event that will essentially act like a hotplug.

 So in the fine details of that user triggered action the system
 firmware will bind the VFs, allowing resources to be allocated to
 the VF.  - Which essentially does all the attaching as we know it
 today but is managed by PHYP not by the kernel.
>>> What exactly does "firmware binding the VFs" mean?  I guess this must
>>> mean assigning a VF to a partition, injecting a hotplug add event to
>>> that partition, and making the VF visible in config space?
>> Firmware basically adds a device node to the device tree as defined
>> in the (PAPR) Power Architecture Platform Requirements document.
> From the point of view of the kernel that consumes this device tree
> and owns the VF, I guess this looks like a hot-add.  
Correct, this is intended. We want to minimize change and only focus
on configure SR-IOV path in the PF on PSeries platform.
> It would be nice
> if this could be exposed to that kernel by having the firmware inject
> a normal PCIe hotplug interrupt, but I'm guessing it's not that
> simple.
I don't understand entirely your suggestion. However, in the case of
interrupts PHYP owns all the resources and will be associated accordingly
with a partitionable endpoint (PE).
> But if I understand correctly, this patch series isn't concerned with
> that kernel; it's concerned with the kernel that owns the PF and sets
> PCI_SRIOV_CTRL_VFE.
Correct, we will enable SR-IOV in the PF through the Linux kernel and map 
system resources
for the new VFs in powerpc platform specific kernel code. Furthermore, not 
binding
the device drivers for the VFs that get mapped within the configure SR-IOV path
is a requirement for this PSeries SR-IOV environment.

Juan J. Alvarez

RE: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Alexander.Steffen]

> On Tue, 2017-10-17 at 11:50 +, alexander.stef...@infineon.com
> wrote:
> > > > Replace the specification of data structures by pointer dereferences
> > > > as the parameter for the operator "sizeof" to make the corresponding
> > > > size
> > > > determination a bit safer according to the Linux coding style
> > > > convention.
> > >
> > >
> > > This patch does one style in favor of the other.
> >
> > I actually prefer that style, so I'd welcome this change :)
> 
> Style changes should be reviewed and documented, like any other code
> change, and added to Documentation/process/coding-style.rst or an
> equivalent file.
> 
> > > At the end it's Jarkko's call, though I would NAK this as I think some
> > > one already told this to you for some other similar patch(es).
> > >
> > >
> > > I even would suggest to stop doing this noisy stuff, which keeps people
> > > busy for nothing.
> >
> > Cleaning up old code is also worth something, even if does not
> > change one bit in the assembly output in the end...
> 
> Wow, you're opening the door really wide for all sorts of trivial
> changes!  Hope you have the time and inclination to review and comment
> on all of them.  I certainly don't.

Well, isn't the point of trivial changes that they are trivial to review? :) 
For things like that there is probably not even a need to run a test, though 
with sufficient automation that should not be a problem either.

> There is a major difference between adding these sorts of checks to
> the tools in the scripts directory or even to the zero day bots that
> catch different sorts of errors, BEFORE code is upstreamed, and
> patches like these, after the fact.

Catching those things early in the process is certainly preferable. But at some 
point you need to fix the existing code, or you'll end up with a mashup of 
different styles, just because you did not want to touch old code.

> After the code has been upstreamed, it is a lot more difficult to
> justify changes like this.  It impacts both code that is being
> developed AND backporting bug fixes.

Backporting could be an argument, but even that should not be allowed to block 
improvements indefinitely. I'd prefer a world in which the current code is nice 
and clean and easy to maintain, to a world where we never touch old code unless 
it is proven to be wrong.

But looking at the code in question, I cannot see how this should ever be a 
serious problem. Even when backporting a change takes now ten minutes instead 
of five, which means it is twice as hard, it is still not difficult.

Alexander

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Juan Alvarez]

On 10/17/17 8:51 AM, Bjorn Helgaas wrote:
> This is where I get confused.  I guess the Linux that sets
> PCI_SRIOV_CTRL_VFE to enable the VFs can also perform config accesses
> to the VFs, since it can enumerate them and build pci_dev structs for
> them, right?

Correct, we are not changing anything related to how the VF gets initialized
in the kernel.
>
> And the Linux in the "Hosting Partition" is a guest that cannot see a
> VF until a management console attaches the VF to the Hosting
> Partition?  

Correct, this is how PHYP does normal adapter assignment.
> I'm not a VFIO or KVM expert but that sounds vaguely like
> what they would do when assigning a VF to a guest.
I do not know the specifics on VFIO and KVM. However, in this
case there is no KVM running on the Linux LPAR. PHYP owns all
the system resources.
>
>> So like existing way of enabling SRIOV we still rely on the PF driver to
>> enable VFs - but in this case the attachment phase is done via a user
>> action via a management console in our case (novalink or hmc) triggered
>> event that will essentially act like a hotplug.
>>
>> So in the fine details of that user triggered action the system
>> firmware will bind the VFs, allowing resources to be allocated to
>> the VF.  - Which essentially does all the attaching as we know it
>> today but is managed by PHYP not by the kernel.
> What exactly does "firmware binding the VFs" mean?  I guess this must
> mean assigning a VF to a partition, injecting a hotplug add event to
> that partition, and making the VF visible in config space?
Firmware basically adds a device node to the device tree as defined
in the (PAPR) Power Architecture Platform Requirements document.

Juan

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Juan Alvarez]

On 10/17/17 8:51 AM, Bjorn Helgaas wrote:

> This is where I get confused.  I guess the Linux that sets
> PCI_SRIOV_CTRL_VFE to enable the VFs can also perform config accesses
> to the VFs, since it can enumerate them and build pci_dev structs for
> them, right?
Correct, we are not changing anything related to how the VF gets initialized
in the Linux kernel.
>
> And the Linux in the "Hosting Partition" is a guest that cannot see a
> VF until a management console attaches the VF to the Hosting
> Partition? 
Correct, this is how PHYP does normal adapter assignment.

>  I'm not a VFIO or KVM expert but that sounds vaguely like
> what they would do when assigning a VF to a guest.
I do not know the specifics on VFIO and KVM. However, in this
case there is no KVM running on the Linux (LPAR) logical partition.
PHYP owns all the system resources.
>
>> So like existing way of enabling SRIOV we still rely on the PF driver to
>> enable VFs - but in this case the attachment phase is done via a user
>> action via a management console in our case (novalink or hmc) triggered
>> event that will essentially act like a hotplug.
>>
>> So in the fine details of that user triggered action the system
>> firmware will bind the VFs, allowing resources to be allocated to
>> the VF.  - Which essentially does all the attaching as we know it
>> today but is managed by PHYP not by the kernel.
> What exactly does "firmware binding the VFs" mean?  I guess this must
> mean assigning a VF to a partition, injecting a hotplug add event to
> that partition, and making the VF visible in config space?
Firmware basically adds a device node to the device tree as defined
in the (PAPR) Power Architecture Platform Requirements document.

Juan J. Alvarez

[PATCH] powerpc/40x: acadia: Fix the 'interrupt-parent' property

[Fabio Estevam]

'interrupts-parent' property does not exist. Fix the typo.

Fixes: 00f3ca740a9c26 ("powerpc/40x: AMCC PowerPC 405EZ Acadia DTS")
Signed-off-by: Fabio Estevam 
---
 arch/powerpc/boot/dts/acadia.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/boot/dts/acadia.dts b/arch/powerpc/boot/dts/acadia.dts
index 57291f6..8626615 100644
--- a/arch/powerpc/boot/dts/acadia.dts
+++ b/arch/powerpc/boot/dts/acadia.dts
@@ -183,7 +183,7 @@
usb@ef603000 {
compatible = "ohci-be";
reg = <0xef603000 0x80>;
-   interrupts-parent = <>;
+   interrupt-parent = <>;
interrupts = <0xd 0x4 0xe 0x4>;
};
 
-- 
2.7.4

RE: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Alexander.Steffen]

> > Replace the specification of data structures by pointer dereferences
> > as the parameter for the operator "sizeof" to make the corresponding
> > size
> > determination a bit safer according to the Linux coding style
> > convention.
> 
> 
> This patch does one style in favor of the other.

I actually prefer that style, so I'd welcome this change :)

> At the end it's Jarkko's call, though I would NAK this as I think some
> one already told this to you for some other similar patch(es).
> 
> 
> I even would suggest to stop doing this noisy stuff, which keeps people
> busy for nothing.

Cleaning up old code is also worth something, even if does not change one bit 
in the assembly output in the end...

Alexander

Re: [PATCH v2 2/2] vgaarb: Factor out EFI and fallback default device selection

[Daniel Axtens]

Hi Daniel,

>> Initially I wondered if this info printk could be moved into
>> vga_arbiter_check_bridge_sharing(), but it's been separated out since
>> 3448a19da479b ("vgaarb: use bridges to control VGA routing where
>> possible."), and upon closer examination, it seems you can't be sure a
>> device doesn't share a bridge until the end of the process, so this is
>> indeed correct.
>> 
>> Everything else also looks good to me.
>> 
>> Reviewed-by: Daniel Axtens 
>
> R-b for both patches? And ok with everyone if I pull this into drm-misc
> for 4.15 (deadline is end of this week for feature-y stuff)?

I had only intended it for patch 2, but I've now read over patch 1 to my
satisfaction, so it too is:

Reviewed-by: Daniel Axtens 

Thanks!

Regards,
Daniel

>
> Thanks, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH v2 5/5] of/fdt: only store the device node basename in full_name

[Frank Rowand]

On 10/17/17 14:46, Rob Herring wrote:
> On Tue, Oct 17, 2017 at 4:32 PM, Alan Tull  wrote:
>> On Mon, Aug 21, 2017 at 10:16 AM, Rob Herring  wrote:
>>
>> Hi Rob,
>>
>>> With dependencies on a statically allocated full path name converted to
>>> use %pOF format specifier, we can store just the basename of node, and
>>> the unflattening of the FDT can be simplified.
>>>
>>> This commit will affect the remaining users of full_name. After
>>> analyzing these users, the remaining cases should only change some print
>>> messages. The main users of full_name are providing a name for struct
>>> resource. The resource names shouldn't be important other than providing
>>> /proc/iomem names.
>>>
>>> We no longer distinguish between pre and post 0x10 dtb formats as either
>>> a full path or basename will work. However, less than 0x10 formats have
>>> been broken since the conversion to use libfdt (and no one has cared).
>>> The conversion of the unflattening code to be non-recursive also broke
>>> pre 0x10 formats as the populate_node function would return 0 in that
>>> case.
>>>
>>> Signed-off-by: Rob Herring 
>>> ---
>>> v2:
>>> - rebase to linux-next
>>>
>>>  drivers/of/fdt.c | 69 
>>> +---
>>>  1 file changed, 11 insertions(+), 58 deletions(-)
>>
>> I've just updated to the latest next branch and am finding problems
>> applying overlays.   Reverting this commit alleviates things.  The
>> errors I get are:
>>
>> [   88.498704] OF: overlay: Failed to apply prop @/__symbols__/clk_0
>> [   88.513447] OF: overlay: apply failed '/__symbols__'
>> [   88.518423] create_overlay: Failed to create overlay (err=-12)
> 
> Frank's series with overlay updates should fix this.

Yes, it does:

  [PATCH v3 11/12] of: overlay: remove a dependency on device node full_name

-Frank

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Joe Perches]

On Tue, 2017-10-17 at 08:57 -0700, James Bottomley wrote:
> On Tue, 2017-10-17 at 11:25 +0200, SF Markus Elfring wrote:
> > > 
> > > Fixes is only for bug fixes.  These don't fix any bugs.
> > 
> > How do you distinguish these in questionable source code
> > from other error categories or software weaknesses?
> 
> A style change is one that doesn't change the effect of the execution.
>  These don't actually even change the assembly, so there's programmatic
> proof they're not fixing anything.

The printk removals do change the objects.

The value of that type of change is only for
resource limited systems.

printk type changes should generally not be
considered fixes.

> Bug means potentially user visible fault.  In any bug fix commit you
> should document the fault and its effects on users so those backporting
> can decide if they care or not.

Markus' changelogs leave much to be desired.

Re: [PATCH v2 5/5] of/fdt: only store the device node basename in full_name

[Rob Herring]

On Tue, Oct 17, 2017 at 4:32 PM, Alan Tull  wrote:
> On Mon, Aug 21, 2017 at 10:16 AM, Rob Herring  wrote:
>
> Hi Rob,
>
>> With dependencies on a statically allocated full path name converted to
>> use %pOF format specifier, we can store just the basename of node, and
>> the unflattening of the FDT can be simplified.
>>
>> This commit will affect the remaining users of full_name. After
>> analyzing these users, the remaining cases should only change some print
>> messages. The main users of full_name are providing a name for struct
>> resource. The resource names shouldn't be important other than providing
>> /proc/iomem names.
>>
>> We no longer distinguish between pre and post 0x10 dtb formats as either
>> a full path or basename will work. However, less than 0x10 formats have
>> been broken since the conversion to use libfdt (and no one has cared).
>> The conversion of the unflattening code to be non-recursive also broke
>> pre 0x10 formats as the populate_node function would return 0 in that
>> case.
>>
>> Signed-off-by: Rob Herring 
>> ---
>> v2:
>> - rebase to linux-next
>>
>>  drivers/of/fdt.c | 69 
>> +---
>>  1 file changed, 11 insertions(+), 58 deletions(-)
>
> I've just updated to the latest next branch and am finding problems
> applying overlays.   Reverting this commit alleviates things.  The
> errors I get are:
>
> [   88.498704] OF: overlay: Failed to apply prop @/__symbols__/clk_0
> [   88.513447] OF: overlay: apply failed '/__symbols__'
> [   88.518423] create_overlay: Failed to create overlay (err=-12)

Frank's series with overlay updates should fix this.

Rob

Re: lpar issue for ZONE_DEVICE p2pmem in 4.14-rc

[Balbir Singh]

On Wed, Oct 18, 2017 at 7:38 AM, Stephen  Bates  wrote:
> Hi All
>
> I am hoping someone can help shed some light on an issue I am seeing with my 
> attempt to add p2pmem [1] to the ppc64el kernel. p2pmem is a (currently) 
> out-of-tree patchset that allows one to add device memory with struct page 
> backing into the Linux kernel. It leverages MEMORY_HOTPLUG and ZONE_DEVICE 
> which were added to ppc64 in 4.14 so I thought it would be fun to try it out.
>
> We constructed a patchset based off 4.14-rc1 [1] and build a kernel based off 
> the pseries defconfig and ran this on upstream qemu-system-ppc64. The exact 
> command to run QEMU was:
>
> qemu-system-ppc64 \
> -machine pseries \
> -cpu power8 \
> -smp 1 -m 2048 \
> -kernel ~/kernel/linux-ppc64el/vmlinux \
> -append "nvme.use_cmb=24 console=hvc root=/dev/sda rootwait rw" \
> -serial mon:stdio -drive 
> file=image-ppc64el.img,if=scsi,format=raw,index=0 \
> -nographic \
> -drive file=../images/nvme.qcow2,if=none,id=nvme1,snapshot=on \
> -device nvme,drive=nvme1,serial=nvme1 \
> -drive file=../images/nvme2.qcow2,if=none,id=nvme2,snapshot=on \
> -device nvme,drive=nvme2,serial=nvme2,cmb_size_mb=64
>
> This resulted in the following extract from dmesg when registering the p2pmem 
> associated with one of the NVMe SSDs.
>
> [3.508497] nvme :00:03.0: enabling device (0100 -> 0102)
> [3.510743] nvme :00:03.0: Using 64-bit direct DMA at offset 
> 800
> [3.535706] p2pmem p2pmem0: registered
> [3.537780] lpar: Attempting to resize HPT to shift 21
> [3.539251] Unable to resize hash page table to target order 21: -1

I am guessing that the hotplug of ZONE_DEVICE memory was done
incorrectly as it lead to HPT resizing (the system thinking this is
normal memory). Ideally one would expect that the driver would online
ZONE_DEVICE memory and not go through the HOTPLUG path. Are you using
devm_memremap_pages() path to add these pages?

Balbir Singh.

lpar issue for ZONE_DEVICE p2pmem in 4.14-rc

[Stephen Bates]

Hi All

I am hoping someone can help shed some light on an issue I am seeing with my 
attempt to add p2pmem [1] to the ppc64el kernel. p2pmem is a (currently) 
out-of-tree patchset that allows one to add device memory with struct page 
backing into the Linux kernel. It leverages MEMORY_HOTPLUG and ZONE_DEVICE 
which were added to ppc64 in 4.14 so I thought it would be fun to try it out.

We constructed a patchset based off 4.14-rc1 [1] and build a kernel based off 
the pseries defconfig and ran this on upstream qemu-system-ppc64. The exact 
command to run QEMU was:

qemu-system-ppc64 \
-machine pseries \
-cpu power8 \
-smp 1 -m 2048 \
-kernel ~/kernel/linux-ppc64el/vmlinux \
-append "nvme.use_cmb=24 console=hvc root=/dev/sda rootwait rw" \
-serial mon:stdio -drive file=image-ppc64el.img,if=scsi,format=raw,index=0 \
-nographic \
-drive file=../images/nvme.qcow2,if=none,id=nvme1,snapshot=on \
-device nvme,drive=nvme1,serial=nvme1 \
-drive file=../images/nvme2.qcow2,if=none,id=nvme2,snapshot=on \
-device nvme,drive=nvme2,serial=nvme2,cmb_size_mb=64

This resulted in the following extract from dmesg when registering the p2pmem 
associated with one of the NVMe SSDs.

[3.508497] nvme :00:03.0: enabling device (0100 -> 0102)
[3.510743] nvme :00:03.0: Using 64-bit direct DMA at offset 
800
[3.535706] p2pmem p2pmem0: registered
[3.537780] lpar: Attempting to resize HPT to shift 21
[3.539251] Unable to resize hash page table to target order 21: -1
[3.541079] Unable to create mapping for hot added memory 
0xc0002100..0xc00021000400: -2
[3.550407] p2pmem p2pmem0: unregistered

I am not that familiar with the pseries architecture so I was hoping for some 
guidance concerning the lpar error message. If any ppc64 coders fancy having a 
go at this the kernel code is at [1] and I’d be happy to provide the .config 
used if needed. Just let me know.

Cheers
 
Stephen

[1] https://github.com/sbates130272/linux-p2pmem

Re: char/tpm: Improve a size determination in nine functions

[SF Markus Elfring]

>> I imagine that a corresponding source code analysis variant could be applied
>> in more cases if sufficient acceptance could be achieved.
> 
> So, then instead of still keeping people busy with this noise you better
> start doing something like CI integration with that for *new* code?

There are various software development challenges to consider.


> I'm pretty sure you may also exercise your achievements on
> drivers/staging where it would be honored.

I am waiting for several improvements also for software components
in this area for a while. Would you like to take another look
at these change possibilities?


> Have you talked to Fengguang (0-day LKP)?

Not directly for this topic so far.


> Have you talked to Arnd (I think he is related to kernel-ci)?

I am curious on how he will respond to remaining open issues.

Regards,
Markus

Re: char/tpm: Improve a size determination in nine functions

[SF Markus Elfring]

>> Do you find my wording “This issue was detected by using the
>> Coccinelle software.” insufficient?
> 
> The question is not whether it is insufficient, but whether it is appropriate.

I am curious on how our corresponding discussion will evolve further.


> Detecting Coccinelle issues is one step.  The next step is deciding
> what to do with them.

Will the clarification achieve any more useful results?


> Up to now, these messages have been sent out as informational, not as patches.

I sent some update suggestions as patches also in this series (as usual).


> Before sending patches to change existing code, address the "problem"
> so that it doesn't continue to happen.

It might be very challenging to achieve such a goal.


> Only afterwards is it appropriate to discuss what to do with existing code.

I would prefer to get corresponding improvements in both areas in parallel
(if it is generally possible).

Regards,
Markus

Re: char/tpm: Improve a size determination in nine functions

[Andy Shevchenko]

On Tue, 2017-10-17 at 20:41 +0200, SF Markus Elfring wrote:
> > >   p = kmalloc(sizeof(*p), ...);
> > > 
> > > The alternative form where struct name is spelled out hurts
> > > readability and
> > > introduces an opportunity for a bug when the pointer variable type
> > > is changed
> > > but the corresponding sizeof that is passed to a memory allocator
> > > is not.
> > 

> > before patches are upstreamed?
> 
> I imagine that a corresponding source code analysis variant could be
> applied
> in more cases if sufficient acceptance could be achieved.

So, then instead of still keeping people busy with this noise you better
start doing something like CI integration with that for *new* code?

I'm pretty sure you may also exercise your achievements on
drivers/staging where it would be honored.

Have you talked to Fengguang (0-day LKP)? Have you talked to Arnd (I
think he is related to kernel-ci)?

-- 
Andy Shevchenko 
Intel Finland Oy

Re: char/tpm: Improve a size determination in nine functions

[Mimi Zohar]

On Tue, 2017-10-17 at 20:41 +0200, SF Markus Elfring wrote:

> Do you find my wording “This issue was detected by using the
> Coccinelle software.” insufficient?

The question is not whether it is insufficient, but whether it is
appropriate.  Detecting Coccinelle issues is one step.  The next step
is deciding what to do with them.  Up to now, these messages have been
sent out as informational, not as patches.

Before sending patches to change existing code, address the "problem"
so that it doesn't continue to happen.  Only afterwards is it
appropriate to discuss what to do with existing code.

Mimi

[PATCH 2/2] powerpc-ps3: Improve a size determination in two functions

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 17 Oct 2017 20:15:17 +0200

Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 arch/powerpc/platforms/ps3/mm.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/ps3/mm.c b/arch/powerpc/platforms/ps3/mm.c
index b0f34663b1ae..257de451e324 100644
--- a/arch/powerpc/platforms/ps3/mm.c
+++ b/arch/powerpc/platforms/ps3/mm.c
@@ -524,8 +524,7 @@ static int dma_sb_map_pages(struct ps3_dma_region *r, 
unsigned long phys_addr,
int result;
struct dma_chunk *c;
 
-   c = kzalloc(sizeof(struct dma_chunk), GFP_ATOMIC);
-
+   c = kzalloc(sizeof(*c), GFP_ATOMIC);
if (!c) {
result = -ENOMEM;
goto fail_alloc;
@@ -570,8 +569,7 @@ static int dma_ioc0_map_pages(struct ps3_dma_region *r, 
unsigned long phys_addr,
 
DBG(KERN_ERR "%s: phy=%#lx, lpar%#lx, len=%#lx\n", __func__,
phys_addr, ps3_mm_phys_to_lpar(phys_addr), len);
-   c = kzalloc(sizeof(struct dma_chunk), GFP_ATOMIC);
-
+   c = kzalloc(sizeof(*c), GFP_ATOMIC);
if (!c) {
result = -ENOMEM;
goto fail_alloc;
-- 
2.14.2

[PATCH 1/2] powerpc-ps3: Delete an error message for a failed memory allocation in update_flash_db()

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 17 Oct 2017 20:00:31 +0200

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 arch/powerpc/platforms/ps3/os-area.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/ps3/os-area.c 
b/arch/powerpc/platforms/ps3/os-area.c
index 3db53e8aff92..f2a8875e25d1 100644
--- a/arch/powerpc/platforms/ps3/os-area.c
+++ b/arch/powerpc/platforms/ps3/os-area.c
@@ -625,10 +625,8 @@ static int update_flash_db(void)
/* Read in header and db from flash. */
 
header = kmalloc(buf_len, GFP_KERNEL);
-   if (!header) {
-   pr_debug("%s: kmalloc failed\n", __func__);
+   if (!header)
return -ENOMEM;
-   }
 
count = os_area_flash_read(header, buf_len, 0);
if (count < 0) {
-- 
2.14.2

[PATCH 0/2] PowerPC-PS3: Adjustments for three function implementations

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 17 Oct 2017 20:22:44 +0200

Two update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Delete an error message for a failed memory allocation in update_flash_db()
  Improve a size determination in two functions

 arch/powerpc/platforms/ps3/mm.c  | 6 ++
 arch/powerpc/platforms/ps3/os-area.c | 4 +---
 2 files changed, 3 insertions(+), 7 deletions(-)

-- 
2.14.2

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bjorn Helgaas]

On Tue, Oct 17, 2017 at 12:23:01PM -0500, Juan Alvarez wrote:
> On 10/17/17 11:26 AM, Bjorn Helgaas wrote:
> > To pop back up to the top of the stack, I think the main point of this
> > patch is to keep from binding a driver to the VFs in the kernel that
> > set PCI_SRIOV_CTRL_VFE.  This patch does that by setting
> > pdev->match_driver to -1 in powerpc-specific code.
> Correct, to add to your comment, we will only add to the PSeries platform
>  (in PowerPC)  configure SR-IOV path.
> > I think all systems, not just powerpc, want to prevent this VF driver
> > binding, so I hope there's a generic solution that everybody can use.
> The patch that we made this change dependent on:
> 
> https://patchwork.kernel.org/patch/9882915/
> 
> Is a generic form of not binding a pci device to a device driver
> and can be used in another environment if needed.

Right.  But that patch isn't really on the path to inclusion (mainly
because it's test framework and doesn't fix a bug or add support for
new hardware or features).

I'm suggesting that maybe there should be a generic way to prevent
binding to VF devices that works for everybody and doesn't require any
arch-specific code at all.

>  So like existing way of enabling SRIOV we still rely on the PF driver to
>  enable VFs - but in this case the attachment phase is done via a user
>  action via a management console in our case (novalink or hmc) triggered
>  event that will essentially act like a hotplug.
> 
>  So in the fine details of that user triggered action the system
>  firmware will bind the VFs, allowing resources to be allocated to
>  the VF.  - Which essentially does all the attaching as we know it
>  today but is managed by PHYP not by the kernel.
> >>> What exactly does "firmware binding the VFs" mean?  I guess this must
> >>> mean assigning a VF to a partition, injecting a hotplug add event to
> >>> that partition, and making the VF visible in config space?
> >> Firmware basically adds a device node to the device tree as defined
> >> in the (PAPR) Power Architecture Platform Requirements document.
> > From the point of view of the kernel that consumes this device tree
> > and owns the VF, I guess this looks like a hot-add.  
> Correct, this is intended. We want to minimize change and only focus
> on configure SR-IOV path in the PF on PSeries platform.
> > It would be nice
> > if this could be exposed to that kernel by having the firmware inject
> > a normal PCIe hotplug interrupt, but I'm guessing it's not that
> > simple.
>
> I don't understand entirely your suggestion. However, in the case of
> interrupts PHYP owns all the resources and will be associated accordingly
> with a partitionable endpoint (PE).

Assume you have a Linux kernel, and PHYP assigns a VF to it.  What
happens in that Linux kernel?  How does it discover this new device?

I'm suggesting that it would be cool if that kernel received a hotplug
interrupt from the Downstream Port leading to the new VF, and the
pciehp driver could read the Slot Status register and see that
"Presence Detect Changed" was set.  If that happened, the pciehp
driver should automatically enumerate the new device and there
wouldn't be much if any powerpc-specific code in the path.

Bjorn

Re: char/tpm: Improve a size determination in nine functions

[SF Markus Elfring]

>>  p = kmalloc(sizeof(*p), ...);
>>
>> The alternative form where struct name is spelled out hurts readability and
>> introduces an opportunity for a bug when the pointer variable type is changed
>> but the corresponding sizeof that is passed to a memory allocator is not.
> 
> True, thanks for the reminder.

Will it trigger further software development considerations (besides my 
contributions)?


> Is this common in new code?

Do you start an official survey here?


> Is there a script/ or some other automated way of catching this usage

Yes. - I am using an approach for the semantic patch language.   ;-)


> before patches are upstreamed?

I imagine that a corresponding source code analysis variant could be applied
in more cases if sufficient acceptance could be achieved.


> Just as you're doing here, the patch description should reference this
> in the patch description.

Do you find my wording “This issue was detected by using the Coccinelle 
software.” insufficient?

Regards,
Markus

[PATCH] powerpc/xmon: check before calling xive functions

[Breno Leitao]

Currently xmon could call XIVE functions from OPAL even if the XIVE is
disabled or does not exist in the system, as in POWER8 machines.  This
causes the following exception:

 1:mon> dx
 cpu 0x1: Vector: 700 (Program Check) at [c00423c93450]
 pc: c009cfa4: opal_xive_dump+0x50/0x68
 lr: c00997b8: opal_return+0x0/0x50

This patch simply checks if XIVE is enabled before calling XIVE
functions.

Suggested-by: Guilherme G. Piccoli 
Signed-off-by: Breno Leitao 
---
 arch/powerpc/xmon/xmon.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 4679aeb84767..b34976c4a6ba 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -2508,6 +2508,12 @@ static void dump_xives(void)
unsigned long num;
int c;
 
+   if (!xive_enabled()) {
+   printf("Xive disabled on this system\n");
+
+   return;
+   }
+
c = inchar();
if (c == 'a') {
dump_all_xives();
-- 
2.14.2

Re: [PATCH 1/2] powerpc/nodes: Ensure enough nodes avail for operations

[Nathan Fontenot]

On 10/17/2017 12:22 PM, Michael Bringmann wrote:
> 
> 
> On 10/17/2017 12:02 PM, Nathan Fontenot wrote:
>> On 10/17/2017 11:14 AM, Michael Bringmann wrote:
>>> See below.
>>>
>>> On 10/16/2017 07:33 AM, Michael Ellerman wrote:
 Michael Bringmann  writes:

> powerpc/nodes: On systems like PowerPC which allow 'hot-add' of CPU

 This is a powerpc-only patch, so saying "systems like PowerPC" is
 confusing. What you should be saying is "On pseries systems".

> or memory resources, it may occur that the new resources are to be
> inserted into nodes that were not used for these resources at bootup.
> In the kernel, any node that is used must be defined and initialized
> at boot.
>
> This patch extracts the value of the lowest domain level (number of
> allocable resources) from the "rtas" device tree property
> "ibm,current-associativity-domains" or the device tree property

 What is current associativity domains? I've not heard of it, where is it
 documented, and what does it mean.

 Why would use the "current" set vs the "max"? I thought the whole point
 was to discover the maximum possible set of nodes that could be
 hotplugged.

> "ibm,max-associativity-domains" to use as the maximum number of nodes
> to setup as possibly available in the system.  This new setting will
> override the instruction,
>
> nodes_and(node_possible_map, node_possible_map, node_online_map);
>
> presently seen in the function arch/powerpc/mm/numa.c:initmem_init().
>
> If the property is not present at boot, no operation will be performed
> to define or enable additional nodes.
>
> Signed-off-by: Michael Bringmann 
> ---
>  arch/powerpc/mm/numa.c |   47 
> +++
>  1 file changed, 47 insertions(+)
>
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index ec098b3..b385cd0 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -892,6 +892,51 @@ static void __init setup_node_data(int nid, u64 
> start_pfn, u64 end_pfn)
>   NODE_DATA(nid)->node_spanned_pages = spanned_pages;
>  }
>  
> +static void __init node_associativity_setup(void)

 This should really be called "find_possible_nodes()" or something more
 descriptive.
>>>
>>> Okay.

> +{
> + struct device_node *rtas;
> +
> + rtas = of_find_node_by_path("/rtas");
> + if (rtas) {

 If you just short-circuit that return the whole function body can be
 deintented, making it significantly more readable.

 ie:
 +  rtas = of_find_node_by_path("/rtas");
 +  if (!rtas)
 +  return;
>>>
>>> Okay.
>>>

> + const __be32 *prop;
> + u32 len, entries, numnodes, i;
> +
> + prop = of_get_property(rtas,
> + "ibm,current-associativity-domains", 
> );

 Please don't use of_get_property() in new code, we have much better
 accessors these days, which do better error checking and handle the
 endian conversions for you.

 In this case you'd use eg:

u32 entries;
rc = of_property_read_u32(rtas, "ibm,current-associativity-domains", 
 );
>>>
>>> The property 'ibm,current-associativity-domains' has the same format as the 
>>> property
>>> 'ibm,max-associativity-domains' i.e. it is an integer array.  The accessor 
>>> of_property_read_32,
>>> however, expects it to be an integer singleton value.  Instead, it needs:
>>
>> I think for this case where the property is an array of values you could use
>> of_property_count_elems_of_size() to get the number of elements in the array
>> and then use of_property_read_u32_array() to read the array.
>>
>> -Nathan
> 
> We only need one value from the array which is why I am using,
> 
> + numnodes = of_read_number([min_common_depth], 1);
> 
> With this implementation I do not need to allocate memory for
> an array, nor execute code to read all elements of the array.
>
> Michael

OK, I didn't see that you just needed a single value from the array.

In this case you could do

of_property_read_u32_index(rtas, "ibm,current-associativity-domains",
   min_common_depth, );

-Nathan

> 
>>
>>>

> + if (!prop || len < sizeof(unsigned int)) {
> + prop = of_get_property(rtas,
> + "ibm,max-associativity-domains", );
>>> if (!prop || len < sizeof(unsigned int))
> + goto endit;
> + }
> +
> + entries = of_read_number(prop++, 1);
> +
> + if (len < (entries * sizeof(unsigned int)))
> + goto endit;
> +
> + if ((0 <= 

Re: [PATCH 1/2] powerpc/nodes: Ensure enough nodes avail for operations

[Michael Bringmann]

On 10/17/2017 12:02 PM, Nathan Fontenot wrote:
> On 10/17/2017 11:14 AM, Michael Bringmann wrote:
>> See below.
>>
>> On 10/16/2017 07:33 AM, Michael Ellerman wrote:
>>> Michael Bringmann  writes:
>>>
 powerpc/nodes: On systems like PowerPC which allow 'hot-add' of CPU
>>>
>>> This is a powerpc-only patch, so saying "systems like PowerPC" is
>>> confusing. What you should be saying is "On pseries systems".
>>>
 or memory resources, it may occur that the new resources are to be
 inserted into nodes that were not used for these resources at bootup.
 In the kernel, any node that is used must be defined and initialized
 at boot.

 This patch extracts the value of the lowest domain level (number of
 allocable resources) from the "rtas" device tree property
 "ibm,current-associativity-domains" or the device tree property
>>>
>>> What is current associativity domains? I've not heard of it, where is it
>>> documented, and what does it mean.
>>>
>>> Why would use the "current" set vs the "max"? I thought the whole point
>>> was to discover the maximum possible set of nodes that could be
>>> hotplugged.
>>>
 "ibm,max-associativity-domains" to use as the maximum number of nodes
 to setup as possibly available in the system.  This new setting will
 override the instruction,

 nodes_and(node_possible_map, node_possible_map, node_online_map);

 presently seen in the function arch/powerpc/mm/numa.c:initmem_init().

 If the property is not present at boot, no operation will be performed
 to define or enable additional nodes.

 Signed-off-by: Michael Bringmann 
 ---
  arch/powerpc/mm/numa.c |   47 
 +++
  1 file changed, 47 insertions(+)

 diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
 index ec098b3..b385cd0 100644
 --- a/arch/powerpc/mm/numa.c
 +++ b/arch/powerpc/mm/numa.c
 @@ -892,6 +892,51 @@ static void __init setup_node_data(int nid, u64 
 start_pfn, u64 end_pfn)
NODE_DATA(nid)->node_spanned_pages = spanned_pages;
  }
  
 +static void __init node_associativity_setup(void)
>>>
>>> This should really be called "find_possible_nodes()" or something more
>>> descriptive.
>>
>> Okay.
>>>
 +{
 +  struct device_node *rtas;
 +
 +  rtas = of_find_node_by_path("/rtas");
 +  if (rtas) {
>>>
>>> If you just short-circuit that return the whole function body can be
>>> deintented, making it significantly more readable.
>>>
>>> ie:
>>> +   rtas = of_find_node_by_path("/rtas");
>>> +   if (!rtas)
>>> +   return;
>>
>> Okay.
>>
>>>
 +  const __be32 *prop;
 +  u32 len, entries, numnodes, i;
 +
 +  prop = of_get_property(rtas,
 +  "ibm,current-associativity-domains", 
 );
>>>
>>> Please don't use of_get_property() in new code, we have much better
>>> accessors these days, which do better error checking and handle the
>>> endian conversions for you.
>>>
>>> In this case you'd use eg:
>>>
>>> u32 entries;
>>> rc = of_property_read_u32(rtas, "ibm,current-associativity-domains", 
>>> );
>>
>> The property 'ibm,current-associativity-domains' has the same format as the 
>> property
>> 'ibm,max-associativity-domains' i.e. it is an integer array.  The accessor 
>> of_property_read_32,
>> however, expects it to be an integer singleton value.  Instead, it needs:
> 
> I think for this case where the property is an array of values you could use
> of_property_count_elems_of_size() to get the number of elements in the array
> and then use of_property_read_u32_array() to read the array.
> 
> -Nathan

We only need one value from the array which is why I am using,

 +  numnodes = of_read_number([min_common_depth], 1);

With this implementation I do not need to allocate memory for
an array, nor execute code to read all elements of the array.

Michael

> 
>>
>>>
 +  if (!prop || len < sizeof(unsigned int)) {
 +  prop = of_get_property(rtas,
 +  "ibm,max-associativity-domains", );
>> if (!prop || len < sizeof(unsigned int))
 +  goto endit;
 +  }
 +
 +  entries = of_read_number(prop++, 1);
 +
 +  if (len < (entries * sizeof(unsigned int)))
 +  goto endit;
 +
 +  if ((0 <= min_common_depth) && (min_common_depth <= 
 (entries-1)))
 +  entries = min_common_depth;
 +  else
 +  entries -= 1;
>>> ^
>>> You can't just guess that will be the right entry.
>>>
>>> If min_common_depth is < 0 the function should have just returned
>>> immediately at the top.
>>
>> Okay.
>>
>>>
>>> If 

Re: [PATCH kernel v2] powerpc/powernv: Reserve a hole which appears after enabling IOV

[Bjorn Helgaas]

On Wed, Sep 27, 2017 at 04:52:31PM +1000, Alexey Kardashevskiy wrote:
> In order to make generic IOV code work, the physical function IOV BAR
> should start from offset of the first VF. Since M64 segments share
> PE number space across PHB, and some PEs may be in use at the time
> when IOV is enabled, the existing code shifts the IOV BAR to the index
> of the first PE/VF. This creates a hole in IOMEM space which can be
> potentially taken by some other device.
> 
> This reserves a temporary hole on a parent and releases it when IOV is
> disabled; the temporary resources are stored in pci_dn to avoid
> kmalloc/free.
> 
> Cc: linux-...@vger.kernel.org
> Cc: Benjamin Herrenschmidt 
> Cc: Bjorn Helgaas 
> Signed-off-by: Alexey Kardashevskiy 

Looks good to me.

Acked-by: Bjorn Helgaas 

I assume this will be merged via the powerpc branch.

> ---
> 
> I assume this goes to powerpc next branch but before this I'd like to
> get Bjorn's opinion as he continously commented on this bit.
> 
> This is the diff in /proc/iomem:
> 
> @@ -11,6 +11,7 @@
>  2008-200b : :04:00.0
>  2100-21fd : /pciex@3fffe4010
>2100-21fdfff0 : PCI Bus 0001:01
> +2100-210009ff : pnv_iov_reserved
>  21000a00-2101 : 0001:01:00.0
>21000a00-21000bff : 0001:01:00.2
>  21000a00-21000bff : mlx5_core
> 
> ---
> Changes:
> v2:
> * changed order - now devm_release_resource() is called before
> pci_update_resource(). Strangely the opposite did not produce a warning
> but still
> ---
>  arch/powerpc/include/asm/pci-bridge.h |  1 +
>  arch/powerpc/platforms/powernv/pci-ioda.c | 24 +---
>  2 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/pci-bridge.h 
> b/arch/powerpc/include/asm/pci-bridge.h
> index 0b8aa1fe2d5f..62ed83db04ae 100644
> --- a/arch/powerpc/include/asm/pci-bridge.h
> +++ b/arch/powerpc/include/asm/pci-bridge.h
> @@ -218,6 +218,7 @@ struct pci_dn {
>  #endif
>   struct list_head child_list;
>   struct list_head list;
> + struct resource holes[PCI_SRIOV_NUM_BARS];
>  };
>  
>  /* Get the pointer to a device_node's pci_dn */
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c 
> b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 57f9e55f4352..d66a758b8efb 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1002,9 +1002,12 @@ static int pnv_pci_vf_resource_shift(struct pci_dev 
> *dev, int offset)
>   }
>  
>   /*
> -  * After doing so, there would be a "hole" in the /proc/iomem when
> -  * offset is a positive value. It looks like the device return some
> -  * mmio back to the system, which actually no one could use it.
> +  * Since M64 BAR shares segments among all possible 256 PEs,
> +  * we have to shift the beginning of PF IOV BAR to make it start from
> +  * the segment which belongs to the PE number assigned to the first VF.
> +  * This creates a "hole" in the /proc/iomem which could be used for
> +  * allocating other resources so we reserve this area below and
> +  * release when IOV is released.
>*/
>   for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
>   res = >resource[i + PCI_IOV_RESOURCES];
> @@ -1018,7 +1021,22 @@ static int pnv_pci_vf_resource_shift(struct pci_dev 
> *dev, int offset)
>   dev_info(>dev, "VF BAR%d: %pR shifted to %pR (%sabling %d 
> VFs shifted by %d)\n",
>i, , res, (offset > 0) ? "En" : "Dis",
>num_vfs, offset);
> +
> + if (offset < 0) {
> + devm_release_resource(>dev, >holes[i]);
> + memset(>holes[i], 0, sizeof(pdn->holes[i]));
> + }
> +
>   pci_update_resource(dev, i + PCI_IOV_RESOURCES);
> +
> + if (offset > 0) {
> + pdn->holes[i].start = res2.start;
> + pdn->holes[i].end = res2.start + size * offset - 1;
> + pdn->holes[i].flags = IORESOURCE_BUS;
> + pdn->holes[i].name = "pnv_iov_reserved";
> + devm_request_resource(>dev, res->parent,
> + >holes[i]);
> + }
>   }
>   return 0;
>  }
> -- 
> 2.11.0
> 

Re: [PATCH 1/2] powerpc/nodes: Ensure enough nodes avail for operations

[Nathan Fontenot]

On 10/17/2017 11:14 AM, Michael Bringmann wrote:
> See below.
> 
> On 10/16/2017 07:33 AM, Michael Ellerman wrote:
>> Michael Bringmann  writes:
>>
>>> powerpc/nodes: On systems like PowerPC which allow 'hot-add' of CPU
>>
>> This is a powerpc-only patch, so saying "systems like PowerPC" is
>> confusing. What you should be saying is "On pseries systems".
>>
>>> or memory resources, it may occur that the new resources are to be
>>> inserted into nodes that were not used for these resources at bootup.
>>> In the kernel, any node that is used must be defined and initialized
>>> at boot.
>>>
>>> This patch extracts the value of the lowest domain level (number of
>>> allocable resources) from the "rtas" device tree property
>>> "ibm,current-associativity-domains" or the device tree property
>>
>> What is current associativity domains? I've not heard of it, where is it
>> documented, and what does it mean.
>>
>> Why would use the "current" set vs the "max"? I thought the whole point
>> was to discover the maximum possible set of nodes that could be
>> hotplugged.
>>
>>> "ibm,max-associativity-domains" to use as the maximum number of nodes
>>> to setup as possibly available in the system.  This new setting will
>>> override the instruction,
>>>
>>> nodes_and(node_possible_map, node_possible_map, node_online_map);
>>>
>>> presently seen in the function arch/powerpc/mm/numa.c:initmem_init().
>>>
>>> If the property is not present at boot, no operation will be performed
>>> to define or enable additional nodes.
>>>
>>> Signed-off-by: Michael Bringmann 
>>> ---
>>>  arch/powerpc/mm/numa.c |   47 
>>> +++
>>>  1 file changed, 47 insertions(+)
>>>
>>> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
>>> index ec098b3..b385cd0 100644
>>> --- a/arch/powerpc/mm/numa.c
>>> +++ b/arch/powerpc/mm/numa.c
>>> @@ -892,6 +892,51 @@ static void __init setup_node_data(int nid, u64 
>>> start_pfn, u64 end_pfn)
>>> NODE_DATA(nid)->node_spanned_pages = spanned_pages;
>>>  }
>>>  
>>> +static void __init node_associativity_setup(void)
>>
>> This should really be called "find_possible_nodes()" or something more
>> descriptive.
> 
> Okay.
>>
>>> +{
>>> +   struct device_node *rtas;
>>> +
>>> +   rtas = of_find_node_by_path("/rtas");
>>> +   if (rtas) {
>>
>> If you just short-circuit that return the whole function body can be
>> deintented, making it significantly more readable.
>>
>> ie:
>> +rtas = of_find_node_by_path("/rtas");
>> +if (!rtas)
>> +return;
> 
> Okay.
> 
>>
>>> +   const __be32 *prop;
>>> +   u32 len, entries, numnodes, i;
>>> +
>>> +   prop = of_get_property(rtas,
>>> +   "ibm,current-associativity-domains", 
>>> );
>>
>> Please don't use of_get_property() in new code, we have much better
>> accessors these days, which do better error checking and handle the
>> endian conversions for you.
>>
>> In this case you'd use eg:
>>
>>  u32 entries;
>>  rc = of_property_read_u32(rtas, "ibm,current-associativity-domains", 
>> );
> 
> The property 'ibm,current-associativity-domains' has the same format as the 
> property
> 'ibm,max-associativity-domains' i.e. it is an integer array.  The accessor 
> of_property_read_32,
> however, expects it to be an integer singleton value.  Instead, it needs:

I think for this case where the property is an array of values you could use
of_property_count_elems_of_size() to get the number of elements in the array
and then use of_property_read_u32_array() to read the array.

-Nathan

> 
>>
>>> +   if (!prop || len < sizeof(unsigned int)) {
>>> +   prop = of_get_property(rtas,
>>> +   "ibm,max-associativity-domains", );
> if (!prop || len < sizeof(unsigned int))
>>> +   goto endit;
>>> +   }
>>> +
>>> +   entries = of_read_number(prop++, 1);
>>> +
>>> +   if (len < (entries * sizeof(unsigned int)))
>>> +   goto endit;
>>> +
>>> +   if ((0 <= min_common_depth) && (min_common_depth <= 
>>> (entries-1)))
>>> +   entries = min_common_depth;
>>> +   else
>>> +   entries -= 1;
>>  ^
>> You can't just guess that will be the right entry.
>>
>> If min_common_depth is < 0 the function should have just returned
>> immediately at the top.
> 
> Okay.
> 
>>
>> If min_common_depth is outside the range of the property that's a buggy
>> device tree, you should print a warning and return.
>>
>>> +   numnodes = of_read_number([entries], 1);
>>
>>  u32 num_nodes;
>>  rc = of_property_read_u32_index(rtas, 
>> "ibm,current-associativity-domains", min_common_depth, _nodes);
>>> +
>>> +   printk(KERN_INFO "numa: Nodes = %d (mcd = %d)\n", numnodes,
>>> +   

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[SF Markus Elfring]

>>> Fixes is only for bug fixes.  These don't fix any bugs.
>>
>> How do you distinguish these in questionable source code
>> from other error categories or software weaknesses?
> 
> A style change is one that doesn't change the effect of the execution.

This can occasionally be fine, can't it?


>  These don't actually even change the assembly,

How did you check it?

I would expect that there are useful run time effects to consider
for three proposed update steps (in this patch series).


> so there's programmatic proof they're not fixing anything.

I find that the software refactoring “Improve a size determination in nine 
functions”
should fit to this observation (while the source code can become a bit better).


> Bug means potentially user visible fault.

Thanks for your constructive feedback.

Regards,
Markus

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bjorn Helgaas]

On Tue, Oct 17, 2017 at 09:33:34AM -0500, Juan Alvarez wrote:
> On 10/17/17 8:51 AM, Bjorn Helgaas wrote:
> > This is where I get confused.  I guess the Linux that sets
> > PCI_SRIOV_CTRL_VFE to enable the VFs can also perform config accesses
> > to the VFs, since it can enumerate them and build pci_dev structs for
> > them, right?
> 
> Correct, we are not changing anything related to how the VF gets initialized
> in the kernel.
> >
> > And the Linux in the "Hosting Partition" is a guest that cannot see a
> > VF until a management console attaches the VF to the Hosting
> > Partition?  
> 
> Correct, this is how PHYP does normal adapter assignment.
> 
> > I'm not a VFIO or KVM expert but that sounds vaguely like
> > what they would do when assigning a VF to a guest.
>
> I do not know the specifics on VFIO and KVM. However, in this
> case there is no KVM running on the Linux LPAR. PHYP owns all
> the system resources.

To pop back up to the top of the stack, I think the main point of this
patch is to keep from binding a driver to the VFs in the kernel that
set PCI_SRIOV_CTRL_VFE.  This patch does that by setting
pdev->match_driver to -1 in powerpc-specific code.

I think all systems, not just powerpc, want to prevent this VF driver
binding, so I hope there's a generic solution that everybody can use.

> >> So like existing way of enabling SRIOV we still rely on the PF driver to
> >> enable VFs - but in this case the attachment phase is done via a user
> >> action via a management console in our case (novalink or hmc) triggered
> >> event that will essentially act like a hotplug.
> >>
> >> So in the fine details of that user triggered action the system
> >> firmware will bind the VFs, allowing resources to be allocated to
> >> the VF.  - Which essentially does all the attaching as we know it
> >> today but is managed by PHYP not by the kernel.
> >
> > What exactly does "firmware binding the VFs" mean?  I guess this must
> > mean assigning a VF to a partition, injecting a hotplug add event to
> > that partition, and making the VF visible in config space?
>
> Firmware basically adds a device node to the device tree as defined
> in the (PAPR) Power Architecture Platform Requirements document.

>From the point of view of the kernel that consumes this device tree
and owns the VF, I guess this looks like a hot-add.  It would be nice
if this could be exposed to that kernel by having the firmware inject
a normal PCIe hotplug interrupt, but I'm guessing it's not that
simple.

But if I understand correctly, this patch series isn't concerned with
that kernel; it's concerned with the kernel that owns the PF and sets
PCI_SRIOV_CTRL_VFE.

Bjorn

Re: [PATCH 1/2] powerpc/nodes: Ensure enough nodes avail for operations

[Michael Bringmann]

See below.

On 10/16/2017 07:33 AM, Michael Ellerman wrote:
> Michael Bringmann  writes:
> 
>> powerpc/nodes: On systems like PowerPC which allow 'hot-add' of CPU
> 
> This is a powerpc-only patch, so saying "systems like PowerPC" is
> confusing. What you should be saying is "On pseries systems".
> 
>> or memory resources, it may occur that the new resources are to be
>> inserted into nodes that were not used for these resources at bootup.
>> In the kernel, any node that is used must be defined and initialized
>> at boot.
>>
>> This patch extracts the value of the lowest domain level (number of
>> allocable resources) from the "rtas" device tree property
>> "ibm,current-associativity-domains" or the device tree property
> 
> What is current associativity domains? I've not heard of it, where is it
> documented, and what does it mean.
> 
> Why would use the "current" set vs the "max"? I thought the whole point
> was to discover the maximum possible set of nodes that could be
> hotplugged.
> 
>> "ibm,max-associativity-domains" to use as the maximum number of nodes
>> to setup as possibly available in the system.  This new setting will
>> override the instruction,
>>
>> nodes_and(node_possible_map, node_possible_map, node_online_map);
>>
>> presently seen in the function arch/powerpc/mm/numa.c:initmem_init().
>>
>> If the property is not present at boot, no operation will be performed
>> to define or enable additional nodes.
>>
>> Signed-off-by: Michael Bringmann 
>> ---
>>  arch/powerpc/mm/numa.c |   47 
>> +++
>>  1 file changed, 47 insertions(+)
>>
>> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
>> index ec098b3..b385cd0 100644
>> --- a/arch/powerpc/mm/numa.c
>> +++ b/arch/powerpc/mm/numa.c
>> @@ -892,6 +892,51 @@ static void __init setup_node_data(int nid, u64 
>> start_pfn, u64 end_pfn)
>>  NODE_DATA(nid)->node_spanned_pages = spanned_pages;
>>  }
>>  
>> +static void __init node_associativity_setup(void)
> 
> This should really be called "find_possible_nodes()" or something more
> descriptive.

Okay.
> 
>> +{
>> +struct device_node *rtas;
>> +
>> +rtas = of_find_node_by_path("/rtas");
>> +if (rtas) {
> 
> If you just short-circuit that return the whole function body can be
> deintented, making it significantly more readable.
> 
> ie:
> + rtas = of_find_node_by_path("/rtas");
> + if (!rtas)
> + return;

Okay.

> 
>> +const __be32 *prop;
>> +u32 len, entries, numnodes, i;
>> +
>> +prop = of_get_property(rtas,
>> +"ibm,current-associativity-domains", 
>> );
> 
> Please don't use of_get_property() in new code, we have much better
> accessors these days, which do better error checking and handle the
> endian conversions for you.
> 
> In this case you'd use eg:
> 
>   u32 entries;
>   rc = of_property_read_u32(rtas, "ibm,current-associativity-domains", 
> );

The property 'ibm,current-associativity-domains' has the same format as the 
property
'ibm,max-associativity-domains' i.e. it is an integer array.  The accessor 
of_property_read_32,
however, expects it to be an integer singleton value.  Instead, it needs:

> 
>> +if (!prop || len < sizeof(unsigned int)) {
>> +prop = of_get_property(rtas,
>> +"ibm,max-associativity-domains", );
if (!prop || len < sizeof(unsigned int))
>> +goto endit;
>> +}
>> +
>> +entries = of_read_number(prop++, 1);
>> +
>> +if (len < (entries * sizeof(unsigned int)))
>> +goto endit;
>> +
>> +if ((0 <= min_common_depth) && (min_common_depth <= 
>> (entries-1)))
>> +entries = min_common_depth;
>> +else
>> +entries -= 1;
>   ^
> You can't just guess that will be the right entry.
> 
> If min_common_depth is < 0 the function should have just returned
> immediately at the top.

Okay.

> 
> If min_common_depth is outside the range of the property that's a buggy
> device tree, you should print a warning and return.
> 
>> +numnodes = of_read_number([entries], 1);
> 
>   u32 num_nodes;
>   rc = of_property_read_u32_index(rtas, 
> "ibm,current-associativity-domains", min_common_depth, _nodes);
>> +
>> +printk(KERN_INFO "numa: Nodes = %d (mcd = %d)\n", numnodes,
>> +min_common_depth);
>> +
>> +for (i = 0; i < numnodes; i++) {
>> +if (!node_possible(i)) {
>> +setup_node_data(i, 0, 0);
> 
> Do we actually need to setup the NODE_DATA() yet? Doing it now ensures
> it will not be allocated node local, which sucks.

Okay.

> 
>> +node_set(i, node_possible_map);

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[James Bottomley]

On Tue, 2017-10-17 at 11:25 +0200, SF Markus Elfring wrote:
> > 
> > Fixes is only for bug fixes.  These don't fix any bugs.
> 
> How do you distinguish these in questionable source code
> from other error categories or software weaknesses?

A style change is one that doesn't change the effect of the execution.
 These don't actually even change the assembly, so there's programmatic
proof they're not fixing anything.

Bug means potentially user visible fault.  In any bug fix commit you
should document the fault and its effects on users so those backporting
can decide if they care or not.

James

[PATCH 3/3] powernv/pci: Improve a size determination in pnv_pci_init_ioda_phb()

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 17 Oct 2017 17:18:10 +0200

Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 arch/powerpc/platforms/powernv/pci-ioda.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c 
b/arch/powerpc/platforms/powernv/pci-ioda.c
index 98d9435240f4..2febdf06a237 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -3802,7 +3802,7 @@ static void __init pnv_pci_init_ioda_phb(struct 
device_node *np,
phb_id = be64_to_cpup(prop64);
pr_debug("  PHB-ID  : 0x%016llx\n", phb_id);
 
-   phb = memblock_virt_alloc(sizeof(struct pnv_phb), 0);
+   phb = memblock_virt_alloc(sizeof(*phb), 0);
 
/* Allocate PCI controller */
phb->hose = hose = pcibios_alloc_controller(np);
-- 
2.14.2

[PATCH 2/3] powernv/pci: Use common code in pnv_ioda_pick_m64_pe()

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 17 Oct 2017 17:07:54 +0200

Add a jump target so that a bit of code can be better reused
at the end of this function.

Signed-off-by: Markus Elfring 
---
 arch/powerpc/platforms/powernv/pci-ioda.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c 
b/arch/powerpc/platforms/powernv/pci-ioda.c
index 17c0330bb059..98d9435240f4 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -364,21 +364,20 @@ static struct pnv_ioda_pe *pnv_ioda_pick_m64_pe(struct 
pci_bus *bus, bool all)
/* Figure out reserved PE numbers by the PE */
pnv_ioda_reserve_m64_pe(bus, pe_alloc, all);
 
+   master_pe = NULL;
+
/*
 * the current bus might not own M64 window and that's all
 * contributed by its child buses. For the case, we needn't
 * pick M64 dependent PE#.
 */
-   if (bitmap_empty(pe_alloc, phb->ioda.total_pe_num)) {
-   kfree(pe_alloc);
-   return NULL;
-   }
+   if (bitmap_empty(pe_alloc, phb->ioda.total_pe_num))
+   goto free_pe;
 
/*
 * Figure out the master PE and put all slave PEs to master
 * PE's list to form compound PE.
 */
-   master_pe = NULL;
i = -1;
while ((i = find_next_bit(pe_alloc, phb->ioda.total_pe_num, i + 1)) <
phb->ioda.total_pe_num) {
@@ -416,6 +415,7 @@ static struct pnv_ioda_pe *pnv_ioda_pick_m64_pe(struct 
pci_bus *bus, bool all)
}
}
 
+free_pe:
kfree(pe_alloc);
return master_pe;
 }
-- 
2.14.2

[PATCH 1/3] powernv/pci: Delete an error message for a failed memory allocation in pnv_ioda_pick_m64_pe()

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 17 Oct 2017 16:52:43 +0200

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 arch/powerpc/platforms/powernv/pci-ioda.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c 
b/arch/powerpc/platforms/powernv/pci-ioda.c
index fb5cd7511189..17c0330bb059 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -358,11 +358,8 @@ static struct pnv_ioda_pe *pnv_ioda_pick_m64_pe(struct 
pci_bus *bus, bool all)
/* Allocate bitmap */
size = _ALIGN_UP(phb->ioda.total_pe_num / 8, sizeof(unsigned long));
pe_alloc = kzalloc(size, GFP_KERNEL);
-   if (!pe_alloc) {
-   pr_warn("%s: Out of memory !\n",
-   __func__);
+   if (!pe_alloc)
return NULL;
-   }
 
/* Figure out reserved PE numbers by the PE */
pnv_ioda_reserve_m64_pe(bus, pe_alloc, all);
-- 
2.14.2

[PATCH 0/3] PowerNV-PCI: Adjustments for two function implementations

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 17 Oct 2017 17:27:37 +0200

Three update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Delete an error message for a failed memory allocation in 
pnv_ioda_pick_m64_pe()
  Use common code in pnv_ioda_pick_m64_pe()
  Improve a size determination in pnv_pci_init_ioda_phb()

 arch/powerpc/platforms/powernv/pci-ioda.c | 17 +++--
 1 file changed, 7 insertions(+), 10 deletions(-)

-- 
2.14.2

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Julia Lawall]

On Tue, 17 Oct 2017, Mimi Zohar wrote:

> On Tue, 2017-10-17 at 14:58 +0200, Julia Lawall wrote:
> >
> > On Tue, 17 Oct 2017, Mimi Zohar wrote:
> >
> > > On Tue, 2017-10-17 at 11:50 +, alexander.stef...@infineon.com
> > > wrote:
> > > > > > Replace the specification of data structures by pointer dereferences
> > > > > > as the parameter for the operator "sizeof" to make the corresponding
> > > > > > size
> > > > > > determination a bit safer according to the Linux coding style
> > > > > > convention.
> > > > >
> > > > >
> > > > > This patch does one style in favor of the other.
> > > >
> > > > I actually prefer that style, so I'd welcome this change :)
> > >
> > > Style changes should be reviewed and documented, like any other code
> > > change, and added to Documentation/process/coding-style.rst or an
> > > equivalent file.
> >
> > Actually, it has been there for many years:
> >
> > 14) Allocating memory
> > -
> > ...
> > The preferred form for passing a size of a struct is the following:
> >
> > .. code-block:: c
> >
> > p = kmalloc(sizeof(*p), ...);
> >
> > The alternative form where struct name is spelled out hurts readability and
> > introduces an opportunity for a bug when the pointer variable type is 
> > changed
> > but the corresponding sizeof that is passed to a memory allocator is not.
>
> True, thanks for the reminder.  Is this common in new code?  Is there
> a script/ or some other automated way of catching this usage before
> patches are upstreamed?
>
> Just as you're doing here, the patch description should reference this
> in the patch description.

The comment in the documentation seems have been there since Linux 2.6.14,
ie 2005.  The fact that a lot of code still doesn't use that style, 12
years later, suggests that actually it is not preferred, or not preferred
by everyone.  Perhaps the paragraph in coding style should just be
dropped.

julia

Re: [PATCH] powerpc/watchdog: Convert timers to use timer_setup()

[Kees Cook]

On Tue, Oct 17, 2017 at 5:29 AM, Michael Ellerman  wrote:
> Nicholas Piggin  writes:
>
>> On Mon, 16 Oct 2017 16:47:10 -0700
>> Kees Cook  wrote:
>>
>>> In preparation for unconditionally passing the struct timer_list pointer to
>>> all timer callbacks, switch to using the new timer_setup() and from_timer()
>>> to pass the timer pointer explicitly.
>>>
>>> Cc: Benjamin Herrenschmidt 
>>> Cc: Paul Mackerras 
>>> Cc: Michael Ellerman 
>>> Cc: Nicholas Piggin 
>>> Cc: linuxppc-dev@lists.ozlabs.org
>>> Signed-off-by: Kees Cook 
>>
>> Looks fine to me. Is this intended to be merged via the powerpc tree
>> in the next merge window?
>
> It relies on the new timer_setup(), which is in one of tglx's trees (I
> think). So I expect it to go via that tree.

It's in -rc3, but the timer tree can carry it if you want. Which do you prefer?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Mimi Zohar]

On Tue, 2017-10-17 at 14:58 +0200, Julia Lawall wrote:
> 
> On Tue, 17 Oct 2017, Mimi Zohar wrote:
> 
> > On Tue, 2017-10-17 at 11:50 +, alexander.stef...@infineon.com
> > wrote:
> > > > > Replace the specification of data structures by pointer dereferences
> > > > > as the parameter for the operator "sizeof" to make the corresponding
> > > > > size
> > > > > determination a bit safer according to the Linux coding style
> > > > > convention.
> > > >
> > > >
> > > > This patch does one style in favor of the other.
> > >
> > > I actually prefer that style, so I'd welcome this change :)
> >
> > Style changes should be reviewed and documented, like any other code
> > change, and added to Documentation/process/coding-style.rst or an
> > equivalent file.
> 
> Actually, it has been there for many years:
> 
> 14) Allocating memory
> -
> ...
> The preferred form for passing a size of a struct is the following:
> 
> .. code-block:: c
> 
>   p = kmalloc(sizeof(*p), ...);
> 
> The alternative form where struct name is spelled out hurts readability and
> introduces an opportunity for a bug when the pointer variable type is changed
> but the corresponding sizeof that is passed to a memory allocator is not.

True, thanks for the reminder.  Is this common in new code?  Is there
a script/ or some other automated way of catching this usage before
patches are upstreamed?

Just as you're doing here, the patch description should reference this
in the patch description.

Mimi

Re: [PATCH v12 01/11] mm: deferred_init_memmap improvements

[Pavel Tatashin]

This really begs to have two patches... I will not insist though. I also
suspect the code can be further simplified but again this is nothing to
block this to go.


Perhaps "page" can be avoided in deferred_init_range(), as pfn is 
converted to page in deferred_free_range, but I have not studied it.


  

Signed-off-by: Pavel Tatashin 
Reviewed-by: Steven Sistare 
Reviewed-by: Daniel Jordan 
Reviewed-by: Bob Picco 


I do not see any obvious issues in the patch

Acked-by: Michal Hocko 


Thank you very much!

Pavel




---
  mm/page_alloc.c | 168 
  1 file changed, 85 insertions(+), 83 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 77e4d3c5c57b..cdbd14829fd3 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1410,14 +1410,17 @@ void clear_zone_contiguous(struct zone *zone)
  }
  
  #ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT

-static void __init deferred_free_range(struct page *page,
-   unsigned long pfn, int nr_pages)
+static void __init deferred_free_range(unsigned long pfn,
+  unsigned long nr_pages)
  {
-   int i;
+   struct page *page;
+   unsigned long i;
  
-	if (!page)

+   if (!nr_pages)
return;
  
+	page = pfn_to_page(pfn);

+
/* Free a large naturally-aligned chunk if possible */
if (nr_pages == pageblock_nr_pages &&
(pfn & (pageblock_nr_pages - 1)) == 0) {
@@ -1443,19 +1446,89 @@ static inline void __init 
pgdat_init_report_one_done(void)
complete(_init_all_done_comp);
  }
  
+/*

+ * Helper for deferred_init_range, free the given range, reset the counters, 
and
+ * return number of pages freed.
+ */
+static inline unsigned long __def_free(unsigned long *nr_free,
+  unsigned long *free_base_pfn,
+  struct page **page)
+{
+   unsigned long nr = *nr_free;
+
+   deferred_free_range(*free_base_pfn, nr);
+   *free_base_pfn = 0;
+   *nr_free = 0;
+   *page = NULL;
+
+   return nr;
+}
+
+static unsigned long deferred_init_range(int nid, int zid, unsigned long pfn,
+unsigned long end_pfn)
+{
+   struct mminit_pfnnid_cache nid_init_state = { };
+   unsigned long nr_pgmask = pageblock_nr_pages - 1;
+   unsigned long free_base_pfn = 0;
+   unsigned long nr_pages = 0;
+   unsigned long nr_free = 0;
+   struct page *page = NULL;
+
+   for (; pfn < end_pfn; pfn++) {
+   /*
+* First we check if pfn is valid on architectures where it is
+* possible to have holes within pageblock_nr_pages. On systems
+* where it is not possible, this function is optimized out.
+*
+* Then, we check if a current large page is valid by only
+* checking the validity of the head pfn.
+*
+* meminit_pfn_in_nid is checked on systems where pfns can
+* interleave within a node: a pfn is between start and end
+* of a node, but does not belong to this memory node.
+*
+* Finally, we minimize pfn page lookups and scheduler checks by
+* performing it only once every pageblock_nr_pages.
+*/
+   if (!pfn_valid_within(pfn)) {
+   nr_pages += __def_free(_free, _base_pfn, );
+   } else if (!(pfn & nr_pgmask) && !pfn_valid(pfn)) {
+   nr_pages += __def_free(_free, _base_pfn, );
+   } else if (!meminit_pfn_in_nid(pfn, nid, _init_state)) {
+   nr_pages += __def_free(_free, _base_pfn, );
+   } else if (page && (pfn & nr_pgmask)) {
+   page++;
+   __init_single_page(page, pfn, zid, nid);
+   nr_free++;
+   } else {
+   nr_pages += __def_free(_free, _base_pfn, );
+   page = pfn_to_page(pfn);
+   __init_single_page(page, pfn, zid, nid);
+   free_base_pfn = pfn;
+   nr_free = 1;
+   cond_resched();
+   }
+   }
+   /* Free the last block of pages to allocator */
+   nr_pages += __def_free(_free, _base_pfn, );
+
+   return nr_pages;
+}
+
  /* Initialise remaining memory on a node */
  static int __init deferred_init_memmap(void *data)
  {
pg_data_t *pgdat = data;
int nid = pgdat->node_id;
-   struct mminit_pfnnid_cache nid_init_state = { };
unsigned long start = jiffies;
unsigned long nr_pages = 0;
-   unsigned long walk_start, walk_end;
-   int i, zid;
+ 

Re: [PATCH 2/2] powerpc/hotplug: Ensure nodes initialized for hotplug

[Michael Bringmann]

On 10/16/2017 07:54 AM, Michael Ellerman wrote:
> Michael Bringmann  writes:
> 
>> powerpc/hotplug: On systems like PowerPC which allow 'hot-add' of CPU,
>> it may occur that the new resources are to be inserted into nodes
>> that were not used for memory resources at bootup.  Many different
>> configurations of PowerPC resources may need to be supported depending
>> upon the environment.
> 
> Give me some detail please?!

Configurations that demonstrated problems included 'memoryless' nodes
that possessed only CPUs at boot, and nodes that contained neither CPUs
nor memory at boot.  The calculations in the kernel resulted in a different
node use layout on many SAP HANA configured systems.

> 
>> This patch fixes some problems encountered at
> 
> What problems?

The previous implementation collapsed all node assignments after affinity
calculations to use only those nodes that had memory at boot.  This
resulted in calculation and configuration differences between the FSP
code and the Linux kernel.

> 
>> runtime with configurations that support memory-less nodes, but which
>> allow CPUs to be added at and after boot.
> 
> How does it fix those problems?

The change involves completing the initialization of nodes that were not
used at boot, but which were selected by VPHN affinity calculations during
subsequent hotplug operations.

Michael

> 
>> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
>> index b385cd0..e811dd1 100644
>> --- a/arch/powerpc/mm/numa.c
>> +++ b/arch/powerpc/mm/numa.c
>> @@ -1325,6 +1325,17 @@ static long vphn_get_associativity(unsigned long cpu,
>>  return rc;
>>  }
>>  
>> +static int verify_node_preparation(int nid)
>> +{
> 
> I would not expect a function called "verify" ...
> 
>> +if ((NODE_DATA(nid) == NULL) ||
>> +(NODE_DATA(nid)->node_spanned_pages == 0)) {
>> +if (try_online_node(nid))
> 
> .. to do something like online a node.
> 
>> +return first_online_node;
>> +}
>> +
>> +return nid;
>> +}
>> +
>>  /*
>>   * Update the CPU maps and sysfs entries for a single CPU when its NUMA
>>   * characteristics change. This function doesn't perform any locking and is
>> @@ -1433,9 +1444,11 @@ int numa_update_cpu_topology(bool cpus_locked)
>>  /* Use associativity from first thread for all siblings */
>>  vphn_get_associativity(cpu, associativity);
>>  new_nid = associativity_to_nid(associativity);
>> -if (new_nid < 0 || !node_online(new_nid))
>> +if (new_nid < 0 || !node_possible(new_nid))
>>  new_nid = first_online_node;
>>  
>> +new_nid = verify_node_preparation(new_nid);
> 
> You're being called part-way through CPU hotplug here, are we sure it's
> safe to go and do memory hotplug from there? What's the locking
> situation?
> 
> cheers
> 
> 

-- 
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:   (512) 466-0650
m...@linux.vnet.ibm.com

Re: [PATCH v2] kernel/module_64.c: Add REL24 relocation support of livepatch symbols

[Torsten Duwe]

On Fri, Oct 06, 2017 at 11:27:42AM +0530, Kamalesh Babulal wrote:
>
> Consider the livepatch sequence[1]. Where function A calls B, B is the
> function which has been livepatched and the call to function B is
> redirected to patched version P. P calls the function C in M2, whereas
> C was local to the function B and have became SHN_UNDEF in function P.
> Local call becoming global.
>
>   ++++++  ++
>   ||   +++--->||  +-->||
>   |  A |   ||  B ||  F |  |   |  P |
>   ||   ||||+--+   ||
>   |+---+||||<-+   ||
>   ||<--+   ++   C|||  |   ||
>   ||   |   | +->||||  |   ||<---+
>   | K / M1 |   |   | |  | K / M2 |  +-+ Kernel |  +---+ Mod3   +--+ |
>   ++   |   | |  ++  | ++  ++  | |
>|   | |  | | |
>+---+-+--+ | |
>| || |
>| ++ |
>++
>
>
> Handling such call with regular stub, triggers another error:
>
> module_64: kpatch_meminfo: Expect noop after relocate, got 3d22
>
> Every branch to SHN_UNDEF is followed by a nop instruction, that gets
> overwritten by an instruction to restore TOC with r2 value that get
> stored onto the stack, before calling the function via global entry
> point.
>
> Given that C was local to function B, it does not store/restore TOC as
> they are not expected to be clobbered for functions called via local
> entry point.

Can you please provide example source code of Mod3 and C? If P calls C, this
is a regular global call, the TOC is saved by the stub and restored after the
call instruction. Why do you think this is not the case? 

Torsten

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bryant G. Ly]

Adding Juan back into the cc: jjalv...@linux.vnet.ibm.com


On 10/16/17 10:38 PM, Michael Ellerman wrote:
> "Bryant G. Ly"  writes:
>> On 10/12/17 1:29 PM, Bjorn Helgaas wrote:
> ...
>>> If that's the case, how to you ever bind a driver to these VFs?  The
>>> changelog says you don't want VF drivers to load *immediately*, so I
>>> assume you do want them to load eventually.
>>>
>> The VF's that get dynamically created within the configure SR-IOV call, 
>> on the Pseries Platform, wont be matched with a driver. - We do not
>> want it to match.
>>
>> The Power Hypervisor will load the VFs. The VF's will get assigned(by 
>> the user) > via the HMC or Novalink in this environment which will
>> then trigger PHYP to load the VF device node to the device tree.
> What about the other "Power Hypervisor"? ie. KVM running on Power.
This path is only exercised when configuring SR-IOV for Pseries LPAR,
therefore it will not affect PowerNV or KVM(opal). Which is the reason for
the separation of calls to the machine dependent stuff.
> We also use the pseries platform when running under KVM.
>
> cheers
>
If anyone plans to enable SR-IOV on Pseries platform, firmware must provide the
resources to enable the VFs and map them with system resources. A new version
of the PAPR Document will be added to document these system resources. Lastly,
we were not aware that there is an intention to enable SR-IOV in adapters 
assigned
to a VM with Pseries running on KVM. Furthermore, this could be left as a todo
for the future if this type of configuration is needed.

-Bryant

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bjorn Helgaas]

On Fri, Oct 13, 2017 at 02:12:32PM -0500, Bryant G. Ly wrote:
> 
> 
> On 10/13/17 1:05 PM, Alex Williamson wrote:
> >On Fri, 13 Oct 2017 07:01:48 -0500
> >Steven Royer  wrote:
> >
> >>On 2017-10-13 06:53, Steven Royer wrote:
> >>>On 2017-10-12 22:34, Bjorn Helgaas wrote:
> [+cc Alex, Bodong, Eli, Saeed]
> 
> On Thu, Oct 12, 2017 at 02:59:23PM -0500, Bryant G. Ly wrote:
> >On 10/12/17 1:29 PM, Bjorn Helgaas wrote:
> >>On Thu, Oct 12, 2017 at 03:09:53PM +1100, Michael Ellerman wrote:
> >>>Bjorn Helgaas  writes:
> On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote:
> >>reading the code what -1/0/1 mean.

> >>Apparently here you *do* want the "-1 means the PCI core will never
> >>set match_driver to 1" functionality, so maybe you do depend on it.
> >We depend on the patch because we want that ability to never set
> >match_driver,
> >for SRIOV on PowerVM.
> Is this really new PowerVM-specific functionality?  ISTR recent
> discussions
> about inhibiting driver binding in a generic way, e.g.,
> http://lkml.kernel.org/r/1490022874-54718-1-git-send-email-bod...@mellanox.com
> >>If that's the case, how to you ever bind a driver to these VFs?  The
> >>changelog says you don't want VF drivers to load *immediately*, so I
> >>assume you do want them to load eventually.
> >The VF's that get dynamically created within the configure SR-IOV
> >call, on the Pseries Platform, wont be matched with a driver. - We
> >do not want it to match.
> >
> >The Power Hypervisor will load the VFs. The VF's will get
> >assigned(by the user) via the HMC or Novalink in this environment
> >which will then trigger PHYP to load the VF device node to the
> >device tree.
> I don't know what it means for the Hypervisor to "load the VFs."  Can
> you explain that in PCI-speak?
> 
> The things I know about are:
> 
>    - we set PCI_SRIOV_CTRL_VFE in the PF, which enables VFs
>    - now the VFs respond to config accesses
>    - the PCI core enumerates the VFs by reading their config space
>    - the PCI core builds pci_dev structs for the VFs
>    - the PCI core adds these pci_devs to the bus
>    - we try to bind drivers to the VFs
>    - the VF driver probe function may read VF config space and VF BARs
>    - the VF may be assigned to a guest VM
> 
> Where does "loading the VFs" fit in?  I don't know what HMC, Novalink,
> or PHYP are.  I don't *need* to know what they are, as long as you can
> explain what's happening in terms of the PCI concepts and generic
> Linux VMs
> and device assignment.
> 
> Bjorn
> >>>The VFs will be hotplugged into the VM separately from the enable
> >>>SR-IOV, so the driver will load as part of the hotplug operation.
> >>>
> >>>Steve
> >>One more point of clarification: when the hotplug happens, the VF will
> >>show up on a virtual PCI bus that is not directly correlated to the real
> >>PCI bus that the PF is on.  On that virtual PCI bus, the driver will
> >>match because it won't be set to -1.
> So lets refer to Bjorn's list of things for SRIOV.
> 
>   - we set PCI_SRIOV_CTRL_VFE in the PF, which enables VFs
>   - now the VFs respond to config accesses
>   - the PCI core enumerates the VFs by reading their config space
>   - the PCI core builds pci_dev structs for the VFs
>   - the PCI core adds these pci_devs to the bus
> 
> So everything is the same up to here.
>   - we try to bind drivers to the VFs
>   - the VF driver probe function may read VF config space and VF BARs
>   - the VF may be assigned to a guest VM
> 
> PowerVM environment is very different than traditional KVM in terms
> of SRIOV.  In our environment the VFs are not usable or view-able by
> the Hosting Partition in this case Linux. This is a very important
> point in that the Host CAN NOT do anything to any of the VFs
> available.

This is where I get confused.  I guess the Linux that sets
PCI_SRIOV_CTRL_VFE to enable the VFs can also perform config accesses
to the VFs, since it can enumerate them and build pci_dev structs for
them, right?

And the Linux in the "Hosting Partition" is a guest that cannot see a
VF until a management console attaches the VF to the Hosting
Partition?  I'm not a VFIO or KVM expert but that sounds vaguely like
what they would do when assigning a VF to a guest.

> So like existing way of enabling SRIOV we still rely on the PF driver to
> enable VFs - but in this case the attachment phase is done via a user
> action via a management console in our case (novalink or hmc) triggered
> event that will essentially act like a hotplug.
> 
> So in the fine details of that user triggered action the system
> firmware will bind the VFs, allowing resources to be allocated to
> the VF.  - Which essentially does all the attaching as we know it
> today but is managed by PHYP not by the 

Re: [bug report] out of bounds read parsing vmode commandline option

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday, October 04, 2017 06:42:37 PM Geert Uytterhoeven wrote:
> Hi Dan,
> 
> On Wed, Oct 4, 2017 at 2:50 PM, Dan Carpenter  
> wrote:
> > This bug predates git but it looks like it might be simple to fix if the
> > right person looked at the code.
> >
> > drivers/video/fbdev/controlfb.c:560 control_setup()
> > error: buffer overflow 'control_mac_modes' 20 <= 21
> >
> > drivers/video/fbdev/controlfb.c
> >549  static void __init control_setup(char *options)
> >550  {
> >551  char *this_opt;
> >552
> >553  if (!options || !*options)
> >554  return;
> >555
> >556  while ((this_opt = strsep(, ",")) != NULL) {
> >557  if (!strncmp(this_opt, "vmode:", 6)) {
> >558  int vmode = simple_strtoul(this_opt+6, 
> > NULL, 0);
> > ^
> > We get vmode from the command line.
> >
> >559  if (vmode > 0 && vmode <= VMODE_MAX &&
> >   ^
> > We check that it's <= 22.
> >
> >560  control_mac_modes[vmode - 1].m[1] >= 0)
> > ^
> > But the problem is that control_mac_modes[] only has 20 elements so the
> > highest valid index is 19.  vmode - 1 can be up to 21.
> 
> Nice catch!
> 
> The bug was introduced in v2.4.5.6, when 2 new modes were added to
> macmodes.h, but control_mac_modes[] wasn't updated:
> 
> https://kernel.opensuse.org/cgit/kernel/diff/include/video/macmodes.h?h=v2.5.2=29f279c764808560eaceb88fef36cbc35c529aad
> 
> A simple fix is to check against ARRAY_SIZE(control_mac_modes) instead.
> 
> A better fix is to add the missing entries to control_mac_modes[], cfr. the
> (gmail-whitespace-damaged) patch below:
> 
> --- a/drivers/video/fbdev/controlfb.h
> +++ b/drivers/video/fbdev/controlfb.h
> @@ -141,5 +141,7 @@ static struct max_cmodes control_mac_modes[] = {
> {{ 1, 2}},  /* 1152x870, 75Hz */
> {{ 0, 1}},  /* 1280x960, 75Hz */
> {{ 0, 1}},  /* 1280x1024, 75Hz */
> +   {{ 1, 2}},  /* 1152x768, 60Hz */
> +   {{ 0, 1}},  /* 1600x1024, 60Hz */
>  };
> 
> (this array lists the maximum color mode (8, 16, or 32 bpp) for each
>  video mode given RAM restrictions (2 or 4 MiB)).
> 
> The 1152x768 mode is probably OK. Given the 1600x1024 mode has a lower
> dotclock (112 MHz) than the supported 1280x960 mode, it's probably OK, too.

Looks fine to me, could you please submit it as a normal patch?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R Institute Poland
Samsung Electronics

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Andy Shevchenko]

On Tue, 2017-10-17 at 08:52 -0400, Mimi Zohar wrote:
> On Tue, 2017-10-17 at 11:50 +, alexander.stef...@infineon.com
> wrote:
> > > > Replace the specification of data structures by pointer
> > > > dereferences
> > > > as the parameter for the operator "sizeof" to make the
> > > > corresponding
> > > > size
> > > > determination a bit safer according to the Linux coding style
> > > > convention.
> > > 
> > > 
> > > This patch does one style in favor of the other.
> > 
> > I actually prefer that style, so I'd welcome this change :)
> 
> Style changes should be reviewed and documented, like any other code
> change, and added to Documentation/process/coding-style.rst or an
> equivalent file.

+1.

> > > At the end it's Jarkko's call, though I would NAK this as I think
> > > some
> > > one already told this to you for some other similar patch(es).
> > > 
> > > 
> > > I even would suggest to stop doing this noisy stuff, which keeps
> > > people
> > > busy for nothing.
> > 
> > Cleaning up old code is also worth something, even if does not
> > change one bit in the assembly output in the end...
> 
> Wow, you're opening the door really wide for all sorts of trivial
> changes!  Hope you have the time and inclination to review and comment
> on all of them.  I certainly don't.

Moreover and not so obvious is an open door for making back port of
*real* fixes much harder!

> There is a major difference between adding these sorts of checks to
> the tools in the scripts directory or even to the zero day bots that
> catch different sorts of errors, BEFORE code is upstreamed, and
> patches like these, after the fact.

+1.

> After the code has been upstreamed, it is a lot more difficult to
> justify changes like this.  It impacts both code that is being
> developed AND backporting bug fixes.

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Julia Lawall]

On Tue, 17 Oct 2017, Mimi Zohar wrote:

> On Tue, 2017-10-17 at 11:50 +, alexander.stef...@infineon.com
> wrote:
> > > > Replace the specification of data structures by pointer dereferences
> > > > as the parameter for the operator "sizeof" to make the corresponding
> > > > size
> > > > determination a bit safer according to the Linux coding style
> > > > convention.
> > >
> > >
> > > This patch does one style in favor of the other.
> >
> > I actually prefer that style, so I'd welcome this change :)
>
> Style changes should be reviewed and documented, like any other code
> change, and added to Documentation/process/coding-style.rst or an
> equivalent file.

Actually, it has been there for many years:

14) Allocating memory
-
...
The preferred form for passing a size of a struct is the following:

.. code-block:: c

p = kmalloc(sizeof(*p), ...);

The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not.

julia

>
> > > At the end it's Jarkko's call, though I would NAK this as I think some
> > > one already told this to you for some other similar patch(es).
> > >
> > >
> > > I even would suggest to stop doing this noisy stuff, which keeps people
> > > busy for nothing.
> >
> > Cleaning up old code is also worth something, even if does not
> > change one bit in the assembly output in the end...
>
> Wow, you're opening the door really wide for all sorts of trivial
> changes!  Hope you have the time and inclination to review and comment
> on all of them.  I certainly don't.
>
> There is a major difference between adding these sorts of checks to
> the tools in the scripts directory or even to the zero day bots that
> catch different sorts of errors, BEFORE code is upstreamed, and
> patches like these, after the fact.
>
> After the code has been upstreamed, it is a lot more difficult to
> justify changes like this.  It impacts both code that is being
> developed AND backporting bug fixes.
>
> Mimi
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Mimi Zohar]

On Tue, 2017-10-17 at 11:50 +, alexander.stef...@infineon.com
wrote:
> > > Replace the specification of data structures by pointer dereferences
> > > as the parameter for the operator "sizeof" to make the corresponding
> > > size
> > > determination a bit safer according to the Linux coding style
> > > convention.
> > 
> > 
> > This patch does one style in favor of the other.
> 
> I actually prefer that style, so I'd welcome this change :)

Style changes should be reviewed and documented, like any other code
change, and added to Documentation/process/coding-style.rst or an
equivalent file.

> > At the end it's Jarkko's call, though I would NAK this as I think some
> > one already told this to you for some other similar patch(es).
> > 
> > 
> > I even would suggest to stop doing this noisy stuff, which keeps people
> > busy for nothing.
> 
> Cleaning up old code is also worth something, even if does not
> change one bit in the assembly output in the end...

Wow, you're opening the door really wide for all sorts of trivial
changes!  Hope you have the time and inclination to review and comment
on all of them.  I certainly don't.

There is a major difference between adding these sorts of checks to
the tools in the scripts directory or even to the zero day bots that
catch different sorts of errors, BEFORE code is upstreamed, and
patches like these, after the fact.

After the code has been upstreamed, it is a lot more difficult to
justify changes like this.  It impacts both code that is being
developed AND backporting bug fixes.

Mimi

Re: [PATCH] powerpc/watchdog: Convert timers to use timer_setup()

[Michael Ellerman]

Nicholas Piggin  writes:

> On Mon, 16 Oct 2017 16:47:10 -0700
> Kees Cook  wrote:
>
>> In preparation for unconditionally passing the struct timer_list pointer to
>> all timer callbacks, switch to using the new timer_setup() and from_timer()
>> to pass the timer pointer explicitly.
>> 
>> Cc: Benjamin Herrenschmidt 
>> Cc: Paul Mackerras 
>> Cc: Michael Ellerman 
>> Cc: Nicholas Piggin 
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Signed-off-by: Kees Cook 
>
> Looks fine to me. Is this intended to be merged via the powerpc tree
> in the next merge window?

It relies on the new timer_setup(), which is in one of tglx's trees (I
think). So I expect it to go via that tree.

cheers

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Michael Ellerman]

Dan Carpenter  writes:

> On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote:
>> 
>> 
>> On Tue, 17 Oct 2017, Dan Carpenter wrote:
>> 
>> > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
>> > >
>> > > A minor complaint: all commits are missing "Fixes:" tag.
>> > >
>> >
>> > Fixes is only for bug fixes.  These don't fix any bugs.
>> 
>> 0-day seems to put Fixes for everything.  Should they be removed when the
>> old code is undesirable but doesn't actually cause a crash, eg out of date
>> API.
>
> Yeah, I feel like Fixes tags don't belong for API updates and cleanups.

I try to use the criteria of "if someone had backported commit A, would
they also want commit B" (where B Fixes: A).

So it's a bit broader than just "A had a *bug*" and this is the fix.

That's obviously still a bit of a slippery slope, but somewhat helpful I
think. eg, pretty much no one is interested in backporting spelling
fixes, so those aren't Fixes.

And generally people aren't interested in backporting commits like these
ones that just update coding style.

cheers

Re: [PATCH v2 2/2] vgaarb: Factor out EFI and fallback default device selection

[Ard Biesheuvel]

On 17 October 2017 at 13:05, Daniel Vetter  wrote:
> On Tue, Oct 17, 2017 at 01:03:46PM +1100, Daniel Axtens wrote:
>> Bjorn Helgaas  writes:
>>
>> > The default VGA device is normally set in vga_arbiter_add_pci_device() when
>> > we call it for the first enabled device that can be accessed with the
>> > legacy VGA resources ([mem 0xa-0xb], etc.)
>> >
>> > That default device can be overridden by an EFI device that owns the boot
>> > framebuffer.  As a fallback, we can also select a VGA device that can't be
>> > accessed via legacy VGA resources, or a VGA device that isn't even enabled.
>> >
>> > Factor out this EFI and fallback selection from vga_arb_device_init() into
>> > a separate vga_arb_select_default_device() function.  This doesn't change
>> > any behavior, but it untangles the "bridge control possible" checking and
>> > messages from the default device selection.
>> >
>> > Tested-by: Zhou Wang   # D05 Hisi Hip07, Hip08
>> > Signed-off-by: Bjorn Helgaas 
>> > ---
>> >  drivers/gpu/vga/vgaarb.c |   57 
>> > --
>> >  1 file changed, 35 insertions(+), 22 deletions(-)
>> >
>> > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
>> > index 8035e38d5110..d35d6d271f3f 100644
>> > --- a/drivers/gpu/vga/vgaarb.c
>> > +++ b/drivers/gpu/vga/vgaarb.c
>> > @@ -1402,29 +1402,14 @@ static struct miscdevice vga_arb_device = {
>> > MISC_DYNAMIC_MINOR, "vga_arbiter", _arb_device_fops
>> >  };
>> >
>> > -static int __init vga_arb_device_init(void)
>> > +static void __init vga_arb_select_default_device(void)
>> >  {
>> > -   int rc;
>> > struct pci_dev *pdev;
>> > struct vga_device *vgadev;
>> >
>> > -   rc = misc_register(_arb_device);
>> > -   if (rc < 0)
>> > -   pr_err("error %d registering device\n", rc);
>> > -
>> > -   bus_register_notifier(_bus_type, _notifier);
>> > -
>> > -   /* We add all pci devices satisfying vga class in the arbiter by
>> > -* default */
>> > -   pdev = NULL;
>> > -   while ((pdev =
>> > -   pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
>> > -  PCI_ANY_ID, pdev)) != NULL)
>> > -   vga_arbiter_add_pci_device(pdev);
>> > -
>> > +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
>> > list_for_each_entry(vgadev, _list, list) {
>> > struct device *dev = >pdev->dev;
>> > -#if defined(CONFIG_X86) || defined(CONFIG_IA64)
>> > /*
>> >  * Override vga_arbiter_add_pci_device()'s I/O based detection
>> >  * as it may take the wrong device (e.g. on Apple system under
>> > @@ -1461,12 +1446,8 @@ static int __init vga_arb_device_init(void)
>> > vgaarb_info(dev, "overriding boot device\n");
>> > vga_set_default_device(vgadev->pdev);
>> > }
>> > -#endif
>> > -   if (vgadev->bridge_has_one_vga)
>> > -   vgaarb_info(dev, "bridge control possible\n");
>> > -   else
>> > -   vgaarb_info(dev, "no bridge control possible\n");
>> > }
>> > +#endif
>> >
>> > if (!vga_default_device()) {
>> > list_for_each_entry(vgadev, _list, list) {
>> > @@ -1492,6 +1473,38 @@ static int __init vga_arb_device_init(void)
>> > vga_set_default_device(vgadev->pdev);
>> > }
>> > }
>> > +}
>> > +
>> > +static int __init vga_arb_device_init(void)
>> > +{
>> > +   int rc;
>> > +   struct pci_dev *pdev;
>> > +   struct vga_device *vgadev;
>> > +
>> > +   rc = misc_register(_arb_device);
>> > +   if (rc < 0)
>> > +   pr_err("error %d registering device\n", rc);
>> > +
>> > +   bus_register_notifier(_bus_type, _notifier);
>> > +
>> > +   /* We add all PCI devices satisfying VGA class in the arbiter by
>> > +* default */
>> > +   pdev = NULL;
>> > +   while ((pdev =
>> > +   pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
>> > +  PCI_ANY_ID, pdev)) != NULL)
>> > +   vga_arbiter_add_pci_device(pdev);
>> > +
>> > +   list_for_each_entry(vgadev, _list, list) {
>> > +   struct device *dev = >pdev->dev;
>> > +
>> > +   if (vgadev->bridge_has_one_vga)
>> > +   vgaarb_info(dev, "bridge control possible\n");
>> > +   else
>> > +   vgaarb_info(dev, "no bridge control possible\n");
>> > +   }
>>
>> Initially I wondered if this info printk could be moved into
>> vga_arbiter_check_bridge_sharing(), but it's been separated out since
>> 3448a19da479b ("vgaarb: use bridges to control VGA routing where
>> possible."), and upon closer examination, it seems you can't be sure a
>> device doesn't share a bridge until the end of the process, so this is
>> indeed correct.
>>
>> Everything else also looks good to me.
>>
>> Reviewed-by: Daniel Axtens 
>
> R-b for both patches? And ok with everyone if I pull this 

[PATCH 3/3] powerpc-opal: Fix a typo in a comment line of two file headers

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 17 Oct 2017 13:31:42 +0200

Fix a word in these descriptions.

Signed-off-by: Markus Elfring 
---
 arch/powerpc/platforms/powernv/opal-hmi.c   | 2 +-
 arch/powerpc/platforms/powernv/opal-memory-errors.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal-hmi.c 
b/arch/powerpc/platforms/powernv/opal-hmi.c
index 92b1e12a7eed..481a004b1b6d 100644
--- a/arch/powerpc/platforms/powernv/opal-hmi.c
+++ b/arch/powerpc/platforms/powernv/opal-hmi.c
@@ -1,5 +1,5 @@
 /*
- * OPAL hypervisor Maintenance interrupt handling support in PowreNV.
+ * OPAL hypervisor Maintenance interrupt handling support in PowerNV.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
diff --git a/arch/powerpc/platforms/powernv/opal-memory-errors.c 
b/arch/powerpc/platforms/powernv/opal-memory-errors.c
index 875e80ab96d9..fd500b379560 100644
--- a/arch/powerpc/platforms/powernv/opal-memory-errors.c
+++ b/arch/powerpc/platforms/powernv/opal-memory-errors.c
@@ -1,5 +1,5 @@
 /*
- * OPAL asynchronus Memory error handling support in PowreNV.
+ * OPAL asynchronus Memory error handling support in PowerNV.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
-- 
2.14.2

[PATCH 2/3] powerpc-opal: Improve 12 size determinations

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 17 Oct 2017 13:20:19 +0200

Replace the specification of data types by variable references
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring 
---
 arch/powerpc/platforms/powernv/opal-flash.c |  4 ++--
 arch/powerpc/platforms/powernv/opal-hmi.c   |  2 +-
 arch/powerpc/platforms/powernv/opal-imc.c   | 10 +-
 arch/powerpc/platforms/powernv/opal-memory-errors.c |  2 +-
 arch/powerpc/platforms/powernv/opal-psr.c   |  2 +-
 arch/powerpc/platforms/powernv/opal-sensor-groups.c |  4 ++--
 arch/powerpc/platforms/powernv/opal-xscom.c |  2 +-
 7 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal-flash.c 
b/arch/powerpc/platforms/powernv/opal-flash.c
index 77d564266a59..9c6b54c86d71 100644
--- a/arch/powerpc/platforms/powernv/opal-flash.c
+++ b/arch/powerpc/platforms/powernv/opal-flash.c
@@ -418,12 +418,12 @@ static int alloc_image_buf(char *buffer, size_t count)
void *addr;
int size;
 
-   if (count < sizeof(struct image_header_t)) {
+   if (count < sizeof(image_header)) {
pr_warn("FLASH: Invalid candidate image\n");
return -EINVAL;
}
 
-   memcpy(_header, (void *)buffer, sizeof(struct image_header_t));
+   memcpy(_header, (void *)buffer, sizeof(image_header));
image_data.size = be32_to_cpu(image_header.size);
pr_debug("FLASH: Candidate image size = %u\n", image_data.size);
 
diff --git a/arch/powerpc/platforms/powernv/opal-hmi.c 
b/arch/powerpc/platforms/powernv/opal-hmi.c
index b7dfe41a0a96..92b1e12a7eed 100644
--- a/arch/powerpc/platforms/powernv/opal-hmi.c
+++ b/arch/powerpc/platforms/powernv/opal-hmi.c
@@ -313,7 +313,7 @@ static int opal_handle_hmi_event(struct notifier_block *nb,
if (!msg_node)
return -ENOMEM;
 
-   memcpy(_node->hmi_evt, hmi_evt, sizeof(struct OpalHMIEvent));
+   memcpy(_node->hmi_evt, hmi_evt, sizeof(*hmi_evt));
 
spin_lock_irqsave(_hmi_evt_lock, flags);
list_add(_node->list, _hmi_evt_list);
diff --git a/arch/powerpc/platforms/powernv/opal-imc.c 
b/arch/powerpc/platforms/powernv/opal-imc.c
index 21f6531fae20..2302cfb953bd 100644
--- a/arch/powerpc/platforms/powernv/opal-imc.c
+++ b/arch/powerpc/platforms/powernv/opal-imc.c
@@ -38,11 +38,11 @@ static int imc_get_mem_addr_nest(struct device_node *node,
if (nr_chips <= 0)
return -ENODEV;
 
-   base_addr_arr = kcalloc(nr_chips, sizeof(u64), GFP_KERNEL);
+   base_addr_arr = kcalloc(nr_chips, sizeof(*base_addr_arr), GFP_KERNEL);
if (!base_addr_arr)
return -ENOMEM;
 
-   chipid_arr = kcalloc(nr_chips, sizeof(u32), GFP_KERNEL);
+   chipid_arr = kcalloc(nr_chips, sizeof(*chipid_arr), GFP_KERNEL);
if (!chipid_arr)
return -ENOMEM;
 
@@ -53,8 +53,8 @@ static int imc_get_mem_addr_nest(struct device_node *node,
nr_chips))
goto error;
 
-   pmu_ptr->mem_info = kcalloc(nr_chips, sizeof(struct imc_mem_info),
-   GFP_KERNEL);
+   pmu_ptr->mem_info = kcalloc(nr_chips, sizeof(*pmu_ptr->mem_info),
+   GFP_KERNEL);
if (!pmu_ptr->mem_info)
goto error;
 
@@ -88,7 +88,7 @@ static int imc_pmu_create(struct device_node *parent, int 
pmu_index, int domain)
u32 offset;
 
/* memory for pmu */
-   pmu_ptr = kzalloc(sizeof(struct imc_pmu), GFP_KERNEL);
+   pmu_ptr = kzalloc(sizeof(*pmu_ptr), GFP_KERNEL);
if (!pmu_ptr)
return -ENOMEM;
 
diff --git a/arch/powerpc/platforms/powernv/opal-memory-errors.c 
b/arch/powerpc/platforms/powernv/opal-memory-errors.c
index 8d76c05252d1..875e80ab96d9 100644
--- a/arch/powerpc/platforms/powernv/opal-memory-errors.c
+++ b/arch/powerpc/platforms/powernv/opal-memory-errors.c
@@ -110,7 +110,7 @@ static int opal_memory_err_event(struct notifier_block *nb,
if (!msg_node)
return -ENOMEM;
 
-   memcpy(_node->msg, msg, sizeof(struct opal_msg));
+   memcpy(_node->msg, msg, sizeof(msg_node->msg));
 
spin_lock_irqsave(_mem_err_lock, flags);
list_add(_node->list, _memory_err_list);
diff --git a/arch/powerpc/platforms/powernv/opal-psr.c 
b/arch/powerpc/platforms/powernv/opal-psr.c
index 7313b7fc9071..74986b35cf77 100644
--- a/arch/powerpc/platforms/powernv/opal-psr.c
+++ b/arch/powerpc/platforms/powernv/opal-psr.c
@@ -136,7 +136,7 @@ void __init opal_psr_init(void)
return;
}
 
-   psr_attrs = kcalloc(of_get_child_count(psr), sizeof(struct psr_attr),
+   psr_attrs = 

[PATCH 1/3] powerpc-opal: Delete ten error messages for a failed memory allocation

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 17 Oct 2017 12:21:40 +0200

Omit extra messages for a memory allocation failure in these functions.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 arch/powerpc/platforms/powernv/opal-async.c|  2 --
 arch/powerpc/platforms/powernv/opal-dump.c |  1 -
 arch/powerpc/platforms/powernv/opal-flash.c|  4 +---
 arch/powerpc/platforms/powernv/opal-hmi.c  |  5 ++---
 .../powerpc/platforms/powernv/opal-memory-errors.c |  6 ++
 arch/powerpc/platforms/powernv/opal-sysparam.c | 25 +-
 6 files changed, 10 insertions(+), 33 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal-async.c 
b/arch/powerpc/platforms/powernv/opal-async.c
index cf33769a7b72..2b11d8444b5a 100644
--- a/arch/powerpc/platforms/powernv/opal-async.c
+++ b/arch/powerpc/platforms/powernv/opal-async.c
@@ -193,8 +193,6 @@ int __init opal_async_comp_init(void)
sizeof(*opal_async_responses) * opal_max_async_tokens,
GFP_KERNEL);
if (!opal_async_responses) {
-   pr_err("%s: Out of memory, failed to do asynchronous "
-   "completion init\n", __func__);
err = -ENOMEM;
goto out_opal_node;
}
diff --git a/arch/powerpc/platforms/powernv/opal-dump.c 
b/arch/powerpc/platforms/powernv/opal-dump.c
index 4c827826c05e..b54864feef84 100644
--- a/arch/powerpc/platforms/powernv/opal-dump.c
+++ b/arch/powerpc/platforms/powernv/opal-dump.c
@@ -244,7 +244,6 @@ static int64_t dump_read_data(struct dump_obj *dump)
/* Allocate memory */
dump->buffer = vzalloc(PAGE_ALIGN(dump->size));
if (!dump->buffer) {
-   pr_err("%s : Failed to allocate memory\n", __func__);
rc = -ENOMEM;
goto out;
}
diff --git a/arch/powerpc/platforms/powernv/opal-flash.c 
b/arch/powerpc/platforms/powernv/opal-flash.c
index 2fa3ac80cb4e..77d564266a59 100644
--- a/arch/powerpc/platforms/powernv/opal-flash.c
+++ b/arch/powerpc/platforms/powernv/opal-flash.c
@@ -437,10 +437,8 @@ static int alloc_image_buf(char *buffer, size_t count)
}
 
image_data.data = vzalloc(PAGE_ALIGN(image_data.size));
-   if (!image_data.data) {
-   pr_err("%s : Failed to allocate memory\n", __func__);
+   if (!image_data.data)
return -ENOMEM;
-   }
 
/* Pin memory */
addr = image_data.data;
diff --git a/arch/powerpc/platforms/powernv/opal-hmi.c 
b/arch/powerpc/platforms/powernv/opal-hmi.c
index d78fed728cdf..b7dfe41a0a96 100644
--- a/arch/powerpc/platforms/powernv/opal-hmi.c
+++ b/arch/powerpc/platforms/powernv/opal-hmi.c
@@ -310,10 +310,9 @@ static int opal_handle_hmi_event(struct notifier_block *nb,
 
/* Delay the logging of HMI events to workqueue. */
msg_node = kzalloc(sizeof(*msg_node), GFP_ATOMIC);
-   if (!msg_node) {
-   pr_err("HMI: out of memory, Opal message event not handled\n");
+   if (!msg_node)
return -ENOMEM;
-   }
+
memcpy(_node->hmi_evt, hmi_evt, sizeof(struct OpalHMIEvent));
 
spin_lock_irqsave(_hmi_evt_lock, flags);
diff --git a/arch/powerpc/platforms/powernv/opal-memory-errors.c 
b/arch/powerpc/platforms/powernv/opal-memory-errors.c
index 4495f428b500..8d76c05252d1 100644
--- a/arch/powerpc/platforms/powernv/opal-memory-errors.c
+++ b/arch/powerpc/platforms/powernv/opal-memory-errors.c
@@ -107,11 +107,9 @@ static int opal_memory_err_event(struct notifier_block *nb,
return 0;
 
msg_node = kzalloc(sizeof(*msg_node), GFP_ATOMIC);
-   if (!msg_node) {
-   pr_err("MEMORY_ERROR: out of memory, Opal message event not"
-  "handled\n");
+   if (!msg_node)
return -ENOMEM;
-   }
+
memcpy(_node->msg, msg, sizeof(struct opal_msg));
 
spin_lock_irqsave(_mem_err_lock, flags);
diff --git a/arch/powerpc/platforms/powernv/opal-sysparam.c 
b/arch/powerpc/platforms/powernv/opal-sysparam.c
index 23fb6647dced..1df05efe55a1 100644
--- a/arch/powerpc/platforms/powernv/opal-sysparam.c
+++ b/arch/powerpc/platforms/powernv/opal-sysparam.c
@@ -184,11 +184,8 @@ void __init opal_sys_param_init(void)
 
/* Allocate big enough buffer for any get/set transactions */
param_data_buf = kzalloc(MAX_PARAM_DATA_LEN, GFP_KERNEL);
-   if (!param_data_buf) {
-   pr_err("SYSPARAM: Failed to allocate memory for param data "
-   "buf\n");
+   if (!param_data_buf)
goto out_kobj_put;
-   }
 
/* Number of parameters exposed through DT */
count = of_property_count_strings(sysparam, "param-name");
@@ -199,25 +196,16 @@ void __init opal_sys_param_init(void)
}
 
id = kzalloc(sizeof(*id) * count, 

[PATCH 0/3] PowerPC-OPAL: Adjustments for some function implementations

[SF Markus Elfring]

From: Markus Elfring 
Date: Tue, 17 Oct 2017 13:45:54 +0200

Three update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Delete ten error messages for a failed memory allocation
  Improve 12 size determinations
  Fix a typo in a comment line of two file headers

 arch/powerpc/platforms/powernv/opal-async.c|  2 --
 arch/powerpc/platforms/powernv/opal-dump.c |  1 -
 arch/powerpc/platforms/powernv/opal-flash.c|  8 +++
 arch/powerpc/platforms/powernv/opal-hmi.c  |  9 
 arch/powerpc/platforms/powernv/opal-imc.c  | 10 -
 .../powerpc/platforms/powernv/opal-memory-errors.c | 10 -
 arch/powerpc/platforms/powernv/opal-psr.c  |  2 +-
 .../powerpc/platforms/powernv/opal-sensor-groups.c |  4 ++--
 arch/powerpc/platforms/powernv/opal-sysparam.c | 25 +-
 arch/powerpc/platforms/powernv/opal-xscom.c|  2 +-
 10 files changed, 25 insertions(+), 48 deletions(-)

-- 
2.14.2