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

Groups > linux.kernel > #1698071 > unrolled thread

Re: [RFC v6 19/62] powerpc: ability to create execute-disabled pkeys

Started byThiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
First post2017-07-27 17:00 +0200
Last post2017-07-30 01:30 +0200
Articles 3 — 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 19/62] powerpc: ability to create execute-disabled pkeys Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> - 2017-07-27 17:00 +0200
    Re: [RFC v6 19/62] powerpc: ability to create execute-disabled pkeys Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> - 2017-07-27 17:40 +0200
    Re: [RFC v6 19/62] powerpc: ability to create execute-disabled pkeys Ram Pai <linuxram@us.ibm.com> - 2017-07-30 01:30 +0200

#1698071 — Re: [RFC v6 19/62] powerpc: ability to create execute-disabled pkeys

FromThiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
Date2017-07-27 17:00 +0200
SubjectRe: [RFC v6 19/62] powerpc: ability to create execute-disabled pkeys
Message-ID<u7SqC-7mN-23@gated-at.bofh.it>
Ram Pai <linuxram@us.ibm.com> writes:

> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -2,6 +2,18 @@
>  #define _ASM_PPC64_PKEYS_H
>
>  extern bool pkey_inited;
> +/* override any generic PKEY Permission defines */
> +#undef  PKEY_DISABLE_ACCESS
> +#define PKEY_DISABLE_ACCESS    0x1
> +#undef  PKEY_DISABLE_WRITE
> +#define PKEY_DISABLE_WRITE     0x2
> +#undef  PKEY_DISABLE_EXECUTE
> +#define PKEY_DISABLE_EXECUTE   0x4
> +#undef  PKEY_ACCESS_MASK
> +#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
> +				PKEY_DISABLE_WRITE  |\
> +				PKEY_DISABLE_EXECUTE)
> +

Is it ok to #undef macros from another header? Especially since said
header is in uapi (include/uapi/asm-generic/mman-common.h).

Also, it's unnecessary to undef the _ACCESS and _WRITE macros since they
are identical to the original definition. And since these macros are
originally defined in an uapi header, the powerpc-specific ones should
be in an uapi header as well, if I understand it correctly.

An alternative solution is to define only PKEY_DISABLE_EXECUTE in
arch/powerpc/include/uapi/asm/mman.h and then test for its existence to
properly define PKEY_ACCESS_MASK in
include/uapi/asm-generic/mman-common.h. What do you think of the code
below?

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index e31f5ee8e81f..67e6a3a343ae 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -4,17 +4,6 @@
 #include <asm/firmware.h>
 
 extern bool pkey_inited;
-/* override any generic PKEY Permission defines */
-#undef  PKEY_DISABLE_ACCESS
-#define PKEY_DISABLE_ACCESS    0x1
-#undef  PKEY_DISABLE_WRITE
-#define PKEY_DISABLE_WRITE     0x2
-#undef  PKEY_DISABLE_EXECUTE
-#define PKEY_DISABLE_EXECUTE   0x4
-#undef  PKEY_ACCESS_MASK
-#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
-				PKEY_DISABLE_WRITE  |\
-				PKEY_DISABLE_EXECUTE)
 
 #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
 				VM_PKEY_BIT3 | VM_PKEY_BIT4)
diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h
index ab45cc2f3101..dee43feb7c53 100644
--- a/arch/powerpc/include/uapi/asm/mman.h
+++ b/arch/powerpc/include/uapi/asm/mman.h
@@ -45,4 +45,6 @@
 #define MAP_HUGE_1GB	(30 << MAP_HUGE_SHIFT)	/* 1GB   HugeTLB Page */
 #define MAP_HUGE_16GB	(34 << MAP_HUGE_SHIFT)	/* 16GB  HugeTLB Page */
 
+#define PKEY_DISABLE_EXECUTE   0x4
+
 #endif /* _UAPI_ASM_POWERPC_MMAN_H */
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 72eb9a1bde79..777f8f8dff47 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -12,7 +12,7 @@
  * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
  * more details.
  */
-#include <uapi/asm-generic/mman-common.h>
+#include <asm/mman.h>
 #include <linux/pkeys.h>                /* PKEY_*                       */
 
 bool pkey_inited;
diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
index 8c27db0c5c08..93e3841d9ada 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -74,7 +74,15 @@
 
 #define PKEY_DISABLE_ACCESS	0x1
 #define PKEY_DISABLE_WRITE	0x2
+
+/* The arch-specific code may define PKEY_DISABLE_EXECUTE */
+#ifdef PKEY_DISABLE_EXECUTE
+#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |	\
+				PKEY_DISABLE_WRITE  |	\
+				PKEY_DISABLE_EXECUTE)
+#else
 #define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\
 				 PKEY_DISABLE_WRITE)
+#endif
 
 #endif /* __ASM_GENERIC_MMAN_COMMON_H */


> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index 98d0391..b9ad98d 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -73,6 +73,7 @@ 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 -1;
> @@ -85,5 +86,14 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
>
>  	init_amr(pkey, new_amr_bits);
>
> +	/*
> +	 * By default execute is disabled.
> +	 * To enable execute, PKEY_ENABLE_EXECUTE
> +	 * needs to be specified.
> +	 */
> +	if ((init_val & PKEY_DISABLE_EXECUTE))
> +		new_iamr_bits |= IAMR_EX_BIT;
> +
> +	init_iamr(pkey, new_iamr_bits);
>  	return 0;
>  }

The comment seems to be from an earlier version which has the logic
inverted, and there is no PKEY_ENABLE_EXECUTE. Should the comment be
updated to the following?

    By default execute is enabled.
    To disable execute, PKEY_DISABLE_EXECUTE needs to be specified.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

[toc] | [next] | [standalone]

#1698112

FromThiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
Date2017-07-27 17:40 +0200
Message-ID<u7T3k-7P8-23@gated-at.bofh.it>
In reply to#1698071
Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index e31f5ee8e81f..67e6a3a343ae 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -4,17 +4,6 @@
>  #include <asm/firmware.h>
>  
>  extern bool pkey_inited;
> -/* override any generic PKEY Permission defines */
> -#undef  PKEY_DISABLE_ACCESS
> -#define PKEY_DISABLE_ACCESS    0x1
> -#undef  PKEY_DISABLE_WRITE
> -#define PKEY_DISABLE_WRITE     0x2
> -#undef  PKEY_DISABLE_EXECUTE
> -#define PKEY_DISABLE_EXECUTE   0x4
> -#undef  PKEY_ACCESS_MASK
> -#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
> -				PKEY_DISABLE_WRITE  |\
> -				PKEY_DISABLE_EXECUTE)
>  
>  #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
>  				VM_PKEY_BIT3 | VM_PKEY_BIT4)
> diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h
> index ab45cc2f3101..dee43feb7c53 100644
> --- a/arch/powerpc/include/uapi/asm/mman.h
> +++ b/arch/powerpc/include/uapi/asm/mman.h
> @@ -45,4 +45,6 @@
>  #define MAP_HUGE_1GB	(30 << MAP_HUGE_SHIFT)	/* 1GB   HugeTLB Page */
>  #define MAP_HUGE_16GB	(34 << MAP_HUGE_SHIFT)	/* 16GB  HugeTLB Page */
>  
> +#define PKEY_DISABLE_EXECUTE   0x4
> +
>  #endif /* _UAPI_ASM_POWERPC_MMAN_H */
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index 72eb9a1bde79..777f8f8dff47 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -12,7 +12,7 @@
>   * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>   * more details.
>   */
> -#include <uapi/asm-generic/mman-common.h>
> +#include <asm/mman.h>
>  #include <linux/pkeys.h>                /* PKEY_*                       */
>  
>  bool pkey_inited;
> diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
> index 8c27db0c5c08..93e3841d9ada 100644
> --- a/include/uapi/asm-generic/mman-common.h
> +++ b/include/uapi/asm-generic/mman-common.h
> @@ -74,7 +74,15 @@
>  
>  #define PKEY_DISABLE_ACCESS	0x1
>  #define PKEY_DISABLE_WRITE	0x2
> +
> +/* The arch-specific code may define PKEY_DISABLE_EXECUTE */
> +#ifdef PKEY_DISABLE_EXECUTE
> +#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |	\
> +				PKEY_DISABLE_WRITE  |	\
> +				PKEY_DISABLE_EXECUTE)
> +#else
>  #define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\
>  				 PKEY_DISABLE_WRITE)
> +#endif
>  
>  #endif /* __ASM_GENERIC_MMAN_COMMON_H */

Actually, I just noticed that arch/powerpc/include/uapi/asm/mman.h
includes <asm-generic/mman-common.h>, so for the #ifdef above to work
the former has to #define PKEY_DISABLE_EXECUTE before including the
latter.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

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

#1699395

FromRam Pai <linuxram@us.ibm.com>
Date2017-07-30 01:30 +0200
Message-ID<u8Jlf-Kh-3@gated-at.bofh.it>
In reply to#1698071
On Thu, Jul 27, 2017 at 11:54:31AM -0300, Thiago Jung Bauermann wrote:
> 
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > --- a/arch/powerpc/include/asm/pkeys.h
> > +++ b/arch/powerpc/include/asm/pkeys.h
> > @@ -2,6 +2,18 @@
> >  #define _ASM_PPC64_PKEYS_H
> >
> >  extern bool pkey_inited;
> > +/* override any generic PKEY Permission defines */
> > +#undef  PKEY_DISABLE_ACCESS
> > +#define PKEY_DISABLE_ACCESS    0x1
> > +#undef  PKEY_DISABLE_WRITE
> > +#define PKEY_DISABLE_WRITE     0x2
> > +#undef  PKEY_DISABLE_EXECUTE
> > +#define PKEY_DISABLE_EXECUTE   0x4
> > +#undef  PKEY_ACCESS_MASK
> > +#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
> > +				PKEY_DISABLE_WRITE  |\
> > +				PKEY_DISABLE_EXECUTE)
> > +
> 
> Is it ok to #undef macros from another header? Especially since said
> header is in uapi (include/uapi/asm-generic/mman-common.h).
> 
> Also, it's unnecessary to undef the _ACCESS and _WRITE macros since they
> are identical to the original definition. And since these macros are
> originally defined in an uapi header, the powerpc-specific ones should
> be in an uapi header as well, if I understand it correctly.

