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

Ira Weiny
2022-02-18T17:25:20Z

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