Re: [PATCH V8 19/44] mm/pkeys: PKS Testing, add pks_mk_*() tests

[Dave Hansen]

On 2/17/22 21:34, Ira Weiny wrote:
> On Tue, Feb 01, 2022 at 09:45:03AM -0800, Dave Hansen wrote:
>> On 1/27/22 09:54, ira.weiny@xxxxxxxxx wrote:
>>>  bool pks_test_callback(void)
>>>  {
>>> -	return false;
>>> +	bool armed = (test_armed_key != 0);
>>> +
>>> +	if (armed) {
>>> +		pks_mk_readwrite(test_armed_key);
>>> +		fault_cnt++;
>>> +	}
>>> +
>>> +	return armed;
>>> +}
>>
>> Where's the locking for all this?  I don't think we need anything fancy,
>> but is there anything preventing the test from being started from
>> multiple threads at the same time?  I think a simple global test mutex
>> would probably suffice.
> 
> Good idea.  Generally I don't see that happening but it is good to be safe.

I'm not sure what you mean.

In the kernel, we always program as if userspace is out to get us.  If
userspace can possibly do something to confuse the kernel, it will.  It
might be malicious or incompetent, but it will happen.

This isn't really a "good to be safe" kind of thing.  Kernel code must
*be* safe.

>> Also, pks_test_callback() needs at least a comment or two about what
>> it's doing.
> 
> The previous patch which adds this call in the fault handler contains the
> following comment which is in the final code:
> 
> /*
>  * pks_test_callback() is called by the fault handler to indicate it saw a pkey
>  * fault.
>  *
>  * NOTE: The callback is responsible for clearing any condition which would
>  * cause the fault to re-trigger.
>  */
> 
> Would you like more comments within the function?

Ahh, it just wasn't in the context.

Looking at this again, I don't really like the name "callback" is almost
always a waste of bytes.  Imagine this was named something like:

	pks_test_induced_fault();

... and had a comment like:

/*
 * Ensure that the fault handler does not treat
 * test-induced faults as actual errors.
 */

>> Does this work if you have a test armed and then you get an unrelated
>> PKS fault on another CPU?  I think this will disarm the test from the
>> unrelated thread.
> 
> This code will detect a false fault.  

That's a bug that's going to get fixed, right? ;)