[PATCH] mm/pkey: Add PKEY_UNRESTRICTED macro

[PATCH] mm/pkey: Add PKEY_UNRESTRICTED macro

[Yury Khrustalev]

Memory protection keys (pkeys) uapi previously used two macros:

 - PKEY_DISABLE_ACCESS 0x1
 - PKEY_DISABLE_WRITE  0x2

with implicit literal value of 0x0 that means "unrestricted". Code that
works with pkeys has to use this literal value when implying that a pkey
imposes no restrictions. This may reduce readability because 0 can be
written in various ways (e.g. 0x0 or 0) and also because 0 in the context
of pkeys can be mistaken for "no permissions" (akin PROT_NONE) while it
actually means "no restrictions". This is important because pkeys are
oftentimes used near mprotect() that uses PROT_ macros.

This patch adds PKEY_UNRESTRICTED macro defined as 0x0.

---
Applies to 42f7652d3eb5 (tag: v6.12-rc4).

For context, this change will also allow for more consistent update of the
Glibc manual [1] which in turn will help with introducing memory protection
keys on AArch64 targets [2].

[1] https://inbox.sourceware.org/libc-alpha/20241022073837.151355-1-yury.khrustalev@arm.com/
[2] https://inbox.sourceware.org/libc-alpha/20241011153614.3189334-1-yury.khrustalev@arm.com/

Is this patch OK?

Kind regards,
Yury

---
 include/uapi/asm-generic/mman-common.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
index 6ce1f1ceb432..ea40e27e6dea 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -82,6 +82,7 @@
 /* compatibility flags */
 #define MAP_FILE	0
 
+#define PKEY_UNRESTRICTED	0x0
 #define PKEY_DISABLE_ACCESS	0x1
 #define PKEY_DISABLE_WRITE	0x2
 #define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\
-- 
2.39.5

Re: [PATCH] mm/pkey: Add PKEY_UNRESTRICTED macro

[Kevin Brodsky]

On 22/10/2024 14:01, Yury Khrustalev wrote:
> Memory protection keys (pkeys) uapi previously used two macros:

Nit: I would remove "previously" as it's not clear what it means here.

>  - PKEY_DISABLE_ACCESS 0x1
>  - PKEY_DISABLE_WRITE  0x2
>
> with implicit literal value of 0x0 that means "unrestricted". Code that
> works with pkeys has to use this literal value when implying that a pkey
> imposes no restrictions. This may reduce readability because 0 can be
> written in various ways (e.g. 0x0 or 0) and also because 0 in the context
> of pkeys can be mistaken for "no permissions" (akin PROT_NONE) while it
> actually means "no restrictions". This is important because pkeys are
> oftentimes used near mprotect() that uses PROT_ macros.
>
> This patch adds PKEY_UNRESTRICTED macro defined as 0x0.

Your Signed-off-by is missing.

With that fixed:

Reviewed-by: Kevin Brodsky <kevin.brodsky@arm.com>

- Kevin

> ---
> Applies to 42f7652d3eb5 (tag: v6.12-rc4).
>
> For context, this change will also allow for more consistent update of the
> Glibc manual [1] which in turn will help with introducing memory protection
> keys on AArch64 targets [2].
>
> [1] https://inbox.sourceware.org/libc-alpha/20241022073837.151355-1-yury.khrustalev@arm.com/
> [2] https://inbox.sourceware.org/libc-alpha/20241011153614.3189334-1-yury.khrustalev@arm.com/
>
> Is this patch OK?
>
> Kind regards,
> Yury
>
> ---
>  include/uapi/asm-generic/mman-common.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
> index 6ce1f1ceb432..ea40e27e6dea 100644
> --- a/include/uapi/asm-generic/mman-common.h
> +++ b/include/uapi/asm-generic/mman-common.h
> @@ -82,6 +82,7 @@
>  /* compatibility flags */
>  #define MAP_FILE	0
>  
> +#define PKEY_UNRESTRICTED	0x0
>  #define PKEY_DISABLE_ACCESS	0x1
>  #define PKEY_DISABLE_WRITE	0x2
>  #define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\

Re: [PATCH] mm/pkey: Add PKEY_UNRESTRICTED macro

[Dave Hansen]

On 10/22/24 05:01, Yury Khrustalev wrote:
> diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
> index 6ce1f1ceb432..ea40e27e6dea 100644
> --- a/include/uapi/asm-generic/mman-common.h
> +++ b/include/uapi/asm-generic/mman-common.h
> @@ -82,6 +82,7 @@
>  /* compatibility flags */
>  #define MAP_FILE	0
>  
> +#define PKEY_UNRESTRICTED	0x0
>  #define PKEY_DISABLE_ACCESS	0x1
>  #define PKEY_DISABLE_WRITE	0x2
>  #define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\

It seems sane, but it would be nice to have at least site or two use it
in the kernel so show that it's useful in practice.  Is there any kernel
code or anything in selftests/ that would be improved with this?

That said, it's pretty harmless:

Acked-by: Dave Hansen <dave.hansen@linux.intel.com>

