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

Ira Weiny
2022-02-18T05:34:47Z

On Tue, Feb 01, 2022 at 09:45:03AM -0800, Dave Hansen wrote:
> On 1/27/22 09:54, ira.weiny@intel.com 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.

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

>
> 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. But the other unrelated fault will work
correctly.

I've debated if the test code should use a specific fault callback... :-/
That breaks my test which iterates all keys... but would fix this problem.

Ira