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:

quoted

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@kernel.org>

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 (local)

Thanks to Kees for rewriting that.

and we can iterate later rather than blocking anything on the testcase.

quoted

+#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

quoted

+#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"

quoted

+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.

quoted

+#ifdef __aarch64__
+	ksft_set_plan(12);
+#else
+	ksft_set_plan(9);
+#endif

That'd just be ksft_test_plan(3 * ARRAY_SIZE(tests).

quoted

+	// 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel