Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1692472 > unrolled thread

Re: [RFC v6 27/62] powerpc: helper to validate key-access permissions of a pte

Started by"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
First post2017-07-20 08:50 +0200
Last post2017-07-21 18:50 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC v6 27/62] powerpc: helper to validate key-access permissions of a pte "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-07-20 08:50 +0200
    Re: [RFC v6 27/62] powerpc: helper to validate key-access  permissions of a pte Ram Pai <linuxram@us.ibm.com> - 2017-07-21 00:20 +0200
      Re: [RFC v6 27/62] powerpc: helper to validate key-access permissions of a pte "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-07-21 09:00 +0200
        Re: [RFC v6 27/62] powerpc: helper to validate key-access  permissions of a pte Ram Pai <linuxram@us.ibm.com> - 2017-07-21 18:50 +0200

#1692472 — Re: [RFC v6 27/62] powerpc: helper to validate key-access permissions of a pte

From"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
Date2017-07-20 08:50 +0200
SubjectRe: [RFC v6 27/62] powerpc: helper to validate key-access permissions of a pte
Message-ID<u5drz-3uU-7@gated-at.bofh.it>
Ram Pai <linuxram@us.ibm.com> writes:

> helper function that checks if the read/write/execute is allowed
> on the pte.
>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
>  arch/powerpc/include/asm/book3s/64/pgtable.h |    4 +++
>  arch/powerpc/include/asm/pkeys.h             |   12 +++++++++
>  arch/powerpc/mm/pkeys.c                      |   33 ++++++++++++++++++++++++++
>  3 files changed, 49 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
> index 30d7f55..0056e58 100644
> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> @@ -472,6 +472,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 bbb5d85..7a9aade 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -53,6 +53,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;

Do we really need that above check ? We should always find it
peky_inited to be set. 

> +
> +	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));
> +}
> +


-aneesh

[toc] | [next] | [standalone]

#1693256 — Re: [RFC v6 27/62] powerpc: helper to validate key-access permissions of a pte

FromRam Pai <linuxram@us.ibm.com>
Date2017-07-21 00:20 +0200
SubjectRe: [RFC v6 27/62] powerpc: helper to validate key-access permissions of a pte
Message-ID<u5rXz-4QU-1@gated-at.bofh.it>
In reply to#1692472
On Thu, Jul 20, 2017 at 12:12:47PM +0530, Aneesh Kumar K.V wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > helper function that checks if the read/write/execute is allowed
> > on the pte.
> >
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > ---
> >  arch/powerpc/include/asm/book3s/64/pgtable.h |    4 +++
> >  arch/powerpc/include/asm/pkeys.h             |   12 +++++++++
> >  arch/powerpc/mm/pkeys.c                      |   33 ++++++++++++++++++++++++++
> >  3 files changed, 49 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
> > index 30d7f55..0056e58 100644
> > --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> > +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> > @@ -472,6 +472,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 bbb5d85..7a9aade 100644
> > --- a/arch/powerpc/include/asm/pkeys.h
> > +++ b/arch/powerpc/include/asm/pkeys.h
> > @@ -53,6 +53,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;
> 
> Do we really need that above check ? We should always find it
> peky_inited to be set. 

Yes. there are cases where pkey_inited is not enabled. 
a) if the MMU is radix.
b) if the PAGE size is 4k.
c) if the device tree says the feature is not available
d) if the CPU is of a older generation. P6 and older.

RP

[toc] | [prev] | [next] | [standalone]

#1693429

From"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
Date2017-07-21 09:00 +0200
Message-ID<u5A4N-1mZ-17@gated-at.bofh.it>
In reply to#1693256
Ram Pai <linuxram@us.ibm.com> writes:

