Re: [PATCH 10/26] x86, pkeys: notify userspace about protection key faults

[Christian Borntraeger <borntraeger@de.ibm.com>]

Am 24.09.2015 um 19:15 schrieb Dave Hansen:
> Christian, can you tell us how big s390's storage protection keys are?
> See the discussion below about siginfo...

Dave, sorry for the late answer.
s390 storage keys are 4bit for the protection key (and 1 bit for fetch protection, 
change and reference bit) per physical page, so 1 byte is enough for us.

We do not have the storage keys per page table, but for the page frame instead 
(shared among all mappers) so I am not sure if the whole thing will fit for s390.
Having a signal for page protection errors might be useful for us - not sure yet.

Christian

PS: In the past we worked hard to get rid of storage key usage in Linux and are now using
software reference and change tracking to be closer what others do, so its a bit odd to
see other coming with the same idea ;-)

> 
> On 09/24/2015 02:23 AM, Ingo Molnar wrote:
>>> +static u16 fetch_pkey(unsigned long address, struct task_struct *tsk)
>>> +{
> ...
>>> +		struct vm_area_struct *vma = find_vma(tsk->mm, address);
>>> +		if (vma) {
>>> +			ret = vma_pkey(vma);
>>> +		} else {
>>> +			WARN_ONCE(1, "no PTE or VMA @ %lx\n", address);
>>> +			ret = 0;
>>> +		}
>>> +	}
>>> +	return ret;
>>
>> Yeah, so I have three observations:
>>
>> 1)
>>
>> I don't think this warning is entirely right, because this is a fundamentally racy 
>> op.
>>
>> fetch_pkey(), called by force_sign_info_fault(), can be called while not holding 
>> the vma - and if we race with any other thread of the mm, the vma might be gone 
>> already.
>>
>> So any threaded app using pkeys and vmas in parallel could trigger that WARN_ON().
> 
> Agreed.  I'll remove the warning.
> 
>> 2)
>>
>> And note that this is a somewhat new scenario: in regular page faults, 
>> 'error_code' always carries a then-valid cause of the page fault with itself. So 
>> we can put that into the siginfo and can be sure that it's the reason for the 
>> fault.
>>
>> With the above pkey code, we fetch the pte separately from the fault, and without 
>> synchronizing with the fault - and we cannot do that, nor do we want to.
>>
>> So I think this code should just accept the fact that races may happen. Perhaps 
>> warn if we get here with only a single mm user. (but even that would be a bit racy 
>> as we don't serialize against exit())
> 
> Good point.
> 
>> 3)
>>
>> For user-space that somehow wants to handle pkeys dynamically and drive them via 
>> faults, this seems somewhat inefficient: we already do a find_vma() in the primary 
>> fault lookup - and with the typical pkey usecase it will find a vma, just with the 
>> wrong access permissions. But when we generate the siginfo here, why do we do a 
>> find_vma() again? Why not pass the vma to the siginfo generating function?
> 
> My assumption was that the signal generation case was pretty slow.
> find_vma() is almost guaranteed to hit the vmacache, and we already hold
> mmap_sem, so the cost is pretty tiny.
> 
> I'm happy to change it if you're really concerned, but I didn't think it
> would be worth the trouble of plumbing it down.
> 
>>> --- a/include/uapi/asm-generic/siginfo.h~pkeys-09-siginfo	2015-09-16 10:48:15.584161859 -0700
>>> +++ b/include/uapi/asm-generic/siginfo.h	2015-09-16 10:48:15.592162222 -0700
>>> @@ -95,6 +95,13 @@ typedef struct siginfo {
>>>  				void __user *_lower;
>>>  				void __user *_upper;
>>>  			} _addr_bnd;
>>> +			int _pkey; /* FIXME: protection key value??
>>> +				    * Do we really need this in here?
>>> +				    * userspace can get the PKRU value in
>>> +				    * the signal handler, but they do not
>>> +				    * easily have access to the PKEY value
>>> +				    * from the PTE.
>>> +				    */
>>>  		} _sigfault;
>>
>> A couple of comments:
>>
>> 1)
>>
>> Please use our ABI types - this one should be 'u32' I think.
>>
>> We could use 'u8' as well here, and mark another 3 bytes next to it as reserved 
>> for future flags. Right now protection keys use 4 bits, but do you really think 
>> they'll ever grow beyond 8 bits? PTE bits are a scarce resource in general.
> 
> I don't expect them to get bigger, at least with anything resembling the
> current architecture.  Agreed about the scarcity of PTE bits.
> 
> siginfo.h is shared everywhere, so I'd ideally like to put a type in
> there that all the other architectures can use.
> 
>> 3)
>>
>> Please add suitable self-tests to tools/tests/selftests/x86/ that both documents 
>> the preferred usage of pkeys, demonstrates all implemented aspects the new ABI and 
>> provokes a fault and prints the resulting siginfo, etc.
>>
>>> @@ -206,7 +214,8 @@ typedef struct siginfo {
>>>  #define SEGV_MAPERR	(__SI_FAULT|1)	/* address not mapped to object */
>>>  #define SEGV_ACCERR	(__SI_FAULT|2)	/* invalid permissions for mapped object */
>>>  #define SEGV_BNDERR	(__SI_FAULT|3)  /* failed address bound checks */
>>> -#define NSIGSEGV	3
>>> +#define SEGV_PKUERR	(__SI_FAULT|4)  /* failed address bound checks */
>>> +#define NSIGSEGV	4
>>
>> You copy & pasted the MPX comment here, it should read something like:
>>
>>    #define SEGV_PKUERR	(__SI_FAULT|4)  /* failed protection keys checks */
> 
> Whoops.  Will fix.
> 

--
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>