* [PATCH v4 0/4] introduce PIDFD_SELF* sentinels
From: Lorenzo Stoakes @ 2024-10-17 21:05 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang, John Hubbard
If you wish to utilise a pidfd interface to refer to the current process or
thread it is rather cumbersome, requiring something like:
int pidfd = pidfd_open(getpid(), 0 or PIDFD_THREAD);
...
close(pidfd);
Or the equivalent call opening /proc/self. It is more convenient to use a
sentinel value to indicate to an interface that accepts a pidfd that we
simply wish to refer to the current process thread.
This series introduces sentinels for this purposes which can be passed as
the pidfd in this instance rather than having to establish a dummy fd for
this purpose.
It is useful to refer to both the current thread from the userland's
perspective for which we use PIDFD_SELF, and the current process from the
userland's perspective, for which we use PIDFD_SELF_PROCESS.
There is unfortunately some confusion between the kernel and userland as to
what constitutes a process - a thread from the userland perspective is a
process in userland, and a userland process is a thread group (more
specifically the thread group leader from the kernel perspective). We
therefore alias things thusly:
* PIDFD_SELF_THREAD aliased by PIDFD_SELF - use PIDTYPE_PID.
* PIDFD_SELF_THREAD_GROUP alised by PIDFD_SELF_PROCESS - use PIDTYPE_TGID.
In all of the kernel code we refer to PIDFD_SELF_THREAD and
PIDFD_SELF_THREAD_GROUP. However we expect users to use PIDFD_SELF and
PIDFD_SELF_PROCESS.
This matters for cases where, for instance, a user unshare()'s FDs or does
thread-specific signal handling and where the user would be hugely confused
if the FDs referenced or signal processed referred to the thread group
leader rather than the individual thread.
We ensure that pidfd_send_signal() and pidfd_getfd() work correctly, and
assert as much in selftests. All other interfaces except setns() will work
implicitly with this new interface, however it doesn't make sense to test
waitid(P_PIDFD, ...) as waiting on ourselves is a blocking operation.
In the case of setns() we explicitly disallow use of PIDFD_SELF* as it
doesn't make sense to obtain the namespaces of our own process, and it
would require work to implement this functionality there that would be of
no use.
We also do not provide the ability to utilise PIDFD_SELF* in ordinary fd
operations such as open() or poll(), as this would require extensive work
and be of no real use.
v4:
* Avoid returning an fd in the __pidfd_get_pid() function as pointed out by
Christian, instead simply always pin the pid and maintain fd scope in the
helper alone.
* Add wrapper header file in tools/include/linux to allow for import of
UAPI pidfd.h header without encountering the collision between system
fcntl.h and linux/fcntl.h as discussed with Shuah and John.
* Fixup tests to import the UAPI pidfd.h header working around conflicts
between system fcntl.h and linux/fcntl.h which the UAPI pidfd.h imports,
as reported by Shuah.
* Use an int for pidfd_is_self_sentinel() to avoid any dependency on
stdbool.h in userland.
v3:
* Do not fput() an invalid fd as reported by kernel test bot.
* Fix unintended churn from moving variable declaration.
https://lore.kernel.org/linux-mm/cover.1729073310.git.lorenzo.stoakes@oracle.com/
v2:
* Fix tests as reported by Shuah.
* Correct RFC version lore link.
https://lore.kernel.org/linux-mm/cover.1728643714.git.lorenzo.stoakes@oracle.com/
Non-RFC v1:
* Removed RFC tag - there seems to be general consensus that this change is
a good idea, but perhaps some debate to be had on implementation. It
seems sensible then to move forward with the RFC flag removed.
* Introduced PIDFD_SELF_THREAD, PIDFD_SELF_THREAD_GROUP and their aliases
PIDFD_SELF and PIDFD_SELF_PROCESS respectively.
* Updated testing accordingly.
https://lore.kernel.org/linux-mm/cover.1728578231.git.lorenzo.stoakes@oracle.com/
RFC version:
https://lore.kernel.org/linux-mm/cover.1727644404.git.lorenzo.stoakes@oracle.com/
Lorenzo Stoakes (4):
pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
selftests: pidfd: add pidfd.h UAPI wrapper
selftests: pidfd: add tests for PIDFD_SELF_*
include/linux/pid.h | 34 ++++-
include/uapi/linux/pidfd.h | 15 ++
kernel/exit.c | 3 +-
kernel/nsproxy.c | 1 +
kernel/pid.c | 65 +++++---
kernel/signal.c | 29 +---
tools/include/linux/pidfd.h | 14 ++
tools/testing/selftests/pidfd/Makefile | 3 +-
tools/testing/selftests/pidfd/pidfd.h | 2 +
.../selftests/pidfd/pidfd_getfd_test.c | 141 ++++++++++++++++++
.../selftests/pidfd/pidfd_setns_test.c | 11 ++
tools/testing/selftests/pidfd/pidfd_test.c | 76 ++++++++--
12 files changed, 333 insertions(+), 61 deletions(-)
create mode 100644 tools/include/linux/pidfd.h
--
2.46.2
^ permalink raw reply
* [PATCH v4 2/4] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: Lorenzo Stoakes @ 2024-10-17 21:05 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang, John Hubbard
In-Reply-To: <cover.1729198898.git.lorenzo.stoakes@oracle.com>
It is useful to be able to utilise the pidfd mechanism to reference the
current thread or process (from a userland point of view - thread group
leader from the kernel's point of view).
Therefore introduce PIDFD_SELF_THREAD to refer to the current thread, and
PIDFD_SELF_THREAD_GROUP to refer to the current thread group leader.
For convenience and to avoid confusion from userland's perspective we alias
these:
* PIDFD_SELF is an alias for PIDFD_SELF_THREAD - This is nearly always what
the user will want to use, as they would find it surprising if for
instance fd's were unshared()'d and they wanted to invoke pidfd_getfd()
and that failed.
* PIDFD_SELF_PROCESS is an alias for PIDFD_SELF_THREAD_GROUP - Most users
have no concept of thread groups or what a thread group leader is, and
from userland's perspective and nomenclature this is what userland
considers to be a process.
Due to the refactoring of the central __pidfd_get_pid() function we can
implement this functionality centrally, providing the use of this sentinel
in most functionality which utilises pidfd's.
We need to explicitly adjust kernel_waitid_prepare() to permit this (though
it wouldn't really make sense to use this there, we provide the ability for
consistency).
We explicitly disallow use of this in setns(), which would otherwise have
required explicit custom handling, as it doesn't make sense to set the
current calling thread to join the namespace of itself.
As the callers of pidfd_get_pid() expect an increased reference count on
the pid we do so in the self case, reducing churn and avoiding any breakage
from existing logic which decrements this reference count.
This change implicitly provides PIDFD_SELF_* support in the waitid(P_PIDFS,
...), process_madvise(), process_mrelease(), pidfd_send_signal(), and
pidfd_getfd() system calls.
Things such as polling a pidfs and general fd operations are not supported,
this strictly provides the sentinel for APIs which explicitly accept a
pidfd.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
include/linux/pid.h | 8 ++++--
include/uapi/linux/pidfd.h | 15 +++++++++++
kernel/exit.c | 3 ++-
kernel/nsproxy.c | 1 +
kernel/pid.c | 51 ++++++++++++++++++++++++--------------
5 files changed, 57 insertions(+), 21 deletions(-)
diff --git a/include/linux/pid.h b/include/linux/pid.h
index d466890e1b35..3b2ac7567a88 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -78,11 +78,15 @@ struct file;
* __pidfd_get_pid() - Retrieve a pid associated with the specified pidfd.
*
* @pidfd: The pidfd whose pid we want, or the fd of a /proc/<pid> file if
- * @alloc_proc is also set.
+ * @alloc_proc is also set, or PIDFD_SELF_* to refer to the current
+ * thread or thread group leader.
* @allow_proc: If set, then an fd of a /proc/<pid> file can be passed instead
* of a pidfd, and this will be used to determine the pid.
+
* @flags: Output variable, if non-NULL, then the file->f_flags of the
- * pidfd will be set here.
+ * pidfd will be set here or If PIDFD_SELF_THREAD is set, this is
+ * set to PIDFD_THREAD, otherwise if PIDFD_SELF_THREAD_GROUP then
+ * this is set to zero.
*
* Returns: If successful, the pid associated with the pidfd, otherwise an
* error.
diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
index 565fc0629fff..0ca2ebf906fd 100644
--- a/include/uapi/linux/pidfd.h
+++ b/include/uapi/linux/pidfd.h
@@ -29,4 +29,19 @@
#define PIDFD_GET_USER_NAMESPACE _IO(PIDFS_IOCTL_MAGIC, 9)
#define PIDFD_GET_UTS_NAMESPACE _IO(PIDFS_IOCTL_MAGIC, 10)
+/*
+ * Special sentinel values which can be used to refer to the current thread or
+ * thread group leader (which from a userland perspective is the process).
+ */
+#define PIDFD_SELF PIDFD_SELF_THREAD
+#define PIDFD_SELF_PROCESS PIDFD_SELF_THREAD_GROUP
+
+#define PIDFD_SELF_THREAD -100 /* Current thread. */
+#define PIDFD_SELF_THREAD_GROUP -200 /* Current thread group leader. */
+
+static inline int pidfd_is_self_sentinel(pid_t pid)
+{
+ return pid == PIDFD_SELF_THREAD || pid == PIDFD_SELF_THREAD_GROUP;
+}
+
#endif /* _UAPI_LINUX_PIDFD_H */
diff --git a/kernel/exit.c b/kernel/exit.c
index 619f0014c33b..3eb20f8252ee 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -71,6 +71,7 @@
#include <linux/user_events.h>
#include <linux/uaccess.h>
+#include <uapi/linux/pidfd.h>
#include <uapi/linux/wait.h>
#include <asm/unistd.h>
@@ -1739,7 +1740,7 @@ int kernel_waitid_prepare(struct wait_opts *wo, int which, pid_t upid,
break;
case P_PIDFD:
type = PIDTYPE_PID;
- if (upid < 0)
+ if (upid < 0 && !pidfd_is_self_sentinel(upid))
return -EINVAL;
pid = pidfd_get_pid(upid, &f_flags);
diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
index dc952c3b05af..d239f7eeaa1f 100644
--- a/kernel/nsproxy.c
+++ b/kernel/nsproxy.c
@@ -550,6 +550,7 @@ SYSCALL_DEFINE2(setns, int, fd, int, flags)
struct nsset nsset = {};
int err = 0;
+ /* If fd is PIDFD_SELF_*, implicitly fail here, as invalid. */
if (!fd_file(f))
return -EBADF;
diff --git a/kernel/pid.c b/kernel/pid.c
index 94c97559e5c5..8742157b36f8 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -535,33 +535,48 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
}
EXPORT_SYMBOL_GPL(find_ge_pid);
+static struct pid *pidfd_get_pid_self(unsigned int pidfd, unsigned int *flags)
+{
+ bool is_thread = pidfd == PIDFD_SELF_THREAD;
+ enum pid_type type = is_thread ? PIDTYPE_PID : PIDTYPE_TGID;
+ struct pid *pid = *task_pid_ptr(current, type);
+
+ /* The caller expects an elevated reference count. */
+ get_pid(pid);
+ return pid;
+}
+
struct pid *__pidfd_get_pid(unsigned int pidfd, bool allow_proc,
unsigned int *flags)
{
- struct pid *pid;
- struct fd f = fdget(pidfd);
- struct file *file = fd_file(f);
+ if (pidfd_is_self_sentinel(pidfd)) {
+ return pidfd_get_pid_self(pidfd, flags);
+ } else {
+ struct pid *pid;
+ struct fd f = fdget(pidfd);
+ struct file *file = fd_file(f);
- if (!file)
- return ERR_PTR(-EBADF);
+ if (!file)
+ return ERR_PTR(-EBADF);
- pid = pidfd_pid(file);
- /* If we allow opening a pidfd via /proc/<pid>, do so. */
- if (IS_ERR(pid) && allow_proc)
- pid = tgid_pidfd_to_pid(file);
+ pid = pidfd_pid(file);
+ /* If we allow opening a pidfd via /proc/<pid>, do so. */
+ if (IS_ERR(pid) && allow_proc)
+ pid = tgid_pidfd_to_pid(file);
- if (IS_ERR(pid)) {
+ if (IS_ERR(pid)) {
+ fdput(f);
+ return pid;
+ }
+
+ /* Pin pid before we release fd. */
+ get_pid(pid);
+ if (flags)
+ *flags = file->f_flags;
fdput(f);
+
return pid;
}
-
- /* Pin pid before we release fd. */
- get_pid(pid);
- if (flags)
- *flags = file->f_flags;
- fdput(f);
-
- return pid;
}
/**
--
2.46.2
^ permalink raw reply related
* [PATCH v4 1/4] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Lorenzo Stoakes @ 2024-10-17 21:05 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang, John Hubbard
In-Reply-To: <cover.1729198898.git.lorenzo.stoakes@oracle.com>
The means by which a pid is determined from a pidfd is duplicated, with
some callers holding a reference to the (pid)fd, and others explicitly
pinning the pid.
Introduce __pidfd_get_pid() which narrows this to one approach of pinning
the pid, with an optional output parameters for file->f_flags to avoid the
need to hold onto a file to retrieve this.
Additionally, allow the ability to open a pidfd by opening a /proc/<pid>
directory, utilised by the pidfd_send_signal() system call, providing a
pidfd_get_pid_proc() helper function to do so.
Doing this allows us to eliminate open-coded pidfd pid lookup and to
consistently handle this in one place.
This lays the groundwork for a subsequent patch which adds a new sentinel
pidfd to explicitly reference the current process (i.e. thread group
leader) without the need for a pidfd.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
include/linux/pid.h | 30 +++++++++++++++++++++++++++++-
kernel/pid.c | 42 ++++++++++++++++++++++++------------------
kernel/signal.c | 29 ++++++-----------------------
3 files changed, 59 insertions(+), 42 deletions(-)
diff --git a/include/linux/pid.h b/include/linux/pid.h
index a3aad9b4074c..d466890e1b35 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -2,6 +2,7 @@
#ifndef _LINUX_PID_H
#define _LINUX_PID_H
+#include <linux/file.h>
#include <linux/pid_types.h>
#include <linux/rculist.h>
#include <linux/rcupdate.h>
@@ -72,8 +73,35 @@ extern struct pid init_struct_pid;
struct file;
+
+/**
+ * __pidfd_get_pid() - Retrieve a pid associated with the specified pidfd.
+ *
+ * @pidfd: The pidfd whose pid we want, or the fd of a /proc/<pid> file if
+ * @alloc_proc is also set.
+ * @allow_proc: If set, then an fd of a /proc/<pid> file can be passed instead
+ * of a pidfd, and this will be used to determine the pid.
+ * @flags: Output variable, if non-NULL, then the file->f_flags of the
+ * pidfd will be set here.
+ *
+ * Returns: If successful, the pid associated with the pidfd, otherwise an
+ * error.
+ */
+struct pid *__pidfd_get_pid(unsigned int pidfd, bool allow_proc,
+ unsigned int *flags);
+
+static inline struct pid *pidfd_get_pid(unsigned int pidfd, unsigned int *flags)
+{
+ return __pidfd_get_pid(pidfd, /* allow_proc = */ false, flags);
+}
+
+static inline struct pid *pidfd_get_pid_proc(unsigned int pidfd,
+ unsigned int *flags)
+{
+ return __pidfd_get_pid(pidfd, /* allow_proc = */ true, flags);
+}
+
struct pid *pidfd_pid(const struct file *file);
-struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags);
struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags);
int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret);
void do_notify_pidfd(struct task_struct *task);
diff --git a/kernel/pid.c b/kernel/pid.c
index 2715afb77eab..94c97559e5c5 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -36,6 +36,7 @@
#include <linux/pid_namespace.h>
#include <linux/init_task.h>
#include <linux/syscalls.h>
+#include <linux/proc_fs.h>
#include <linux/proc_ns.h>
#include <linux/refcount.h>
#include <linux/anon_inodes.h>
@@ -534,22 +535,32 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
}
EXPORT_SYMBOL_GPL(find_ge_pid);
-struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
+struct pid *__pidfd_get_pid(unsigned int pidfd, bool allow_proc,
+ unsigned int *flags)
{
- struct fd f;
struct pid *pid;
+ struct fd f = fdget(pidfd);
+ struct file *file = fd_file(f);
- f = fdget(fd);
- if (!fd_file(f))
+ if (!file)
return ERR_PTR(-EBADF);
- pid = pidfd_pid(fd_file(f));
- if (!IS_ERR(pid)) {
- get_pid(pid);
- *flags = fd_file(f)->f_flags;
+ pid = pidfd_pid(file);
+ /* If we allow opening a pidfd via /proc/<pid>, do so. */
+ if (IS_ERR(pid) && allow_proc)
+ pid = tgid_pidfd_to_pid(file);
+
+ if (IS_ERR(pid)) {
+ fdput(f);
+ return pid;
}
+ /* Pin pid before we release fd. */
+ get_pid(pid);
+ if (flags)
+ *flags = file->f_flags;
fdput(f);
+
return pid;
}
@@ -747,23 +758,18 @@ SYSCALL_DEFINE3(pidfd_getfd, int, pidfd, int, fd,
unsigned int, flags)
{
struct pid *pid;
- struct fd f;
int ret;
/* flags is currently unused - make sure it's unset */
if (flags)
return -EINVAL;
- f = fdget(pidfd);
- if (!fd_file(f))
- return -EBADF;
-
- pid = pidfd_pid(fd_file(f));
+ pid = pidfd_get_pid(pidfd, NULL);
if (IS_ERR(pid))
- ret = PTR_ERR(pid);
- else
- ret = pidfd_getfd(pid, fd);
+ return PTR_ERR(pid);
- fdput(f);
+ ret = pidfd_getfd(pid, fd);
+
+ put_pid(pid);
return ret;
}
diff --git a/kernel/signal.c b/kernel/signal.c
index 4344860ffcac..9a35b1cf40ad 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3875,17 +3875,6 @@ static int copy_siginfo_from_user_any(kernel_siginfo_t *kinfo,
return copy_siginfo_from_user(kinfo, info);
}
-static struct pid *pidfd_to_pid(const struct file *file)
-{
- struct pid *pid;
-
- pid = pidfd_pid(file);
- if (!IS_ERR(pid))
- return pid;
-
- return tgid_pidfd_to_pid(file);
-}
-
#define PIDFD_SEND_SIGNAL_FLAGS \
(PIDFD_SIGNAL_THREAD | PIDFD_SIGNAL_THREAD_GROUP | \
PIDFD_SIGNAL_PROCESS_GROUP)
@@ -3908,10 +3897,10 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
siginfo_t __user *, info, unsigned int, flags)
{
int ret;
- struct fd f;
struct pid *pid;
kernel_siginfo_t kinfo;
enum pid_type type;
+ unsigned int f_flags;
/* Enforce flags be set to 0 until we add an extension. */
if (flags & ~PIDFD_SEND_SIGNAL_FLAGS)
@@ -3921,16 +3910,10 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
if (hweight32(flags & PIDFD_SEND_SIGNAL_FLAGS) > 1)
return -EINVAL;
- f = fdget(pidfd);
- if (!fd_file(f))
- return -EBADF;
-
/* Is this a pidfd? */
- pid = pidfd_to_pid(fd_file(f));
- if (IS_ERR(pid)) {
- ret = PTR_ERR(pid);
- goto err;
- }
+ pid = pidfd_get_pid_proc(pidfd, &f_flags);
+ if (IS_ERR(pid))
+ return PTR_ERR(pid);
ret = -EINVAL;
if (!access_pidfd_pidns(pid))
@@ -3939,7 +3922,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
switch (flags) {
case 0:
/* Infer scope from the type of pidfd. */
- if (fd_file(f)->f_flags & PIDFD_THREAD)
+ if (f_flags & PIDFD_THREAD)
type = PIDTYPE_PID;
else
type = PIDTYPE_TGID;
@@ -3978,7 +3961,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
else
ret = kill_pid_info_type(sig, &kinfo, pid, type);
err:
- fdput(f);
+ put_pid(pid);
return ret;
}
--
2.46.2
^ permalink raw reply related
* [PATCH v4 3/4] selftests: pidfd: add pidfd.h UAPI wrapper
From: Lorenzo Stoakes @ 2024-10-17 21:05 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang, John Hubbard
In-Reply-To: <cover.1729198898.git.lorenzo.stoakes@oracle.com>
Conflicts can arise between system fcntl.h and linux/fcntl.h, imported by
the linux/pidfd.h UAPI header.
Work around this by adding a wrapper for linux/pidfd.h to
tools/include/ which sets the linux/fcntl.h header guard ahead of
importing the pidfd.h header file.
Adjust the pidfd selftests Makefile to reference this include directory and
put it at a higher precidence than any make header installed headers to
ensure the wrapper is preferred.
This way we can directly import the UAPI header file without issue, use the
latest system header file without having to duplicate anything.
Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
tools/include/linux/pidfd.h | 14 ++++++++++++++
tools/testing/selftests/pidfd/Makefile | 3 +--
2 files changed, 15 insertions(+), 2 deletions(-)
create mode 100644 tools/include/linux/pidfd.h
diff --git a/tools/include/linux/pidfd.h b/tools/include/linux/pidfd.h
new file mode 100644
index 000000000000..113c8023072d
--- /dev/null
+++ b/tools/include/linux/pidfd.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef _TOOLS_LINUX_PIDFD_H
+#define _TOOLS_LINUX_PIDFD_H
+
+/*
+ * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so
+ * work around this by setting the header guard.
+ */
+#define _LINUX_FCNTL_H
+#include "../../../include/uapi/linux/pidfd.h"
+#undef _LINUX_FCNTL_H
+
+#endif /* _TOOLS_LINUX_PIDFD_H */
diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
index d731e3e76d5b..f5038c9dae14 100644
--- a/tools/testing/selftests/pidfd/Makefile
+++ b/tools/testing/selftests/pidfd/Makefile
@@ -1,8 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
-CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall
+CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
TEST_GEN_PROGS := pidfd_test pidfd_fdinfo_test pidfd_open_test \
pidfd_poll_test pidfd_wait pidfd_getfd_test pidfd_setns_test
include ../lib.mk
-
--
2.46.2
^ permalink raw reply related
* [PATCH v4 4/4] selftests: pidfd: add tests for PIDFD_SELF_*
From: Lorenzo Stoakes @ 2024-10-17 21:05 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang, John Hubbard
In-Reply-To: <cover.1729198898.git.lorenzo.stoakes@oracle.com>
Add tests to assert that PIDFD_SELF_* correctly refers to the current
thread and process.
This is only practically meaningful to pidfd_send_signal() and
pidfd_getfd(), but also explicitly test that we disallow this feature for
setns() where it would make no sense.
We cannot reasonably wait on ourself using waitid(P_PIDFD, ...) so while in
theory PIDFD_SELF_* would work here, we'd be left blocked if we tried it.
We defer testing of mm-specific functionality which uses pidfd, namely
process_madvise() and process_mrelease() to mm testing (though note the
latter can not be sensibly tested as it would require the testing process
to be dying).
Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
tools/testing/selftests/pidfd/pidfd.h | 2 +
.../selftests/pidfd/pidfd_getfd_test.c | 141 ++++++++++++++++++
.../selftests/pidfd/pidfd_setns_test.c | 11 ++
tools/testing/selftests/pidfd/pidfd_test.c | 76 ++++++++--
4 files changed, 218 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h
index 88d6830ee004..cbe1a2be3cec 100644
--- a/tools/testing/selftests/pidfd/pidfd.h
+++ b/tools/testing/selftests/pidfd/pidfd.h
@@ -16,6 +16,8 @@
#include <sys/types.h>
#include <sys/wait.h>
+#include <linux/pidfd.h>
+
#include "../kselftest.h"
#ifndef P_PIDFD
diff --git a/tools/testing/selftests/pidfd/pidfd_getfd_test.c b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
index cd51d547b751..48d224b13c01 100644
--- a/tools/testing/selftests/pidfd/pidfd_getfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
@@ -6,6 +6,7 @@
#include <limits.h>
#include <linux/types.h>
#include <poll.h>
+#include <pthread.h>
#include <sched.h>
#include <signal.h>
#include <stdio.h>
@@ -15,6 +16,7 @@
#include <sys/prctl.h>
#include <sys/wait.h>
#include <unistd.h>
+#include <sys/mman.h>
#include <sys/socket.h>
#include <linux/kcmp.h>
@@ -114,6 +116,94 @@ static int child(int sk)
return ret;
}
+static int __pidfd_self_thread_worker(unsigned long page_size)
+{
+ int memfd;
+ int newfd;
+ char *ptr;
+ int err = 0;
+
+ /*
+ * Unshare our FDs so we have our own set. This means
+ * PIDFD_SELF_THREAD_GROUP will fal.
+ */
+ if (unshare(CLONE_FILES) < 0) {
+ err = -errno;
+ goto exit;
+ }
+
+ /* Truncate, map in and write to our memfd. */
+ memfd = sys_memfd_create("test_self_child", 0);
+ if (memfd < 0) {
+ err = -errno;
+ goto exit;
+ }
+
+ if (ftruncate(memfd, page_size)) {
+ err = -errno;
+ goto exit_close_memfd;
+ }
+
+ ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+ MAP_SHARED, memfd, 0);
+ if (ptr == MAP_FAILED) {
+ err = -errno;
+ goto exit_close_memfd;
+ }
+ ptr[0] = 'y';
+ if (munmap(ptr, page_size)) {
+ err = -errno;
+ goto exit_close_memfd;
+ }
+
+ /* Get a thread-local duplicate of our memfd. */
+ newfd = sys_pidfd_getfd(PIDFD_SELF_THREAD, memfd, 0);
+ if (newfd < 0) {
+ err = -errno;
+ goto exit_close_memfd;
+ }
+
+ if (memfd == newfd) {
+ err = -EINVAL;
+ goto exit_close_fds;
+ }
+
+ /* Map in new fd and make sure that the data is as expected. */
+ ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+ MAP_SHARED, newfd, 0);
+ if (ptr == MAP_FAILED) {
+ err = -errno;
+ goto exit_close_fds;
+ }
+
+ if (ptr[0] != 'y') {
+ err = -EINVAL;
+ goto exit_close_fds;
+ }
+
+ if (munmap(ptr, page_size)) {
+ err = -errno;
+ goto exit_close_fds;
+ }
+
+exit_close_fds:
+ close(newfd);
+exit_close_memfd:
+ close(memfd);
+exit:
+ return err;
+}
+
+static void *pidfd_self_thread_worker(void *arg)
+{
+ unsigned long page_size = (unsigned long)arg;
+ int ret;
+
+ /* We forward any errors for the caller to handle. */
+ ret = __pidfd_self_thread_worker(page_size);
+ return (void *)(intptr_t)ret;
+}
+
FIXTURE(child)
{
/*
@@ -264,6 +354,57 @@ TEST_F(child, no_strange_EBADF)
EXPECT_EQ(errno, ESRCH);
}
+TEST(pidfd_self)
+{
+ int memfd = sys_memfd_create("test_self", 0);
+ unsigned long page_size = sysconf(_SC_PAGESIZE);
+ int newfd;
+ char *ptr;
+ pthread_t thread;
+ void *res;
+ int err;
+
+ ASSERT_GE(memfd, 0);
+ ASSERT_EQ(ftruncate(memfd, page_size), 0);
+
+ /*
+ * Map so we can assert that the duplicated fd references the same
+ * memory.
+ */
+ ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+ MAP_SHARED, memfd, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+ ptr[0] = 'x';
+ ASSERT_EQ(munmap(ptr, page_size), 0);
+
+ /* Now get a duplicate of our memfd. */
+ newfd = sys_pidfd_getfd(PIDFD_SELF_THREAD_GROUP, memfd, 0);
+ ASSERT_GE(newfd, 0);
+ ASSERT_NE(memfd, newfd);
+
+ /* Now map duplicate fd and make sure it references the same memory. */
+ ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+ MAP_SHARED, newfd, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+ ASSERT_EQ(ptr[0], 'x');
+ ASSERT_EQ(munmap(ptr, page_size), 0);
+
+ /* Cleanup. */
+ close(memfd);
+ close(newfd);
+
+ /*
+ * Fire up the thread and assert that we can lookup the thread-specific
+ * PIDFD_SELF_THREAD (also aliased by PIDFD_SELF).
+ */
+ ASSERT_EQ(pthread_create(&thread, NULL, pidfd_self_thread_worker,
+ (void *)page_size), 0);
+ ASSERT_EQ(pthread_join(thread, &res), 0);
+ err = (int)(intptr_t)res;
+
+ ASSERT_EQ(err, 0);
+}
+
#if __NR_pidfd_getfd == -1
int main(void)
{
diff --git a/tools/testing/selftests/pidfd/pidfd_setns_test.c b/tools/testing/selftests/pidfd/pidfd_setns_test.c
index 7c2a4349170a..bbd39dc5ceb7 100644
--- a/tools/testing/selftests/pidfd/pidfd_setns_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_setns_test.c
@@ -752,4 +752,15 @@ TEST(setns_einval)
close(fd);
}
+TEST(setns_pidfd_self_disallowed)
+{
+ ASSERT_EQ(setns(PIDFD_SELF_THREAD, 0), -1);
+ EXPECT_EQ(errno, EBADF);
+
+ errno = 0;
+
+ ASSERT_EQ(setns(PIDFD_SELF_THREAD_GROUP, 0), -1);
+ EXPECT_EQ(errno, EBADF);
+}
+
TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
index 9faa686f90e4..440447cf89ba 100644
--- a/tools/testing/selftests/pidfd/pidfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_test.c
@@ -42,12 +42,41 @@ static pid_t pidfd_clone(int flags, int *pidfd, int (*fn)(void *))
#endif
}
-static int signal_received;
+static pthread_t signal_received;
static void set_signal_received_on_sigusr1(int sig)
{
if (sig == SIGUSR1)
- signal_received = 1;
+ signal_received = pthread_self();
+}
+
+static int send_signal(int pidfd)
+{
+ int ret = 0;
+
+ if (sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0) < 0) {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ if (signal_received != pthread_self()) {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+exit:
+ signal_received = 0;
+ return ret;
+}
+
+static void *send_signal_worker(void *arg)
+{
+ int pidfd = (int)(intptr_t)arg;
+ int ret;
+
+ /* We forward any errors for the caller to handle. */
+ ret = send_signal(pidfd);
+ return (void *)(intptr_t)ret;
}
/*
@@ -56,8 +85,11 @@ static void set_signal_received_on_sigusr1(int sig)
*/
static int test_pidfd_send_signal_simple_success(void)
{
- int pidfd, ret;
+ int pidfd;
const char *test_name = "pidfd_send_signal send SIGUSR1";
+ pthread_t thread;
+ void *thread_res;
+ int err;
if (!have_pidfd_send_signal) {
ksft_test_result_skip(
@@ -66,25 +98,45 @@ static int test_pidfd_send_signal_simple_success(void)
return 0;
}
+ signal(SIGUSR1, set_signal_received_on_sigusr1);
+
+ /* Try sending a signal to ourselves via /proc/self. */
pidfd = open("/proc/self", O_DIRECTORY | O_CLOEXEC);
if (pidfd < 0)
ksft_exit_fail_msg(
"%s test: Failed to open process file descriptor\n",
test_name);
+ err = send_signal(pidfd);
+ if (err)
+ ksft_exit_fail_msg(
+ "%s test: Error %d on sending pidfd signal\n",
+ test_name, err);
+ close(pidfd);
- signal(SIGUSR1, set_signal_received_on_sigusr1);
+ /* Now try the same thing only using PIDFD_SELF_THREAD_GROUP. */
+ err = send_signal(PIDFD_SELF_THREAD_GROUP);
+ if (err)
+ ksft_exit_fail_msg(
+ "%s test: Error %d on PIDFD_SELF_THREAD_GROUP signal\n",
+ test_name, err);
- ret = sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0);
- close(pidfd);
- if (ret < 0)
- ksft_exit_fail_msg("%s test: Failed to send signal\n",
+ /*
+ * Now try the same thing in a thread and assert thread ID is equal to
+ * worker thread ID.
+ */
+ if (pthread_create(&thread, NULL, send_signal_worker,
+ (void *)(intptr_t)PIDFD_SELF_THREAD))
+ ksft_exit_fail_msg("%s test: Failed to create thread\n",
test_name);
-
- if (signal_received != 1)
- ksft_exit_fail_msg("%s test: Failed to receive signal\n",
+ if (pthread_join(thread, &thread_res))
+ ksft_exit_fail_msg("%s test: Failed to join thread\n",
test_name);
+ err = (int)(intptr_t)thread_res;
+ if (err)
+ ksft_exit_fail_msg(
+ "%s test: Error %d on PIDFD_SELF_THREAD signal\n",
+ test_name, err);
- signal_received = 0;
ksft_test_result_pass("%s test: Sent signal\n", test_name);
return 0;
}
--
2.46.2
^ permalink raw reply related
* Re: [PATCH v4 3/4] selftests: pidfd: add pidfd.h UAPI wrapper
From: John Hubbard @ 2024-10-17 21:45 UTC (permalink / raw)
To: Lorenzo Stoakes, Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang
In-Reply-To: <d8d1a8c6ade7f13a100a5c11fd1e53f7f2fddba3.1729198898.git.lorenzo.stoakes@oracle.com>
On 10/17/24 2:05 PM, Lorenzo Stoakes wrote:
> Conflicts can arise between system fcntl.h and linux/fcntl.h, imported by
> the linux/pidfd.h UAPI header.
>
> Work around this by adding a wrapper for linux/pidfd.h to
> tools/include/ which sets the linux/fcntl.h header guard ahead of
> importing the pidfd.h header file.
>
> Adjust the pidfd selftests Makefile to reference this include directory and
> put it at a higher precidence than any make header installed headers to
> ensure the wrapper is preferred.
...but we are not actually using the installed headers, now. And we intend
to continue avoiding them. So the ordering shouldn't matter. More below:
>
> This way we can directly import the UAPI header file without issue, use the
> latest system header file without having to duplicate anything.
>
> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
> tools/include/linux/pidfd.h | 14 ++++++++++++++
> tools/testing/selftests/pidfd/Makefile | 3 +--
> 2 files changed, 15 insertions(+), 2 deletions(-)
> create mode 100644 tools/include/linux/pidfd.h
>
> diff --git a/tools/include/linux/pidfd.h b/tools/include/linux/pidfd.h
> new file mode 100644
> index 000000000000..113c8023072d
> --- /dev/null
> +++ b/tools/include/linux/pidfd.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef _TOOLS_LINUX_PIDFD_H
> +#define _TOOLS_LINUX_PIDFD_H
> +
> +/*
> + * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so
> + * work around this by setting the header guard.
> + */
> +#define _LINUX_FCNTL_H
> +#include "../../../include/uapi/linux/pidfd.h"
> +#undef _LINUX_FCNTL_H
Oh shoot, I think you, Shuah and I were referring to different uapi locations,
the whole time. And so the basic approach is different after all.
Your include path above actually refers to:
$(top_srcdir)/include/uapi/linux/fcntl.h
...but what I was intending was to copy a snapshot of that file (or a
snapshot from the one generated by "make headers"), to here:
$(top_srcdir)/tools/include/uapi/linux/fcntl.h
...and then use $(TOOLS_INCLUDES), which is already in selftests/lib.mk,
for that reason: to be available to all of the kselftests:
TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapi
The reasoning for this directory is further explained here:
tools/include/uapi/README
(And I see that selftests/proc has started using $(TOOLS_INCLUDES), that's
progress.)
And now, it's possible to change fcntl.h in place, instead of using a wrapper.
Although either way seems OK to me. (I'm sort of ignoring the details of
the actual header file conflict itself, for now.)
> +
> +#endif /* _TOOLS_LINUX_PIDFD_H */
> diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
> index d731e3e76d5b..f5038c9dae14 100644
> --- a/tools/testing/selftests/pidfd/Makefile
> +++ b/tools/testing/selftests/pidfd/Makefile
> @@ -1,8 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0-only
> -CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall
> +CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
Instead, it would look like this, which now mostly matches selftests/mm/Makefile,
which is also helpful, because eventually this can be factored into a common
piece for all selftests:
CFLAGS += -g -isystem $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -pthread -Wall
I apologize for just now noticing this! And these kselftests shouldn't require
so much fussing around, I know. But once we get this just right, it will work
well and last a long time. :)
thanks,
--
John Hubbard
^ permalink raw reply
* Re: [PATCH v4 3/4] selftests: pidfd: add pidfd.h UAPI wrapper
From: Shuah Khan @ 2024-10-17 22:11 UTC (permalink / raw)
To: John Hubbard, Lorenzo Stoakes, Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang, Shuah Khan
In-Reply-To: <7df771b9-bfd6-465e-b0ba-12d2aab13ec6@nvidia.com>
On 10/17/24 15:45, John Hubbard wrote:
> On 10/17/24 2:05 PM, Lorenzo Stoakes wrote:
>> Conflicts can arise between system fcntl.h and linux/fcntl.h, imported by
>> the linux/pidfd.h UAPI header.
>>
>> Work around this by adding a wrapper for linux/pidfd.h to
>> tools/include/ which sets the linux/fcntl.h header guard ahead of
>> importing the pidfd.h header file.
>>
>> Adjust the pidfd selftests Makefile to reference this include directory and
>> put it at a higher precidence than any make header installed headers to
>> ensure the wrapper is preferred.
>
> ...but we are not actually using the installed headers, now. And we intend
> to continue avoiding them. So the ordering shouldn't matter. More below:
>
>>
>> This way we can directly import the UAPI header file without issue, use the
>> latest system header file without having to duplicate anything.
>>
>> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
>> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>> ---
>> tools/include/linux/pidfd.h | 14 ++++++++++++++
>> tools/testing/selftests/pidfd/Makefile | 3 +--
>> 2 files changed, 15 insertions(+), 2 deletions(-)
>> create mode 100644 tools/include/linux/pidfd.h
>>
>> diff --git a/tools/include/linux/pidfd.h b/tools/include/linux/pidfd.h
>> new file mode 100644
>> index 000000000000..113c8023072d
>> --- /dev/null
>> +++ b/tools/include/linux/pidfd.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef _TOOLS_LINUX_PIDFD_H
>> +#define _TOOLS_LINUX_PIDFD_H
>> +
>> +/*
>> + * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so
>> + * work around this by setting the header guard.
>> + */
>> +#define _LINUX_FCNTL_H
>> +#include "../../../include/uapi/linux/pidfd.h"
>> +#undef _LINUX_FCNTL_H
>
> Oh shoot, I think you, Shuah and I were referring to different uapi locations,
> the whole time. And so the basic approach is different after all.
>
> Your include path above actually refers to:
>
> $(top_srcdir)/include/uapi/linux/fcntl.h
Correct. I am glad we are on the same page now.
>
> ...but what I was intending was to copy a snapshot of that file (or a
> snapshot from the one generated by "make headers"), to here:
>
> $(top_srcdir)/tools/include/uapi/linux/fcntl.h
So why do the copy and snapshot. Anytime you build userspace in the
reoo - you will need to run "make headers: whether you install
them under tools/include or include.
>
> ...and then use $(TOOLS_INCLUDES), which is already in selftests/lib.mk,
> for that reason: to be available to all of the kselftests:
>
> TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapi
Yes some tests do include that.
>
> The reasoning for this directory is further explained here:
>
> tools/include/uapi/README
>
> (And I see that selftests/proc has started using $(TOOLS_INCLUDES), that's
> progress.)
Yes the same problems apply here - what complicates this more is
selftests are supposed to test kernel changes, hence the need to
include latest kernel headers. The simple solution is adding a
dependency so we don't have to duplicate the headers. I don't
believe the perf solution works here. We will have to figure out
a solution.
>
> And now, it's possible to change fcntl.h in place, instead of using a wrapper.
> Although either way seems OK to me. (I'm sort of ignoring the details of
> the actual header file conflict itself, for now.)
>
>
>> +
>> +#endif /* _TOOLS_LINUX_PIDFD_H */
>> diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
>> index d731e3e76d5b..f5038c9dae14 100644
>> --- a/tools/testing/selftests/pidfd/Makefile
>> +++ b/tools/testing/selftests/pidfd/Makefile
>> @@ -1,8 +1,7 @@
>> # SPDX-License-Identifier: GPL-2.0-only
>> -CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall
>> +CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
>
> Instead, it would look like this, which now mostly matches selftests/mm/Makefile,
> which is also helpful, because eventually this can be factored into a common
> piece for all selftests:
>
> CFLAGS += -g -isystem $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -pthread -Wall
KHDR_INCLUDES is "make headers" location under the root directory. So what
happens when you add TOOLS_INCLUDES to it.
Does "make kselftest-all" work as it is supposed to? If it and all tests
can build then I am all for it.
>
> I apologize for just now noticing this! And these kselftests shouldn't require
> so much fussing around, I know. But once we get this just right, it will work
> well and last a long time. :)
>
On the contrary if we don't discuss/fuss and get this right, we have to
deal with changes like adding local defines and adhoc approaches in
individual tests - that is one reason we made the "make headers"
as a dependency. I would like to solve the problem of proliferation
of local defines and even system calls in some cases.
For now I am going let this patch go through as it is important to
add tests.
My goals are simple:
- no local defines unless it is abslulutely necessary
- be able to build tests that add coverage for new kernel
api and features before we release the kernel.
- make it easier for CIs to build and run tests
- continue to have tests works for kernel developres
e.g: mm developers build tests in mm directory. They
don't see the issues that crop up in CIs or running
the entire kselftest default run like CIs do.
Adhoc changes break some use-cases.
thanks,
-- Shuah
^ permalink raw reply
* Re: [PATCH v4 3/4] selftests: pidfd: add pidfd.h UAPI wrapper
From: John Hubbard @ 2024-10-17 22:22 UTC (permalink / raw)
To: Shuah Khan, Lorenzo Stoakes, Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang
In-Reply-To: <f24e645f-e7e6-436d-875b-7e15feeae8eb@linuxfoundation.org>
On 10/17/24 3:11 PM, Shuah Khan wrote:
> On 10/17/24 15:45, John Hubbard wrote:
>> On 10/17/24 2:05 PM, Lorenzo Stoakes wrote:
>>> Conflicts can arise between system fcntl.h and linux/fcntl.h, imported by
>>> the linux/pidfd.h UAPI header.
>>>
>>> Work around this by adding a wrapper for linux/pidfd.h to
>>> tools/include/ which sets the linux/fcntl.h header guard ahead of
>>> importing the pidfd.h header file.
>>>
>>> Adjust the pidfd selftests Makefile to reference this include directory and
>>> put it at a higher precidence than any make header installed headers to
>>> ensure the wrapper is preferred.
>>
>> ...but we are not actually using the installed headers, now. And we intend
>> to continue avoiding them. So the ordering shouldn't matter. More below:
>>
>>>
>>> This way we can directly import the UAPI header file without issue, use the
>>> latest system header file without having to duplicate anything.
>>>
>>> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
>>> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>>> ---
>>> tools/include/linux/pidfd.h | 14 ++++++++++++++
>>> tools/testing/selftests/pidfd/Makefile | 3 +--
>>> 2 files changed, 15 insertions(+), 2 deletions(-)
>>> create mode 100644 tools/include/linux/pidfd.h
>>>
>>> diff --git a/tools/include/linux/pidfd.h b/tools/include/linux/pidfd.h
>>> new file mode 100644
>>> index 000000000000..113c8023072d
>>> --- /dev/null
>>> +++ b/tools/include/linux/pidfd.h
>>> @@ -0,0 +1,14 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +
>>> +#ifndef _TOOLS_LINUX_PIDFD_H
>>> +#define _TOOLS_LINUX_PIDFD_H
>>> +
>>> +/*
>>> + * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so
>>> + * work around this by setting the header guard.
>>> + */
>>> +#define _LINUX_FCNTL_H
>>> +#include "../../../include/uapi/linux/pidfd.h"
>>> +#undef _LINUX_FCNTL_H
>>
>> Oh shoot, I think you, Shuah and I were referring to different uapi locations,
>> the whole time. And so the basic approach is different after all.
>>
>> Your include path above actually refers to:
>>
>> $(top_srcdir)/include/uapi/linux/fcntl.h
>
> Correct. I am glad we are on the same page now.
>
>>
>> ...but what I was intending was to copy a snapshot of that file (or a
>> snapshot from the one generated by "make headers"), to here:
>>
>> $(top_srcdir)/tools/include/uapi/linux/fcntl.h
>
> So why do the copy and snapshot. Anytime you build userspace in the
> reoo - you will need to run "make headers: whether you install
> them under tools/include or include.
No, you only do "make headers" once, and that's a temporary thing too.
After the author of the new selftest runs "make headers", the author
will typically remove those headers, in order to verify that the
selftests still build without the kernel generated header files.
That's it! No one else has to deal with it. And that's what is being
asked for here.
>
>
>>
>> ...and then use $(TOOLS_INCLUDES), which is already in selftests/lib.mk,
>> for that reason: to be available to all of the kselftests:
>>
>> TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapi
>
> Yes some tests do include that.
>
>>
>> The reasoning for this directory is further explained here:
>>
>> tools/include/uapi/README
>>
>> (And I see that selftests/proc has started using $(TOOLS_INCLUDES), that's
>> progress.)
>
> Yes the same problems apply here - what complicates this more is
> selftests are supposed to test kernel changes, hence the need to
> include latest kernel headers. The simple solution is adding a
> dependency so we don't have to duplicate the headers. I don't
> believe the perf solution works here. We will have to figure out
> a solution.
That's the key point: the fact that the selftests are intended to
test kernel changes does *not* mean that we have to include the
dynamically generated header files. That's going further than
necessary. And it leads to problems and complaints for kernel
developers, who are really one of the main user groups here.
>
>>
>> And now, it's possible to change fcntl.h in place, instead of using a wrapper.
>> Although either way seems OK to me. (I'm sort of ignoring the details of
>> the actual header file conflict itself, for now.)
>>
>>
>>> +
>>> +#endif /* _TOOLS_LINUX_PIDFD_H */
>>> diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
>>> index d731e3e76d5b..f5038c9dae14 100644
>>> --- a/tools/testing/selftests/pidfd/Makefile
>>> +++ b/tools/testing/selftests/pidfd/Makefile
>>> @@ -1,8 +1,7 @@
>>> # SPDX-License-Identifier: GPL-2.0-only
>>> -CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall
>>> +CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
>>
>> Instead, it would look like this, which now mostly matches selftests/mm/Makefile,
>> which is also helpful, because eventually this can be factored into a common
>> piece for all selftests:
>>
>> CFLAGS += -g -isystem $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -pthread -Wall
>
> KHDR_INCLUDES is "make headers" location under the root directory. So what
> happens when you add TOOLS_INCLUDES to it.
>
> Does "make kselftest-all" work as it is supposed to? If it and all tests
> can build then I am all for it.
>
>>
>> I apologize for just now noticing this! And these kselftests shouldn't require
>> so much fussing around, I know. But once we get this just right, it will work
>> well and last a long time. :)
>>
>
>
> On the contrary if we don't discuss/fuss and get this right, we have to
> deal with changes like adding local defines and adhoc approaches in
> individual tests - that is one reason we made the "make headers"
> as a dependency. I would like to solve the problem of proliferation
> of local defines and even system calls in some cases.
Yes, let's work through this.
>
> For now I am going let this patch go through as it is important to
> add tests.
Sure, we can iterate on it as necessary.
>
> My goals are simple:
>
> - no local defines unless it is abslulutely necessary
Agreed.
> - be able to build tests that add coverage for new kernel
> api and features before we release the kernel.
> - make it easier for CIs to build and run tests
Up to a point! And that point should be somewhere *before*
it inflicts extra pain and annoyance on the kernel developers.
Let's not let CI drive this all by itself. CI is not writing
these tests, and furthermore it can adapt a little bit if necessary.
But even more to the point, avoiding "make headers" simplifies
CI, rather than complicating it.
> - continue to have tests works for kernel developres
> e.g: mm developers build tests in mm directory. They
> don't see the issues that crop up in CIs or running
> the entire kselftest default run like CIs do.
>
> Adhoc changes break some use-cases.
>
> thanks,
> -- Shuah
>
>
thanks,
--
John Hubbard
^ permalink raw reply
* Re: [PATCH v4 3/4] selftests: pidfd: add pidfd.h UAPI wrapper
From: Lorenzo Stoakes @ 2024-10-18 6:49 UTC (permalink / raw)
To: John Hubbard
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel,
Oliver Sang
In-Reply-To: <7df771b9-bfd6-465e-b0ba-12d2aab13ec6@nvidia.com>
On Thu, Oct 17, 2024 at 02:45:43PM -0700, John Hubbard wrote:
> On 10/17/24 2:05 PM, Lorenzo Stoakes wrote:
> > Conflicts can arise between system fcntl.h and linux/fcntl.h, imported by
> > the linux/pidfd.h UAPI header.
> >
> > Work around this by adding a wrapper for linux/pidfd.h to
> > tools/include/ which sets the linux/fcntl.h header guard ahead of
> > importing the pidfd.h header file.
> >
> > Adjust the pidfd selftests Makefile to reference this include directory and
> > put it at a higher precidence than any make header installed headers to
> > ensure the wrapper is preferred.
>
> ...but we are not actually using the installed headers, now. And we intend
> to continue avoiding them. So the ordering shouldn't matter. More below:
>
> >
> > This way we can directly import the UAPI header file without issue, use the
> > latest system header file without having to duplicate anything.
> >
> > Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> > tools/include/linux/pidfd.h | 14 ++++++++++++++
> > tools/testing/selftests/pidfd/Makefile | 3 +--
> > 2 files changed, 15 insertions(+), 2 deletions(-)
> > create mode 100644 tools/include/linux/pidfd.h
> >
> > diff --git a/tools/include/linux/pidfd.h b/tools/include/linux/pidfd.h
> > new file mode 100644
> > index 000000000000..113c8023072d
> > --- /dev/null
> > +++ b/tools/include/linux/pidfd.h
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef _TOOLS_LINUX_PIDFD_H
> > +#define _TOOLS_LINUX_PIDFD_H
> > +
> > +/*
> > + * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so
> > + * work around this by setting the header guard.
> > + */
> > +#define _LINUX_FCNTL_H
> > +#include "../../../include/uapi/linux/pidfd.h"
> > +#undef _LINUX_FCNTL_H
>
> Oh shoot, I think you, Shuah and I were referring to different uapi locations,
> the whole time. And so the basic approach is different after all.
>
> Your include path above actually refers to:
>
> $(top_srcdir)/include/uapi/linux/fcntl.h
>
> ...but what I was intending was to copy a snapshot of that file (or a
> snapshot from the one generated by "make headers"), to here:
>
> $(top_srcdir)/tools/include/uapi/linux/fcntl.h
Yeah my first version of this used the uapi one but I thought doing that
might conflict with snapshotting? Also it'd mean you'd absolutely have to
have the $(TOOLS_INCLUDES) earlier in the include priority list and better
maybe to special case in this instance.
>
> ...and then use $(TOOLS_INCLUDES), which is already in selftests/lib.mk,
> for that reason: to be available to all of the kselftests:
>
> TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapi
>
> The reasoning for this directory is further explained here:
>
> tools/include/uapi/README
>
> (And I see that selftests/proc has started using $(TOOLS_INCLUDES), that's
> progress.)
>
> And now, it's possible to change fcntl.h in place, instead of using a wrapper.
> Although either way seems OK to me. (I'm sort of ignoring the details of
> the actual header file conflict itself, for now.)
The fcntl.h and linux/fcntl.h conflict is apparently a rather well-known
horror show. It's a difficult one to resolve as the UAPI pidfd.h header
needs O_xxx defines but we also need to include this header in kernel code.
An #ifdef __KERNEL__ block might be a solution here but fixing that is out
of scope for these changes.
>
>
> > +
> > +#endif /* _TOOLS_LINUX_PIDFD_H */
> > diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
> > index d731e3e76d5b..f5038c9dae14 100644
> > --- a/tools/testing/selftests/pidfd/Makefile
> > +++ b/tools/testing/selftests/pidfd/Makefile
> > @@ -1,8 +1,7 @@
> > # SPDX-License-Identifier: GPL-2.0-only
> > -CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall
> > +CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
>
> Instead, it would look like this, which now mostly matches selftests/mm/Makefile,
> which is also helpful, because eventually this can be factored into a common
> piece for all selftests:
>
> CFLAGS += -g -isystem $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -pthread -Wall
>
> I apologize for just now noticing this! And these kselftests shouldn't require
> so much fussing around, I know. But once we get this just right, it will work
> well and last a long time. :)
Yeah I know, but this won't work due to the header conflict, I was doing
this previously.
Also doing it this way means that uapi snapshot doesn't override the usr/
one if you have that, which I guess you want?
>
> thanks,
> --
> John Hubbard
^ permalink raw reply
* Re: [PATCH 0/4] implement lightweight guard pages
From: Vlastimil Babka @ 2024-10-18 16:10 UTC (permalink / raw)
To: Lorenzo Stoakes, Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Paul E . McKenney, Jann Horn, David Hildenbrand, linux-mm,
linux-kernel, Muchun Song, Richard Henderson, Ivan Kokshaysky,
Matt Turner, Thomas Bogendoerfer, James E . J . Bottomley,
Helge Deller, Chris Zankel, Max Filippov, Arnd Bergmann,
linux-alpha, linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, Linux API
In-Reply-To: <cover.1729196871.git.lorenzo.stoakes@oracle.com>
+CC linux-api (also should on future revisions)
On 10/17/24 22:42, Lorenzo Stoakes wrote:
> Userland library functions such as allocators and threading implementations
> often require regions of memory to act as 'guard pages' - mappings which,
> when accessed, result in a fatal signal being sent to the accessing
> process.
>
> The current means by which these are implemented is via a PROT_NONE mmap()
> mapping, which provides the required semantics however incur an overhead of
> a VMA for each such region.
>
> With a great many processes and threads, this can rapidly add up and incur
> a significant memory penalty. It also has the added problem of preventing
> merges that might otherwise be permitted.
>
> This series takes a different approach - an idea suggested by Vlasimil
> Babka (and before him David Hildenbrand and Jann Horn - perhaps more - the
> provenance becomes a little tricky to ascertain after this - please forgive
> any omissions!) - rather than locating the guard pages at the VMA layer,
> instead placing them in page tables mapping the required ranges.
>
> Early testing of the prototype version of this code suggests a 5 times
> speed up in memory mapping invocations (in conjunction with use of
> process_madvise()) and a 13% reduction in VMAs on an entirely idle android
> system and unoptimised code.
>
> We expect with optimisation and a loaded system with a larger number of
> guard pages this could significantly increase, but in any case these
> numbers are encouraging.
>
> This way, rather than having separate VMAs specifying which parts of a
> range are guard pages, instead we have a VMA spanning the entire range of
> memory a user is permitted to access and including ranges which are to be
> 'guarded'.
>
> After mapping this, a user can specify which parts of the range should
> result in a fatal signal when accessed.
>
> By restricting the ability to specify guard pages to memory mapped by
> existing VMAs, we can rely on the mappings being torn down when the
> mappings are ultimately unmapped and everything works simply as if the
> memory were not faulted in, from the point of view of the containing VMAs.
>
> This mechanism in effect poisons memory ranges similar to hardware memory
> poisoning, only it is an entirely software-controlled form of poisoning.
>
> Any poisoned region of memory is also able to 'unpoisoned', that is, to
> have its poison markers removed.
>
> The mechanism is implemented via madvise() behaviour - MADV_GUARD_POISON
> which simply poisons ranges - and MADV_GUARD_UNPOISON - which clears this
> poisoning.
>
> Poisoning can be performed across multiple VMAs and any existing mappings
> will be cleared, that is zapped, before installing the poisoned page table
> mappings.
>
> There is no concept of 'nested' poisoning, multiple attempts to poison a
> range will, after the first poisoning, have no effect.
>
> Importantly, unpoisoning of poisoned ranges has no effect on non-poisoned
> memory, so a user can safely unpoison a range of memory and clear only
> poison page table mappings leaving the rest intact.
>
> The actual mechanism by which the page table entries are specified makes
> use of existing logic - PTE markers, which are used for the userfaultfd
> UFFDIO_POISON mechanism.
>
> Unfortunately PTE_MARKER_POISONED is not suited for the guard page
> mechanism as it results in VM_FAULT_HWPOISON semantics in the fault
> handler, so we add our own specific PTE_MARKER_GUARD and adapt existing
> logic to handle it.
>
> We also extend the generic page walk mechanism to allow for installation of
> PTEs (carefully restricted to memory management logic only to prevent
> unwanted abuse).
>
> We ensure that zapping performed by, for instance, MADV_DONTNEED, does not
> remove guard poison markers, nor does forking (except when VM_WIPEONFORK is
> specified for a VMA which implies a total removal of memory
> characteristics).
>
> It's important to note that the guard page implementation is emphatically
> NOT a security feature, so a user can remove the poisoning if they wish. We
> simply implement it in such a way as to provide the least surprising
> behaviour.
>
> An extensive set of self-tests are provided which ensure behaviour is as
> expected and additionally self-documents expected behaviour of poisoned
> ranges.
>
> Suggested-by: Vlastimil Babka <vbabka@suze.cz>
Please fix the domain typo (also in patch 3 :)
Thanks for implementing this,
Vlastimil
> Suggested-by: Jann Horn <jannh@google.com>
> Suggested-by: David Hildenbrand <david@redhat.com>
>
> v1
> * Un-RFC'd as appears no major objections to approach but rather debate on
> implementation.
> * Fixed issue with arches which need mmu_context.h and
> tlbfush.h. header imports in pagewalker logic to be able to use
> update_mmu_cache() as reported by the kernel test bot.
> * Added comments in page walker logic to clarify who can use
> ops->install_pte and why as well as adding a check_ops_valid() helper
> function, as suggested by Christoph.
> * Pass false in full parameter in pte_clear_not_present_full() as suggested
> by Jann.
> * Stopped erroneously requiring a write lock for the poison operation as
> suggested by Jann and Suren.
> * Moved anon_vma_prepare() to the start of madvise_guard_poison() to be
> consistent with how this is used elsewhere in the kernel as suggested by
> Jann.
> * Avoid returning -EAGAIN if we are raced on page faults, just keep looping
> and duck out if a fatal signal is pending or a conditional reschedule is
> needed, as suggested by Jann.
> * Avoid needlessly splitting huge PUDs and PMDs by specifying
> ACTION_CONTINUE, as suggested by Jann.
>
> RFC
> https://lore.kernel.org/all/cover.1727440966.git.lorenzo.stoakes@oracle.com/
>
> Lorenzo Stoakes (4):
> mm: pagewalk: add the ability to install PTEs
> mm: add PTE_MARKER_GUARD PTE marker
> mm: madvise: implement lightweight guard page mechanism
> selftests/mm: add self tests for guard page feature
>
> arch/alpha/include/uapi/asm/mman.h | 3 +
> arch/mips/include/uapi/asm/mman.h | 3 +
> arch/parisc/include/uapi/asm/mman.h | 3 +
> arch/xtensa/include/uapi/asm/mman.h | 3 +
> include/linux/mm_inline.h | 2 +-
> include/linux/pagewalk.h | 18 +-
> include/linux/swapops.h | 26 +-
> include/uapi/asm-generic/mman-common.h | 3 +
> mm/hugetlb.c | 3 +
> mm/internal.h | 6 +
> mm/madvise.c | 168 ++++
> mm/memory.c | 18 +-
> mm/mprotect.c | 3 +-
> mm/mseal.c | 1 +
> mm/pagewalk.c | 200 ++--
> tools/testing/selftests/mm/.gitignore | 1 +
> tools/testing/selftests/mm/Makefile | 1 +
> tools/testing/selftests/mm/guard-pages.c | 1168 ++++++++++++++++++++++
> 18 files changed, 1564 insertions(+), 66 deletions(-)
> create mode 100644 tools/testing/selftests/mm/guard-pages.c
>
> --
> 2.46.2
^ permalink raw reply
* Re: [PATCH 0/4] implement lightweight guard pages
From: Lorenzo Stoakes @ 2024-10-18 16:17 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, Linux API
In-Reply-To: <e4985328-dbfa-4c47-9cf9-12aa89ba9798@suse.cz>
On Fri, Oct 18, 2024 at 06:10:37PM +0200, Vlastimil Babka wrote:
> +CC linux-api (also should on future revisions)
>
They're cc'd :) assuming Linux API <linux-api@vger.kernel.org> is correct
right?
> On 10/17/24 22:42, Lorenzo Stoakes wrote:
> > Userland library functions such as allocators and threading implementations
> > often require regions of memory to act as 'guard pages' - mappings which,
> > when accessed, result in a fatal signal being sent to the accessing
> > process.
> >
> > The current means by which these are implemented is via a PROT_NONE mmap()
> > mapping, which provides the required semantics however incur an overhead of
> > a VMA for each such region.
> >
> > With a great many processes and threads, this can rapidly add up and incur
> > a significant memory penalty. It also has the added problem of preventing
> > merges that might otherwise be permitted.
> >
> > This series takes a different approach - an idea suggested by Vlasimil
> > Babka (and before him David Hildenbrand and Jann Horn - perhaps more - the
> > provenance becomes a little tricky to ascertain after this - please forgive
> > any omissions!) - rather than locating the guard pages at the VMA layer,
> > instead placing them in page tables mapping the required ranges.
> >
> > Early testing of the prototype version of this code suggests a 5 times
> > speed up in memory mapping invocations (in conjunction with use of
> > process_madvise()) and a 13% reduction in VMAs on an entirely idle android
> > system and unoptimised code.
> >
> > We expect with optimisation and a loaded system with a larger number of
> > guard pages this could significantly increase, but in any case these
> > numbers are encouraging.
> >
> > This way, rather than having separate VMAs specifying which parts of a
> > range are guard pages, instead we have a VMA spanning the entire range of
> > memory a user is permitted to access and including ranges which are to be
> > 'guarded'.
> >
> > After mapping this, a user can specify which parts of the range should
> > result in a fatal signal when accessed.
> >
> > By restricting the ability to specify guard pages to memory mapped by
> > existing VMAs, we can rely on the mappings being torn down when the
> > mappings are ultimately unmapped and everything works simply as if the
> > memory were not faulted in, from the point of view of the containing VMAs.
> >
> > This mechanism in effect poisons memory ranges similar to hardware memory
> > poisoning, only it is an entirely software-controlled form of poisoning.
> >
> > Any poisoned region of memory is also able to 'unpoisoned', that is, to
> > have its poison markers removed.
> >
> > The mechanism is implemented via madvise() behaviour - MADV_GUARD_POISON
> > which simply poisons ranges - and MADV_GUARD_UNPOISON - which clears this
> > poisoning.
> >
> > Poisoning can be performed across multiple VMAs and any existing mappings
> > will be cleared, that is zapped, before installing the poisoned page table
> > mappings.
> >
> > There is no concept of 'nested' poisoning, multiple attempts to poison a
> > range will, after the first poisoning, have no effect.
> >
> > Importantly, unpoisoning of poisoned ranges has no effect on non-poisoned
> > memory, so a user can safely unpoison a range of memory and clear only
> > poison page table mappings leaving the rest intact.
> >
> > The actual mechanism by which the page table entries are specified makes
> > use of existing logic - PTE markers, which are used for the userfaultfd
> > UFFDIO_POISON mechanism.
> >
> > Unfortunately PTE_MARKER_POISONED is not suited for the guard page
> > mechanism as it results in VM_FAULT_HWPOISON semantics in the fault
> > handler, so we add our own specific PTE_MARKER_GUARD and adapt existing
> > logic to handle it.
> >
> > We also extend the generic page walk mechanism to allow for installation of
> > PTEs (carefully restricted to memory management logic only to prevent
> > unwanted abuse).
> >
> > We ensure that zapping performed by, for instance, MADV_DONTNEED, does not
> > remove guard poison markers, nor does forking (except when VM_WIPEONFORK is
> > specified for a VMA which implies a total removal of memory
> > characteristics).
> >
> > It's important to note that the guard page implementation is emphatically
> > NOT a security feature, so a user can remove the poisoning if they wish. We
> > simply implement it in such a way as to provide the least surprising
> > behaviour.
> >
> > An extensive set of self-tests are provided which ensure behaviour is as
> > expected and additionally self-documents expected behaviour of poisoned
> > ranges.
> >
> > Suggested-by: Vlastimil Babka <vbabka@suze.cz>
>
> Please fix the domain typo (also in patch 3 :)
>
Damnnn it! I can't believe I left that in. Sorry about that! Will fix on
respin.
Hopefully not to suse.cs ;)
> Thanks for implementing this,
> Vlastimil
Thanks!
>
> > Suggested-by: Jann Horn <jannh@google.com>
> > Suggested-by: David Hildenbrand <david@redhat.com>
> >
> > v1
> > * Un-RFC'd as appears no major objections to approach but rather debate on
> > implementation.
> > * Fixed issue with arches which need mmu_context.h and
> > tlbfush.h. header imports in pagewalker logic to be able to use
> > update_mmu_cache() as reported by the kernel test bot.
> > * Added comments in page walker logic to clarify who can use
> > ops->install_pte and why as well as adding a check_ops_valid() helper
> > function, as suggested by Christoph.
> > * Pass false in full parameter in pte_clear_not_present_full() as suggested
> > by Jann.
> > * Stopped erroneously requiring a write lock for the poison operation as
> > suggested by Jann and Suren.
> > * Moved anon_vma_prepare() to the start of madvise_guard_poison() to be
> > consistent with how this is used elsewhere in the kernel as suggested by
> > Jann.
> > * Avoid returning -EAGAIN if we are raced on page faults, just keep looping
> > and duck out if a fatal signal is pending or a conditional reschedule is
> > needed, as suggested by Jann.
> > * Avoid needlessly splitting huge PUDs and PMDs by specifying
> > ACTION_CONTINUE, as suggested by Jann.
> >
> > RFC
> > https://lore.kernel.org/all/cover.1727440966.git.lorenzo.stoakes@oracle.com/
> >
> > Lorenzo Stoakes (4):
> > mm: pagewalk: add the ability to install PTEs
> > mm: add PTE_MARKER_GUARD PTE marker
> > mm: madvise: implement lightweight guard page mechanism
> > selftests/mm: add self tests for guard page feature
> >
> > arch/alpha/include/uapi/asm/mman.h | 3 +
> > arch/mips/include/uapi/asm/mman.h | 3 +
> > arch/parisc/include/uapi/asm/mman.h | 3 +
> > arch/xtensa/include/uapi/asm/mman.h | 3 +
> > include/linux/mm_inline.h | 2 +-
> > include/linux/pagewalk.h | 18 +-
> > include/linux/swapops.h | 26 +-
> > include/uapi/asm-generic/mman-common.h | 3 +
> > mm/hugetlb.c | 3 +
> > mm/internal.h | 6 +
> > mm/madvise.c | 168 ++++
> > mm/memory.c | 18 +-
> > mm/mprotect.c | 3 +-
> > mm/mseal.c | 1 +
> > mm/pagewalk.c | 200 ++--
> > tools/testing/selftests/mm/.gitignore | 1 +
> > tools/testing/selftests/mm/Makefile | 1 +
> > tools/testing/selftests/mm/guard-pages.c | 1168 ++++++++++++++++++++++
> > 18 files changed, 1564 insertions(+), 66 deletions(-)
> > create mode 100644 tools/testing/selftests/mm/guard-pages.c
> >
> > --
> > 2.46.2
>
^ permalink raw reply
* Re: [PATCH 0/4] implement lightweight guard pages
From: Lorenzo Stoakes @ 2024-10-18 21:30 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, Linux API
In-Reply-To: <4b5382fd-e553-4fef-a7c7-a2d3130b948d@lucifer.local>
On Fri, Oct 18, 2024 at 05:17:56PM +0100, Lorenzo Stoakes wrote:
> On Fri, Oct 18, 2024 at 06:10:37PM +0200, Vlastimil Babka wrote:
> > +CC linux-api (also should on future revisions)
> >
>
> They're cc'd :) assuming Linux API <linux-api@vger.kernel.org> is correct
> right?
As discussed on IRC, no I was being a little slow here and hadn't realised
you'd added them, apologies!
Will add them on future respins, sorry guys :)
>
> > On 10/17/24 22:42, Lorenzo Stoakes wrote:
> > > Userland library functions such as allocators and threading implementations
> > > often require regions of memory to act as 'guard pages' - mappings which,
> > > when accessed, result in a fatal signal being sent to the accessing
> > > process.
> > >
> > > The current means by which these are implemented is via a PROT_NONE mmap()
> > > mapping, which provides the required semantics however incur an overhead of
> > > a VMA for each such region.
> > >
> > > With a great many processes and threads, this can rapidly add up and incur
> > > a significant memory penalty. It also has the added problem of preventing
> > > merges that might otherwise be permitted.
> > >
> > > This series takes a different approach - an idea suggested by Vlasimil
> > > Babka (and before him David Hildenbrand and Jann Horn - perhaps more - the
> > > provenance becomes a little tricky to ascertain after this - please forgive
> > > any omissions!) - rather than locating the guard pages at the VMA layer,
> > > instead placing them in page tables mapping the required ranges.
> > >
> > > Early testing of the prototype version of this code suggests a 5 times
> > > speed up in memory mapping invocations (in conjunction with use of
> > > process_madvise()) and a 13% reduction in VMAs on an entirely idle android
> > > system and unoptimised code.
> > >
> > > We expect with optimisation and a loaded system with a larger number of
> > > guard pages this could significantly increase, but in any case these
> > > numbers are encouraging.
> > >
> > > This way, rather than having separate VMAs specifying which parts of a
> > > range are guard pages, instead we have a VMA spanning the entire range of
> > > memory a user is permitted to access and including ranges which are to be
> > > 'guarded'.
> > >
> > > After mapping this, a user can specify which parts of the range should
> > > result in a fatal signal when accessed.
> > >
> > > By restricting the ability to specify guard pages to memory mapped by
> > > existing VMAs, we can rely on the mappings being torn down when the
> > > mappings are ultimately unmapped and everything works simply as if the
> > > memory were not faulted in, from the point of view of the containing VMAs.
> > >
> > > This mechanism in effect poisons memory ranges similar to hardware memory
> > > poisoning, only it is an entirely software-controlled form of poisoning.
> > >
> > > Any poisoned region of memory is also able to 'unpoisoned', that is, to
> > > have its poison markers removed.
> > >
> > > The mechanism is implemented via madvise() behaviour - MADV_GUARD_POISON
> > > which simply poisons ranges - and MADV_GUARD_UNPOISON - which clears this
> > > poisoning.
> > >
> > > Poisoning can be performed across multiple VMAs and any existing mappings
> > > will be cleared, that is zapped, before installing the poisoned page table
> > > mappings.
> > >
> > > There is no concept of 'nested' poisoning, multiple attempts to poison a
> > > range will, after the first poisoning, have no effect.
> > >
> > > Importantly, unpoisoning of poisoned ranges has no effect on non-poisoned
> > > memory, so a user can safely unpoison a range of memory and clear only
> > > poison page table mappings leaving the rest intact.
> > >
> > > The actual mechanism by which the page table entries are specified makes
> > > use of existing logic - PTE markers, which are used for the userfaultfd
> > > UFFDIO_POISON mechanism.
> > >
> > > Unfortunately PTE_MARKER_POISONED is not suited for the guard page
> > > mechanism as it results in VM_FAULT_HWPOISON semantics in the fault
> > > handler, so we add our own specific PTE_MARKER_GUARD and adapt existing
> > > logic to handle it.
> > >
> > > We also extend the generic page walk mechanism to allow for installation of
> > > PTEs (carefully restricted to memory management logic only to prevent
> > > unwanted abuse).
> > >
> > > We ensure that zapping performed by, for instance, MADV_DONTNEED, does not
> > > remove guard poison markers, nor does forking (except when VM_WIPEONFORK is
> > > specified for a VMA which implies a total removal of memory
> > > characteristics).
> > >
> > > It's important to note that the guard page implementation is emphatically
> > > NOT a security feature, so a user can remove the poisoning if they wish. We
> > > simply implement it in such a way as to provide the least surprising
> > > behaviour.
> > >
> > > An extensive set of self-tests are provided which ensure behaviour is as
> > > expected and additionally self-documents expected behaviour of poisoned
> > > ranges.
> > >
> > > Suggested-by: Vlastimil Babka <vbabka@suze.cz>
> >
> > Please fix the domain typo (also in patch 3 :)
> >
>
> Damnnn it! I can't believe I left that in. Sorry about that! Will fix on
> respin.
>
> Hopefully not to suse.cs ;)
>
> > Thanks for implementing this,
> > Vlastimil
>
> Thanks!
>
> >
> > > Suggested-by: Jann Horn <jannh@google.com>
> > > Suggested-by: David Hildenbrand <david@redhat.com>
> > >
> > > v1
> > > * Un-RFC'd as appears no major objections to approach but rather debate on
> > > implementation.
> > > * Fixed issue with arches which need mmu_context.h and
> > > tlbfush.h. header imports in pagewalker logic to be able to use
> > > update_mmu_cache() as reported by the kernel test bot.
> > > * Added comments in page walker logic to clarify who can use
> > > ops->install_pte and why as well as adding a check_ops_valid() helper
> > > function, as suggested by Christoph.
> > > * Pass false in full parameter in pte_clear_not_present_full() as suggested
> > > by Jann.
> > > * Stopped erroneously requiring a write lock for the poison operation as
> > > suggested by Jann and Suren.
> > > * Moved anon_vma_prepare() to the start of madvise_guard_poison() to be
> > > consistent with how this is used elsewhere in the kernel as suggested by
> > > Jann.
> > > * Avoid returning -EAGAIN if we are raced on page faults, just keep looping
> > > and duck out if a fatal signal is pending or a conditional reschedule is
> > > needed, as suggested by Jann.
> > > * Avoid needlessly splitting huge PUDs and PMDs by specifying
> > > ACTION_CONTINUE, as suggested by Jann.
> > >
> > > RFC
> > > https://lore.kernel.org/all/cover.1727440966.git.lorenzo.stoakes@oracle.com/
> > >
> > > Lorenzo Stoakes (4):
> > > mm: pagewalk: add the ability to install PTEs
> > > mm: add PTE_MARKER_GUARD PTE marker
> > > mm: madvise: implement lightweight guard page mechanism
> > > selftests/mm: add self tests for guard page feature
> > >
> > > arch/alpha/include/uapi/asm/mman.h | 3 +
> > > arch/mips/include/uapi/asm/mman.h | 3 +
> > > arch/parisc/include/uapi/asm/mman.h | 3 +
> > > arch/xtensa/include/uapi/asm/mman.h | 3 +
> > > include/linux/mm_inline.h | 2 +-
> > > include/linux/pagewalk.h | 18 +-
> > > include/linux/swapops.h | 26 +-
> > > include/uapi/asm-generic/mman-common.h | 3 +
> > > mm/hugetlb.c | 3 +
> > > mm/internal.h | 6 +
> > > mm/madvise.c | 168 ++++
> > > mm/memory.c | 18 +-
> > > mm/mprotect.c | 3 +-
> > > mm/mseal.c | 1 +
> > > mm/pagewalk.c | 200 ++--
> > > tools/testing/selftests/mm/.gitignore | 1 +
> > > tools/testing/selftests/mm/Makefile | 1 +
> > > tools/testing/selftests/mm/guard-pages.c | 1168 ++++++++++++++++++++++
> > > 18 files changed, 1564 insertions(+), 66 deletions(-)
> > > create mode 100644 tools/testing/selftests/mm/guard-pages.c
> > >
> > > --
> > > 2.46.2
> >
^ permalink raw reply
* Re: [PATCH v4 3/4] selftests: pidfd: add pidfd.h UAPI wrapper
From: John Hubbard @ 2024-10-18 23:55 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel,
Oliver Sang
In-Reply-To: <bc07dba0-90c7-4926-ae38-71c3c3e17e16@lucifer.local>
On 10/17/24 11:49 PM, Lorenzo Stoakes wrote:
> On Thu, Oct 17, 2024 at 02:45:43PM -0700, John Hubbard wrote:
>> On 10/17/24 2:05 PM, Lorenzo Stoakes wrote:
...
>> Your include path above actually refers to:
>>
>> $(top_srcdir)/include/uapi/linux/fcntl.h
>>
>> ...but what I was intending was to copy a snapshot of that file (or a
>> snapshot from the one generated by "make headers"), to here:
>>
>> $(top_srcdir)/tools/include/uapi/linux/fcntl.h
>
> Yeah my first version of this used the uapi one but I thought doing that
> might conflict with snapshotting? Also it'd mean you'd absolutely have to
> have the $(TOOLS_INCLUDES) earlier in the include priority list and better
> maybe to special case in this instance.
Actually, I think the goal is to just stop using KHDR_INCLUDES (./usr/include)
entirely!
More below...
>
>>
>> ...and then use $(TOOLS_INCLUDES), which is already in selftests/lib.mk,
>> for that reason: to be available to all of the kselftests:
>>
>> TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapi
>>
>> The reasoning for this directory is further explained here:
>>
>> tools/include/uapi/README
>>
>> (And I see that selftests/proc has started using $(TOOLS_INCLUDES), that's
>> progress.)
>>
>> And now, it's possible to change fcntl.h in place, instead of using a wrapper.
>> Although either way seems OK to me. (I'm sort of ignoring the details of
>> the actual header file conflict itself, for now.)
>
> The fcntl.h and linux/fcntl.h conflict is apparently a rather well-known
> horror show. It's a difficult one to resolve as the UAPI pidfd.h header
> needs O_xxx defines but we also need to include this header in kernel code.
>
> An #ifdef __KERNEL__ block might be a solution here but fixing that is out
> of scope for these changes.
Certainly out of scope! Your patch already avoids the biggest issue: it no
longer requires "make headers", in order to build it. That's fine for now. Sorry
to put you into the middle of a pre-existing kselftests debate.
And the #ifdef __KERNEL__ sounds like a potential solution, or at least a
building block for one. I need to take a closer look at this particular header
file mess, the fcntl.h situation is new to me.
>>> +#endif /* _TOOLS_LINUX_PIDFD_H */
>>> diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
>>> index d731e3e76d5b..f5038c9dae14 100644
>>> --- a/tools/testing/selftests/pidfd/Makefile
>>> +++ b/tools/testing/selftests/pidfd/Makefile
>>> @@ -1,8 +1,7 @@
>>> # SPDX-License-Identifier: GPL-2.0-only
>>> -CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall
>>> +CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
>>
>> Instead, it would look like this, which now mostly matches selftests/mm/Makefile,
>> which is also helpful, because eventually this can be factored into a common
>> piece for all selftests:
>>
>> CFLAGS += -g -isystem $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -pthread -Wall
>>
>> I apologize for just now noticing this! And these kselftests shouldn't require
>> so much fussing around, I know. But once we get this just right, it will work
>> well and last a long time. :)
>
> Yeah I know, but this won't work due to the header conflict, I was doing
> this previously.
>
> Also doing it this way means that uapi snapshot doesn't override the usr/
> one if you have that, which I guess you want?
Actually, given that we want (or should want, so I claim) to build without first
running "make headers", and given that "make headers" populates ./usr/include/,
which in turn is what $(KHDR_INCLUDES) points to, this means that eventually we
should end up with approximately:
CFLAGS += -g -isystem $(TOOLS_INCLUDES) -pthread -Wall
And I just checked, today's selftests/mm builds just fine, with a similar
diff applied, so I'm not totally crazy:
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index 02e1204971b0..b004a8edcba5 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -33,7 +33,7 @@ endif
# LDLIBS.
MAKEFLAGS += --no-builtin-rules
-CFLAGS = -Wall -I $(top_srcdir) $(EXTRA_CFLAGS) $(KHDR_INCLUDES) $(TOOLS_INCLUDES)
+CFLAGS = -Wall -I $(top_srcdir) $(EXTRA_CFLAGS) $(TOOLS_INCLUDES)
LDLIBS = -lrt -lpthread -lm
TEST_GEN_FILES = cow
thanks,
--
John Hubbard
^ permalink raw reply related
* [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: Lorenzo Stoakes @ 2024-10-20 16:20 UTC (permalink / raw)
To: Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Vlastimil Babka, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <cover.1729440856.git.lorenzo.stoakes@oracle.com>
Add a new PTE marker that results in any access causing the accessing
process to segfault.
This is preferable to PTE_MARKER_POISONED, which results in the same
handling as hardware poisoned memory, and is thus undesirable for cases
where we simply wish to 'soft' poison a range.
This is in preparation for implementing the ability to specify guard pages
at the page table level, i.e. ranges that, when accessed, should cause
process termination.
Additionally, rename zap_drop_file_uffd_wp() to zap_drop_markers() - the
function checks the ZAP_FLAG_DROP_MARKER flag so naming it for this single
purpose was simply incorrect.
We then reuse the same logic to determine whether a zap should clear a
guard entry - this should only be performed on teardown and never on
MADV_DONTNEED or the like.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
include/linux/mm_inline.h | 2 +-
include/linux/swapops.h | 26 ++++++++++++++++++++++++--
mm/hugetlb.c | 3 +++
mm/memory.c | 18 +++++++++++++++---
4 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
index 355cf46a01a6..1b6a917fffa4 100644
--- a/include/linux/mm_inline.h
+++ b/include/linux/mm_inline.h
@@ -544,7 +544,7 @@ static inline pte_marker copy_pte_marker(
{
pte_marker srcm = pte_marker_get(entry);
/* Always copy error entries. */
- pte_marker dstm = srcm & PTE_MARKER_POISONED;
+ pte_marker dstm = srcm & (PTE_MARKER_POISONED | PTE_MARKER_GUARD);
/* Only copy PTE markers if UFFD register matches. */
if ((srcm & PTE_MARKER_UFFD_WP) && userfaultfd_wp(dst_vma))
diff --git a/include/linux/swapops.h b/include/linux/swapops.h
index cb468e418ea1..4d0606df0791 100644
--- a/include/linux/swapops.h
+++ b/include/linux/swapops.h
@@ -426,9 +426,15 @@ typedef unsigned long pte_marker;
* "Poisoned" here is meant in the very general sense of "future accesses are
* invalid", instead of referring very specifically to hardware memory errors.
* This marker is meant to represent any of various different causes of this.
+ *
+ * Note that, when encountered by the faulting logic, PTEs with this marker will
+ * result in VM_FAULT_HWPOISON and thus regardless trigger hardware memory error
+ * logic.
*/
#define PTE_MARKER_POISONED BIT(1)
-#define PTE_MARKER_MASK (BIT(2) - 1)
+/* Indicates that, on fault, this PTE will case a SIGSEGV signal to be sent. */
+#define PTE_MARKER_GUARD BIT(2)
+#define PTE_MARKER_MASK (BIT(3) - 1)
static inline swp_entry_t make_pte_marker_entry(pte_marker marker)
{
@@ -461,9 +467,25 @@ static inline swp_entry_t make_poisoned_swp_entry(void)
}
static inline int is_poisoned_swp_entry(swp_entry_t entry)
+{
+ /*
+ * We treat guard pages as poisoned too as these have the same semantics
+ * as poisoned ranges, only with different fault handling.
+ */
+ return is_pte_marker_entry(entry) &&
+ (pte_marker_get(entry) &
+ (PTE_MARKER_POISONED | PTE_MARKER_GUARD));
+}
+
+static inline swp_entry_t make_guard_swp_entry(void)
+{
+ return make_pte_marker_entry(PTE_MARKER_GUARD);
+}
+
+static inline int is_guard_swp_entry(swp_entry_t entry)
{
return is_pte_marker_entry(entry) &&
- (pte_marker_get(entry) & PTE_MARKER_POISONED);
+ (pte_marker_get(entry) & PTE_MARKER_GUARD);
}
/*
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 906294ac85dc..50e3f6ed73ac 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -6353,6 +6353,9 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
ret = VM_FAULT_HWPOISON_LARGE |
VM_FAULT_SET_HINDEX(hstate_index(h));
goto out_mutex;
+ } else if (marker & PTE_MARKER_GUARD) {
+ ret = VM_FAULT_SIGSEGV;
+ goto out_mutex;
}
}
diff --git a/mm/memory.c b/mm/memory.c
index 0f614523b9f4..551455cd453f 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1455,7 +1455,7 @@ static inline bool should_zap_folio(struct zap_details *details,
return !folio_test_anon(folio);
}
-static inline bool zap_drop_file_uffd_wp(struct zap_details *details)
+static inline bool zap_drop_markers(struct zap_details *details)
{
if (!details)
return false;
@@ -1476,7 +1476,7 @@ zap_install_uffd_wp_if_needed(struct vm_area_struct *vma,
if (vma_is_anonymous(vma))
return;
- if (zap_drop_file_uffd_wp(details))
+ if (zap_drop_markers(details))
return;
for (;;) {
@@ -1671,7 +1671,15 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
* drop the marker if explicitly requested.
*/
if (!vma_is_anonymous(vma) &&
- !zap_drop_file_uffd_wp(details))
+ !zap_drop_markers(details))
+ continue;
+ } else if (is_guard_swp_entry(entry)) {
+ /*
+ * Ordinary zapping should not remove guard PTE
+ * markers. Only do so if we should remove PTE markers
+ * in general.
+ */
+ if (!zap_drop_markers(details))
continue;
} else if (is_hwpoison_entry(entry) ||
is_poisoned_swp_entry(entry)) {
@@ -4003,6 +4011,10 @@ static vm_fault_t handle_pte_marker(struct vm_fault *vmf)
if (marker & PTE_MARKER_POISONED)
return VM_FAULT_HWPOISON;
+ /* Hitting a guard page is always a fatal condition. */
+ if (marker & PTE_MARKER_GUARD)
+ return VM_FAULT_SIGSEGV;
+
if (pte_marker_entry_uffd_wp(entry))
return pte_marker_handle_uffd_wp(vmf);
--
2.47.0
^ permalink raw reply related
* [PATCH v2 0/5] implement lightweight guard pages
From: Lorenzo Stoakes @ 2024-10-20 16:20 UTC (permalink / raw)
To: Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Vlastimil Babka, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
Userland library functions such as allocators and threading implementations
often require regions of memory to act as 'guard pages' - mappings which,
when accessed, result in a fatal signal being sent to the accessing
process.
The current means by which these are implemented is via a PROT_NONE mmap()
mapping, which provides the required semantics however incur an overhead of
a VMA for each such region.
With a great many processes and threads, this can rapidly add up and incur
a significant memory penalty. It also has the added problem of preventing
merges that might otherwise be permitted.
This series takes a different approach - an idea suggested by Vlasimil
Babka (and before him David Hildenbrand and Jann Horn - perhaps more - the
provenance becomes a little tricky to ascertain after this - please forgive
any omissions!) - rather than locating the guard pages at the VMA layer,
instead placing them in page tables mapping the required ranges.
Early testing of the prototype version of this code suggests a 5 times
speed up in memory mapping invocations (in conjunction with use of
process_madvise()) and a 13% reduction in VMAs on an entirely idle android
system and unoptimised code.
We expect with optimisation and a loaded system with a larger number of
guard pages this could significantly increase, but in any case these
numbers are encouraging.
This way, rather than having separate VMAs specifying which parts of a
range are guard pages, instead we have a VMA spanning the entire range of
memory a user is permitted to access and including ranges which are to be
'guarded'.
After mapping this, a user can specify which parts of the range should
result in a fatal signal when accessed.
By restricting the ability to specify guard pages to memory mapped by
existing VMAs, we can rely on the mappings being torn down when the
mappings are ultimately unmapped and everything works simply as if the
memory were not faulted in, from the point of view of the containing VMAs.
This mechanism in effect poisons memory ranges similar to hardware memory
poisoning, only it is an entirely software-controlled form of poisoning.
Any poisoned region of memory is also able to 'unpoisoned', that is, to
have its poison markers removed.
The mechanism is implemented via madvise() behaviour - MADV_GUARD_POISON
which simply poisons ranges - and MADV_GUARD_UNPOISON - which clears this
poisoning.
Poisoning can be performed across multiple VMAs and any existing mappings
will be cleared, that is zapped, before installing the poisoned page table
mappings.
There is no concept of 'nested' poisoning, multiple attempts to poison a
range will, after the first poisoning, have no effect.
Importantly, unpoisoning of poisoned ranges has no effect on non-poisoned
memory, so a user can safely unpoison a range of memory and clear only
poison page table mappings leaving the rest intact.
The actual mechanism by which the page table entries are specified makes
use of existing logic - PTE markers, which are used for the userfaultfd
UFFDIO_POISON mechanism.
Unfortunately PTE_MARKER_POISONED is not suited for the guard page
mechanism as it results in VM_FAULT_HWPOISON semantics in the fault
handler, so we add our own specific PTE_MARKER_GUARD and adapt existing
logic to handle it.
We also extend the generic page walk mechanism to allow for installation of
PTEs (carefully restricted to memory management logic only to prevent
unwanted abuse).
We ensure that zapping performed by, for instance, MADV_DONTNEED, does not
remove guard poison markers, nor does forking (except when VM_WIPEONFORK is
specified for a VMA which implies a total removal of memory
characteristics).
It's important to note that the guard page implementation is emphatically
NOT a security feature, so a user can remove the poisoning if they wish. We
simply implement it in such a way as to provide the least surprising
behaviour.
An extensive set of self-tests are provided which ensure behaviour is as
expected and additionally self-documents expected behaviour of poisoned
ranges.
Suggested-by: Vlastimil Babka <vbabka@suse.cz>
Suggested-by: Jann Horn <jannh@google.com>
Suggested-by: David Hildenbrand <david@redhat.com>
v2
* The macros in kselftest_harness.h seem to be broken - __EXPECT() is
terminated by '} while (0); OPTIONAL_HANDLER(_assert)' meaning it is not
safe in single line if / else or for /which blocks, however working
around this results in checkpatch producing invalid warnings, as reported
by Shuah.
* Fixing these macros is out of scope for this series, so compromise and
instead rewrite test blocks so as to use multiple lines by separating out
a decl in most cases. This has the side effect of, for the most part,
making things more readable.
* Heavily document the use of the volatile keyword - we can't avoid
checkpatch complaining about this, so we explain it, as reported by
Shuah.
* Updated commit message to highlight that we skip tests we lack
permissions for, as reported by Shuah.
* Replaced a perror() with ksft_exit_fail_perror(), as reported by Shuah.
* Added user friendly messages to cases where tests are skipped due to lack
of permissions, as reported by Shuah.
* Update the tool header to include the new MADV_GUARD_POISON/UNPOISON
defines and directly include asm-generic/mman.h to get the
platform-neutral versions to ensure we import them.
* Finally fixed Vlastimil's email address in Suggested-by tags from suze to
suse, as reported by Vlastimil.
* Added linux-api to cc list, as reported by Vlastimil.
v1
* Un-RFC'd as appears no major objections to approach but rather debate on
implementation.
* Fixed issue with arches which need mmu_context.h and
tlbfush.h. header imports in pagewalker logic to be able to use
update_mmu_cache() as reported by the kernel test bot.
* Added comments in page walker logic to clarify who can use
ops->install_pte and why as well as adding a check_ops_valid() helper
function, as suggested by Christoph.
* Pass false in full parameter in pte_clear_not_present_full() as suggested
by Jann.
* Stopped erroneously requiring a write lock for the poison operation as
suggested by Jann and Suren.
* Moved anon_vma_prepare() to the start of madvise_guard_poison() to be
consistent with how this is used elsewhere in the kernel as suggested by
Jann.
* Avoid returning -EAGAIN if we are raced on page faults, just keep looping
and duck out if a fatal signal is pending or a conditional reschedule is
needed, as suggested by Jann.
* Avoid needlessly splitting huge PUDs and PMDs by specifying
ACTION_CONTINUE, as suggested by Jann.
https://lore.kernel.org/all/cover.1729196871.git.lorenzo.stoakes@oracle.com/
RFC
https://lore.kernel.org/all/cover.1727440966.git.lorenzo.stoakes@oracle.com/
Lorenzo Stoakes (5):
mm: pagewalk: add the ability to install PTEs
mm: add PTE_MARKER_GUARD PTE marker
mm: madvise: implement lightweight guard page mechanism
tools: testing: update tools UAPI header for mman-common.h
selftests/mm: add self tests for guard page feature
arch/alpha/include/uapi/asm/mman.h | 3 +
arch/mips/include/uapi/asm/mman.h | 3 +
arch/parisc/include/uapi/asm/mman.h | 3 +
arch/xtensa/include/uapi/asm/mman.h | 3 +
include/linux/mm_inline.h | 2 +-
include/linux/pagewalk.h | 18 +-
include/linux/swapops.h | 26 +-
include/uapi/asm-generic/mman-common.h | 3 +
mm/hugetlb.c | 3 +
mm/internal.h | 6 +
mm/madvise.c | 168 +++
mm/memory.c | 18 +-
mm/mprotect.c | 3 +-
mm/mseal.c | 1 +
mm/pagewalk.c | 200 ++-
tools/include/uapi/asm-generic/mman-common.h | 3 +
tools/testing/selftests/mm/.gitignore | 1 +
tools/testing/selftests/mm/Makefile | 1 +
tools/testing/selftests/mm/guard-pages.c | 1228 ++++++++++++++++++
19 files changed, 1627 insertions(+), 66 deletions(-)
create mode 100644 tools/testing/selftests/mm/guard-pages.c
--
2.47.0
^ permalink raw reply
* [PATCH v2 5/5] selftests/mm: add self tests for guard page feature
From: Lorenzo Stoakes @ 2024-10-20 16:20 UTC (permalink / raw)
To: Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Vlastimil Babka, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <cover.1729440856.git.lorenzo.stoakes@oracle.com>
Utilise the kselftest harmness to implement tests for the guard page
implementation.
We start by implement basic tests asserting that guard pages can be
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.
If any test is unable to be run due to lack of permissions, that test is
skipped.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
tools/testing/selftests/mm/.gitignore | 1 +
tools/testing/selftests/mm/Makefile | 1 +
tools/testing/selftests/mm/guard-pages.c | 1228 ++++++++++++++++++++++
3 files changed, 1230 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..f67d2700d44a
--- /dev/null
+++ b/tools/testing/selftests/mm/guard-pages.c
@@ -0,0 +1,1228 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#define _GNU_SOURCE
+#include "../kselftest_harness.h"
+#include <asm-generic/mman.h> /* Force the import of the tools version. */
+#include <assert.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/userfaultfd.h>
+#include <setjmp.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>
+
+/*
+ * Ignore the checkpatch warning, as per the C99 standard, section 7.14.1.1:
+ *
+ * "If the signal occurs other than as the result of calling the abort or raise
+ * function, the behavior is undefined if the signal handler refers to any
+ * object with static storage duration other than by assigning a value to an
+ * object declared as volatile sig_atomic_t"
+ */
+static volatile sig_atomic_t signal_jump_set;
+static sigjmp_buf signal_jmp_buf;
+
+/*
+ * Ignore the checkpatch warning, we must read from x but don't want to do
+ * anything with it in order to trigger a read page fault. We therefore must use
+ * volatile to stop the compiler from optimising this away.
+ */
+#define FORCE_READ(x) (*(volatile typeof(x) *)x)
+
+static int userfaultfd(int flags)
+{
+ return syscall(SYS_userfaultfd, flags);
+}
+
+static void handle_fatal(int c)
+{
+ if (!signal_jump_set)
+ return;
+
+ siglongjmp(signal_jmp_buf, c);
+}
+
+static int pidfd_open(pid_t pid, unsigned int flags)
+{
+ return syscall(SYS_pidfd_open, pid, flags);
+}
+
+/*
+ * Enable our signal catcher and try to read/write the specified buffer. The
+ * return value indicates whether the read/write succeeds without a fatal
+ * signal.
+ */
+static bool try_access_buf(char *ptr, bool write)
+{
+ bool failed;
+
+ /* Tell signal handler to jump back here on fatal signal. */
+ signal_jump_set = true;
+ /* If a fatal signal arose, we will jump back here and failed is set. */
+ failed = sigsetjmp(signal_jmp_buf, 0) != 0;
+
+ if (!failed) {
+ if (write)
+ *ptr = 'x';
+ else
+ FORCE_READ(ptr);
+ }
+
+ signal_jump_set = false;
+ return !failed;
+}
+
+/* Try and read from a buffer, return true if no fatal signal. */
+static bool try_read_buf(char *ptr)
+{
+ return try_access_buf(ptr, false);
+}
+
+/* Try and write to a buffer, return true if no fatal signal. */
+static bool try_write_buf(char *ptr)
+{
+ return try_access_buf(ptr, true);
+}
+
+/*
+ * Try and BOTH read from AND write to a buffer, return true if BOTH operations
+ * succeed.
+ */
+static bool try_read_write_buf(char *ptr)
+{
+ return try_read_buf(ptr) && try_write_buf(ptr);
+}
+
+FIXTURE(guard_pages)
+{
+ unsigned long page_size;
+};
+
+FIXTURE_SETUP(guard_pages)
+{
+ struct sigaction act = {
+ .sa_handler = &handle_fatal,
+ .sa_flags = SA_NODEFER,
+ };
+
+ sigemptyset(&act.sa_mask);
+ if (sigaction(SIGSEGV, &act, NULL))
+ ksft_exit_fail_perror("sigaction");
+
+ self->page_size = (unsigned long)sysconf(_SC_PAGESIZE);
+};
+
+FIXTURE_TEARDOWN(guard_pages)
+{
+ struct sigaction act = {
+ .sa_handler = SIG_DFL,
+ .sa_flags = SA_NODEFER,
+ };
+
+ sigemptyset(&act.sa_mask);
+ sigaction(SIGSEGV, &act, NULL);
+}
+
+TEST_F(guard_pages, basic)
+{
+ const unsigned long NUM_PAGES = 10;
+ const unsigned long page_size = self->page_size;
+ char *ptr;
+ int i;
+
+ ptr = mmap(NULL, NUM_PAGES * page_size, PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANON, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* Trivially assert we can touch the first page. */
+ ASSERT_TRUE(try_read_write_buf(ptr));
+
+ ASSERT_EQ(madvise(ptr, page_size, MADV_GUARD_POISON), 0);
+
+ /* Establish that 1st page SIGSEGV's. */
+ ASSERT_FALSE(try_read_write_buf(ptr));
+
+ /* Ensure we can touch everything else.*/
+ for (i = 1; i < NUM_PAGES; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_TRUE(try_read_write_buf(curr));
+ }
+
+ /* Establish a guard page at the end of the mapping. */
+ ASSERT_EQ(madvise(&ptr[(NUM_PAGES - 1) * page_size], page_size,
+ MADV_GUARD_POISON), 0);
+
+ /* Check that both guard pages result in SIGSEGV. */
+ ASSERT_FALSE(try_read_write_buf(ptr));
+ ASSERT_FALSE(try_read_write_buf(&ptr[(NUM_PAGES - 1) * page_size]));
+
+ /* Unpoison the first. */
+ ASSERT_FALSE(madvise(ptr, page_size, MADV_GUARD_UNPOISON));
+
+ /* Make sure we can touch it. */
+ ASSERT_TRUE(try_read_write_buf(ptr));
+
+ /* Unpoison the last. */
+ ASSERT_FALSE(madvise(&ptr[(NUM_PAGES - 1) * page_size], page_size,
+ MADV_GUARD_UNPOISON));
+
+ /* Make sure we can touch it. */
+ ASSERT_TRUE(try_read_write_buf(&ptr[(NUM_PAGES - 1) * page_size]));
+
+ /*
+ * Test setting a _range_ of pages, namely the first 3. The first of
+ * these be faulted in, so this also tests that we can poison backed
+ * pages.
+ */
+ ASSERT_EQ(madvise(ptr, 3 * page_size, MADV_GUARD_POISON), 0);
+
+ /* Make sure they are all poisoned. */
+ for (i = 0; i < 3; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_FALSE(try_read_write_buf(curr));
+ }
+
+ /* Make sure the rest are not. */
+ for (i = 3; i < NUM_PAGES; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_TRUE(try_read_write_buf(curr));
+ }
+
+ /* Unpoison them. */
+ ASSERT_EQ(madvise(ptr, NUM_PAGES * page_size, MADV_GUARD_UNPOISON), 0);
+
+ /* Now make sure we can touch everything. */
+ for (i = 0; i < NUM_PAGES; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_TRUE(try_read_write_buf(curr));
+ }
+
+ /* Now unpoison everything, make sure we don't remove existing entries */
+ ASSERT_EQ(madvise(ptr, NUM_PAGES * page_size, MADV_GUARD_UNPOISON), 0);
+
+ for (i = 0; i < NUM_PAGES * page_size; i += page_size) {
+ char chr = ptr[i];
+
+ ASSERT_EQ(chr, 'x');
+ }
+
+ ASSERT_EQ(munmap(ptr, NUM_PAGES * page_size), 0);
+}
+
+/* Assert that operations applied across multiple VMAs work as expected. */
+TEST_F(guard_pages, multi_vma)
+{
+ const unsigned long page_size = self->page_size;
+ char *ptr_region, *ptr, *ptr1, *ptr2, *ptr3;
+ int i;
+
+ /* Reserve a 100 page region over which we can install VMAs. */
+ ptr_region = mmap(NULL, 100 * page_size, PROT_NONE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr_region, MAP_FAILED);
+
+ /* Place a VMA of 10 pages size at the start of the region. */
+ ptr1 = mmap(ptr_region, 10 * page_size, PROT_READ | PROT_WRITE,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr1, MAP_FAILED);
+
+ /* Place a VMA of 5 pages size 50 pages into the region. */
+ ptr2 = mmap(&ptr_region[50 * page_size], 5 * page_size,
+ PROT_READ | PROT_WRITE,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr2, MAP_FAILED);
+
+ /* Place a VMA of 20 pages size at the end of the region. */
+ ptr3 = mmap(&ptr_region[80 * page_size], 20 * page_size,
+ PROT_READ | PROT_WRITE,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr3, MAP_FAILED);
+
+ /* Unmap gaps. */
+ ASSERT_EQ(munmap(&ptr_region[10 * page_size], 40 * page_size), 0);
+ ASSERT_EQ(munmap(&ptr_region[55 * page_size], 25 * page_size), 0);
+
+ /*
+ * We end up with VMAs like this:
+ *
+ * 0 10 .. 50 55 .. 80 100
+ * [---] [---] [---]
+ */
+
+ /* Now poison the whole range and make sure all VMAs are poisoned. */
+
+ /*
+ * madvise() is certifiable and lets you perform operations over gaps,
+ * everything works, but it indicates an error and errno is set to
+ * -ENOMEM. Also if anything runs out of memory it is set to
+ * -ENOMEM. You are meant to guess which is which.
+ */
+ ASSERT_EQ(madvise(ptr_region, 100 * page_size, MADV_GUARD_POISON), -1);
+ ASSERT_EQ(errno, ENOMEM);
+
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr1[i * page_size];
+
+ ASSERT_FALSE(try_read_write_buf(curr));
+ }
+
+ for (i = 0; i < 5; i++) {
+ char *curr = &ptr2[i * page_size];
+
+ ASSERT_FALSE(try_read_write_buf(curr));
+ }
+
+ for (i = 0; i < 20; i++) {
+ char *curr = &ptr3[i * page_size];
+
+ ASSERT_FALSE(try_read_write_buf(curr));
+ }
+
+ /* Now unpoison the range and assert the opposite. */
+
+ ASSERT_EQ(madvise(ptr_region, 100 * page_size, MADV_GUARD_UNPOISON), -1);
+ ASSERT_EQ(errno, ENOMEM);
+
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr1[i * page_size];
+
+ ASSERT_TRUE(try_read_write_buf(curr));
+ }
+
+ for (i = 0; i < 5; i++) {
+ char *curr = &ptr2[i * page_size];
+
+ ASSERT_TRUE(try_read_write_buf(curr));
+ }
+
+ for (i = 0; i < 20; i++) {
+ char *curr = &ptr3[i * page_size];
+
+ ASSERT_TRUE(try_read_write_buf(curr));
+ }
+
+ /* Now map incompatible VMAs in the gaps. */
+ ptr = mmap(&ptr_region[10 * page_size], 40 * page_size,
+ PROT_READ | PROT_WRITE | PROT_EXEC,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+ ptr = mmap(&ptr_region[55 * page_size], 25 * page_size,
+ PROT_READ | PROT_WRITE | PROT_EXEC,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /*
+ * We end up with VMAs like this:
+ *
+ * 0 10 .. 50 55 .. 80 100
+ * [---][xxxx][---][xxxx][---]
+ *
+ * Where 'x' signifies VMAs that cannot be merged with those adjacent to
+ * them.
+ */
+
+ /* Multiple VMAs adjacent to one another should result in no error. */
+ ASSERT_EQ(madvise(ptr_region, 100 * page_size, MADV_GUARD_POISON), 0);
+ for (i = 0; i < 100; i++) {
+ char *curr = &ptr_region[i * page_size];
+
+ ASSERT_FALSE(try_read_write_buf(curr));
+ }
+ ASSERT_EQ(madvise(ptr_region, 100 * page_size, MADV_GUARD_UNPOISON), 0);
+ for (i = 0; i < 100; i++) {
+ char *curr = &ptr_region[i * page_size];
+
+ ASSERT_TRUE(try_read_write_buf(curr));
+ }
+
+ /* Cleanup. */
+ ASSERT_EQ(munmap(ptr_region, 100 * page_size), 0);
+}
+
+/*
+ * Assert that batched operations performed using process_madvise() work as
+ * expected.
+ */
+TEST_F(guard_pages, process_madvise)
+{
+ const unsigned long page_size = self->page_size;
+ pid_t pid = getpid();
+ int pidfd = pidfd_open(pid, 0);
+ char *ptr_region, *ptr1, *ptr2, *ptr3;
+ ssize_t count;
+ struct iovec vec[6];
+
+ ASSERT_NE(pidfd, -1);
+
+ /* Reserve region to map over. */
+ ptr_region = mmap(NULL, 100 * page_size, PROT_NONE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr_region, MAP_FAILED);
+
+ /* 10 pages offset 1 page into reserve region. */
+ ptr1 = mmap(&ptr_region[page_size], 10 * page_size,
+ PROT_READ | PROT_WRITE,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr1, MAP_FAILED);
+ /* We want poison markers at start/end of each VMA. */
+ vec[0].iov_base = ptr1;
+ vec[0].iov_len = page_size;
+ vec[1].iov_base = &ptr1[9 * page_size];
+ vec[1].iov_len = page_size;
+
+ /* 5 pages offset 50 pages into reserve region. */
+ ptr2 = mmap(&ptr_region[50 * page_size], 5 * page_size,
+ PROT_READ | PROT_WRITE,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr2, MAP_FAILED);
+ vec[2].iov_base = ptr2;
+ vec[2].iov_len = page_size;
+ vec[3].iov_base = &ptr2[4 * page_size];
+ vec[3].iov_len = page_size;
+
+ /* 20 pages offset 79 pages into reserve region. */
+ ptr3 = mmap(&ptr_region[79 * page_size], 20 * page_size,
+ PROT_READ | PROT_WRITE,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr3, MAP_FAILED);
+ vec[4].iov_base = ptr3;
+ vec[4].iov_len = page_size;
+ vec[5].iov_base = &ptr3[19 * page_size];
+ vec[5].iov_len = page_size;
+
+ /* Free surrounding VMAs. */
+ ASSERT_EQ(munmap(ptr_region, page_size), 0);
+ ASSERT_EQ(munmap(&ptr_region[11 * page_size], 39 * page_size), 0);
+ ASSERT_EQ(munmap(&ptr_region[55 * page_size], 24 * page_size), 0);
+ ASSERT_EQ(munmap(&ptr_region[99 * page_size], page_size), 0);
+
+ /* Now poison in one step. */
+ count = process_madvise(pidfd, vec, 6, MADV_GUARD_POISON, 0);
+
+ /* OK we don't have permission to do this, skip. */
+ if (count == -1 && errno == EPERM)
+ ksft_exit_skip("No process_madvise() permissions, try running as root.\n");
+
+ /* Returns the number of bytes advised. */
+ ASSERT_EQ(count, 6 * page_size);
+
+ /* Now make sure the poisoning was applied. */
+
+ ASSERT_FALSE(try_read_write_buf(ptr1));
+ ASSERT_FALSE(try_read_write_buf(&ptr1[9 * page_size]));
+
+ ASSERT_FALSE(try_read_write_buf(ptr2));
+ ASSERT_FALSE(try_read_write_buf(&ptr2[4 * page_size]));
+
+ ASSERT_FALSE(try_read_write_buf(ptr3));
+ ASSERT_FALSE(try_read_write_buf(&ptr3[19 * page_size]));
+
+ /* Now do the same with unpoison... */
+ count = process_madvise(pidfd, vec, 6, MADV_GUARD_UNPOISON, 0);
+
+ /* ...and everything should now succeed. */
+
+ ASSERT_TRUE(try_read_write_buf(ptr1));
+ ASSERT_TRUE(try_read_write_buf(&ptr1[9 * page_size]));
+
+ ASSERT_TRUE(try_read_write_buf(ptr2));
+ ASSERT_TRUE(try_read_write_buf(&ptr2[4 * page_size]));
+
+ ASSERT_TRUE(try_read_write_buf(ptr3));
+ ASSERT_TRUE(try_read_write_buf(&ptr3[19 * page_size]));
+
+ /* Cleanup. */
+ ASSERT_EQ(munmap(ptr1, 10 * page_size), 0);
+ ASSERT_EQ(munmap(ptr2, 5 * page_size), 0);
+ ASSERT_EQ(munmap(ptr3, 20 * page_size), 0);
+ close(pidfd);
+}
+
+/* Assert that unmapping ranges does not leave poison behind. */
+TEST_F(guard_pages, munmap)
+{
+ const unsigned long page_size = self->page_size;
+ char *ptr, *ptr_new1, *ptr_new2;
+
+ ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* Poison first and last pages. */
+ ASSERT_EQ(madvise(ptr, page_size, MADV_GUARD_POISON), 0);
+ ASSERT_EQ(madvise(&ptr[9 * page_size], page_size, MADV_GUARD_POISON), 0);
+
+ /* Assert that they are poisoned. */
+ ASSERT_FALSE(try_read_write_buf(ptr));
+ ASSERT_FALSE(try_read_write_buf(&ptr[9 * page_size]));
+
+ /* Unmap them. */
+ ASSERT_EQ(munmap(ptr, page_size), 0);
+ ASSERT_EQ(munmap(&ptr[9 * page_size], page_size), 0);
+
+ /* Map over them.*/
+ ptr_new1 = mmap(ptr, page_size, PROT_READ | PROT_WRITE,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr_new1, MAP_FAILED);
+ ptr_new2 = mmap(&ptr[9 * page_size], page_size, PROT_READ | PROT_WRITE,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr_new2, MAP_FAILED);
+
+ /* Assert that they are now not poisoned. */
+ ASSERT_TRUE(try_read_write_buf(ptr_new1));
+ ASSERT_TRUE(try_read_write_buf(ptr_new2));
+
+ /* Cleanup. */
+ ASSERT_EQ(munmap(ptr, 10 * page_size), 0);
+}
+
+/* Assert that mprotect() operations have no bearing on guard poison markers. */
+TEST_F(guard_pages, mprotect)
+{
+ const unsigned long page_size = self->page_size;
+ char *ptr;
+ int i;
+
+ ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* Poison the middle of the range. */
+ ASSERT_EQ(madvise(&ptr[5 * page_size], 2 * page_size,
+ MADV_GUARD_POISON), 0);
+
+ /* Assert that it is indeed poisoned. */
+ ASSERT_FALSE(try_read_write_buf(&ptr[5 * page_size]));
+ ASSERT_FALSE(try_read_write_buf(&ptr[6 * page_size]));
+
+ /* Now make these pages read-only. */
+ ASSERT_EQ(mprotect(&ptr[5 * page_size], 2 * page_size, PROT_READ), 0);
+
+ /* Make sure the range is still poisoned. */
+ ASSERT_FALSE(try_read_buf(&ptr[5 * page_size]));
+ ASSERT_FALSE(try_read_buf(&ptr[6 * page_size]));
+
+ /* Make sure we can poison again without issue.*/
+ ASSERT_EQ(madvise(&ptr[5 * page_size], 2 * page_size,
+ MADV_GUARD_POISON), 0);
+
+ /* Make sure the range is, yet again, still poisoned. */
+ ASSERT_FALSE(try_read_buf(&ptr[5 * page_size]));
+ ASSERT_FALSE(try_read_buf(&ptr[6 * page_size]));
+
+ /* Now unpoison the whole range. */
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_GUARD_UNPOISON), 0);
+
+ /* Make sure the whole range is readable. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_TRUE(try_read_buf(curr));
+ }
+
+ /* Cleanup. */
+ ASSERT_EQ(munmap(ptr, 10 * page_size), 0);
+}
+
+/* Split and merge VMAs and make sure guard pages still behave. */
+TEST_F(guard_pages, split_merge)
+{
+ const unsigned long page_size = self->page_size;
+ char *ptr, *ptr_new;
+ int i;
+
+ ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* Poison the whole range. */
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_GUARD_POISON), 0);
+
+ /* Make sure the whole range is poisoned. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_FALSE(try_read_write_buf(curr));
+ }
+
+ /* Now unmap some pages in the range so we split. */
+ ASSERT_EQ(munmap(&ptr[2 * page_size], page_size), 0);
+ ASSERT_EQ(munmap(&ptr[5 * page_size], page_size), 0);
+ ASSERT_EQ(munmap(&ptr[8 * page_size], page_size), 0);
+
+ /* Make sure the remaining ranges are poisoned post-split. */
+ for (i = 0; i < 2; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_FALSE(try_read_write_buf(curr));
+ }
+ for (i = 2; i < 5; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_FALSE(try_read_write_buf(curr));
+ }
+ for (i = 6; i < 8; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_FALSE(try_read_write_buf(curr));
+ }
+ for (i = 9; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_FALSE(try_read_write_buf(curr));
+ }
+
+ /* Now map them again - the unmap will have cleared the poison. */
+ ptr_new = mmap(&ptr[2 * page_size], page_size, PROT_READ | PROT_WRITE,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr_new, MAP_FAILED);
+ ptr_new = mmap(&ptr[5 * page_size], page_size, PROT_READ | PROT_WRITE,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr_new, MAP_FAILED);
+ ptr_new = mmap(&ptr[8 * page_size], page_size, PROT_READ | PROT_WRITE,
+ MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr_new, MAP_FAILED);
+
+ /* Now make sure poisoning is as expected. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+ bool result = try_read_write_buf(curr);
+ bool expect_true = i == 2 || i == 5 || i == 8;
+
+ ASSERT_TRUE(expect_true ? result : !result);
+ }
+
+ /* Now poison everything again. */
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_GUARD_POISON), 0);
+
+ /* Make sure the whole range is poisoned. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_FALSE(try_read_write_buf(curr));
+ }
+
+ /* Now split the range into three. */
+ ASSERT_EQ(mprotect(ptr, 3 * page_size, PROT_READ), 0);
+ ASSERT_EQ(mprotect(&ptr[7 * page_size], 3 * page_size, PROT_READ), 0);
+
+ /* Make sure the whole range is poisoned for read. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_FALSE(try_read_buf(curr));
+ }
+
+ /* Now reset protection bits so we merge the whole thing. */
+ ASSERT_EQ(mprotect(ptr, 3 * page_size, PROT_READ | PROT_WRITE), 0);
+ ASSERT_EQ(mprotect(&ptr[7 * page_size], 3 * page_size,
+ PROT_READ | PROT_WRITE), 0);
+
+ /* Make sure the whole range is still poisoned. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_FALSE(try_read_write_buf(curr));
+ }
+
+ /* Split range into 3 again... */
+ ASSERT_EQ(mprotect(ptr, 3 * page_size, PROT_READ), 0);
+ ASSERT_EQ(mprotect(&ptr[7 * page_size], 3 * page_size, PROT_READ), 0);
+
+ /* ...and unpoison the whole range. */
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_GUARD_UNPOISON), 0);
+
+ /* Make sure the whole range is remedied for read. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_TRUE(try_read_buf(curr));
+ }
+
+ /* Merge them again. */
+ ASSERT_EQ(mprotect(ptr, 3 * page_size, PROT_READ | PROT_WRITE), 0);
+ ASSERT_EQ(mprotect(&ptr[7 * page_size], 3 * page_size,
+ PROT_READ | PROT_WRITE), 0);
+
+ /* Now ensure the merged range is remedied for read/write. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_TRUE(try_read_write_buf(curr));
+ }
+
+ /* Cleanup. */
+ ASSERT_EQ(munmap(ptr, 10 * page_size), 0);
+}
+
+/* Assert that MADV_DONTNEED does not remove guard poison markers. */
+TEST_F(guard_pages, dontneed)
+{
+ const unsigned long page_size = self->page_size;
+ char *ptr;
+ int i;
+
+ ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* Back the whole range. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ *curr = 'y';
+ }
+
+ /* Poison every other page. */
+ for (i = 0; i < 10; i += 2) {
+ char *curr = &ptr[i * page_size];
+ int res = madvise(curr, page_size, MADV_GUARD_POISON);
+
+ ASSERT_EQ(res, 0);
+ }
+
+ /* Indicate that we don't need any of the range. */
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_DONTNEED), 0);
+
+ /* Check to ensure poison markers are still in place. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+ bool result = try_read_buf(curr);
+
+ if (i % 2 == 0) {
+ ASSERT_FALSE(result);
+ } else {
+ ASSERT_TRUE(result);
+ /* Make sure we really did get reset to zero page. */
+ ASSERT_EQ(*curr, '\0');
+ }
+
+ /* Now write... */
+ result = try_write_buf(&ptr[i * page_size]);
+
+ /* ...and make sure same result. */
+ ASSERT_TRUE(i % 2 != 0 ? result : !result);
+ }
+
+ /* Cleanup. */
+ ASSERT_EQ(munmap(ptr, 10 * page_size), 0);
+}
+
+/* Assert that mlock()'ed pages work correctly with poison markers. */
+TEST_F(guard_pages, mlock)
+{
+ const unsigned long page_size = self->page_size;
+ char *ptr;
+ int i;
+
+ ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* Populate. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ *curr = 'y';
+ }
+
+ /* Lock. */
+ ASSERT_EQ(mlock(ptr, 10 * page_size), 0);
+
+ /* Now try to poison, should fail with EINVAL. */
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_GUARD_POISON), -1);
+ ASSERT_EQ(errno, EINVAL);
+
+ /* OK unlock. */
+ ASSERT_EQ(munlock(ptr, 10 * page_size), 0);
+
+ /* Poison first half of range, should now succeed. */
+ ASSERT_EQ(madvise(ptr, 5 * page_size, MADV_GUARD_POISON), 0);
+
+ /* Make sure poison works. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+ bool result = try_read_write_buf(curr);
+
+ if (i < 5) {
+ ASSERT_FALSE(result);
+ } else {
+ ASSERT_TRUE(result);
+ ASSERT_EQ(*curr, 'x');
+ }
+ }
+
+ /*
+ * Now lock the latter part of the range. We can't lock the poisoned
+ * pages, as this would result in the pages being populated and the
+ * poisoning would cause this to error out.
+ */
+ ASSERT_EQ(mlock(&ptr[5 * page_size], 5 * page_size), 0);
+
+ /*
+ * Now unpoison, we do not permit mlock()'d ranges to be remedied as it is
+ * a non-destructive operation.
+ */
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_GUARD_UNPOISON), 0);
+
+ /* Now check that everything is remedied. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_TRUE(try_read_write_buf(curr));
+ }
+
+ /* Cleanup. */
+ ASSERT_EQ(munmap(ptr, 10 * page_size), 0);
+}
+
+/*
+ * Assert that moving, extending and shrinking memory via mremap() retains
+ * poison markers where possible.
+ *
+ * - Moving a mapping alone should retain markers as they are.
+ */
+TEST_F(guard_pages, mremap_move)
+{
+ const unsigned long page_size = self->page_size;
+ char *ptr, *ptr_new;
+
+ /* Map 5 pages. */
+ ptr = mmap(NULL, 5 * page_size, PROT_READ | PROT_WRITE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* Place poison markers at both ends of the 5 page span. */
+ ASSERT_EQ(madvise(ptr, page_size, MADV_GUARD_POISON), 0);
+ ASSERT_EQ(madvise(&ptr[4 * page_size], page_size, MADV_GUARD_POISON), 0);
+
+ /* Make sure the poison is in effect. */
+ ASSERT_FALSE(try_read_write_buf(ptr));
+ ASSERT_FALSE(try_read_write_buf(&ptr[4 * page_size]));
+
+ /* Map a new region we will move this range into. Doing this ensures
+ * that we have reserved a range to map into.
+ */
+ ptr_new = mmap(NULL, 5 * page_size, PROT_NONE, MAP_ANON | MAP_PRIVATE,
+ -1, 0);
+ ASSERT_NE(ptr_new, MAP_FAILED);
+
+ ASSERT_EQ(mremap(ptr, 5 * page_size, 5 * page_size,
+ MREMAP_MAYMOVE | MREMAP_FIXED, ptr_new), ptr_new);
+
+ /* Make sure the poison is retained. */
+ ASSERT_FALSE(try_read_write_buf(ptr_new));
+ ASSERT_FALSE(try_read_write_buf(&ptr_new[4 * page_size]));
+
+ /*
+ * Clean up - we only need reference the new pointer as we overwrote the
+ * PROT_NONE range and moved the existing one.
+ */
+ munmap(ptr_new, 5 * page_size);
+}
+
+/*
+ * Assert that moving, extending and shrinking memory via mremap() retains
+ * poison markers where possible.
+ *
+ * - Expanding should retain, only now in different position. The user will have
+ * to unpoison manually to fix up (they'd have to do the same if it were a
+ * PROT_NONE mapping)
+ */
+TEST_F(guard_pages, mremap_expand)
+{
+ const unsigned long page_size = self->page_size;
+ char *ptr, *ptr_new;
+
+ /* Map 10 pages... */
+ ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+ /* ...But unmap the last 5 so we can ensure we can expand into them. */
+ ASSERT_EQ(munmap(&ptr[5 * page_size], 5 * page_size), 0);
+
+ /* Place poison markers at both ends of the 5 page span. */
+ ASSERT_EQ(madvise(ptr, page_size, MADV_GUARD_POISON), 0);
+ ASSERT_EQ(madvise(&ptr[4 * page_size], page_size, MADV_GUARD_POISON), 0);
+
+ /* Make sure the poison is in effect. */
+ ASSERT_FALSE(try_read_write_buf(ptr));
+ ASSERT_FALSE(try_read_write_buf(&ptr[4 * page_size]));
+
+ /* Now expand to 10 pages. */
+ ptr = mremap(ptr, 5 * page_size, 10 * page_size, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* Make sure the poison is retained in its original positions. */
+ ASSERT_FALSE(try_read_write_buf(ptr));
+ ASSERT_FALSE(try_read_write_buf(&ptr[4 * page_size]));
+
+ /* Reserve a region which we can move to and expand into. */
+ ptr_new = mmap(NULL, 20 * page_size, PROT_NONE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr_new, MAP_FAILED);
+
+ /* Now move and expand into it. */
+ ptr = mremap(ptr, 10 * page_size, 20 * page_size,
+ MREMAP_MAYMOVE | MREMAP_FIXED, ptr_new);
+ ASSERT_EQ(ptr, ptr_new);
+
+ /* Again, make sure the poison is retained in its original positions. */
+ ASSERT_FALSE(try_read_write_buf(ptr));
+ ASSERT_FALSE(try_read_write_buf(&ptr[4 * page_size]));
+
+ /*
+ * A real user would have to unpoison, but would reasonably expect all
+ * characteristics of the mapping to be retained, including poison
+ * markers.
+ */
+
+ /* Cleanup. */
+ munmap(ptr, 20 * page_size);
+}
+/*
+ * Assert that moving, extending and shrinking memory via mremap() retains
+ * poison markers where possible.
+ *
+ * - Shrinking will result in markers that are shrunk over being removed. Again,
+ * if the user were using a PROT_NONE mapping they'd have to manually fix this
+ * up also so this is OK.
+ */
+TEST_F(guard_pages, mremap_shrink)
+{
+ const unsigned long page_size = self->page_size;
+ char *ptr;
+ int i;
+
+ /* Map 5 pages. */
+ ptr = mmap(NULL, 5 * page_size, PROT_READ | PROT_WRITE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* Place poison markers at both ends of the 5 page span. */
+ ASSERT_EQ(madvise(ptr, page_size, MADV_GUARD_POISON), 0);
+ ASSERT_EQ(madvise(&ptr[4 * page_size], page_size, MADV_GUARD_POISON), 0);
+
+ /* Make sure the poison is in effect. */
+ ASSERT_FALSE(try_read_write_buf(ptr));
+ ASSERT_FALSE(try_read_write_buf(&ptr[4 * page_size]));
+
+ /* Now shrink to 3 pages. */
+ ptr = mremap(ptr, 5 * page_size, 3 * page_size, MREMAP_MAYMOVE);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* We expect the poison marker at the start to be retained... */
+ ASSERT_FALSE(try_read_write_buf(ptr));
+
+ /* ...But remaining pages will not have poison markers. */
+ for (i = 1; i < 3; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_TRUE(try_read_write_buf(curr));
+ }
+
+ /*
+ * As with expansion, a real user would have to unpoison and fixup. But
+ * you'd have to do similar manual things with PROT_NONE mappings too.
+ */
+
+ /*
+ * If we expand back to the original size, the end marker will, of
+ * course, no longer be present.
+ */
+ ptr = mremap(ptr, 3 * page_size, 5 * page_size, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* Again, we expect the poison marker at the start to be retained... */
+ ASSERT_FALSE(try_read_write_buf(ptr));
+
+ /* ...But remaining pages will not have poison markers. */
+ for (i = 1; i < 5; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_TRUE(try_read_write_buf(curr));
+ }
+
+ /* Cleanup. */
+ munmap(ptr, 5 * page_size);
+}
+
+/*
+ * Assert that forking a process with VMAs that do not have VM_WIPEONFORK set
+ * retain guard pages.
+ */
+TEST_F(guard_pages, fork)
+{
+ const unsigned long page_size = self->page_size;
+ char *ptr;
+ pid_t pid;
+ int i;
+
+ /* Map 10 pages. */
+ ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* Poison the first 5 pages. */
+ ASSERT_EQ(madvise(ptr, 5 * page_size, MADV_GUARD_POISON), 0);
+
+ pid = fork();
+ ASSERT_NE(pid, -1);
+ if (!pid) {
+ /* This is the child process now. */
+
+ /* Assert that the poisoning is in effect. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+ bool result = try_read_write_buf(curr);
+
+ ASSERT_TRUE(i >= 5 ? result : !result);
+ }
+
+ /* Now unpoison the range.*/
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_GUARD_UNPOISON), 0);
+
+ exit(0);
+ }
+
+ /* Parent process. */
+
+ /* Parent simply waits on child. */
+ waitpid(pid, NULL, 0);
+
+ /* Child unpoison does not impact parent page table state. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+ bool result = try_read_write_buf(curr);
+
+ ASSERT_TRUE(i >= 5 ? result : !result);
+ }
+
+ /* Cleanup. */
+ ASSERT_EQ(munmap(ptr, 10 * page_size), 0);
+}
+
+/*
+ * Assert that forking a process with VMAs that do have VM_WIPEONFORK set
+ * behave as expected.
+ */
+TEST_F(guard_pages, fork_wipeonfork)
+{
+ const unsigned long page_size = self->page_size;
+ char *ptr;
+ pid_t pid;
+ int i;
+
+ /* Map 10 pages. */
+ ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* Mark wipe on fork. */
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_WIPEONFORK), 0);
+
+ /* Poison the first 5 pages. */
+ ASSERT_EQ(madvise(ptr, 5 * page_size, MADV_GUARD_POISON), 0);
+
+ pid = fork();
+ ASSERT_NE(pid, -1);
+ if (!pid) {
+ /* This is the child process now. */
+
+ /* Poison will have been wiped. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_TRUE(try_read_write_buf(curr));
+ }
+
+ exit(0);
+ }
+
+ /* Parent process. */
+
+ waitpid(pid, NULL, 0);
+
+ /* Poison should be in effect.*/
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+ bool result = try_read_write_buf(curr);
+
+ ASSERT_TRUE(i >= 5 ? result : !result);
+ }
+
+ /* Cleanup. */
+ ASSERT_EQ(munmap(ptr, 10 * page_size), 0);
+}
+
+/* Ensure that MADV_FREE frees poison entries as expected. */
+TEST_F(guard_pages, lazyfree)
+{
+ const unsigned long page_size = self->page_size;
+ char *ptr;
+ int i;
+
+ /* Map 10 pages. */
+ ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* Poison range. */
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_GUARD_POISON), 0);
+
+ /* Ensure poisoned. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_FALSE(try_read_write_buf(curr));
+ }
+
+ /* Lazyfree range. */
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_FREE), 0);
+
+ /* This should simply clear the poison markers. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_TRUE(try_read_write_buf(curr));
+ }
+
+ /* Cleanup. */
+ ASSERT_EQ(munmap(ptr, 10 * page_size), 0);
+}
+
+/* Ensure that MADV_POPULATE_READ, MADV_POPULATE_WRITE behave as expected. */
+TEST_F(guard_pages, populate)
+{
+ const unsigned long page_size = self->page_size;
+ char *ptr;
+
+ /* Map 10 pages. */
+ ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* Poison range. */
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_GUARD_POISON), 0);
+
+ /* Populate read should error out... */
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_POPULATE_READ), -1);
+ ASSERT_EQ(errno, EFAULT);
+
+ /* ...as should populate write. */
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_POPULATE_WRITE), -1);
+ ASSERT_EQ(errno, EFAULT);
+
+ /* Cleanup. */
+ ASSERT_EQ(munmap(ptr, 10 * page_size), 0);
+}
+
+/* Ensure that MADV_COLD, MADV_PAGEOUT do not remove poison markers. */
+TEST_F(guard_pages, cold_pageout)
+{
+ const unsigned long page_size = self->page_size;
+ char *ptr;
+ int i;
+
+ /* Map 10 pages. */
+ ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* Poison range. */
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_GUARD_POISON), 0);
+
+ /* Ensured poisoned. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_FALSE(try_read_write_buf(curr));
+ }
+
+ /* Now mark cold. This should have no impact on poison markers. */
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_COLD), 0);
+
+ /* Should remain poisoned. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_FALSE(try_read_write_buf(curr));
+ }
+
+ /* OK, now page out. This should equally, have no effect on markers. */
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_PAGEOUT), 0);
+
+ /* Should remain poisoned. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_FALSE(try_read_write_buf(curr));
+ }
+
+ /* Cleanup. */
+ ASSERT_EQ(munmap(ptr, 10 * page_size), 0);
+}
+
+/* Ensure that guard pages do not break userfaultd. */
+TEST_F(guard_pages, uffd)
+{
+ const unsigned long page_size = self->page_size;
+ int uffd;
+ char *ptr;
+ int i;
+ struct uffdio_api api = {
+ .api = UFFD_API,
+ .features = 0,
+ };
+ struct uffdio_register reg;
+ struct uffdio_range range;
+
+ /* Set up uffd. */
+ uffd = userfaultfd(0);
+ if (uffd == -1 && errno == EPERM)
+ ksft_exit_skip("No userfaultfd permissions, try running as root.\n");
+ ASSERT_NE(uffd, -1);
+
+ ASSERT_EQ(ioctl(uffd, UFFDIO_API, &api), 0);
+
+ /* Map 10 pages. */
+ ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* Register the range with uffd. */
+ range.start = (unsigned long)ptr;
+ range.len = 10 * page_size;
+ reg.range = range;
+ reg.mode = UFFDIO_REGISTER_MODE_MISSING;
+ ASSERT_EQ(ioctl(uffd, UFFDIO_REGISTER, ®), 0);
+
+ /* Poison the range. This should not trigger the uffd. */
+ ASSERT_EQ(madvise(ptr, 10 * page_size, MADV_GUARD_POISON), 0);
+
+ /* The poisoning should behave as usual with no uffd intervention. */
+ for (i = 0; i < 10; i++) {
+ char *curr = &ptr[i * page_size];
+
+ ASSERT_FALSE(try_read_write_buf(curr));
+ }
+
+ /* Cleanup. */
+ ASSERT_EQ(ioctl(uffd, UFFDIO_UNREGISTER, &range), 0);
+ close(uffd);
+ ASSERT_EQ(munmap(ptr, 10 * page_size), 0);
+}
+
+TEST_HARNESS_MAIN
--
2.47.0
^ permalink raw reply related
* [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism
From: Lorenzo Stoakes @ 2024-10-20 16:20 UTC (permalink / raw)
To: Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Vlastimil Babka, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <cover.1729440856.git.lorenzo.stoakes@oracle.com>
Implement a new lightweight guard page feature, that is regions of userland
virtual memory that, when accessed, cause a fatal signal to arise.
Currently users must establish PROT_NONE ranges to achieve this.
However this is very costly memory-wise - we need a VMA for each and every
one of these regions AND they become unmergeable with surrounding VMAs.
In addition repeated mmap() calls require repeated kernel context switches
and contention of the mmap lock to install these ranges, potentially also
having to unmap memory if installed over existing ranges.
The lightweight guard approach eliminates the VMA cost altogether - rather
than establishing a PROT_NONE VMA, it operates at the level of page table
entries - poisoning PTEs such that accesses to them cause a fault followed
by a SIGSGEV signal being raised.
This is achieved through the PTE marker mechanism, which a previous commit
in this series extended to permit this to be done, installed via the
generic page walking logic, also extended by a prior commit for this
purpose.
These poison ranges are established with MADV_GUARD_POISON, and if the
range in which they are installed contain any existing mappings, they will
be zapped, i.e. free the range and unmap memory (thus mimicking the
behaviour of MADV_DONTNEED in this respect).
Any existing poison entries will be left untouched. There is no nesting of
poisoned pages.
Poisoned ranges are NOT cleared by MADV_DONTNEED, as this would be rather
unexpected behaviour, but are cleared on process teardown or unmapping of
memory ranges.
Ranges can have the poison property removed by MADV_GUARD_UNPOISON -
'remedying' the poisoning. The ranges over which this is applied, should
they contain non-poison entries, will be untouched, only poison entries
will be cleared.
We permit this operation on anonymous memory only, and only VMAs which are
non-special, non-huge and not mlock()'d (if we permitted this we'd have to
drop locked pages which would be rather counterintuitive).
Suggested-by: Vlastimil Babka <vbabka@suse.cz>
Suggested-by: Jann Horn <jannh@google.com>
Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
arch/alpha/include/uapi/asm/mman.h | 3 +
arch/mips/include/uapi/asm/mman.h | 3 +
arch/parisc/include/uapi/asm/mman.h | 3 +
arch/xtensa/include/uapi/asm/mman.h | 3 +
include/uapi/asm-generic/mman-common.h | 3 +
mm/madvise.c | 168 +++++++++++++++++++++++++
mm/mprotect.c | 3 +-
mm/mseal.c | 1 +
8 files changed, 186 insertions(+), 1 deletion(-)
diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h
index 763929e814e9..71e13f27742d 100644
--- a/arch/alpha/include/uapi/asm/mman.h
+++ b/arch/alpha/include/uapi/asm/mman.h
@@ -78,6 +78,9 @@
#define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
+#define MADV_GUARD_POISON 102 /* fatal signal on access to range */
+#define MADV_GUARD_UNPOISON 103 /* revoke guard poisoning */
+
/* compatibility flags */
#define MAP_FILE 0
diff --git a/arch/mips/include/uapi/asm/mman.h b/arch/mips/include/uapi/asm/mman.h
index 9c48d9a21aa0..1a2222322f77 100644
--- a/arch/mips/include/uapi/asm/mman.h
+++ b/arch/mips/include/uapi/asm/mman.h
@@ -105,6 +105,9 @@
#define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
+#define MADV_GUARD_POISON 102 /* fatal signal on access to range */
+#define MADV_GUARD_UNPOISON 103 /* revoke guard poisoning */
+
/* compatibility flags */
#define MAP_FILE 0
diff --git a/arch/parisc/include/uapi/asm/mman.h b/arch/parisc/include/uapi/asm/mman.h
index 68c44f99bc93..380905522397 100644
--- a/arch/parisc/include/uapi/asm/mman.h
+++ b/arch/parisc/include/uapi/asm/mman.h
@@ -75,6 +75,9 @@
#define MADV_HWPOISON 100 /* poison a page for testing */
#define MADV_SOFT_OFFLINE 101 /* soft offline page for testing */
+#define MADV_GUARD_POISON 102 /* fatal signal on access to range */
+#define MADV_GUARD_UNPOISON 103 /* revoke guard poisoning */
+
/* compatibility flags */
#define MAP_FILE 0
diff --git a/arch/xtensa/include/uapi/asm/mman.h b/arch/xtensa/include/uapi/asm/mman.h
index 1ff0c858544f..e8d5affceb28 100644
--- a/arch/xtensa/include/uapi/asm/mman.h
+++ b/arch/xtensa/include/uapi/asm/mman.h
@@ -113,6 +113,9 @@
#define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
+#define MADV_GUARD_POISON 102 /* fatal signal on access to range */
+#define MADV_GUARD_UNPOISON 103 /* revoke guard poisoning */
+
/* compatibility flags */
#define MAP_FILE 0
diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
index 6ce1f1ceb432..5dfd3d442de4 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -79,6 +79,9 @@
#define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
+#define MADV_GUARD_POISON 102 /* fatal signal on access to range */
+#define MADV_GUARD_UNPOISON 103 /* revoke guard poisoning */
+
/* compatibility flags */
#define MAP_FILE 0
diff --git a/mm/madvise.c b/mm/madvise.c
index e871a72a6c32..7b9a357b84d2 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -60,6 +60,8 @@ static int madvise_need_mmap_write(int behavior)
case MADV_POPULATE_READ:
case MADV_POPULATE_WRITE:
case MADV_COLLAPSE:
+ case MADV_GUARD_POISON:
+ case MADV_GUARD_UNPOISON:
return 0;
default:
/* be safe, default to 1. list exceptions explicitly */
@@ -1017,6 +1019,166 @@ static long madvise_remove(struct vm_area_struct *vma,
return error;
}
+static bool is_valid_guard_vma(struct vm_area_struct *vma, bool allow_locked)
+{
+ vm_flags_t disallowed = VM_SPECIAL | VM_HUGETLB;
+
+ /*
+ * A user could lock after poisoning but that's fine, as they'd not be
+ * able to fault in. The issue arises when we try to zap existing locked
+ * VMAs. We don't want to do that.
+ */
+ if (!allow_locked)
+ disallowed |= VM_LOCKED;
+
+ if (!vma_is_anonymous(vma))
+ return false;
+
+ if ((vma->vm_flags & (VM_MAYWRITE | disallowed)) != VM_MAYWRITE)
+ return false;
+
+ return true;
+}
+
+static bool is_guard_pte_marker(pte_t ptent)
+{
+ return is_pte_marker(ptent) &&
+ is_guard_swp_entry(pte_to_swp_entry(ptent));
+}
+
+static int guard_poison_pud_entry(pud_t *pud, unsigned long addr, unsigned long next,
+ struct mm_walk *walk)
+{
+ pud_t pudval = pudp_get(pud);
+
+ /* Do not split a huge pud - we do nothing with these so just ignore. */
+ if (pud_trans_huge(pudval) || pud_devmap(pudval))
+ walk->action = ACTION_CONTINUE;
+
+ return 0;
+}
+
+static int guard_poison_pmd_entry(pmd_t *pmd, unsigned long addr, unsigned long next,
+ struct mm_walk *walk)
+{
+ pmd_t pmdval = pmdp_get(pmd);
+
+ /* Do not split a huge pmd - we do nothing with these so just ignore. */
+ if (pmd_trans_huge(pmdval) || pmd_devmap(pmdval))
+ walk->action = ACTION_CONTINUE;
+
+ return 0;
+}
+
+static int guard_poison_pte_entry(pte_t *pte, unsigned long addr,
+ unsigned long next, struct mm_walk *walk)
+{
+ pte_t pteval = ptep_get(pte);
+
+ /*
+ * If not a guard marker, simply abort the operation. We return a value
+ * > 0 indicating a non-error abort.
+ */
+ return !is_guard_pte_marker(pteval);
+}
+
+static int guard_poison_install_pte(unsigned long addr, unsigned long next,
+ pte_t *ptep, struct mm_walk *walk)
+{
+ /* Simply install a PTE marker, this causes segfault on access. */
+ *ptep = make_pte_marker(PTE_MARKER_GUARD);
+
+ return 0;
+}
+
+static const struct mm_walk_ops guard_poison_walk_ops = {
+ .pud_entry = guard_poison_pud_entry,
+ .pmd_entry = guard_poison_pmd_entry,
+ .pte_entry = guard_poison_pte_entry,
+ .install_pte = guard_poison_install_pte,
+ .walk_lock = PGWALK_RDLOCK,
+};
+
+static long madvise_guard_poison(struct vm_area_struct *vma,
+ struct vm_area_struct **prev,
+ unsigned long start, unsigned long end)
+{
+ long err;
+
+ *prev = vma;
+ if (!is_valid_guard_vma(vma, /* allow_locked = */false))
+ return -EINVAL;
+
+ /*
+ * If we install poison markers, then the range is no longer
+ * empty from a page table perspective and therefore it's
+ * appropriate to have an anon_vma.
+ *
+ * This ensures that on fork, we copy page tables correctly.
+ */
+ err = anon_vma_prepare(vma);
+ if (err)
+ return err;
+
+ /*
+ * Optimistically try to install the guard poison pages first. If any
+ * non-guard pages are encountered, give up and zap the range before
+ * trying again.
+ */
+ while (true) {
+ /* Returns < 0 on error, == 0 if success, > 0 if zap needed. */
+ err = walk_page_range_mm(vma->vm_mm, start, end,
+ &guard_poison_walk_ops, NULL);
+ if (err <= 0)
+ return err;
+
+ /*
+ * OK some of the range have non-guard pages mapped, zap
+ * them. This leaves existing guard pages in place.
+ */
+ zap_page_range_single(vma, start, end - start, NULL);
+
+ if (fatal_signal_pending(current))
+ return -EINTR;
+ cond_resched();
+ }
+}
+
+static int guard_unpoison_pte_entry(pte_t *pte, unsigned long addr,
+ unsigned long next, struct mm_walk *walk)
+{
+ pte_t ptent = ptep_get(pte);
+
+ if (is_guard_pte_marker(ptent)) {
+ /* Simply clear the PTE marker. */
+ pte_clear_not_present_full(walk->mm, addr, pte, false);
+ update_mmu_cache(walk->vma, addr, pte);
+ }
+
+ return 0;
+}
+
+static const struct mm_walk_ops guard_unpoison_walk_ops = {
+ .pte_entry = guard_unpoison_pte_entry,
+ .walk_lock = PGWALK_RDLOCK,
+};
+
+static long madvise_guard_unpoison(struct vm_area_struct *vma,
+ struct vm_area_struct **prev,
+ unsigned long start, unsigned long end)
+{
+ *prev = vma;
+ /*
+ * We're ok with unpoisoning mlock()'d ranges, as this is a
+ * non-destructive action.
+ */
+ if (!is_valid_guard_vma(vma, /* allow_locked = */true))
+ return -EINVAL;
+
+ return walk_page_range(vma->vm_mm, start, end,
+ &guard_unpoison_walk_ops, NULL);
+}
+
/*
* Apply an madvise behavior to a region of a vma. madvise_update_vma
* will handle splitting a vm area into separate areas, each area with its own
@@ -1098,6 +1260,10 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
break;
case MADV_COLLAPSE:
return madvise_collapse(vma, prev, start, end);
+ case MADV_GUARD_POISON:
+ return madvise_guard_poison(vma, prev, start, end);
+ case MADV_GUARD_UNPOISON:
+ return madvise_guard_unpoison(vma, prev, start, end);
}
anon_name = anon_vma_name(vma);
@@ -1197,6 +1363,8 @@ madvise_behavior_valid(int behavior)
case MADV_DODUMP:
case MADV_WIPEONFORK:
case MADV_KEEPONFORK:
+ case MADV_GUARD_POISON:
+ case MADV_GUARD_UNPOISON:
#ifdef CONFIG_MEMORY_FAILURE
case MADV_SOFT_OFFLINE:
case MADV_HWPOISON:
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 0c5d6d06107d..d0e3ebfadef8 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -236,7 +236,8 @@ static long change_pte_range(struct mmu_gather *tlb,
} else if (is_pte_marker_entry(entry)) {
/*
* Ignore error swap entries unconditionally,
- * because any access should sigbus anyway.
+ * because any access should sigbus/sigsegv
+ * anyway.
*/
if (is_poisoned_swp_entry(entry))
continue;
diff --git a/mm/mseal.c b/mm/mseal.c
index ece977bd21e1..21bf5534bcf5 100644
--- a/mm/mseal.c
+++ b/mm/mseal.c
@@ -30,6 +30,7 @@ static bool is_madv_discard(int behavior)
case MADV_REMOVE:
case MADV_DONTFORK:
case MADV_WIPEONFORK:
+ case MADV_GUARD_POISON:
return true;
}
--
2.47.0
^ permalink raw reply related
* [PATCH v2 1/5] mm: pagewalk: add the ability to install PTEs
From: Lorenzo Stoakes @ 2024-10-20 16:20 UTC (permalink / raw)
To: Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Vlastimil Babka, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <cover.1729440856.git.lorenzo.stoakes@oracle.com>
The existing generic pagewalk logic permits the walking of page tables,
invoking callbacks at individual page table levels via user-provided
mm_walk_ops callbacks.
This is useful for traversing existing page table entries, but precludes
the ability to establish new ones.
Existing mechanism for performing a walk which also installs page table
entries if necessary are heavily duplicated throughout the kernel, each
with semantic differences from one another and largely unavailable for use
elsewhere.
Rather than add yet another implementation, we extend the generic pagewalk
logic to enable the installation of page table entries by adding a new
install_pte() callback in mm_walk_ops. If this is specified, then upon
encountering a missing page table entry, we allocate and install a new one
and continue the traversal.
If a THP huge page is encountered, we make use of existing logic to split
it. Then once we reach the PTE level, we invoke the install_pte() callback
which provides a PTE entry to install. We do not support hugetlb at this
stage.
If this function returns an error, or an allocation fails during the
operation, we abort the operation altogether. It is up to the caller to
deal appropriately with partially populated page table ranges.
If install_pte() is defined, the semantics of pte_entry() change - this
callback is then only invoked if the entry already exists. This is a useful
property, as it allows a caller to handle existing PTEs while installing
new ones where necessary in the specified range.
If install_pte() is not defined, then there is no functional difference to
this patch, so all existing logic will work precisely as it did before.
As we only permit the installation of PTEs where a mapping does not already
exist there is no need for TLB management, however we do invoke
update_mmu_cache() for architectures which require manual maintenance of
mappings for other CPUs.
We explicitly do not allow the existing page walk API to expose this
feature as it is dangerous and intended for internal mm use only. Therefore
we provide a new walk_page_range_mm() function exposed only to
mm/internal.h.
Reviewed-by: Jann Horn <jannh@google.com>
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
include/linux/pagewalk.h | 18 +++-
mm/internal.h | 6 ++
mm/pagewalk.c | 200 ++++++++++++++++++++++++++++-----------
3 files changed, 165 insertions(+), 59 deletions(-)
diff --git a/include/linux/pagewalk.h b/include/linux/pagewalk.h
index f5eb5a32aeed..9700a29f8afb 100644
--- a/include/linux/pagewalk.h
+++ b/include/linux/pagewalk.h
@@ -25,12 +25,15 @@ enum page_walk_lock {
* this handler is required to be able to handle
* pmd_trans_huge() pmds. They may simply choose to
* split_huge_page() instead of handling it explicitly.
- * @pte_entry: if set, called for each PTE (lowest-level) entry,
- * including empty ones
+ * @pte_entry: if set, called for each PTE (lowest-level) entry
+ * including empty ones, except if @install_pte is set.
+ * If @install_pte is set, @pte_entry is called only for
+ * existing PTEs.
* @pte_hole: if set, called for each hole at all levels,
* depth is -1 if not known, 0:PGD, 1:P4D, 2:PUD, 3:PMD.
* Any folded depths (where PTRS_PER_P?D is equal to 1)
- * are skipped.
+ * are skipped. If @install_pte is specified, this will
+ * not trigger for any populated ranges.
* @hugetlb_entry: if set, called for each hugetlb entry. This hook
* function is called with the vma lock held, in order to
* protect against a concurrent freeing of the pte_t* or
@@ -51,6 +54,13 @@ enum page_walk_lock {
* @pre_vma: if set, called before starting walk on a non-null vma.
* @post_vma: if set, called after a walk on a non-null vma, provided
* that @pre_vma and the vma walk succeeded.
+ * @install_pte: if set, missing page table entries are installed and
+ * thus all levels are always walked in the specified
+ * range. This callback is then invoked at the PTE level
+ * (having split any THP pages prior), providing the PTE to
+ * install. If allocations fail, the walk is aborted. This
+ * operation is only available for userland memory. Not
+ * usable for hugetlb ranges.
*
* p?d_entry callbacks are called even if those levels are folded on a
* particular architecture/configuration.
@@ -76,6 +86,8 @@ struct mm_walk_ops {
int (*pre_vma)(unsigned long start, unsigned long end,
struct mm_walk *walk);
void (*post_vma)(struct mm_walk *walk);
+ int (*install_pte)(unsigned long addr, unsigned long next,
+ pte_t *ptep, struct mm_walk *walk);
enum page_walk_lock walk_lock;
};
diff --git a/mm/internal.h b/mm/internal.h
index 508f7802dd2b..fb1fb0c984e4 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -12,6 +12,7 @@
#include <linux/mm.h>
#include <linux/mm_inline.h>
#include <linux/pagemap.h>
+#include <linux/pagewalk.h>
#include <linux/rmap.h>
#include <linux/swap.h>
#include <linux/swapops.h>
@@ -1451,4 +1452,9 @@ static inline void accept_page(struct page *page)
}
#endif /* CONFIG_UNACCEPTED_MEMORY */
+/* pagewalk.c */
+int walk_page_range_mm(struct mm_struct *mm, unsigned long start,
+ unsigned long end, const struct mm_walk_ops *ops,
+ void *private);
+
#endif /* __MM_INTERNAL_H */
diff --git a/mm/pagewalk.c b/mm/pagewalk.c
index 5f9f01532e67..261cd5f2de38 100644
--- a/mm/pagewalk.c
+++ b/mm/pagewalk.c
@@ -3,9 +3,14 @@
#include <linux/highmem.h>
#include <linux/sched.h>
#include <linux/hugetlb.h>
+#include <linux/mmu_context.h>
#include <linux/swap.h>
#include <linux/swapops.h>
+#include <asm/tlbflush.h>
+
+#include "internal.h"
+
/*
* We want to know the real level where a entry is located ignoring any
* folding of levels which may be happening. For example if p4d is folded then
@@ -29,9 +34,23 @@ static int walk_pte_range_inner(pte_t *pte, unsigned long addr,
int err = 0;
for (;;) {
- err = ops->pte_entry(pte, addr, addr + PAGE_SIZE, walk);
- if (err)
- break;
+ if (ops->install_pte && pte_none(ptep_get(pte))) {
+ pte_t new_pte;
+
+ err = ops->install_pte(addr, addr + PAGE_SIZE, &new_pte,
+ walk);
+ if (err)
+ break;
+
+ set_pte_at(walk->mm, addr, pte, new_pte);
+ /* Non-present before, so for arches that need it. */
+ if (!WARN_ON_ONCE(walk->no_vma))
+ update_mmu_cache(walk->vma, addr, pte);
+ } else {
+ err = ops->pte_entry(pte, addr, addr + PAGE_SIZE, walk);
+ if (err)
+ break;
+ }
if (addr >= end - PAGE_SIZE)
break;
addr += PAGE_SIZE;
@@ -89,11 +108,14 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end,
again:
next = pmd_addr_end(addr, end);
if (pmd_none(*pmd)) {
- if (ops->pte_hole)
+ if (ops->install_pte)
+ err = __pte_alloc(walk->mm, pmd);
+ else if (ops->pte_hole)
err = ops->pte_hole(addr, next, depth, walk);
if (err)
break;
- continue;
+ if (!ops->install_pte)
+ continue;
}
walk->action = ACTION_SUBTREE;
@@ -116,7 +138,7 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end,
*/
if ((!walk->vma && (pmd_leaf(*pmd) || !pmd_present(*pmd))) ||
walk->action == ACTION_CONTINUE ||
- !(ops->pte_entry))
+ !(ops->pte_entry || ops->install_pte))
continue;
if (walk->vma)
@@ -148,11 +170,14 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end,
again:
next = pud_addr_end(addr, end);
if (pud_none(*pud)) {
- if (ops->pte_hole)
+ if (ops->install_pte)
+ err = __pmd_alloc(walk->mm, pud, addr);
+ else if (ops->pte_hole)
err = ops->pte_hole(addr, next, depth, walk);
if (err)
break;
- continue;
+ if (!ops->install_pte)
+ continue;
}
walk->action = ACTION_SUBTREE;
@@ -167,7 +192,7 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end,
if ((!walk->vma && (pud_leaf(*pud) || !pud_present(*pud))) ||
walk->action == ACTION_CONTINUE ||
- !(ops->pmd_entry || ops->pte_entry))
+ !(ops->pmd_entry || ops->pte_entry || ops->install_pte))
continue;
if (walk->vma)
@@ -196,18 +221,22 @@ static int walk_p4d_range(pgd_t *pgd, unsigned long addr, unsigned long end,
do {
next = p4d_addr_end(addr, end);
if (p4d_none_or_clear_bad(p4d)) {
- if (ops->pte_hole)
+ if (ops->install_pte)
+ err = __pud_alloc(walk->mm, p4d, addr);
+ else if (ops->pte_hole)
err = ops->pte_hole(addr, next, depth, walk);
if (err)
break;
- continue;
+ if (!ops->install_pte)
+ continue;
}
if (ops->p4d_entry) {
err = ops->p4d_entry(p4d, addr, next, walk);
if (err)
break;
}
- if (ops->pud_entry || ops->pmd_entry || ops->pte_entry)
+ if (ops->pud_entry || ops->pmd_entry || ops->pte_entry ||
+ ops->install_pte)
err = walk_pud_range(p4d, addr, next, walk);
if (err)
break;
@@ -231,18 +260,22 @@ static int walk_pgd_range(unsigned long addr, unsigned long end,
do {
next = pgd_addr_end(addr, end);
if (pgd_none_or_clear_bad(pgd)) {
- if (ops->pte_hole)
+ if (ops->install_pte)
+ err = __p4d_alloc(walk->mm, pgd, addr);
+ else if (ops->pte_hole)
err = ops->pte_hole(addr, next, 0, walk);
if (err)
break;
- continue;
+ if (!ops->install_pte)
+ continue;
}
if (ops->pgd_entry) {
err = ops->pgd_entry(pgd, addr, next, walk);
if (err)
break;
}
- if (ops->p4d_entry || ops->pud_entry || ops->pmd_entry || ops->pte_entry)
+ if (ops->p4d_entry || ops->pud_entry || ops->pmd_entry ||
+ ops->pte_entry || ops->install_pte)
err = walk_p4d_range(pgd, addr, next, walk);
if (err)
break;
@@ -334,6 +367,11 @@ static int __walk_page_range(unsigned long start, unsigned long end,
int err = 0;
struct vm_area_struct *vma = walk->vma;
const struct mm_walk_ops *ops = walk->ops;
+ bool is_hugetlb = is_vm_hugetlb_page(vma);
+
+ /* We do not support hugetlb PTE installation. */
+ if (ops->install_pte && is_hugetlb)
+ return -EINVAL;
if (ops->pre_vma) {
err = ops->pre_vma(start, end, walk);
@@ -341,7 +379,7 @@ static int __walk_page_range(unsigned long start, unsigned long end,
return err;
}
- if (is_vm_hugetlb_page(vma)) {
+ if (is_hugetlb) {
if (ops->hugetlb_entry)
err = walk_hugetlb_range(start, end, walk);
} else
@@ -380,47 +418,14 @@ static inline void process_vma_walk_lock(struct vm_area_struct *vma,
#endif
}
-/**
- * walk_page_range - walk page table with caller specific callbacks
- * @mm: mm_struct representing the target process of page table walk
- * @start: start address of the virtual address range
- * @end: end address of the virtual address range
- * @ops: operation to call during the walk
- * @private: private data for callbacks' usage
- *
- * Recursively walk the page table tree of the process represented by @mm
- * within the virtual address range [@start, @end). During walking, we can do
- * some caller-specific works for each entry, by setting up pmd_entry(),
- * pte_entry(), and/or hugetlb_entry(). If you don't set up for some of these
- * callbacks, the associated entries/pages are just ignored.
- * The return values of these callbacks are commonly defined like below:
- *
- * - 0 : succeeded to handle the current entry, and if you don't reach the
- * end address yet, continue to walk.
- * - >0 : succeeded to handle the current entry, and return to the caller
- * with caller specific value.
- * - <0 : failed to handle the current entry, and return to the caller
- * with error code.
- *
- * Before starting to walk page table, some callers want to check whether
- * they really want to walk over the current vma, typically by checking
- * its vm_flags. walk_page_test() and @ops->test_walk() are used for this
- * purpose.
- *
- * If operations need to be staged before and committed after a vma is walked,
- * there are two callbacks, pre_vma() and post_vma(). Note that post_vma(),
- * since it is intended to handle commit-type operations, can't return any
- * errors.
- *
- * struct mm_walk keeps current values of some common data like vma and pmd,
- * which are useful for the access from callbacks. If you want to pass some
- * caller-specific data to callbacks, @private should be helpful.
+/*
+ * See the comment for walk_page_range(), this performs the heavy lifting of the
+ * operation, only sets no restrictions on how the walk proceeds.
*
- * Locking:
- * Callers of walk_page_range() and walk_page_vma() should hold @mm->mmap_lock,
- * because these function traverse vma list and/or access to vma's data.
+ * We usually restrict the ability to install PTEs, but this functionality is
+ * available to internal memory management code and provided in mm/internal.h.
*/
-int walk_page_range(struct mm_struct *mm, unsigned long start,
+int walk_page_range_mm(struct mm_struct *mm, unsigned long start,
unsigned long end, const struct mm_walk_ops *ops,
void *private)
{
@@ -479,6 +484,80 @@ int walk_page_range(struct mm_struct *mm, unsigned long start,
return err;
}
+/*
+ * Determine if the walk operations specified are permitted to be used for a
+ * page table walk.
+ *
+ * This check is performed on all functions which are parameterised by walk
+ * operations and exposed in include/linux/pagewalk.h.
+ *
+ * Internal memory management code can use the walk_page_range_mm() function to
+ * be able to use all page walking operations.
+ */
+static bool check_ops_valid(const struct mm_walk_ops *ops)
+{
+ /*
+ * The installation of PTEs is solely under the control of memory
+ * management logic and subject to many subtle locking, security and
+ * cache considerations so we cannot permit other users to do so, and
+ * certainly not for exported symbols.
+ */
+ if (ops->install_pte)
+ return false;
+
+ return true;
+}
+
+/**
+ * walk_page_range - walk page table with caller specific callbacks
+ * @mm: mm_struct representing the target process of page table walk
+ * @start: start address of the virtual address range
+ * @end: end address of the virtual address range
+ * @ops: operation to call during the walk
+ * @private: private data for callbacks' usage
+ *
+ * Recursively walk the page table tree of the process represented by @mm
+ * within the virtual address range [@start, @end). During walking, we can do
+ * some caller-specific works for each entry, by setting up pmd_entry(),
+ * pte_entry(), and/or hugetlb_entry(). If you don't set up for some of these
+ * callbacks, the associated entries/pages are just ignored.
+ * The return values of these callbacks are commonly defined like below:
+ *
+ * - 0 : succeeded to handle the current entry, and if you don't reach the
+ * end address yet, continue to walk.
+ * - >0 : succeeded to handle the current entry, and return to the caller
+ * with caller specific value.
+ * - <0 : failed to handle the current entry, and return to the caller
+ * with error code.
+ *
+ * Before starting to walk page table, some callers want to check whether
+ * they really want to walk over the current vma, typically by checking
+ * its vm_flags. walk_page_test() and @ops->test_walk() are used for this
+ * purpose.
+ *
+ * If operations need to be staged before and committed after a vma is walked,
+ * there are two callbacks, pre_vma() and post_vma(). Note that post_vma(),
+ * since it is intended to handle commit-type operations, can't return any
+ * errors.
+ *
+ * struct mm_walk keeps current values of some common data like vma and pmd,
+ * which are useful for the access from callbacks. If you want to pass some
+ * caller-specific data to callbacks, @private should be helpful.
+ *
+ * Locking:
+ * Callers of walk_page_range() and walk_page_vma() should hold @mm->mmap_lock,
+ * because these function traverse vma list and/or access to vma's data.
+ */
+int walk_page_range(struct mm_struct *mm, unsigned long start,
+ unsigned long end, const struct mm_walk_ops *ops,
+ void *private)
+{
+ if (!check_ops_valid(ops))
+ return -EINVAL;
+
+ return walk_page_range_mm(mm, start, end, ops, private);
+}
+
/**
* walk_page_range_novma - walk a range of pagetables not backed by a vma
* @mm: mm_struct representing the target process of page table walk
@@ -494,7 +573,7 @@ int walk_page_range(struct mm_struct *mm, unsigned long start,
* walking the kernel pages tables or page tables for firmware.
*
* Note: Be careful to walk the kernel pages tables, the caller may be need to
- * take other effective approache (mmap lock may be insufficient) to prevent
+ * take other effective approaches (mmap lock may be insufficient) to prevent
* the intermediate kernel page tables belonging to the specified address range
* from being freed (e.g. memory hot-remove).
*/
@@ -513,6 +592,8 @@ int walk_page_range_novma(struct mm_struct *mm, unsigned long start,
if (start >= end || !walk.mm)
return -EINVAL;
+ if (!check_ops_valid(ops))
+ return -EINVAL;
/*
* 1) For walking the user virtual address space:
@@ -556,6 +637,8 @@ int walk_page_range_vma(struct vm_area_struct *vma, unsigned long start,
return -EINVAL;
if (start < vma->vm_start || end > vma->vm_end)
return -EINVAL;
+ if (!check_ops_valid(ops))
+ return -EINVAL;
process_mm_walk_lock(walk.mm, ops->walk_lock);
process_vma_walk_lock(vma, ops->walk_lock);
@@ -574,6 +657,8 @@ int walk_page_vma(struct vm_area_struct *vma, const struct mm_walk_ops *ops,
if (!walk.mm)
return -EINVAL;
+ if (!check_ops_valid(ops))
+ return -EINVAL;
process_mm_walk_lock(walk.mm, ops->walk_lock);
process_vma_walk_lock(vma, ops->walk_lock);
@@ -623,6 +708,9 @@ int walk_page_mapping(struct address_space *mapping, pgoff_t first_index,
unsigned long start_addr, end_addr;
int err = 0;
+ if (!check_ops_valid(ops))
+ return -EINVAL;
+
lockdep_assert_held(&mapping->i_mmap_rwsem);
vma_interval_tree_foreach(vma, &mapping->i_mmap, first_index,
first_index + nr - 1) {
--
2.47.0
^ permalink raw reply related
* [PATCH v2 4/5] tools: testing: update tools UAPI header for mman-common.h
From: Lorenzo Stoakes @ 2024-10-20 16:20 UTC (permalink / raw)
To: Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Vlastimil Babka, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <cover.1729440856.git.lorenzo.stoakes@oracle.com>
Import the new MADV_GUARD_POISON/UNPOISON madvise flags.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
tools/include/uapi/asm-generic/mman-common.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tools/include/uapi/asm-generic/mman-common.h b/tools/include/uapi/asm-generic/mman-common.h
index 6ce1f1ceb432..5dfd3d442de4 100644
--- a/tools/include/uapi/asm-generic/mman-common.h
+++ b/tools/include/uapi/asm-generic/mman-common.h
@@ -79,6 +79,9 @@
#define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
+#define MADV_GUARD_POISON 102 /* fatal signal on access to range */
+#define MADV_GUARD_UNPOISON 103 /* revoke guard poisoning */
+
/* compatibility flags */
#define MAP_FILE 0
--
2.47.0
^ permalink raw reply related
* Re: [PATCH v2 0/5] implement lightweight guard pages
From: Florian Weimer @ 2024-10-20 17:37 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
David Hildenbrand, linux-mm, linux-kernel, Muchun Song,
Richard Henderson, Ivan Kokshaysky, Matt Turner,
Thomas Bogendoerfer, James E . J . Bottomley, Helge Deller,
Chris Zankel, Max Filippov, Arnd Bergmann, linux-alpha,
linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <cover.1729440856.git.lorenzo.stoakes@oracle.com>
* Lorenzo Stoakes:
> Early testing of the prototype version of this code suggests a 5 times
> speed up in memory mapping invocations (in conjunction with use of
> process_madvise()) and a 13% reduction in VMAs on an entirely idle android
> system and unoptimised code.
>
> We expect with optimisation and a loaded system with a larger number of
> guard pages this could significantly increase, but in any case these
> numbers are encouraging.
>
> This way, rather than having separate VMAs specifying which parts of a
> range are guard pages, instead we have a VMA spanning the entire range of
> memory a user is permitted to access and including ranges which are to be
> 'guarded'.
>
> After mapping this, a user can specify which parts of the range should
> result in a fatal signal when accessed.
>
> By restricting the ability to specify guard pages to memory mapped by
> existing VMAs, we can rely on the mappings being torn down when the
> mappings are ultimately unmapped and everything works simply as if the
> memory were not faulted in, from the point of view of the containing VMAs.
We have a glibc (so not Android) dynamic linker bug that asks us to
remove PROT_NONE mappings in mapped shared objects:
Extra struct vm_area_struct with ---p created when PAGE_SIZE < max-page-size
<https://sourceware.org/bugzilla/show_bug.cgi?id=31076>
It's slightly different from a guard page because our main goal is to
avoid other mappings to end up in those gaps, which has been shown to
cause odd application behavior in cases where it happens. If I
understand the series correctly, the kernel would not automatically
attribute those PROT_NONE gaps to the previous or subsequent mapping.
We would have to extend one of the surrounding mapps and apply
MADV_POISON to that over-mapped part. That doesn't seem too onerous.
Could the ELF loader in the kernel do the same thing for the main
executable and the program loader?
^ permalink raw reply
* Re: [PATCH v2 0/5] implement lightweight guard pages
From: Lorenzo Stoakes @ 2024-10-20 19:45 UTC (permalink / raw)
To: Florian Weimer
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
David Hildenbrand, linux-mm, linux-kernel, Muchun Song,
Richard Henderson, Ivan Kokshaysky, Matt Turner,
Thomas Bogendoerfer, James E . J . Bottomley, Helge Deller,
Chris Zankel, Max Filippov, Arnd Bergmann, linux-alpha,
linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <87a5eysmj1.fsf@mid.deneb.enyo.de>
On Sun, Oct 20, 2024 at 07:37:54PM +0200, Florian Weimer wrote:
> * Lorenzo Stoakes:
>
> > Early testing of the prototype version of this code suggests a 5 times
> > speed up in memory mapping invocations (in conjunction with use of
> > process_madvise()) and a 13% reduction in VMAs on an entirely idle android
> > system and unoptimised code.
> >
> > We expect with optimisation and a loaded system with a larger number of
> > guard pages this could significantly increase, but in any case these
> > numbers are encouraging.
> >
> > This way, rather than having separate VMAs specifying which parts of a
> > range are guard pages, instead we have a VMA spanning the entire range of
> > memory a user is permitted to access and including ranges which are to be
> > 'guarded'.
> >
> > After mapping this, a user can specify which parts of the range should
> > result in a fatal signal when accessed.
> >
> > By restricting the ability to specify guard pages to memory mapped by
> > existing VMAs, we can rely on the mappings being torn down when the
> > mappings are ultimately unmapped and everything works simply as if the
> > memory were not faulted in, from the point of view of the containing VMAs.
>
> We have a glibc (so not Android) dynamic linker bug that asks us to
> remove PROT_NONE mappings in mapped shared objects:
>
> Extra struct vm_area_struct with ---p created when PAGE_SIZE < max-page-size
> <https://sourceware.org/bugzilla/show_bug.cgi?id=31076>
>
> It's slightly different from a guard page because our main goal is to
> avoid other mappings to end up in those gaps, which has been shown to
> cause odd application behavior in cases where it happens. If I
> understand the series correctly, the kernel would not automatically
> attribute those PROT_NONE gaps to the previous or subsequent mapping.
> We would have to extend one of the surrounding mapps and apply
> MADV_POISON to that over-mapped part. That doesn't seem too onerous.
>
> Could the ELF loader in the kernel do the same thing for the main
> executable and the program loader?
Currently this implementation is only available for private anonymous
memory. We may look at extending it to shmem and file-backed memory in the
future but there are a whole host of things to overcome to make that work
so it's one step at a time! :)
^ permalink raw reply
* Re: [PATCH v4 2/4] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: Lorenzo Stoakes @ 2024-10-21 8:11 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang, John Hubbard
In-Reply-To: <3bf7f2d8efc768007b5de8122275405afc9942d4.1729198898.git.lorenzo.stoakes@oracle.com>
On Thu, Oct 17, 2024 at 10:05:50PM +0100, Lorenzo Stoakes wrote:
> It is useful to be able to utilise the pidfd mechanism to reference the
> current thread or process (from a userland point of view - thread group
> leader from the kernel's point of view).
>
> Therefore introduce PIDFD_SELF_THREAD to refer to the current thread, and
> PIDFD_SELF_THREAD_GROUP to refer to the current thread group leader.
>
> For convenience and to avoid confusion from userland's perspective we alias
> these:
>
> * PIDFD_SELF is an alias for PIDFD_SELF_THREAD - This is nearly always what
> the user will want to use, as they would find it surprising if for
> instance fd's were unshared()'d and they wanted to invoke pidfd_getfd()
> and that failed.
>
> * PIDFD_SELF_PROCESS is an alias for PIDFD_SELF_THREAD_GROUP - Most users
> have no concept of thread groups or what a thread group leader is, and
> from userland's perspective and nomenclature this is what userland
> considers to be a process.
>
> Due to the refactoring of the central __pidfd_get_pid() function we can
> implement this functionality centrally, providing the use of this sentinel
> in most functionality which utilises pidfd's.
>
> We need to explicitly adjust kernel_waitid_prepare() to permit this (though
> it wouldn't really make sense to use this there, we provide the ability for
> consistency).
>
> We explicitly disallow use of this in setns(), which would otherwise have
> required explicit custom handling, as it doesn't make sense to set the
> current calling thread to join the namespace of itself.
>
> As the callers of pidfd_get_pid() expect an increased reference count on
> the pid we do so in the self case, reducing churn and avoiding any breakage
> from existing logic which decrements this reference count.
>
> This change implicitly provides PIDFD_SELF_* support in the waitid(P_PIDFS,
> ...), process_madvise(), process_mrelease(), pidfd_send_signal(), and
> pidfd_getfd() system calls.
>
> Things such as polling a pidfs and general fd operations are not supported,
> this strictly provides the sentinel for APIs which explicitly accept a
> pidfd.
>
We need a:
Suggested-by: Pedro Falcato <pedro.falcato@gmail.com>
here, will add if respin, otherwise could you add Christian?
My apologies Pedro, this was wholly an oversight and my mistake!
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
^ permalink raw reply
* Re: [PATCH v2 1/5] mm: pagewalk: add the ability to install PTEs
From: Vlastimil Babka @ 2024-10-21 13:27 UTC (permalink / raw)
To: Lorenzo Stoakes, Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Paul E . McKenney, Jann Horn, David Hildenbrand, linux-mm,
linux-kernel, Muchun Song, Richard Henderson, Ivan Kokshaysky,
Matt Turner, Thomas Bogendoerfer, James E . J . Bottomley,
Helge Deller, Chris Zankel, Max Filippov, Arnd Bergmann,
linux-alpha, linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <cf91e3936c2dee42aa8ac15af3e76c90c098d570.1729440856.git.lorenzo.stoakes@oracle.com>
On 10/20/24 18:20, Lorenzo Stoakes wrote:
> The existing generic pagewalk logic permits the walking of page tables,
> invoking callbacks at individual page table levels via user-provided
> mm_walk_ops callbacks.
>
> This is useful for traversing existing page table entries, but precludes
> the ability to establish new ones.
>
> Existing mechanism for performing a walk which also installs page table
> entries if necessary are heavily duplicated throughout the kernel, each
> with semantic differences from one another and largely unavailable for use
> elsewhere.
>
> Rather than add yet another implementation, we extend the generic pagewalk
> logic to enable the installation of page table entries by adding a new
> install_pte() callback in mm_walk_ops. If this is specified, then upon
> encountering a missing page table entry, we allocate and install a new one
> and continue the traversal.
>
> If a THP huge page is encountered, we make use of existing logic to split
> it. Then once we reach the PTE level, we invoke the install_pte() callback
> which provides a PTE entry to install. We do not support hugetlb at this
> stage.
>
> If this function returns an error, or an allocation fails during the
> operation, we abort the operation altogether. It is up to the caller to
> deal appropriately with partially populated page table ranges.
>
> If install_pte() is defined, the semantics of pte_entry() change - this
> callback is then only invoked if the entry already exists. This is a useful
> property, as it allows a caller to handle existing PTEs while installing
> new ones where necessary in the specified range.
>
> If install_pte() is not defined, then there is no functional difference to
> this patch, so all existing logic will work precisely as it did before.
>
> As we only permit the installation of PTEs where a mapping does not already
> exist there is no need for TLB management, however we do invoke
> update_mmu_cache() for architectures which require manual maintenance of
> mappings for other CPUs.
>
> We explicitly do not allow the existing page walk API to expose this
> feature as it is dangerous and intended for internal mm use only. Therefore
> we provide a new walk_page_range_mm() function exposed only to
> mm/internal.h.
>
> Reviewed-by: Jann Horn <jannh@google.com>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
<snip>
> /*
> * We want to know the real level where a entry is located ignoring any
> * folding of levels which may be happening. For example if p4d is folded then
> @@ -29,9 +34,23 @@ static int walk_pte_range_inner(pte_t *pte, unsigned long addr,
> int err = 0;
>
> for (;;) {
> - err = ops->pte_entry(pte, addr, addr + PAGE_SIZE, walk);
> - if (err)
> - break;
> + if (ops->install_pte && pte_none(ptep_get(pte))) {
> + pte_t new_pte;
> +
> + err = ops->install_pte(addr, addr + PAGE_SIZE, &new_pte,
> + walk);
> + if (err)
> + break;
> +
> + set_pte_at(walk->mm, addr, pte, new_pte);
While the guard pages install ptes unconditionally, maybe some install_pte
handler implementation would sometimes want to skip, should ve define an
error code that means its skipped and just continue instead of set_pte_at()?
Or leave it until such handler appears.
> + /* Non-present before, so for arches that need it. */
> + if (!WARN_ON_ONCE(walk->no_vma))
> + update_mmu_cache(walk->vma, addr, pte);
> + } else {
> + err = ops->pte_entry(pte, addr, addr + PAGE_SIZE, walk);
> + if (err)
> + break;
> + }
> if (addr >= end - PAGE_SIZE)
> break;
> addr += PAGE_SIZE;
> @@ -89,11 +108,14 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end,
> again:
> next = pmd_addr_end(addr, end);
> if (pmd_none(*pmd)) {
> - if (ops->pte_hole)
> + if (ops->install_pte)
> + err = __pte_alloc(walk->mm, pmd);
> + else if (ops->pte_hole)
> err = ops->pte_hole(addr, next, depth, walk);
> if (err)
> break;
> - continue;
> + if (!ops->install_pte)
> + continue;
> }
>
> walk->action = ACTION_SUBTREE;
> @@ -116,7 +138,7 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end,
> */
> if ((!walk->vma && (pmd_leaf(*pmd) || !pmd_present(*pmd))) ||
> walk->action == ACTION_CONTINUE ||
> - !(ops->pte_entry))
> + !(ops->pte_entry || ops->install_pte))
> continue;
BTW, I find it hard to read this condition even before your patch, oh well.
But if I read it correctly, does it mean we're going to split a pmd-mapped
THP if we have a install_pte handler? But is that really necessary if the
pmd splitting results in all ptes populated, and thus the install_pte
handler can't do anything with any pte anyway?
> if (walk->vma)
> @@ -148,11 +170,14 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end,
> again:
> next = pud_addr_end(addr, end);
> if (pud_none(*pud)) {
> - if (ops->pte_hole)
> + if (ops->install_pte)
> + err = __pmd_alloc(walk->mm, pud, addr);
> + else if (ops->pte_hole)
> err = ops->pte_hole(addr, next, depth, walk);
> if (err)
> break;
> - continue;
> + if (!ops->install_pte)
> + continue;
> }
>
> walk->action = ACTION_SUBTREE;
> @@ -167,7 +192,7 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end,
>
> if ((!walk->vma && (pud_leaf(*pud) || !pud_present(*pud))) ||
> walk->action == ACTION_CONTINUE ||
> - !(ops->pmd_entry || ops->pte_entry))
> + !(ops->pmd_entry || ops->pte_entry || ops->install_pte))
> continue;
Ditto?
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: add PTE_MARKER_GUARD PTE marker
From: Vlastimil Babka @ 2024-10-21 13:45 UTC (permalink / raw)
To: Lorenzo Stoakes, Andrew Morton
Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Paul E . McKenney, Jann Horn, David Hildenbrand, linux-mm,
linux-kernel, Muchun Song, Richard Henderson, Ivan Kokshaysky,
Matt Turner, Thomas Bogendoerfer, James E . J . Bottomley,
Helge Deller, Chris Zankel, Max Filippov, Arnd Bergmann,
linux-alpha, linux-mips, linux-parisc, linux-arch, Shuah Khan,
Christian Brauner, linux-kselftest, Sidhartha Kumar, Jeff Xu,
Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <081837b697a98c7fa5832542b20f603d49e0b557.1729440856.git.lorenzo.stoakes@oracle.com>
On 10/20/24 18:20, Lorenzo Stoakes wrote:
> Add a new PTE marker that results in any access causing the accessing
> process to segfault.
Should we distinguish it from other segfaults? Is there a way? I can see
memory protection keys use SEGV_PKUERR, but no idea if we have any free values.
> This is preferable to PTE_MARKER_POISONED, which results in the same
> handling as hardware poisoned memory, and is thus undesirable for cases
> where we simply wish to 'soft' poison a range.
>
> This is in preparation for implementing the ability to specify guard pages
> at the page table level, i.e. ranges that, when accessed, should cause
> process termination.
>
> Additionally, rename zap_drop_file_uffd_wp() to zap_drop_markers() - the
> function checks the ZAP_FLAG_DROP_MARKER flag so naming it for this single
> purpose was simply incorrect.
>
> We then reuse the same logic to determine whether a zap should clear a
> guard entry - this should only be performed on teardown and never on
> MADV_DONTNEED or the like.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
A nit below:
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 906294ac85dc..50e3f6ed73ac 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6353,6 +6353,9 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
> ret = VM_FAULT_HWPOISON_LARGE |
> VM_FAULT_SET_HINDEX(hstate_index(h));
> goto out_mutex;
> + } else if (marker & PTE_MARKER_GUARD) {
> + ret = VM_FAULT_SIGSEGV;
> + goto out_mutex;
Given we don't support hugetlb, should we WARN_ON_ONCE() if such unexpected
marker appears there?
> }
> }
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 0f614523b9f4..551455cd453f 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1455,7 +1455,7 @@ static inline bool should_zap_folio(struct zap_details *details,
> return !folio_test_anon(folio);
> }
>
> -static inline bool zap_drop_file_uffd_wp(struct zap_details *details)
> +static inline bool zap_drop_markers(struct zap_details *details)
> {
> if (!details)
> return false;
> @@ -1476,7 +1476,7 @@ zap_install_uffd_wp_if_needed(struct vm_area_struct *vma,
> if (vma_is_anonymous(vma))
> return;
>
> - if (zap_drop_file_uffd_wp(details))
> + if (zap_drop_markers(details))
> return;
>
> for (;;) {
> @@ -1671,7 +1671,15 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
> * drop the marker if explicitly requested.
> */
> if (!vma_is_anonymous(vma) &&
> - !zap_drop_file_uffd_wp(details))
> + !zap_drop_markers(details))
> + continue;
> + } else if (is_guard_swp_entry(entry)) {
> + /*
> + * Ordinary zapping should not remove guard PTE
> + * markers. Only do so if we should remove PTE markers
> + * in general.
> + */
> + if (!zap_drop_markers(details))
> continue;
> } else if (is_hwpoison_entry(entry) ||
> is_poisoned_swp_entry(entry)) {
> @@ -4003,6 +4011,10 @@ static vm_fault_t handle_pte_marker(struct vm_fault *vmf)
> if (marker & PTE_MARKER_POISONED)
> return VM_FAULT_HWPOISON;
>
> + /* Hitting a guard page is always a fatal condition. */
> + if (marker & PTE_MARKER_GUARD)
> + return VM_FAULT_SIGSEGV;
> +
> if (pte_marker_entry_uffd_wp(entry))
> return pte_marker_handle_uffd_wp(vmf);
>
^ permalink raw reply
* Re: [PATCH v2 1/5] mm: pagewalk: add the ability to install PTEs
From: Lorenzo Stoakes @ 2024-10-21 13:50 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, David Hildenbrand,
linux-mm, linux-kernel, Muchun Song, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Thomas Bogendoerfer,
James E . J . Bottomley, Helge Deller, Chris Zankel, Max Filippov,
Arnd Bergmann, linux-alpha, linux-mips, linux-parisc, linux-arch,
Shuah Khan, Christian Brauner, linux-kselftest, Sidhartha Kumar,
Jeff Xu, Christoph Hellwig, linux-api, John Hubbard
In-Reply-To: <fdd2be0a-cae9-4508-ba20-eb04c9a1e7f9@suse.cz>
On Mon, Oct 21, 2024 at 03:27:55PM +0200, Vlastimil Babka wrote:
> On 10/20/24 18:20, Lorenzo Stoakes wrote:
> > The existing generic pagewalk logic permits the walking of page tables,
> > invoking callbacks at individual page table levels via user-provided
> > mm_walk_ops callbacks.
> >
> > This is useful for traversing existing page table entries, but precludes
> > the ability to establish new ones.
> >
> > Existing mechanism for performing a walk which also installs page table
> > entries if necessary are heavily duplicated throughout the kernel, each
> > with semantic differences from one another and largely unavailable for use
> > elsewhere.
> >
> > Rather than add yet another implementation, we extend the generic pagewalk
> > logic to enable the installation of page table entries by adding a new
> > install_pte() callback in mm_walk_ops. If this is specified, then upon
> > encountering a missing page table entry, we allocate and install a new one
> > and continue the traversal.
> >
> > If a THP huge page is encountered, we make use of existing logic to split
> > it. Then once we reach the PTE level, we invoke the install_pte() callback
> > which provides a PTE entry to install. We do not support hugetlb at this
> > stage.
> >
> > If this function returns an error, or an allocation fails during the
> > operation, we abort the operation altogether. It is up to the caller to
> > deal appropriately with partially populated page table ranges.
> >
> > If install_pte() is defined, the semantics of pte_entry() change - this
> > callback is then only invoked if the entry already exists. This is a useful
> > property, as it allows a caller to handle existing PTEs while installing
> > new ones where necessary in the specified range.
> >
> > If install_pte() is not defined, then there is no functional difference to
> > this patch, so all existing logic will work precisely as it did before.
> >
> > As we only permit the installation of PTEs where a mapping does not already
> > exist there is no need for TLB management, however we do invoke
> > update_mmu_cache() for architectures which require manual maintenance of
> > mappings for other CPUs.
> >
> > We explicitly do not allow the existing page walk API to expose this
> > feature as it is dangerous and intended for internal mm use only. Therefore
> > we provide a new walk_page_range_mm() function exposed only to
> > mm/internal.h.
> >
> > Reviewed-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> <snip>
>
> > /*
> > * We want to know the real level where a entry is located ignoring any
> > * folding of levels which may be happening. For example if p4d is folded then
> > @@ -29,9 +34,23 @@ static int walk_pte_range_inner(pte_t *pte, unsigned long addr,
> > int err = 0;
> >
> > for (;;) {
> > - err = ops->pte_entry(pte, addr, addr + PAGE_SIZE, walk);
> > - if (err)
> > - break;
> > + if (ops->install_pte && pte_none(ptep_get(pte))) {
> > + pte_t new_pte;
> > +
> > + err = ops->install_pte(addr, addr + PAGE_SIZE, &new_pte,
> > + walk);
> > + if (err)
> > + break;
> > +
> > + set_pte_at(walk->mm, addr, pte, new_pte);
>
> While the guard pages install ptes unconditionally, maybe some install_pte
> handler implementation would sometimes want to skip, should ve define an
> error code that means its skipped and just continue instead of set_pte_at()?
> Or leave it until such handler appears.
I'm not sure under what circumstances you'd want to skip though precisely?
There's nothing populated, and the user is defining the range in which to
install a PTE if nothing is populated.
If they wanted more complicated handling they could do multiple, smaller
calls. Things are inherently racey with these walks so there's no benefit
in doing everything at once.
>
> > + /* Non-present before, so for arches that need it. */
> > + if (!WARN_ON_ONCE(walk->no_vma))
> > + update_mmu_cache(walk->vma, addr, pte);
> > + } else {
> > + err = ops->pte_entry(pte, addr, addr + PAGE_SIZE, walk);
> > + if (err)
> > + break;
> > + }
> > if (addr >= end - PAGE_SIZE)
> > break;
> > addr += PAGE_SIZE;
> > @@ -89,11 +108,14 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end,
> > again:
> > next = pmd_addr_end(addr, end);
> > if (pmd_none(*pmd)) {
> > - if (ops->pte_hole)
> > + if (ops->install_pte)
> > + err = __pte_alloc(walk->mm, pmd);
> > + else if (ops->pte_hole)
> > err = ops->pte_hole(addr, next, depth, walk);
> > if (err)
> > break;
> > - continue;
> > + if (!ops->install_pte)
> > + continue;
> > }
> >
> > walk->action = ACTION_SUBTREE;
> > @@ -116,7 +138,7 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end,
> > */
> > if ((!walk->vma && (pmd_leaf(*pmd) || !pmd_present(*pmd))) ||
> > walk->action == ACTION_CONTINUE ||
> > - !(ops->pte_entry))
> > + !(ops->pte_entry || ops->install_pte))
> > continue;
>
> BTW, I find it hard to read this condition even before your patch, oh well.
Agreed, this badly needs refactoring, but felt out of scope for this change.
> But if I read it correctly, does it mean we're going to split a pmd-mapped
> THP if we have a install_pte handler? But is that really necessary if the
> pmd splitting results in all ptes populated, and thus the install_pte
> handler can't do anything with any pte anyway?
Yes... however nothing else here in the logic has special handling for
transhuge pages AND there is already an interface provided to prevent this
if you want, which we use in commit 3/5, that is to provide pud, pmd
walkers that set walk->action = ACTION_CONTINUE if transhuge.
Having said that, it kind of sucks that we are doing a useless split
here. Hmm. In the pte_entry() case you might very well want to split and do
something with the PTE. With the install you are only interested if it's
non-present...
It's not _completely_ infeasible that a user would want this, but it's very
unlikely.
OK so yeah let's add it and clean up this expression while we're at it, will
fix on respin.
>
> > if (walk->vma)
> > @@ -148,11 +170,14 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end,
> > again:
> > next = pud_addr_end(addr, end);
> > if (pud_none(*pud)) {
> > - if (ops->pte_hole)
> > + if (ops->install_pte)
> > + err = __pmd_alloc(walk->mm, pud, addr);
> > + else if (ops->pte_hole)
> > err = ops->pte_hole(addr, next, depth, walk);
> > if (err)
> > break;
> > - continue;
> > + if (!ops->install_pte)
> > + continue;
> > }
> >
> > walk->action = ACTION_SUBTREE;
> > @@ -167,7 +192,7 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end,
> >
> > if ((!walk->vma && (pud_leaf(*pud) || !pud_present(*pud))) ||
> > walk->action == ACTION_CONTINUE ||
> > - !(ops->pmd_entry || ops->pte_entry))
> > + !(ops->pmd_entry || ops->pte_entry || ops->install_pte))
> > continue;
>
> Ditto?
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox