Re: [PATCH v6 01/30] mm: Introduce kpkeys

[David Hildenbrand (Arm)]

On 4/17/26 17:59, Kevin Brodsky wrote:
> On 17/04/2026 16:37, David Hildenbrand (Arm) wrote:
>> On 2/27/26 18:54, Kevin Brodsky wrote:
>>> kpkeys is a simple framework to enable the use of protection keys
>>> (pkeys) to harden the kernel itself. This patch introduces the basic
>>> API in <linux/kpkeys.h>: a couple of functions to set and restore
>>> the pkey register and macros to define guard objects.
>>>
>>> kpkeys introduces a new concept on top of pkeys: the kpkeys level.
>>> Each level is associated to a set of permissions for the pkeys
>>> managed by the kpkeys framework. kpkeys_set_level(lvl) sets those
>>> permissions according to lvl, and returns the original pkey
>>> register, to be later restored by kpkeys_restore_pkey_reg(). To
>>> start with, only KPKEYS_LVL_DEFAULT is available, which is meant
>>> to grant RW access to KPKEYS_PKEY_DEFAULT (i.e. all memory since
>>> this is the only available pkey for now).
>>>
>>> Because each architecture implementing pkeys uses a different
>>> representation for the pkey register, and may reserve certain pkeys
>>> for specific uses, support for kpkeys must be explicitly indicated
>>> by selecting ARCH_HAS_KPKEYS and defining the following functions in
>>> <asm/kpkeys.h>, in addition to the macros provided in
>>> <asm-generic/kpkeys.h>:
>>>
>>> - arch_kpkeys_set_level()
>>> - arch_kpkeys_restore_pkey_reg()
>>> - arch_kpkeys_enabled()
>> Another thing: why not simply drop the "arch_" stuff from these helpers?
> 
> The first two are not meant to be directly called, they're the
> arch-specific implementation of kpkeys_set_level() and
> kpkeys_restore_pkey_reg(), and those generic functions handle some
> generic logic.
> 
> arch_kpkeys_enabled() is directly used in generic code, so I suppose it
> could be renamed to kpkeys_enabled()? It's actually implemented in an
> arch header so I wasn't too sure about it.
I was skimming over patch #13 and spotted:

+void·__init·kpkeys_hardened_pgtables_init(void)
+{
+›	if·(!arch_kpkeys_enabled())
+›	›	return;
+
+›	static_branch_enable(&kpkeys_hardened_pgtables_key);
+}

The arch_* there can just go IMHO.

I'd also do it for the two ones used by the GUARD macros. If we don't
expect common code wrappers (arch_kpkeys_enabled() vs. kpkeys_enabled),
then the arch_ is unnecessary information -- IMHO

-- 
Cheers,

David