The architectural neutral code allows the implementation to define the
macros to its taste. powerpc headers due to legacy reason includes the
include/uapi/asm-generic/mman-common.h header. That header includes the
generic definitions of only PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE.
Unfortunately we end up importing them. I dont want to depend on them.
Any changes there could effect us. Example if the generic uapi header
changed PKEY_DISABLE_ACCESS to 0x4, we will have a conflict with
PKEY_DISABLE_EXECUTE.  Hence I undef them and define the it my way.

> 
> An alternative solution is to define only PKEY_DISABLE_EXECUTE in
> arch/powerpc/include/uapi/asm/mman.h and then test for its existence to
> properly define PKEY_ACCESS_MASK in
> include/uapi/asm-generic/mman-common.h. What do you think of the code
> below?
> 
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index e31f5ee8e81f..67e6a3a343ae 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -4,17 +4,6 @@
>  #include <asm/firmware.h>
> 
>  extern bool pkey_inited;
> -/* override any generic PKEY Permission defines */
> -#undef  PKEY_DISABLE_ACCESS
> -#define PKEY_DISABLE_ACCESS    0x1
> -#undef  PKEY_DISABLE_WRITE
> -#define PKEY_DISABLE_WRITE     0x2
> -#undef  PKEY_DISABLE_EXECUTE
> -#define PKEY_DISABLE_EXECUTE   0x4
> -#undef  PKEY_ACCESS_MASK
> -#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
> -				PKEY_DISABLE_WRITE  |\
> -				PKEY_DISABLE_EXECUTE)
> 
>  #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
>  				VM_PKEY_BIT3 | VM_PKEY_BIT4)
> diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h
> index ab45cc2f3101..dee43feb7c53 100644
> --- a/arch/powerpc/include/uapi/asm/mman.h
> +++ b/arch/powerpc/include/uapi/asm/mman.h
> @@ -45,4 +45,6 @@
>  #define MAP_HUGE_1GB	(30 << MAP_HUGE_SHIFT)	/* 1GB   HugeTLB Page */
>  #define MAP_HUGE_16GB	(34 << MAP_HUGE_SHIFT)	/* 16GB  HugeTLB Page */
> 
> +#define PKEY_DISABLE_EXECUTE   0x4
> +
>  #endif /* _UAPI_ASM_POWERPC_MMAN_H */
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index 72eb9a1bde79..777f8f8dff47 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -12,7 +12,7 @@
>   * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>   * more details.
>   */
> -#include <uapi/asm-generic/mman-common.h>
> +#include <asm/mman.h>
>  #include <linux/pkeys.h>                /* PKEY_*                       */
> 
>  bool pkey_inited;
> diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
> index 8c27db0c5c08..93e3841d9ada 100644
> --- a/include/uapi/asm-generic/mman-common.h
> +++ b/include/uapi/asm-generic/mman-common.h
> @@ -74,7 +74,15 @@
> 
>  #define PKEY_DISABLE_ACCESS	0x1
>  #define PKEY_DISABLE_WRITE	0x2
> +
> +/* The arch-specific code may define PKEY_DISABLE_EXECUTE */
> +#ifdef PKEY_DISABLE_EXECUTE
> +#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |	\
> +				PKEY_DISABLE_WRITE  |	\
> +				PKEY_DISABLE_EXECUTE)
> +#else
>  #define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\
>  				 PKEY_DISABLE_WRITE)
> +#endif
> 
>  #endif /* __ASM_GENERIC_MMAN_COMMON_H */

I suppose we can do it this way aswell. but dont like the way it is
spreading the defines accross multiple files.

> 
> 
> > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> > index 98d0391..b9ad98d 100644
> > --- a/arch/powerpc/mm/pkeys.c
> > +++ b/arch/powerpc/mm/pkeys.c
> > @@ -73,6 +73,7 @@ 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 -1;
> > @@ -85,5 +86,14 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> >
> >  	init_amr(pkey, new_amr_bits);
> >
> > +	/*
> > +	 * By default execute is disabled.
> > +	 * To enable execute, PKEY_ENABLE_EXECUTE
> > +	 * needs to be specified.
> > +	 */
> > +	if ((init_val & PKEY_DISABLE_EXECUTE))
> > +		new_iamr_bits |= IAMR_EX_BIT;
> > +
> > +	init_iamr(pkey, new_iamr_bits);
> >  	return 0;
> >  }
> 
> The comment seems to be from an earlier version which has the logic
> inverted, and there is no PKEY_ENABLE_EXECUTE. Should the comment be
> updated to the following?
> 
>     By default execute is enabled.
>     To disable execute, PKEY_DISABLE_EXECUTE needs to be specified.

yes. the comment is misleading. I just took it out.

RP

> 
> -- 
> Thiago Jung Bauermann
> IBM Linux Technology Center

-- 
Ram Pai

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web