Re: MPK: pkey_free and key reuse

[Florian Weimer <fweimer@redhat.com>]

On 11/08/2017 09:41 PM, Dave Hansen wrote:
> On 11/05/2017 02:35 AM, Florian Weimer wrote:
>> I don't think pkey_free, as it is implemented today, is very safe due to
>> key reuse by a subsequent pkey_alloc.  I see two problems:
>>
>> (A) pkey_free allows reuse for they key while there are still mappings
>> that use it.
> 
> I don't agree with this assessment.  Is malloc() unsafe?  If someone
> free()s memory that is still in use, a subsequent malloc() would hand
> the address out again for reuse.

I think the disagreement is not about what is considered acceptable 
behavior as such, but what constitutes “use”.

And even if with concurrent use, the behavior can be well-defined.  We 
make sure that if munmap is called, we do not return before all threads 
have observed in principle that the page is gone (at considerable cost, 
of course, and in most cases, that is total overkill).

I'm pretty sure there is another key reuse scenario which does not even 
involve pkey_free, but I need to write a test first.

>> (B) If a key is reused, existing threads retain their access rights,
>> while there is an expectation that pkey_alloc denies access for the
>> threads except the current one.
> Where does this expectation come from?

For me, it was the access_rights argument to pkey_alloc.  What else 
would it do?  For the current thread, I can already set the rights with 
a PKRU write, so the existence of the syscall argument is puzzling.

> Using the malloc() analogy, we
> don't expect that free() in one thread actively takes away references to
> the memory held by other threads.

But malloc/free isn't expected to be a partial antidote to random 
pointer scribbling.

> We define free() as only being called on resources to which there are no
> active references.  If you free() things in use, bad things happen.
> pkey_free() is only to be called when there is nothing actively using
> the key.  If you pkey_free() an in-use key, bad things happen.

My impression was that MPK was intended as a fallback in case you did 
that, and unrelated code suddenly writes through a dangling pointer and 
accidentally hits the DAX-mapped persistent memory of the database.  To 
prevent that, the those pages are mapped write-disabled on all threads 
almost all the time, and only if the database needs to write something, 
it temporarily tweaks PKRU so that it gains access.  All that assumes 
that you can actually restrict all threads in the process, but with the 
current implementation, that's not true even if threads never touch keys 
they don't know.

I think we should either implement revoke on pkey_alloc, with a 
broadcast to all threads (the pkey_set race can be closed by having a 
vDSO for that an the revocation code can check %rip to see if the old 
PKRU value needs to be fixed up).  Or we add the two pkey_alloc flags I 
mentioned earlier.

Thanks,
Florian

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>