Ram Pai <linuxram@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
