On 10/18/24 10:07, Lorenzo Stoakes wrote:
On Fri, Oct 18, 2024 at 09:32:17AM -0600, Shuah Khan wrote:
On 10/18/24 01:12, Lorenzo Stoakes wrote:
On Thu, Oct 17, 2024 at 03:24:49PM -0600, Shuah Khan wrote:
On 10/17/24 14:42, Lorenzo Stoakes wrote:
Utilise the kselftest harmness to implement tests for the guard page
Splleing NIT - harmness -> harness
implementation.
We start by implement basic tests asserting that guard pages can be
implmenting? By the way checkpatch will catch spelling stuuf.
Please see comments about warnings below.
Thanks. The majority of the checkpatch warnings are invalid so I missed
this. Will fix on respin.
established (poisoned), cleared (remedied) and that touching poisoned pages
result in SIGSEGV. We also assert that, in remedying a range, non-poison
pages remain intact.
We then examine different operations on regions containing poison markers
behave to ensure correct behaviour:
* Operations over multiple VMAs operate as expected.
* Invoking MADV_GUARD_POISION / MADV_GUARD_REMEDY via process_madvise() in
batches works correctly.
* Ensuring that munmap() correctly tears down poison markers.
* Using mprotect() to adjust protection bits does not in any way override
or cause issues with poison markers.
* Ensuring that splitting and merging VMAs around poison markers causes no
issue - i.e. that a marker which 'belongs' to one VMA can function just
as well 'belonging' to another.
* Ensuring that madvise(..., MADV_DONTNEED) does not remove poison markers.
* Ensuring that mlock()'ing a range containing poison markers does not
cause issues.
* Ensuring that mremap() can move a poisoned range and retain poison
markers.
* Ensuring that mremap() can expand a poisoned range and retain poison
markers (perhaps moving the range).
* Ensuring that mremap() can shrink a poisoned range and retain poison
markers.
* Ensuring that forking a process correctly retains poison markers.
* Ensuring that forking a VMA with VM_WIPEONFORK set behaves sanely.
* Ensuring that lazyfree simply clears poison markers.
* Ensuring that userfaultfd can co-exist with guard pages.
* Ensuring that madvise(..., MADV_POPULATE_READ) and
madvise(..., MADV_POPULATE_WRITE) error out when encountering
poison markers.
* Ensuring that madvise(..., MADV_COLD) and madvise(..., MADV_PAGEOUT) do
not remove poison markers.
Good summary of test. Does the test require root access?
If so does it check and skip appropriately?
Thanks and some do, in those cases we skip.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
---
tools/testing/selftests/mm/.gitignore | 1 +
tools/testing/selftests/mm/Makefile | 1 +
tools/testing/selftests/mm/guard-pages.c | 1168 ++++++++++++++++++++++
3 files changed, 1170 insertions(+)
create mode 100644 tools/testing/selftests/mm/guard-pages.c
diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore
index 689bbd520296..8f01f4da1c0d 100644
--- a/tools/testing/selftests/mm/.gitignore
+++ b/tools/testing/selftests/mm/.gitignore
@@ -54,3 +54,4 @@ droppable
hugetlb_dio
pkey_sighandler_tests_32
pkey_sighandler_tests_64
+guard-pages
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index 02e1204971b0..15c734d6cfec 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -79,6 +79,7 @@ TEST_GEN_FILES += hugetlb_fault_after_madv
TEST_GEN_FILES += hugetlb_madv_vs_map
TEST_GEN_FILES += hugetlb_dio
TEST_GEN_FILES += droppable
+TEST_GEN_FILES += guard-pages
ifneq ($(ARCH),arm64)
TEST_GEN_FILES += soft-dirty
diff --git a/tools/testing/selftests/mm/guard-pages.c b/tools/testing/selftests/mm/guard-pages.c
new file mode 100644
index 000000000000..2ab0ff3ba5a0
--- /dev/null
+++ b/tools/testing/selftests/mm/guard-pages.c
@@ -0,0 +1,1168 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#define _GNU_SOURCE
+#include "../kselftest_harness.h"
+#include <assert.h>
+#include <fcntl.h>
+#include <setjmp.h>
+#include <errno.h>
+#include <linux/userfaultfd.h>
+#include <signal.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <sys/mman.h>
+#include <sys/syscall.h>
+#include <sys/uio.h>
+#include <unistd.h>
+
+/* These may not yet be available in the uAPI so define if not. */
+
+#ifndef MADV_GUARD_POISON
+#define MADV_GUARD_POISON 102
+#endif
+
+#ifndef MADV_GUARD_UNPOISON
+#define MADV_GUARD_UNPOISON 103
+#endif
+
+volatile bool signal_jump_set;
Can you add a comment about why volatile is needed.
I'm not sure it's really necessary, it's completely standard to do this
with signal handling and is one of the exceptions to the 'volatile
considered harmful' rule.
By the way did you happen to run checkpatck on this. There are
several instances where single statement blocks with braces {}
I noticed a few and ran checkpatch on your patch. There are
45 warnings regarding codeing style.
Please run checkpatch and clean them up so we can avoid followup
checkpatch cleanup patches.
No sorry I won't, checkpatch isn't infallible and series trying to 'clean
up' things that aren't issues will be a waste of everybody's time.
Sorry - this violates the coding styles and makes it hard to read.
See process/coding-style.rst:
Do not unnecessarily use braces where a single statement will do.
.. code-block:: c
if (condition)
action();
and
.. code-block:: c
if (condition)
do_this();
else
do_that();
This does not apply if only one branch of a conditional statement is a single
statement; in the latter case use braces in both branches:
.. code-block:: c
if (condition) {
do_this();
do_that();
} else {
otherwise();
}
Also, use braces when a loop contains more than a single simple statement:
.. code-block:: c
while (condition) {
if (test)
do_something();
}
thanks,
-- Shuah
Shuah, quoting coding standards to an experienced kernel developer
(maintainer now) is maybe not the best way to engage here + it may have
been more productive for you to first engage on why it is I'm deviating
here.
Firstly, as I said, the code _does not compile_ if I do not use braces in
many cases. This is probably an issue with the macros, but it is out of
scope for this series for me to fix that.
I am not trying to throw the book at you. When I see 45 of
them I have to ask the reasons why.
'Fixing' these cases results in:
CC guard-pages
guard-pages.c: In function ‘guard_pages_split_merge’:
guard-pages.c:566:17: error: ‘else’ without a previous ‘if’
566 | else
| ^~~~
guard-pages.c: In function ‘guard_pages_dontneed’:
guard-pages.c:666:17: error: ‘else’ without a previous ‘if’
666 | else
| ^~~~
guard-pages.c: In function ‘guard_pages_fork’:
guard-pages.c:957:17: error: ‘else’ without a previous ‘if’
957 | else
| ^~~~
guard-pages.c: In function ‘guard_pages_fork_wipeonfork’:
guard-pages.c:1010:17: error: ‘else’ without a previous ‘if’
1010 | else
| ^~~~
In other cases I am simply not a fan of single line loops where there is a
lot of compound stuff going on:
for (i = 0; i < 10; i++) {
ASSERT_FALSE(try_read_write_buf(&ptr1[i * page_size]));
}
vs.
for (i = 0; i < 10; i++)
ASSERT_FALSE(try_read_write_buf(&ptr1[i * page_size]));
When there are very many loops like this. This is kind of a test-specific
thing, you'd maybe put more effort into splitting this up + have less
repetition in non-test code.
I'm not going to die on the hill of single-line-for-loops though, so if you
insist I'll change those.
However I simply _cannot_ change the if/else blocks that cause compilation
errors.
This is what I mean about checkpatch being fallible. It's also fallible in
other cases, like variable declarations via macro (understandably).
Expecting checkpatch to give zero warnings is simply unattainable,
unfortunately.
As you seem adamant about strict adherence to checkpatch, and I always try
to be accommodating where I can be, I suggest I fix everything _except
where it breaks the compilation_ does that work for you?
Yes Please. If you leave these in here as soon as the patch hits
next, we start seeing a flood of patches. It becomes harder to fix
these later due to merge conflicts.
It becomes a patch overload issue. Yes I would like to see the ones
that don't result in compile errors fixed.
thanks,
-- Shuah
Thanks.
