Hi, On Fri, Oct 28, 2022 at 06:03:18PM +0100, Mark Brown wrote: > On Wed, Oct 26, 2022 at 04:04:57PM +0100, Joey Gouly wrote: > > > Add some tests to cover the new PR_SET_MDWE prctl. > > Some comments below but they're all stylistic and let's not make perfect > be the enemy of the good here so > > Reviewed-by: Mark Brown <broonie@xxxxxxxxxx> Thanks for the review, however I won't keep your R-b tag because I'm going to move forward with Kees' approach from: https://lore.kernel.org/linux-arm-kernel/202210281314.C5D3414722@keescook/T/#m45ac9de6c205b560d072a65e4e67e2a7ee363588 Thanks to Kees for rewriting that. > > and we can iterate later rather than blocking anything on the testcase. > > > +#ifdef __aarch64__ > > +#define PROT_BTI 0x10 /* BTI guarded page */ > > +#endif > > We should get this from the kernel headers shouldn't we? We generally > rely on things getting pulled in from there rather than locally > defining. I believe the mman.h included is from the toolchain, not the kernel's uapi headers. The toolchain I was using didn't have PROT_BTI defined in its mman.h > > > +#define TEST1 "mmap(PROT_WRITE | PROT_EXEC)\n" > > +#define TEST2 "mmap(PROT_WRITE); mprotect(PROT_EXEC)\n" > > +#define TEST3 "mmap(PROT_EXEC); mprotect(PROT_EXEC | PROT_READ)\n" > > +#define TEST4 "mmap(PROT_EXEC); mprotect(PROT_EXEC | PROT_BTI)\n" > > > +int test1(int mdwe_enabled) > > +{ > > It feels like we could usefully make an array of > > struct test { > int (*run)(bool mdwe_enabled); > char *name; > } > > then we'd need fewer ifdefs, things could be more usefully named and > it'd be a bit easier to add new cases. > > > +#ifdef __aarch64__ > > + ksft_set_plan(12); > > +#else > > + ksft_set_plan(9); > > +#endif > > That'd just be ksft_test_plan(3 * ARRAY_SIZE(tests). > > > + // First run the tests without MDWE > > + test_result(test1(0), TEST1); > > + test_result(test2(0), TEST2); > > + test_result(test3(0), TEST3); > > +#ifdef __aarch64__ > > + test_result(test4(0), TEST4); > > +#endif > > and these calls to the tests would all be iterating over the array. These comments are solved by the kselftest_harness approach that Kees suggested. Thanks, Joey
