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
