Ram Pai [off-list ref] writes:

On Tue, Oct 24, 2017 at 11:55:41AM +0530, Aneesh Kumar K.V wrote:

quoted

Ram Pai [off-list ref] writes:

quoted

+
+static void pkey_status_change(int pkey, bool enable)
+{
+	u64 old_uamor;
+
+	/* reset the AMR and IAMR bits for this key */
+	init_amr(pkey, 0x0);
+	init_iamr(pkey, 0x0);
+
+	/* enable/disable key */
+	old_uamor = read_uamor();
+	if (enable)
+		old_uamor |= (0x3ul << pkeyshift(pkey));
+	else
+		old_uamor &= ~(0x3ul << pkeyshift(pkey));
+	write_uamor(old_uamor);
+}

That one is confusing, we discussed this outside the list, but want to
bring the list to further discussion. So now the core kernel request
for a key via mm_pkey_alloc(). Why not do the below there

static inline int mm_pkey_alloc(struct mm_struct *mm)
{
	/*
	 * Note: this is the one and only place we make sure
	 * that the pkey is valid as far as the hardware is
	 * concerned.  The rest of the kernel trusts that
	 * only good, valid pkeys come out of here.
	 */
	u32 all_pkeys_mask = (u32)(~(0x0));
	int ret;

	if (!pkey_inited)
		return -1;
	/*
	 * Are we out of pkeys?  We must handle this specially
	 * because ffz() behavior is undefined if there are no
	 * zeros.
	 */
	if (mm_pkey_allocation_map(mm) == all_pkeys_mask)
		return -1;

	ret = ffz((u32)mm_pkey_allocation_map(mm));
	mm_set_pkey_allocated(mm, ret);

	return ret;
}

your mm_pkey_allocation_map() should have the keys specified in AMOR and
UAMOR marked as allocatied. It is in use by hypervisor and OS respectively.


There is no need of __arch_activate_key() etc. and by default if the OS
has not requested for a key for its internal use UAMOR should be
0xFFFFFFFF and that AMOR value you derive from the device tree based of
what keys hypervisor has reserved.

Ok. You are suggesting a programming model where
a) keys that are reserved by hypervisor are enabled by default.

No he's not saying that.

b) keys that are reserved by the OS are enabled by default.

Or that.

c) keys that are not yet allocated to userspace is enabled by default.

But he is saying this.

The problem with this model is that, the userspace can change the
permissions of an unallocated key, and those permissions will go into
effect immediately,

Correct, but an unallocated key should not be used anywhere, so it
should have no effect, unless there's a bug.

because every key is enabled by default. If it

Not every key, every key that is not reserved by the hypervisor or OS.

happens to be a key that is reserved by the OS or the hypervisor, bad
things can happen.

So no nothing bad should be able to happen.

the programming model that I have implemented; **which is the programming
model expected by linux**, is

a)  keys that are reserved by hypervisor are left to the hypervisor to
	enable/disable whenever it needs to.

We agree on that.

b)  keys that are reserved by the OS are left to the OS to
	enable/disable whenever it needs to.

And that.

c)  keys that are not yet allocated to userspace are not yet enabled.
	Will be enabled when the key is allocated to userspace
	through sys_pkey_alloc() syscall.

This is the only part that is under discussion.

In this programming model, userspace is expected to use only keys that
are allocated. And in case it inadvertetly uses a key that is not
allocated, nothing bad happens because that key is not activated unless
it is allocated.

This sames like a good design to me.

The only downside I see is we have to switch an extra SPR, but that's
not much in the scheme of things.

cheers