> On Thu, Jul 20, 2017 at 12:12:47PM +0530, Aneesh Kumar K.V wrote:
>> Ram Pai <linuxram@us.ibm.com> writes:
>> 
>> > helper function that checks if the read/write/execute is allowed
>> > on the pte.
>> >
>> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>> > ---
>> >  arch/powerpc/include/asm/book3s/64/pgtable.h |    4 +++
>> >  arch/powerpc/include/asm/pkeys.h             |   12 +++++++++
>> >  arch/powerpc/mm/pkeys.c                      |   33 ++++++++++++++++++++++++++
>> >  3 files changed, 49 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
>> > index 30d7f55..0056e58 100644
>> > --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
>> > +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
>> > @@ -472,6 +472,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 bbb5d85..7a9aade 100644
>> > --- a/arch/powerpc/include/asm/pkeys.h
>> > +++ b/arch/powerpc/include/asm/pkeys.h
>> > @@ -53,6 +53,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;
>> 
>> Do we really need that above check ? We should always find it
>> peky_inited to be set. 
>
> Yes. there are cases where pkey_inited is not enabled. 
> a) if the MMU is radix.
That should be be a feature check

> b) if the PAGE size is 4k.

That is a kernel config change

> c) if the device tree says the feature is not available
> d) if the CPU is of a older generation. P6 and older.

Both feature check.

how about doing something like

static inline u16 pte_to_pkey_bits(u64 pteflags)
{
	if (!(pteflags & H_PAGE_KEY_MASK))
		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));
}

We still have to look at the code generated to see it is really saving
something.

-aneesh

[toc] | [prev] | [next] | [standalone]

#1693845 — Re: [RFC v6 27/62] powerpc: helper to validate key-access permissions of a pte

FromRam Pai <linuxram@us.ibm.com>
Date2017-07-21 18:50 +0200
SubjectRe: [RFC v6 27/62] powerpc: helper to validate key-access permissions of a pte
Message-ID<u5JhM-7eu-5@gated-at.bofh.it>
In reply to#1693429
On Fri, Jul 21, 2017 at 12:21:50PM +0530, Aneesh Kumar K.V wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > On Thu, Jul 20, 2017 at 12:12:47PM +0530, Aneesh Kumar K.V wrote:
> >> Ram Pai <linuxram@us.ibm.com> writes:
> >> 
> >> > helper function that checks if the read/write/execute is allowed
> >> > on the pte.
> >> >
> >> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> >> > ---
> >> >  arch/powerpc/include/asm/book3s/64/pgtable.h |    4 +++
> >> >  arch/powerpc/include/asm/pkeys.h             |   12 +++++++++
> >> >  arch/powerpc/mm/pkeys.c                      |   33 ++++++++++++++++++++++++++
> >> >  3 files changed, 49 insertions(+), 0 deletions(-)
> >> >
> >> > diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
> >> > index 30d7f55..0056e58 100644
> >> > --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> >> > +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> >> > @@ -472,6 +472,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 bbb5d85..7a9aade 100644
> >> > --- a/arch/powerpc/include/asm/pkeys.h
> >> > +++ b/arch/powerpc/include/asm/pkeys.h
> >> > @@ -53,6 +53,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;
> >> 
> >> Do we really need that above check ? We should always find it
> >> peky_inited to be set. 
> >
> > Yes. there are cases where pkey_inited is not enabled. 
> > a) if the MMU is radix.
> That should be be a feature check
> 
> > b) if the PAGE size is 4k.
> 
> That is a kernel config change
> 
> > c) if the device tree says the feature is not available
> > d) if the CPU is of a older generation. P6 and older.
> 
> Both feature check.
> 
> how about doing something like
> 
> static inline u16 pte_to_pkey_bits(u64 pteflags)
> {
> 	if (!(pteflags & H_PAGE_KEY_MASK))
> 		return 0x0UL;

This check accomplishes the same thing as the return below.
When (pteflag & H_PAGE_KEY_MASK) is 0,
the code below returns the same 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));
> }

The idea  behind
	       if (!pkey_inited)
	               return 0x0UL;

was to not interpret the ptebits if we knew they were not initialized
to begin with. 


-- 
Ram Pai

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web