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

[Ira Weiny <ira.weiny@intel.com>]

On Fri, Feb 18, 2022 at 07:28:04AM -0800, Dave Hansen wrote:
> 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@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.
> 
> 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.

Yes

> 
> >> 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.
>  */

Ok.  At this point this may go away depending on how I resolve the ability to
test all the keys.  pks_test_callback() was critical for that feature without
introducing a bunch of ugly test code in pks-keys.h and pkeys.c.

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

Yep.  Not sure how at the moment.

Ira