Re: [PATCH] mm/pkey: Add PKEY_UNRESTRICTED macro

[Kevin Brodsky]

On 22/10/2024 16:04, Dave Hansen wrote:
> On 10/22/24 05:01, Yury Khrustalev wrote:
>> diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
>> index 6ce1f1ceb432..ea40e27e6dea 100644
>> --- a/include/uapi/asm-generic/mman-common.h
>> +++ b/include/uapi/asm-generic/mman-common.h
>> @@ -82,6 +82,7 @@
>>  /* compatibility flags */
>>  #define MAP_FILE	0
>>  
>> +#define PKEY_UNRESTRICTED	0x0
>>  #define PKEY_DISABLE_ACCESS	0x1
>>  #define PKEY_DISABLE_WRITE	0x2
>>  #define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\
> It seems sane, but it would be nice to have at least site or two use it
> in the kernel so show that it's useful in practice.  Is there any kernel
> code or anything in selftests/ that would be improved with this?

As a matter of fact this would improve the readability of [1] quite a
bit: all those set_pkey_bits(..., 0) calls would become
set_pkey_bits(..., PKEY_UNRESTRICTED). I'm sure other pkey-related
kselftests could benefit too.

Kevin

[1]
https://lore.kernel.org/linux-arm-kernel/20241017133909.3837547-5-kevin.brodsky@arm.com/

Re: [PATCH] mm/pkey: Add PKEY_UNRESTRICTED macro

[Dave Hansen]

On 10/22/24 08:11, Kevin Brodsky wrote:
>>> +#define PKEY_UNRESTRICTED	0x0
>>>  #define PKEY_DISABLE_ACCESS	0x1
>>>  #define PKEY_DISABLE_WRITE	0x2
>>>  #define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\
>> It seems sane, but it would be nice to have at least site or two use it
>> in the kernel so show that it's useful in practice.  Is there any kernel
>> code or anything in selftests/ that would be improved with this?
> As a matter of fact this would improve the readability of [1] quite a
> bit: all those set_pkey_bits(..., 0) calls would become
> set_pkey_bits(..., PKEY_UNRESTRICTED). I'm sure other pkey-related
> kselftests could benefit too.

It would be much appreciated if someone could make a pass over kernel
code and fix up the places where PKEY_UNRESTRICTED makes things more clear.

Re: [PATCH] mm/pkey: Add PKEY_UNRESTRICTED macro

[Kevin Brodsky]

On 22/10/2024 19:15, Dave Hansen wrote:
> On 10/22/24 08:11, Kevin Brodsky wrote:
>>>> +#define PKEY_UNRESTRICTED	0x0
>>>>  #define PKEY_DISABLE_ACCESS	0x1
>>>>  #define PKEY_DISABLE_WRITE	0x2
>>>>  #define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\
>>> It seems sane, but it would be nice to have at least site or two use it
>>> in the kernel so show that it's useful in practice.  Is there any kernel
>>> code or anything in selftests/ that would be improved with this?
>> As a matter of fact this would improve the readability of [1] quite a
>> bit: all those set_pkey_bits(..., 0) calls would become
>> set_pkey_bits(..., PKEY_UNRESTRICTED). I'm sure other pkey-related
>> kselftests could benefit too.
> It would be much appreciated if someone could make a pass over kernel
> code and fix up the places where PKEY_UNRESTRICTED makes things more clear.

It doesn't look like kernel code would have a use for it at the moment,
but I have found a few places in kselftests (mm, ppc) where 0 could be
replaced with PKEY_UNRESTRICTED. I can send a follow-up series.

Kevin

Re: [PATCH] mm/pkey: Add PKEY_UNRESTRICTED macro

[Dave Hansen]

On 10/25/24 01:26, Kevin Brodsky wrote:
>> It would be much appreciated if someone could make a pass over kernel
>> code and fix up the places where PKEY_UNRESTRICTED makes things more clear.
> It doesn't look like kernel code would have a use for it at the moment,
> but I have found a few places in kselftests (mm, ppc) where 0 could be
> replaced with PKEY_UNRESTRICTED. I can send a follow-up series.

Just doing it in the selftests would be great.

Re: [PATCH] mm/pkey: Add PKEY_UNRESTRICTED macro

[Yury Khrustalev]

Hi,

On Fri, Oct 25, 2024 at 07:48:01AM -0700, Dave Hansen wrote:
> On 10/25/24 01:26, Kevin Brodsky wrote:
> >> It would be much appreciated if someone could make a pass over kernel
> >> code and fix up the places where PKEY_UNRESTRICTED makes things more clear.
> > It doesn't look like kernel code would have a use for it at the moment,
> > but I have found a few places in kselftests (mm, ppc) where 0 could be
> > replaced with PKEY_UNRESTRICTED. I can send a follow-up series.
> 
> Just doing it in the selftests would be great.


Thank you for the feedback, I've sent v2 with updated tests [1].

[1] https://lore.kernel.org/linux-arch/20241027170006.464252-1-yury.khrustalev@arm.com/

Kind regards,
Yury