* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Amir Goldstein @ 2024-05-24 4:58 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Alexander Aring, linux-fsdevel, linux-nfs,
linux-kernel, linux-api
In-Reply-To: <20240523-exportfs-u64-mount-id-v2-1-f9f959f17eb1@cyphar.com>
On Thu, May 23, 2024 at 11:57 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> Now that we provide a unique 64-bit mount ID interface in statx, we can
> now provide a race-free way for name_to_handle_at(2) to provide a file
> handle and corresponding mount without needing to worry about racing
> with /proc/mountinfo parsing.
>
> While this is not necessary if you are using AT_EMPTY_PATH and don't
> care about an extra statx(2) call, users that pass full paths into
> name_to_handle_at(2) need to know which mount the file handle comes from
> (to make sure they don't try to open_by_handle_at a file handle from a
> different filesystem) and switching to AT_EMPTY_PATH would require
> allocating a file for every name_to_handle_at(2) call, turning
>
> err = name_to_handle_at(-EBADF, "/foo/bar/baz", &handle, &mntid,
> AT_HANDLE_MNT_ID_UNIQUE);
>
> into
>
> int fd = openat(-EBADF, "/foo/bar/baz", O_PATH | O_CLOEXEC);
> err1 = name_to_handle_at(fd, "", &handle, &unused_mntid, AT_EMPTY_PATH);
> err2 = statx(fd, "", AT_EMPTY_PATH, STATX_MNT_ID_UNIQUE, &statxbuf);
> mntid = statxbuf.stx_mnt_id;
> close(fd);
>
> Unlike AT_HANDLE_FID, as per Amir's suggestion, AT_HANDLE_MNT_ID_UNIQUE
> uses a new AT_* bit from the historically-unused 0xFF range (which we
> now define as being the "per-syscall" range for AT_* bits).
>
Sorry for nit picking, but I think that "Unlike AT_HANDLE_FID,..." is confusing
in this statement.
AT_HANDLE_FID is using a bit that was already effectively allocated for a
"per-syscall" range.
I don't think that mentioning AT_HANDLE_FID adds any clarity to the statement
so better drop it?
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> Changes in v2:
> - Fixed a few minor compiler warnings and a buggy copy_to_user() check.
> - Rename AT_HANDLE_UNIQUE_MOUNT_ID -> AT_HANDLE_MNT_ID_UNIQUE to match statx.
> - Switched to using an AT_* bit from 0xFF and defining that range as
> being "per-syscall" for future usage.
> - Sync tools/ copy of <linux/fcntl.h> to include changes.
> - v1: <https://lore.kernel.org/r/20240520-exportfs-u64-mount-id-v1-1-f55fd9215b8e@cyphar.com>
> ---
> fs/fhandle.c | 29 ++++++++++++++++++++++-------
> include/linux/syscalls.h | 2 +-
> include/uapi/linux/fcntl.h | 28 +++++++++++++++++++++-------
> tools/include/uapi/linux/fcntl.h | 28 +++++++++++++++++++++-------
> 4 files changed, 65 insertions(+), 22 deletions(-)
>
[...]
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index c0bcc185fa48..9ed9d65842c1 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -87,22 +87,24 @@
> #define DN_ATTRIB 0x00000020 /* File changed attibutes */
> #define DN_MULTISHOT 0x80000000 /* Don't remove notifier */
>
> +#define AT_FDCWD -100 /* Special value used to indicate
> + openat should use the current
> + working directory. */
(more nit picking)
If you are changing this line, please at least add a new line,
this is a different namespace :-/
and perhaps change it to "Special value of dirfd argument..."
Also, better leave a comment here to discourage allocation from this range:
+ /* Reserved for per-syscall flags 0xff */
> +#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */
> +
> /*
> - * The constants AT_REMOVEDIR and AT_EACCESS have the same value. AT_EACCESS is
> - * meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
> + * The constants AT_REMOVEDIR and AT_EACCESS have the same value. AT_EACCESS
> + * is meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
> * unlinkat. The two functions do completely different things and therefore,
> * the flags can be allowed to overlap. For example, passing AT_REMOVEDIR to
> * faccessat would be undefined behavior and thus treating it equivalent to
> * AT_EACCESS is valid undefined behavior.
> */
If you are going to add this churn in this patch, please do it otherwise.
It does not make sense to have this long explanation about pre-syscall
AT_* flags in a different location from the comment you added about
"All new purely-syscall-specific AT_* flags.."
if this explanation is needed at all, it should be after the new comment
as an example.
> -#define AT_FDCWD -100 /* Special value used to indicate
> - openat should use the current
> - working directory. */
> -#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */
> #define AT_EACCESS 0x200 /* Test access permitted for
> effective IDs, not real IDs. */
> #define AT_REMOVEDIR 0x200 /* Remove directory instead of
> unlinking file. */
I really prefer to move those to the per-syscall section
right next to AT_HANDLE_FID and leave a comment here:
/* Reserved for per-syscall flags 0x200 */
> +
> #define AT_SYMLINK_FOLLOW 0x400 /* Follow symbolic links. */
> #define AT_NO_AUTOMOUNT 0x800 /* Suppress terminal automount traversal */
> #define AT_EMPTY_PATH 0x1000 /* Allow empty relative pathname */
> @@ -114,10 +116,22 @@
>
> #define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
>
> -/* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
> +/*
> + * All new purely-syscall-specific AT_* flags should consider using bits from
> + * 0xFF, but the bits used by RENAME_* (0x7) should be avoided in case users
> + * decide to pass AT_* flags to renameat2() by accident.
Sorry, but I find the use of my renameat2() example a bit confusing
in this sentence.
If you mention it at all, please use "For example, the bits used by..."
but I think it is more important to say "...should consider re-using bits
already used by other per-syscalls flags".
> These flag bits are
> + * free for re-use by other syscall's syscall-specific flags without worry.
> + */
> +
> +/*
> + * Flags for name_to_handle_at(2). To save AT_ flag space we re-use the
> + * AT_EACCESS/AT_REMOVEDIR bit for AT_HANDLE_FID.
> + */
AT_EACCESS/AT_REMOVEDIR/AT_HANDLE_FID have exact same status,
so instead of this asymmetric comment:
+/* Flags for faccessat(2) */
+#define AT_EACCESS 0x200 /* Test access permitted for
+ effective IDs, not real IDs. */
+/* Flags for unlinkat(2) */
+#define AT_REMOVEDIR 0x200 /* Remove directory instead of
+ unlinking file. */
+/* Flags for name_to_handle_at(2) */
+#define AT_HANDLE_FID 0x200 /* file handle is needed to
compare object identity and may not
be usable to open_by_handle_at(2) */
> +#define AT_HANDLE_MNT_ID_UNIQUE 0x80 /* return the u64 unique mount id */
IDGI, I think we may have been miscommunicating :-/
If 0x7 range is to be avoided for generic AT_ flags, then it *should* be used
for new per-syscall flags such as this one.
The reservation of 0xff is not a strong guarantee.
As long as people re-use new per-syscalls flags efficiently, we could
decide to reclaim some of this space for generic AT_ flags in the future
if it is needed.
I know most of the mess was here before your patch, but I think
it got to a point where we must put a little order before introducing
the new per-syscall flag.
Thanks,
Amir.
^ permalink raw reply
* [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Aleksa Sarai @ 2024-05-23 20:57 UTC (permalink / raw)
To: Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Amir Goldstein, Alexander Aring
Cc: linux-fsdevel, linux-nfs, linux-kernel, linux-api, Aleksa Sarai
Now that we provide a unique 64-bit mount ID interface in statx, we can
now provide a race-free way for name_to_handle_at(2) to provide a file
handle and corresponding mount without needing to worry about racing
with /proc/mountinfo parsing.
While this is not necessary if you are using AT_EMPTY_PATH and don't
care about an extra statx(2) call, users that pass full paths into
name_to_handle_at(2) need to know which mount the file handle comes from
(to make sure they don't try to open_by_handle_at a file handle from a
different filesystem) and switching to AT_EMPTY_PATH would require
allocating a file for every name_to_handle_at(2) call, turning
err = name_to_handle_at(-EBADF, "/foo/bar/baz", &handle, &mntid,
AT_HANDLE_MNT_ID_UNIQUE);
into
int fd = openat(-EBADF, "/foo/bar/baz", O_PATH | O_CLOEXEC);
err1 = name_to_handle_at(fd, "", &handle, &unused_mntid, AT_EMPTY_PATH);
err2 = statx(fd, "", AT_EMPTY_PATH, STATX_MNT_ID_UNIQUE, &statxbuf);
mntid = statxbuf.stx_mnt_id;
close(fd);
Unlike AT_HANDLE_FID, as per Amir's suggestion, AT_HANDLE_MNT_ID_UNIQUE
uses a new AT_* bit from the historically-unused 0xFF range (which we
now define as being the "per-syscall" range for AT_* bits).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
Changes in v2:
- Fixed a few minor compiler warnings and a buggy copy_to_user() check.
- Rename AT_HANDLE_UNIQUE_MOUNT_ID -> AT_HANDLE_MNT_ID_UNIQUE to match statx.
- Switched to using an AT_* bit from 0xFF and defining that range as
being "per-syscall" for future usage.
- Sync tools/ copy of <linux/fcntl.h> to include changes.
- v1: <https://lore.kernel.org/r/20240520-exportfs-u64-mount-id-v1-1-f55fd9215b8e@cyphar.com>
---
fs/fhandle.c | 29 ++++++++++++++++++++++-------
include/linux/syscalls.h | 2 +-
include/uapi/linux/fcntl.h | 28 +++++++++++++++++++++-------
tools/include/uapi/linux/fcntl.h | 28 +++++++++++++++++++++-------
4 files changed, 65 insertions(+), 22 deletions(-)
diff --git a/fs/fhandle.c b/fs/fhandle.c
index 8a7f86c2139a..c6e90161b900 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -16,7 +16,8 @@
static long do_sys_name_to_handle(const struct path *path,
struct file_handle __user *ufh,
- int __user *mnt_id, int fh_flags)
+ void __user *mnt_id, bool unique_mntid,
+ int fh_flags)
{
long retval;
struct file_handle f_handle;
@@ -69,9 +70,19 @@ static long do_sys_name_to_handle(const struct path *path,
} else
retval = 0;
/* copy the mount id */
- if (put_user(real_mount(path->mnt)->mnt_id, mnt_id) ||
- copy_to_user(ufh, handle,
- struct_size(handle, f_handle, handle_bytes)))
+ if (unique_mntid) {
+ if (put_user(real_mount(path->mnt)->mnt_id_unique,
+ (u64 __user *) mnt_id))
+ retval = -EFAULT;
+ } else {
+ if (put_user(real_mount(path->mnt)->mnt_id,
+ (int __user *) mnt_id))
+ retval = -EFAULT;
+ }
+ /* copy the handle */
+ if (retval != -EFAULT &&
+ copy_to_user(ufh, handle,
+ struct_size(handle, f_handle, handle_bytes)))
retval = -EFAULT;
kfree(handle);
return retval;
@@ -83,6 +94,7 @@ static long do_sys_name_to_handle(const struct path *path,
* @name: name that should be converted to handle.
* @handle: resulting file handle
* @mnt_id: mount id of the file system containing the file
+ * (u64 if AT_HANDLE_MNT_ID_UNIQUE, otherwise int)
* @flag: flag value to indicate whether to follow symlink or not
* and whether a decodable file handle is required.
*
@@ -92,7 +104,7 @@ static long do_sys_name_to_handle(const struct path *path,
* value required.
*/
SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
- struct file_handle __user *, handle, int __user *, mnt_id,
+ struct file_handle __user *, handle, void __user *, mnt_id,
int, flag)
{
struct path path;
@@ -100,7 +112,8 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
int fh_flags;
int err;
- if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID))
+ if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID |
+ AT_HANDLE_MNT_ID_UNIQUE))
return -EINVAL;
lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0;
@@ -109,7 +122,9 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
lookup_flags |= LOOKUP_EMPTY;
err = user_path_at(dfd, name, lookup_flags, &path);
if (!err) {
- err = do_sys_name_to_handle(&path, handle, mnt_id, fh_flags);
+ err = do_sys_name_to_handle(&path, handle, mnt_id,
+ flag & AT_HANDLE_MNT_ID_UNIQUE,
+ fh_flags);
path_put(&path);
}
return err;
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e619ac10cd23..99c5a783a01e 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -863,7 +863,7 @@ asmlinkage long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
const char __user *pathname);
asmlinkage long sys_name_to_handle_at(int dfd, const char __user *name,
struct file_handle __user *handle,
- int __user *mnt_id, int flag);
+ void __user *mnt_id, int flag);
asmlinkage long sys_open_by_handle_at(int mountdirfd,
struct file_handle __user *handle,
int flags);
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index c0bcc185fa48..9ed9d65842c1 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -87,22 +87,24 @@
#define DN_ATTRIB 0x00000020 /* File changed attibutes */
#define DN_MULTISHOT 0x80000000 /* Don't remove notifier */
+#define AT_FDCWD -100 /* Special value used to indicate
+ openat should use the current
+ working directory. */
+#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */
+
/*
- * The constants AT_REMOVEDIR and AT_EACCESS have the same value. AT_EACCESS is
- * meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
+ * The constants AT_REMOVEDIR and AT_EACCESS have the same value. AT_EACCESS
+ * is meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
* unlinkat. The two functions do completely different things and therefore,
* the flags can be allowed to overlap. For example, passing AT_REMOVEDIR to
* faccessat would be undefined behavior and thus treating it equivalent to
* AT_EACCESS is valid undefined behavior.
*/
-#define AT_FDCWD -100 /* Special value used to indicate
- openat should use the current
- working directory. */
-#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */
#define AT_EACCESS 0x200 /* Test access permitted for
effective IDs, not real IDs. */
#define AT_REMOVEDIR 0x200 /* Remove directory instead of
unlinking file. */
+
#define AT_SYMLINK_FOLLOW 0x400 /* Follow symbolic links. */
#define AT_NO_AUTOMOUNT 0x800 /* Suppress terminal automount traversal */
#define AT_EMPTY_PATH 0x1000 /* Allow empty relative pathname */
@@ -114,10 +116,22 @@
#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
-/* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
+/*
+ * All new purely-syscall-specific AT_* flags should consider using bits from
+ * 0xFF, but the bits used by RENAME_* (0x7) should be avoided in case users
+ * decide to pass AT_* flags to renameat2() by accident. These flag bits are
+ * free for re-use by other syscall's syscall-specific flags without worry.
+ */
+
+/*
+ * Flags for name_to_handle_at(2). To save AT_ flag space we re-use the
+ * AT_EACCESS/AT_REMOVEDIR bit for AT_HANDLE_FID.
+ */
#define AT_HANDLE_FID AT_REMOVEDIR /* file handle is needed to
compare object identity and may not
be usable to open_by_handle_at(2) */
+#define AT_HANDLE_MNT_ID_UNIQUE 0x80 /* return the u64 unique mount id */
+
#if defined(__KERNEL__)
#define AT_GETATTR_NOSEC 0x80000000
#endif
diff --git a/tools/include/uapi/linux/fcntl.h b/tools/include/uapi/linux/fcntl.h
index 282e90aeb163..f671312ca481 100644
--- a/tools/include/uapi/linux/fcntl.h
+++ b/tools/include/uapi/linux/fcntl.h
@@ -85,22 +85,24 @@
#define DN_ATTRIB 0x00000020 /* File changed attibutes */
#define DN_MULTISHOT 0x80000000 /* Don't remove notifier */
+#define AT_FDCWD -100 /* Special value used to indicate
+ openat should use the current
+ working directory. */
+#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */
+
/*
- * The constants AT_REMOVEDIR and AT_EACCESS have the same value. AT_EACCESS is
- * meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
+ * The constants AT_REMOVEDIR and AT_EACCESS have the same value. AT_EACCESS
+ * is meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
* unlinkat. The two functions do completely different things and therefore,
* the flags can be allowed to overlap. For example, passing AT_REMOVEDIR to
* faccessat would be undefined behavior and thus treating it equivalent to
* AT_EACCESS is valid undefined behavior.
*/
-#define AT_FDCWD -100 /* Special value used to indicate
- openat should use the current
- working directory. */
-#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */
#define AT_EACCESS 0x200 /* Test access permitted for
effective IDs, not real IDs. */
#define AT_REMOVEDIR 0x200 /* Remove directory instead of
unlinking file. */
+
#define AT_SYMLINK_FOLLOW 0x400 /* Follow symbolic links. */
#define AT_NO_AUTOMOUNT 0x800 /* Suppress terminal automount traversal */
#define AT_EMPTY_PATH 0x1000 /* Allow empty relative pathname */
@@ -112,10 +114,22 @@
#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
-/* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
+/*
+ * All new purely-syscall-specific AT_* flags should consider using bits from
+ * 0xFF, but the bits used by RENAME_* (0x7) should be avoided in case users
+ * decide to pass AT_* flags to renameat2() by accident. These flag bits are
+ * free for re-use by other syscall's syscall-specific flags without worry.
+ */
+
+/*
+ * Flags for name_to_handle_at(2). To save AT_ flag space we re-use the
+ * AT_EACCESS/AT_REMOVEDIR bit for AT_HANDLE_FID.
+ */
#define AT_HANDLE_FID AT_REMOVEDIR /* file handle is needed to
compare object identity and may not
be usable to open_by_handle_at(2) */
+#define AT_HANDLE_MNT_ID_UNIQUE 0x80 /* returned mount id is the u64 unique mount id */
+
#if defined(__KERNEL__)
#define AT_GETATTR_NOSEC 0x80000000
#endif
---
base-commit: 584bbf439d0fa83d728ec49f3a38c581bdc828b4
change-id: 20240515-exportfs-u64-mount-id-9ebb5c58b53c
Best regards,
--
Aleksa Sarai <cyphar@cyphar.com>
^ permalink raw reply related
* Re: [PATCH v2 0/1] Add FUTEX_SPIN operation
From: Mathieu Desnoyers @ 2024-05-23 20:39 UTC (permalink / raw)
To: André Almeida, Peter Zijlstra
Cc: linux-kernel, Thomas Gleixner, Paul E . McKenney, Boqun Feng,
H . Peter Anvin, Paul Turner, linux-api, Christian Brauner,
Florian Weimer, David.Laight, carlos, Peter Oskolkov,
Alexander Mikhalitsyn, Chris Kennelly, Ingo Molnar, Darren Hart,
Davidlohr Bueso, libc-alpha, Steven Rostedt, Jonathan Corbet,
Noah Goldstein, longman, kernel-dev
In-Reply-To: <20240523200704.281514-1-andrealmeid@igalia.com>
On 2024-05-23 16:07, André Almeida wrote:
> Hi,
>
> In the last LPC, Mathieu Desnoyers and I presented[0] a proposal to extend the
> rseq interface to be able to implement spin locks in userspace correctly. Thomas
> Gleixner agreed that this is something that Linux could improve, but asked for
> an alternative proposal first: a futex operation that allows to spin a user
> lock inside the kernel. This patchset implements a prototype of this idea for
> further discussion.
>
> With FUTEX2_SPIN flag set during a futex_wait(), the futex value is expected to
> be the TID of the lock owner. Then, the kernel gets the task_struct of the
> corresponding TID, and checks if it's running. It spins until the futex
> is awaken, the task is scheduled out or if a timeout happens. If the lock owner
> is scheduled out at any time, then the syscall follows the normal path of
> sleeping as usual. The user input is masked with FUTEX_TID_MASK so we have some
> bits to play.
>
> If the futex is awaken and we are spinning, we can return to userspace quickly,
> avoid the scheduling out and in again to wake from a futex_wait(), thus
> speeding up the wait operation. The user input is masked with FUTEX_TID_MASK so
> we have some bits to play.
>
> Christian Brauner suggested using pidfd to avoid race conditions, and I will
> implement that in the next patch iteration. I benchmarked the implementation
> measuring the time required to wait for a futex for a simple loop using the code
> at [2]. In my setup, the total wait time for 1000 futexes using the spin method
> was almost 10% lower than just using the normal futex wait:
>
> Testing with FUTEX2_SPIN | FUTEX_WAIT
> Total wait time: 8650089 usecs
>
> Testing with FUTEX_WAIT
> Total wait time: 9447291 usecs
>
> However, as I played with how long the lock owner would be busy, the
> benchmark results of spinning vs no spinning would match, showing that the
> spinning will be effective for some specific scheduling scenarios, but depending
> on the wait time, there's no big difference either spinning or not.
>
> [0] https://lpc.events/event/17/contributions/1481/
>
> You can find a small snippet to play with this interface here:
>
> [1] https://gist.github.com/andrealmeid/f0b8c93a3c7a5c50458247c47f7078e1
What exactly are you trying to benchmark here ? I've looked at this toy
program, and I suspect that most of the delay you observe is due to
initial scheduling of a newly cloned thread, because this is what is
repeatedly being done in the delay you measure.
I would recommend to change this benchmark program to measure something
meaningful, e.g.:
- N threads repeatedly contending on a lock, until a "stop" flag is set,
- run for "duration" seconds, after which main() sets a "stop" flag.
- delay loop of "work_delay" us within the lock critical section,
- delay loop of "inactive_delay" us between locking attempts,
- measure the time it takes to grab the lock, report stats on this,
- measure the total number of operations done within the given
"duration".
- report statistics on the number of operations per thread to see
the impact on fairness,
The run the program with the following constraints:
- Pin one thread per core, with nb thread <= nb cores. This should
be a best case scenario for spinning.
- Pin all threads to a single core. when nb threads > nb cores, this
should be the worse scenario for spinning.
- Groups things between those two extremes to see how things evolve.
I would not be surprised that if you measure relevant delays, you will
observe much better speedups than what you currently have.
Thanks,
Mathieu
>
> Changelog:
>
> v1: - s/PID/TID
> - masked user input with FUTEX_TID_MASK
> - add benchmark tool to the cover letter
> - dropped debug prints
> - added missing put_task_struct()
>
> André Almeida (1):
> futex: Add FUTEX_SPIN operation
>
> include/uapi/linux/futex.h | 2 +-
> kernel/futex/futex.h | 6 ++-
> kernel/futex/waitwake.c | 78 +++++++++++++++++++++++++++++++++++++-
> 3 files changed, 82 insertions(+), 4 deletions(-)
>
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
^ permalink raw reply
* Re: [PATCH v2 1/1] futex: Add FUTEX_SPIN operation
From: Mathieu Desnoyers @ 2024-05-23 20:20 UTC (permalink / raw)
To: André Almeida, Peter Zijlstra
Cc: linux-kernel, Thomas Gleixner, Paul E . McKenney, Boqun Feng,
H . Peter Anvin, Paul Turner, linux-api, Christian Brauner,
Florian Weimer, David.Laight, carlos, Peter Oskolkov,
Alexander Mikhalitsyn, Chris Kennelly, Ingo Molnar, Darren Hart,
Davidlohr Bueso, libc-alpha, Steven Rostedt, Jonathan Corbet,
Noah Goldstein, Daniel Colascione, longman, kernel-dev
In-Reply-To: <20240523200704.281514-2-andrealmeid@igalia.com>
On 2024-05-23 16:07, André Almeida wrote:
> Add a new mode for futex wait, the futex spin.
>
> Given the FUTEX2_SPIN flag, parse the futex value as the TID of the lock
> owner. Then, before going to the normal wait path, spins while the lock
> owner is running in a different CPU, to avoid the whole context switch
> operation and to quickly return to userspace. If the lock owner is not
> running, just sleep as the normal futex wait path.
>
> The user value is masked with FUTEX_TID_MASK, to allow some bits for
> future use.
>
> The check for the owner to be running or not is important to avoid
> spinning for something that won't be released quickly. Userspace is
> responsible on providing the proper TID, the kernel does a basic check.
>
> Signed-off-by: André Almeida <andrealmeid@igalia.com>
> ---
[...]
> +
> +static int futex_spin(struct futex_hash_bucket *hb, struct futex_q *q,
> + struct hrtimer_sleeper *timeout, u32 uval)
> +{
> + struct task_struct *p;
> + pid_t pid = uval & FUTEX_TID_MASK;
> +
> + p = find_get_task_by_vpid(pid);
> +
> + /* no task found, maybe it already exited */
> + if (!p) {
> + futex_q_unlock(hb);
> + return -EAGAIN;
> + }
> +
> + /* can't spin in a kernel task */
> + if (unlikely(p->flags & PF_KTHREAD)) {
> + put_task_struct(p);
> + futex_q_unlock(hb);
> + return -EPERM;
> + }
> +
> + futex_queue(q, hb);
> +
> + if (timeout)
> + hrtimer_sleeper_start_expires(timeout, HRTIMER_MODE_ABS);
> +
> + while (1) {
Infinite loops in other kernel/futex/ files appear to use "for (;;) {"
instead.
> + if (likely(!plist_node_empty(&q->list))) {
> + if (timeout && !timeout->task)
> + goto exit;
> +
> + if (task_on_cpu(p)) {
> + /* spin */
You may want to add a "cpu_relax();" here to lessen the
power consumption of this busy-loop.
> + continue;
> + } else {
> + /* task is not running, sleep */
> + break;
> + }
> + } else {
> + goto exit;
> + }
> + }
> +
> + /* spinning didn't work, go to the normal path */
> + set_current_state(TASK_INTERRUPTIBLE|TASK_FREEZABLE);
I wonder if flipping the order between:
set_current_state(TASK_INTERRUPTIBLE|TASK_FREEZABLE);
futex_queue(q, hb);
as done in futex_wait_queue() and what is done here matters ?
Does it introduce a race where a wakeup could be missed ?
If it's an issue, then setting the current state could be done
before invoking futex_queue(), and whenever the spin exits,
set it back to TASK_RUNNING.
> +
> + if (likely(!plist_node_empty(&q->list))) {
> + if (!timeout || timeout->task)
> + schedule();
> + }
> +
> + __set_current_state(TASK_RUNNING);
> +
> +exit:
> + put_task_struct(p);
> + return 0;
> +}
> +
> /**
> * futex_unqueue_multiple - Remove various futexes from their hash bucket
> * @v: The list of futexes to unqueue
> @@ -665,8 +732,15 @@ int __futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
> if (ret)
> return ret;
>
> - /* futex_queue and wait for wakeup, timeout, or a signal. */
> - futex_wait_queue(hb, &q, to);
> + if (flags & FLAGS_SPIN) {
> + ret = futex_spin(hb, &q, to, val);
The empty line below could be removed.
Thanks,
Mathieu
> +
> + if (ret)
> + return ret;
> + } else {
> + /* futex_queue and wait for wakeup, timeout, or a signal. */
> + futex_wait_queue(hb, &q, to);
> + }
>
> /* If we were woken (and unqueued), we succeeded, whatever. */
> if (!futex_unqueue(&q))
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
^ permalink raw reply
* [PATCH v2 1/1] futex: Add FUTEX_SPIN operation
From: André Almeida @ 2024-05-23 20:07 UTC (permalink / raw)
To: Mathieu Desnoyers, Peter Zijlstra
Cc: linux-kernel, Thomas Gleixner, Paul E . McKenney, Boqun Feng,
H . Peter Anvin, Paul Turner, linux-api, Christian Brauner,
Florian Weimer, David.Laight, carlos, Peter Oskolkov,
Alexander Mikhalitsyn, Chris Kennelly, Ingo Molnar, Darren Hart,
Davidlohr Bueso, André Almeida, libc-alpha, Steven Rostedt,
Jonathan Corbet, Noah Goldstein, Daniel Colascione, longman,
kernel-dev
In-Reply-To: <20240523200704.281514-1-andrealmeid@igalia.com>
Add a new mode for futex wait, the futex spin.
Given the FUTEX2_SPIN flag, parse the futex value as the TID of the lock
owner. Then, before going to the normal wait path, spins while the lock
owner is running in a different CPU, to avoid the whole context switch
operation and to quickly return to userspace. If the lock owner is not
running, just sleep as the normal futex wait path.
The user value is masked with FUTEX_TID_MASK, to allow some bits for
future use.
The check for the owner to be running or not is important to avoid
spinning for something that won't be released quickly. Userspace is
responsible on providing the proper TID, the kernel does a basic check.
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
include/uapi/linux/futex.h | 2 +-
kernel/futex/futex.h | 6 ++-
kernel/futex/waitwake.c | 78 +++++++++++++++++++++++++++++++++++++-
3 files changed, 82 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/futex.h b/include/uapi/linux/futex.h
index d2ee625ea189..d77d692ffac2 100644
--- a/include/uapi/linux/futex.h
+++ b/include/uapi/linux/futex.h
@@ -63,7 +63,7 @@
#define FUTEX2_SIZE_U32 0x02
#define FUTEX2_SIZE_U64 0x03
#define FUTEX2_NUMA 0x04
- /* 0x08 */
+#define FUTEX2_SPIN 0x08
/* 0x10 */
/* 0x20 */
/* 0x40 */
diff --git a/kernel/futex/futex.h b/kernel/futex/futex.h
index 8b195d06f4e8..180c1c10dc81 100644
--- a/kernel/futex/futex.h
+++ b/kernel/futex/futex.h
@@ -37,6 +37,7 @@
#define FLAGS_HAS_TIMEOUT 0x0040
#define FLAGS_NUMA 0x0080
#define FLAGS_STRICT 0x0100
+#define FLAGS_SPIN 0x0200
/* FUTEX_ to FLAGS_ */
static inline unsigned int futex_to_flags(unsigned int op)
@@ -52,7 +53,7 @@ static inline unsigned int futex_to_flags(unsigned int op)
return flags;
}
-#define FUTEX2_VALID_MASK (FUTEX2_SIZE_MASK | FUTEX2_PRIVATE)
+#define FUTEX2_VALID_MASK (FUTEX2_SIZE_MASK | FUTEX2_PRIVATE | FUTEX2_SPIN)
/* FUTEX2_ to FLAGS_ */
static inline unsigned int futex2_to_flags(unsigned int flags2)
@@ -65,6 +66,9 @@ static inline unsigned int futex2_to_flags(unsigned int flags2)
if (flags2 & FUTEX2_NUMA)
flags |= FLAGS_NUMA;
+ if (flags2 & FUTEX2_SPIN)
+ flags |= FLAGS_SPIN;
+
return flags;
}
diff --git a/kernel/futex/waitwake.c b/kernel/futex/waitwake.c
index 3a10375d9521..276c03804b92 100644
--- a/kernel/futex/waitwake.c
+++ b/kernel/futex/waitwake.c
@@ -372,6 +372,73 @@ void futex_wait_queue(struct futex_hash_bucket *hb, struct futex_q *q,
__set_current_state(TASK_RUNNING);
}
+static inline bool task_on_cpu(struct task_struct *p)
+{
+#ifdef CONFIG_SMP
+ return !!(p->on_cpu);
+#else
+ return false;
+#endif
+}
+
+static int futex_spin(struct futex_hash_bucket *hb, struct futex_q *q,
+ struct hrtimer_sleeper *timeout, u32 uval)
+{
+ struct task_struct *p;
+ pid_t pid = uval & FUTEX_TID_MASK;
+
+ p = find_get_task_by_vpid(pid);
+
+ /* no task found, maybe it already exited */
+ if (!p) {
+ futex_q_unlock(hb);
+ return -EAGAIN;
+ }
+
+ /* can't spin in a kernel task */
+ if (unlikely(p->flags & PF_KTHREAD)) {
+ put_task_struct(p);
+ futex_q_unlock(hb);
+ return -EPERM;
+ }
+
+ futex_queue(q, hb);
+
+ if (timeout)
+ hrtimer_sleeper_start_expires(timeout, HRTIMER_MODE_ABS);
+
+ while (1) {
+ if (likely(!plist_node_empty(&q->list))) {
+ if (timeout && !timeout->task)
+ goto exit;
+
+ if (task_on_cpu(p)) {
+ /* spin */
+ continue;
+ } else {
+ /* task is not running, sleep */
+ break;
+ }
+ } else {
+ goto exit;
+ }
+ }
+
+ /* spinning didn't work, go to the normal path */
+ set_current_state(TASK_INTERRUPTIBLE|TASK_FREEZABLE);
+
+ if (likely(!plist_node_empty(&q->list))) {
+ if (!timeout || timeout->task)
+ schedule();
+ }
+
+ __set_current_state(TASK_RUNNING);
+
+exit:
+ put_task_struct(p);
+ return 0;
+}
+
/**
* futex_unqueue_multiple - Remove various futexes from their hash bucket
* @v: The list of futexes to unqueue
@@ -665,8 +732,15 @@ int __futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
if (ret)
return ret;
- /* futex_queue and wait for wakeup, timeout, or a signal. */
- futex_wait_queue(hb, &q, to);
+ if (flags & FLAGS_SPIN) {
+ ret = futex_spin(hb, &q, to, val);
+
+ if (ret)
+ return ret;
+ } else {
+ /* futex_queue and wait for wakeup, timeout, or a signal. */
+ futex_wait_queue(hb, &q, to);
+ }
/* If we were woken (and unqueued), we succeeded, whatever. */
if (!futex_unqueue(&q))
--
2.45.1
^ permalink raw reply related
* [PATCH v2 0/1] Add FUTEX_SPIN operation
From: André Almeida @ 2024-05-23 20:07 UTC (permalink / raw)
To: Mathieu Desnoyers, Peter Zijlstra
Cc: linux-kernel, Thomas Gleixner, Paul E . McKenney, Boqun Feng,
H . Peter Anvin, Paul Turner, linux-api, Christian Brauner,
Florian Weimer, David.Laight, carlos, Peter Oskolkov,
Alexander Mikhalitsyn, Chris Kennelly, Ingo Molnar, Darren Hart,
Davidlohr Bueso, André Almeida, libc-alpha, Steven Rostedt,
Jonathan Corbet, Noah Goldstein, Daniel Colascione, longman,
kernel-dev
Hi,
In the last LPC, Mathieu Desnoyers and I presented[0] a proposal to extend the
rseq interface to be able to implement spin locks in userspace correctly. Thomas
Gleixner agreed that this is something that Linux could improve, but asked for
an alternative proposal first: a futex operation that allows to spin a user
lock inside the kernel. This patchset implements a prototype of this idea for
further discussion.
With FUTEX2_SPIN flag set during a futex_wait(), the futex value is expected to
be the TID of the lock owner. Then, the kernel gets the task_struct of the
corresponding TID, and checks if it's running. It spins until the futex
is awaken, the task is scheduled out or if a timeout happens. If the lock owner
is scheduled out at any time, then the syscall follows the normal path of
sleeping as usual. The user input is masked with FUTEX_TID_MASK so we have some
bits to play.
If the futex is awaken and we are spinning, we can return to userspace quickly,
avoid the scheduling out and in again to wake from a futex_wait(), thus
speeding up the wait operation. The user input is masked with FUTEX_TID_MASK so
we have some bits to play.
Christian Brauner suggested using pidfd to avoid race conditions, and I will
implement that in the next patch iteration. I benchmarked the implementation
measuring the time required to wait for a futex for a simple loop using the code
at [2]. In my setup, the total wait time for 1000 futexes using the spin method
was almost 10% lower than just using the normal futex wait:
Testing with FUTEX2_SPIN | FUTEX_WAIT
Total wait time: 8650089 usecs
Testing with FUTEX_WAIT
Total wait time: 9447291 usecs
However, as I played with how long the lock owner would be busy, the
benchmark results of spinning vs no spinning would match, showing that the
spinning will be effective for some specific scheduling scenarios, but depending
on the wait time, there's no big difference either spinning or not.
[0] https://lpc.events/event/17/contributions/1481/
You can find a small snippet to play with this interface here:
[1] https://gist.github.com/andrealmeid/f0b8c93a3c7a5c50458247c47f7078e1
Changelog:
v1: - s/PID/TID
- masked user input with FUTEX_TID_MASK
- add benchmark tool to the cover letter
- dropped debug prints
- added missing put_task_struct()
André Almeida (1):
futex: Add FUTEX_SPIN operation
include/uapi/linux/futex.h | 2 +-
kernel/futex/futex.h | 6 ++-
kernel/futex/waitwake.c | 78 +++++++++++++++++++++++++++++++++++++-
3 files changed, 82 insertions(+), 4 deletions(-)
--
2.45.1
^ permalink raw reply
* [PATCHv7 9/9] man2: Add uretprobe syscall page
From: Jiri Olsa @ 2024-05-23 12:11 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: Alejandro Colomar, linux-kernel, linux-trace-kernel, linux-api,
linux-man, x86, bpf, Song Liu, Yonghong Song, John Fastabend,
Peter Zijlstra, Thomas Gleixner, Borislav Petkov (AMD),
Ingo Molnar, Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240523121149.575616-1-jolsa@kernel.org>
Adding man page for new uretprobe syscall.
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
man/man2/uretprobe.2 | 56 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 56 insertions(+)
create mode 100644 man/man2/uretprobe.2
diff --git a/man/man2/uretprobe.2 b/man/man2/uretprobe.2
new file mode 100644
index 000000000000..cf1c2b0d852e
--- /dev/null
+++ b/man/man2/uretprobe.2
@@ -0,0 +1,56 @@
+.\" Copyright (C) 2024, Jiri Olsa <jolsa@kernel.org>
+.\"
+.\" SPDX-License-Identifier: Linux-man-pages-copyleft
+.\"
+.TH uretprobe 2 (date) "Linux man-pages (unreleased)"
+.SH NAME
+uretprobe \- execute pending return uprobes
+.SH SYNOPSIS
+.nf
+.B int uretprobe(void)
+.fi
+.SH DESCRIPTION
+The
+.BR uretprobe ()
+system call is an alternative to breakpoint instructions for triggering return
+uprobe consumers.
+.P
+Calls to
+.BR uretprobe ()
+system call are only made from the user-space trampoline provided by the kernel.
+Calls from any other place result in a
+.BR SIGILL .
+.SH RETURN VALUE
+The
+.BR uretprobe ()
+system call return value is architecture-specific.
+.SH ERRORS
+.TP
+.B SIGILL
+The
+.BR uretprobe ()
+system call was called by a user-space program.
+.SH VERSIONS
+Details of the
+.BR uretprobe ()
+system call behavior vary across systems.
+.SH STANDARDS
+None.
+.SH HISTORY
+TBD
+.SH NOTES
+The
+.BR uretprobe ()
+system call was initially introduced for the x86_64 architecture
+where it was shown to be faster than breakpoint traps.
+It might be extended to other architectures.
+.P
+The
+.BR uretprobe ()
+system call exists only to allow the invocation of return uprobe consumers.
+It should
+.B never
+be called directly.
+Details of the arguments (if any) passed to
+.BR uretprobe ()
+and the return value are architecture-specific.
--
2.45.1
^ permalink raw reply related
* [PATCHv7 bpf-next 8/9] selftests/bpf: Add uretprobe shadow stack test
From: Jiri Olsa @ 2024-05-23 12:11 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240523121149.575616-1-jolsa@kernel.org>
Adding uretprobe shadow stack test that runs all existing
uretprobe tests with shadow stack enabled if it's available.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../selftests/bpf/prog_tests/uprobe_syscall.c | 60 +++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
index 3ef324c2db50..fda456401284 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
@@ -9,6 +9,9 @@
#include <linux/compiler.h>
#include <linux/stringify.h>
#include <sys/wait.h>
+#include <sys/syscall.h>
+#include <sys/prctl.h>
+#include <asm/prctl.h>
#include "uprobe_syscall.skel.h"
#include "uprobe_syscall_executed.skel.h"
@@ -297,6 +300,56 @@ static void test_uretprobe_syscall_call(void)
close(go[1]);
close(go[0]);
}
+
+/*
+ * Borrowed from tools/testing/selftests/x86/test_shadow_stack.c.
+ *
+ * For use in inline enablement of shadow stack.
+ *
+ * The program can't return from the point where shadow stack gets enabled
+ * because there will be no address on the shadow stack. So it can't use
+ * syscall() for enablement, since it is a function.
+ *
+ * Based on code from nolibc.h. Keep a copy here because this can't pull
+ * in all of nolibc.h.
+ */
+#define ARCH_PRCTL(arg1, arg2) \
+({ \
+ long _ret; \
+ register long _num asm("eax") = __NR_arch_prctl; \
+ register long _arg1 asm("rdi") = (long)(arg1); \
+ register long _arg2 asm("rsi") = (long)(arg2); \
+ \
+ asm volatile ( \
+ "syscall\n" \
+ : "=a"(_ret) \
+ : "r"(_arg1), "r"(_arg2), \
+ "0"(_num) \
+ : "rcx", "r11", "memory", "cc" \
+ ); \
+ _ret; \
+})
+
+#ifndef ARCH_SHSTK_ENABLE
+#define ARCH_SHSTK_ENABLE 0x5001
+#define ARCH_SHSTK_DISABLE 0x5002
+#define ARCH_SHSTK_SHSTK (1ULL << 0)
+#endif
+
+static void test_uretprobe_shadow_stack(void)
+{
+ if (ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK)) {
+ test__skip();
+ return;
+ }
+
+ /* Run all of the uretprobe tests. */
+ test_uretprobe_regs_equal();
+ test_uretprobe_regs_change();
+ test_uretprobe_syscall_call();
+
+ ARCH_PRCTL(ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK);
+}
#else
static void test_uretprobe_regs_equal(void)
{
@@ -312,6 +365,11 @@ static void test_uretprobe_syscall_call(void)
{
test__skip();
}
+
+static void test_uretprobe_shadow_stack(void)
+{
+ test__skip();
+}
#endif
void test_uprobe_syscall(void)
@@ -322,4 +380,6 @@ void test_uprobe_syscall(void)
test_uretprobe_regs_change();
if (test__start_subtest("uretprobe_syscall_call"))
test_uretprobe_syscall_call();
+ if (test__start_subtest("uretprobe_shadow_stack"))
+ test_uretprobe_shadow_stack();
}
--
2.45.1
^ permalink raw reply related
* [PATCHv7 bpf-next 7/9] selftests/bpf: Add uretprobe syscall call from user space test
From: Jiri Olsa @ 2024-05-23 12:11 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240523121149.575616-1-jolsa@kernel.org>
Adding test to verify that when called from outside of the
trampoline provided by kernel, the uretprobe syscall will cause
calling process to receive SIGILL signal and the attached bpf
program is not executed.
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../selftests/bpf/prog_tests/uprobe_syscall.c | 95 +++++++++++++++++++
.../bpf/progs/uprobe_syscall_executed.c | 17 ++++
2 files changed, 112 insertions(+)
create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
index 1a50cd35205d..3ef324c2db50 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
@@ -7,7 +7,10 @@
#include <unistd.h>
#include <asm/ptrace.h>
#include <linux/compiler.h>
+#include <linux/stringify.h>
+#include <sys/wait.h>
#include "uprobe_syscall.skel.h"
+#include "uprobe_syscall_executed.skel.h"
__naked unsigned long uretprobe_regs_trigger(void)
{
@@ -209,6 +212,91 @@ static void test_uretprobe_regs_change(void)
}
}
+#ifndef __NR_uretprobe
+#define __NR_uretprobe 462
+#endif
+
+__naked unsigned long uretprobe_syscall_call_1(void)
+{
+ /*
+ * Pretend we are uretprobe trampoline to trigger the return
+ * probe invocation in order to verify we get SIGILL.
+ */
+ asm volatile (
+ "pushq %rax\n"
+ "pushq %rcx\n"
+ "pushq %r11\n"
+ "movq $" __stringify(__NR_uretprobe) ", %rax\n"
+ "syscall\n"
+ "popq %r11\n"
+ "popq %rcx\n"
+ "retq\n"
+ );
+}
+
+__naked unsigned long uretprobe_syscall_call(void)
+{
+ asm volatile (
+ "call uretprobe_syscall_call_1\n"
+ "retq\n"
+ );
+}
+
+static void test_uretprobe_syscall_call(void)
+{
+ LIBBPF_OPTS(bpf_uprobe_multi_opts, opts,
+ .retprobe = true,
+ );
+ struct uprobe_syscall_executed *skel;
+ int pid, status, err, go[2], c;
+
+ if (ASSERT_OK(pipe(go), "pipe"))
+ return;
+
+ skel = uprobe_syscall_executed__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "uprobe_syscall_executed__open_and_load"))
+ goto cleanup;
+
+ pid = fork();
+ if (!ASSERT_GE(pid, 0, "fork"))
+ goto cleanup;
+
+ /* child */
+ if (pid == 0) {
+ close(go[1]);
+
+ /* wait for parent's kick */
+ err = read(go[0], &c, 1);
+ if (err != 1)
+ exit(-1);
+
+ uretprobe_syscall_call();
+ _exit(0);
+ }
+
+ skel->links.test = bpf_program__attach_uprobe_multi(skel->progs.test, pid,
+ "/proc/self/exe",
+ "uretprobe_syscall_call", &opts);
+ if (!ASSERT_OK_PTR(skel->links.test, "bpf_program__attach_uprobe_multi"))
+ goto cleanup;
+
+ /* kick the child */
+ write(go[1], &c, 1);
+ err = waitpid(pid, &status, 0);
+ ASSERT_EQ(err, pid, "waitpid");
+
+ /* verify the child got killed with SIGILL */
+ ASSERT_EQ(WIFSIGNALED(status), 1, "WIFSIGNALED");
+ ASSERT_EQ(WTERMSIG(status), SIGILL, "WTERMSIG");
+
+ /* verify the uretprobe program wasn't called */
+ ASSERT_EQ(skel->bss->executed, 0, "executed");
+
+cleanup:
+ uprobe_syscall_executed__destroy(skel);
+ close(go[1]);
+ close(go[0]);
+}
#else
static void test_uretprobe_regs_equal(void)
{
@@ -219,6 +307,11 @@ static void test_uretprobe_regs_change(void)
{
test__skip();
}
+
+static void test_uretprobe_syscall_call(void)
+{
+ test__skip();
+}
#endif
void test_uprobe_syscall(void)
@@ -227,4 +320,6 @@ void test_uprobe_syscall(void)
test_uretprobe_regs_equal();
if (test__start_subtest("uretprobe_regs_change"))
test_uretprobe_regs_change();
+ if (test__start_subtest("uretprobe_syscall_call"))
+ test_uretprobe_syscall_call();
}
diff --git a/tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c b/tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c
new file mode 100644
index 000000000000..0d7f1a7db2e2
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <string.h>
+
+struct pt_regs regs;
+
+char _license[] SEC("license") = "GPL";
+
+int executed = 0;
+
+SEC("uretprobe.multi")
+int test(struct pt_regs *regs)
+{
+ executed = 1;
+ return 0;
+}
--
2.45.1
^ permalink raw reply related
* [PATCHv7 bpf-next 6/9] selftests/bpf: Add uretprobe syscall test for regs changes
From: Jiri Olsa @ 2024-05-23 12:11 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240523121149.575616-1-jolsa@kernel.org>
Adding test that creates uprobe consumer on uretprobe which changes some
of the registers. Making sure the changed registers are propagated to the
user space when the ureptobe syscall trampoline is used on x86_64.
To be able to do this, adding support to bpf_testmod to create uprobe via
new attribute file:
/sys/kernel/bpf_testmod_uprobe
This file is expecting file offset and creates related uprobe on current
process exe file and removes existing uprobe if offset is 0. The can be
only single uprobe at any time.
The uprobe has specific consumer that changes registers used in ureprobe
syscall trampoline and which are later checked in the test.
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 123 +++++++++++++++++-
.../selftests/bpf/prog_tests/uprobe_syscall.c | 67 ++++++++++
2 files changed, 189 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 2a18bd320e92..b0132a342bb5 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -18,6 +18,7 @@
#include <linux/in6.h>
#include <linux/un.h>
#include <net/sock.h>
+#include <linux/namei.h>
#include "bpf_testmod.h"
#include "bpf_testmod_kfunc.h"
@@ -358,6 +359,119 @@ static struct bin_attribute bin_attr_bpf_testmod_file __ro_after_init = {
.write = bpf_testmod_test_write,
};
+/* bpf_testmod_uprobe sysfs attribute is so far enabled for x86_64 only,
+ * please see test_uretprobe_regs_change test
+ */
+#ifdef __x86_64__
+
+static int
+uprobe_ret_handler(struct uprobe_consumer *self, unsigned long func,
+ struct pt_regs *regs)
+
+{
+ regs->ax = 0x12345678deadbeef;
+ regs->cx = 0x87654321feebdaed;
+ regs->r11 = (u64) -1;
+ return true;
+}
+
+struct testmod_uprobe {
+ struct path path;
+ loff_t offset;
+ struct uprobe_consumer consumer;
+};
+
+static DEFINE_MUTEX(testmod_uprobe_mutex);
+
+static struct testmod_uprobe uprobe = {
+ .consumer.ret_handler = uprobe_ret_handler,
+};
+
+static int testmod_register_uprobe(loff_t offset)
+{
+ int err = -EBUSY;
+
+ if (uprobe.offset)
+ return -EBUSY;
+
+ mutex_lock(&testmod_uprobe_mutex);
+
+ if (uprobe.offset)
+ goto out;
+
+ err = kern_path("/proc/self/exe", LOOKUP_FOLLOW, &uprobe.path);
+ if (err)
+ goto out;
+
+ err = uprobe_register_refctr(d_real_inode(uprobe.path.dentry),
+ offset, 0, &uprobe.consumer);
+ if (err)
+ path_put(&uprobe.path);
+ else
+ uprobe.offset = offset;
+
+out:
+ mutex_unlock(&testmod_uprobe_mutex);
+ return err;
+}
+
+static void testmod_unregister_uprobe(void)
+{
+ mutex_lock(&testmod_uprobe_mutex);
+
+ if (uprobe.offset) {
+ uprobe_unregister(d_real_inode(uprobe.path.dentry),
+ uprobe.offset, &uprobe.consumer);
+ uprobe.offset = 0;
+ }
+
+ mutex_unlock(&testmod_uprobe_mutex);
+}
+
+static ssize_t
+bpf_testmod_uprobe_write(struct file *file, struct kobject *kobj,
+ struct bin_attribute *bin_attr,
+ char *buf, loff_t off, size_t len)
+{
+ unsigned long offset = 0;
+ int err = 0;
+
+ if (kstrtoul(buf, 0, &offset))
+ return -EINVAL;
+
+ if (offset)
+ err = testmod_register_uprobe(offset);
+ else
+ testmod_unregister_uprobe();
+
+ return err ?: strlen(buf);
+}
+
+static struct bin_attribute bin_attr_bpf_testmod_uprobe_file __ro_after_init = {
+ .attr = { .name = "bpf_testmod_uprobe", .mode = 0666, },
+ .write = bpf_testmod_uprobe_write,
+};
+
+static int register_bpf_testmod_uprobe(void)
+{
+ return sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_uprobe_file);
+}
+
+static void unregister_bpf_testmod_uprobe(void)
+{
+ testmod_unregister_uprobe();
+ sysfs_remove_bin_file(kernel_kobj, &bin_attr_bpf_testmod_uprobe_file);
+}
+
+#else
+static int register_bpf_testmod_uprobe(void)
+{
+ return 0;
+}
+
+static void unregister_bpf_testmod_uprobe(void) { }
+#endif
+
BTF_KFUNCS_START(bpf_testmod_common_kfunc_ids)
BTF_ID_FLAGS(func, bpf_iter_testmod_seq_new, KF_ITER_NEW)
BTF_ID_FLAGS(func, bpf_iter_testmod_seq_next, KF_ITER_NEXT | KF_RET_NULL)
@@ -912,7 +1026,13 @@ static int bpf_testmod_init(void)
return -EINVAL;
sock = NULL;
mutex_init(&sock_lock);
- return sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
+ ret = sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
+ if (ret < 0)
+ return ret;
+ ret = register_bpf_testmod_uprobe();
+ if (ret < 0)
+ return ret;
+ return 0;
}
static void bpf_testmod_exit(void)
@@ -927,6 +1047,7 @@ static void bpf_testmod_exit(void)
bpf_kfunc_close_sock();
sysfs_remove_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
+ unregister_bpf_testmod_uprobe();
}
module_init(bpf_testmod_init);
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
index 311ac19d8992..1a50cd35205d 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
@@ -149,15 +149,82 @@ static void test_uretprobe_regs_equal(void)
cleanup:
uprobe_syscall__destroy(skel);
}
+
+#define BPF_TESTMOD_UPROBE_TEST_FILE "/sys/kernel/bpf_testmod_uprobe"
+
+static int write_bpf_testmod_uprobe(unsigned long offset)
+{
+ size_t n, ret;
+ char buf[30];
+ int fd;
+
+ n = sprintf(buf, "%lu", offset);
+
+ fd = open(BPF_TESTMOD_UPROBE_TEST_FILE, O_WRONLY);
+ if (fd < 0)
+ return -errno;
+
+ ret = write(fd, buf, n);
+ close(fd);
+ return ret != n ? (int) ret : 0;
+}
+
+static void test_uretprobe_regs_change(void)
+{
+ struct pt_regs before = {}, after = {};
+ unsigned long *pb = (unsigned long *) &before;
+ unsigned long *pa = (unsigned long *) &after;
+ unsigned long cnt = sizeof(before)/sizeof(*pb);
+ unsigned int i, err, offset;
+
+ offset = get_uprobe_offset(uretprobe_regs_trigger);
+
+ err = write_bpf_testmod_uprobe(offset);
+ if (!ASSERT_OK(err, "register_uprobe"))
+ return;
+
+ uretprobe_regs(&before, &after);
+
+ err = write_bpf_testmod_uprobe(0);
+ if (!ASSERT_OK(err, "unregister_uprobe"))
+ return;
+
+ for (i = 0; i < cnt; i++) {
+ unsigned int offset = i * sizeof(unsigned long);
+
+ switch (offset) {
+ case offsetof(struct pt_regs, rax):
+ ASSERT_EQ(pa[i], 0x12345678deadbeef, "rax");
+ break;
+ case offsetof(struct pt_regs, rcx):
+ ASSERT_EQ(pa[i], 0x87654321feebdaed, "rcx");
+ break;
+ case offsetof(struct pt_regs, r11):
+ ASSERT_EQ(pa[i], (__u64) -1, "r11");
+ break;
+ default:
+ if (!ASSERT_EQ(pa[i], pb[i], "register before-after value check"))
+ fprintf(stdout, "failed register offset %u\n", offset);
+ }
+ }
+}
+
#else
static void test_uretprobe_regs_equal(void)
{
test__skip();
}
+
+static void test_uretprobe_regs_change(void)
+{
+ test__skip();
+}
#endif
void test_uprobe_syscall(void)
{
if (test__start_subtest("uretprobe_regs_equal"))
test_uretprobe_regs_equal();
+ if (test__start_subtest("uretprobe_regs_change"))
+ test_uretprobe_regs_change();
}
--
2.45.1
^ permalink raw reply related
* [PATCHv7 bpf-next 5/9] selftests/bpf: Add uretprobe syscall test for regs integrity
From: Jiri Olsa @ 2024-05-23 12:11 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240523121149.575616-1-jolsa@kernel.org>
Add uretprobe syscall test that compares register values before
and after the uretprobe is hit. It also compares the register
values seen from attached bpf program.
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/include/linux/compiler.h | 4 +
.../selftests/bpf/prog_tests/uprobe_syscall.c | 163 ++++++++++++++++++
.../selftests/bpf/progs/uprobe_syscall.c | 15 ++
3 files changed, 182 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall.c
diff --git a/tools/include/linux/compiler.h b/tools/include/linux/compiler.h
index 8a63a9913495..6f7f22ac9da5 100644
--- a/tools/include/linux/compiler.h
+++ b/tools/include/linux/compiler.h
@@ -62,6 +62,10 @@
#define __nocf_check __attribute__((nocf_check))
#endif
+#ifndef __naked
+#define __naked __attribute__((__naked__))
+#endif
+
/* Are two types/vars the same type (ignoring qualifiers)? */
#ifndef __same_type
# define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
new file mode 100644
index 000000000000..311ac19d8992
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
@@ -0,0 +1,163 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+
+#ifdef __x86_64__
+
+#include <unistd.h>
+#include <asm/ptrace.h>
+#include <linux/compiler.h>
+#include "uprobe_syscall.skel.h"
+
+__naked unsigned long uretprobe_regs_trigger(void)
+{
+ asm volatile (
+ "movq $0xdeadbeef, %rax\n"
+ "ret\n"
+ );
+}
+
+__naked void uretprobe_regs(struct pt_regs *before, struct pt_regs *after)
+{
+ asm volatile (
+ "movq %r15, 0(%rdi)\n"
+ "movq %r14, 8(%rdi)\n"
+ "movq %r13, 16(%rdi)\n"
+ "movq %r12, 24(%rdi)\n"
+ "movq %rbp, 32(%rdi)\n"
+ "movq %rbx, 40(%rdi)\n"
+ "movq %r11, 48(%rdi)\n"
+ "movq %r10, 56(%rdi)\n"
+ "movq %r9, 64(%rdi)\n"
+ "movq %r8, 72(%rdi)\n"
+ "movq %rax, 80(%rdi)\n"
+ "movq %rcx, 88(%rdi)\n"
+ "movq %rdx, 96(%rdi)\n"
+ "movq %rsi, 104(%rdi)\n"
+ "movq %rdi, 112(%rdi)\n"
+ "movq $0, 120(%rdi)\n" /* orig_rax */
+ "movq $0, 128(%rdi)\n" /* rip */
+ "movq $0, 136(%rdi)\n" /* cs */
+ "pushf\n"
+ "pop %rax\n"
+ "movq %rax, 144(%rdi)\n" /* eflags */
+ "movq %rsp, 152(%rdi)\n" /* rsp */
+ "movq $0, 160(%rdi)\n" /* ss */
+
+ /* save 2nd argument */
+ "pushq %rsi\n"
+ "call uretprobe_regs_trigger\n"
+
+ /* save return value and load 2nd argument pointer to rax */
+ "pushq %rax\n"
+ "movq 8(%rsp), %rax\n"
+
+ "movq %r15, 0(%rax)\n"
+ "movq %r14, 8(%rax)\n"
+ "movq %r13, 16(%rax)\n"
+ "movq %r12, 24(%rax)\n"
+ "movq %rbp, 32(%rax)\n"
+ "movq %rbx, 40(%rax)\n"
+ "movq %r11, 48(%rax)\n"
+ "movq %r10, 56(%rax)\n"
+ "movq %r9, 64(%rax)\n"
+ "movq %r8, 72(%rax)\n"
+ "movq %rcx, 88(%rax)\n"
+ "movq %rdx, 96(%rax)\n"
+ "movq %rsi, 104(%rax)\n"
+ "movq %rdi, 112(%rax)\n"
+ "movq $0, 120(%rax)\n" /* orig_rax */
+ "movq $0, 128(%rax)\n" /* rip */
+ "movq $0, 136(%rax)\n" /* cs */
+
+ /* restore return value and 2nd argument */
+ "pop %rax\n"
+ "pop %rsi\n"
+
+ "movq %rax, 80(%rsi)\n"
+
+ "pushf\n"
+ "pop %rax\n"
+
+ "movq %rax, 144(%rsi)\n" /* eflags */
+ "movq %rsp, 152(%rsi)\n" /* rsp */
+ "movq $0, 160(%rsi)\n" /* ss */
+ "ret\n"
+);
+}
+
+static void test_uretprobe_regs_equal(void)
+{
+ struct uprobe_syscall *skel = NULL;
+ struct pt_regs before = {}, after = {};
+ unsigned long *pb = (unsigned long *) &before;
+ unsigned long *pa = (unsigned long *) &after;
+ unsigned long *pp;
+ unsigned int i, cnt;
+ int err;
+
+ skel = uprobe_syscall__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "uprobe_syscall__open_and_load"))
+ goto cleanup;
+
+ err = uprobe_syscall__attach(skel);
+ if (!ASSERT_OK(err, "uprobe_syscall__attach"))
+ goto cleanup;
+
+ uretprobe_regs(&before, &after);
+
+ pp = (unsigned long *) &skel->bss->regs;
+ cnt = sizeof(before)/sizeof(*pb);
+
+ for (i = 0; i < cnt; i++) {
+ unsigned int offset = i * sizeof(unsigned long);
+
+ /*
+ * Check register before and after uretprobe_regs_trigger call
+ * that triggers the uretprobe.
+ */
+ switch (offset) {
+ case offsetof(struct pt_regs, rax):
+ ASSERT_EQ(pa[i], 0xdeadbeef, "return value");
+ break;
+ default:
+ if (!ASSERT_EQ(pb[i], pa[i], "register before-after value check"))
+ fprintf(stdout, "failed register offset %u\n", offset);
+ }
+
+ /*
+ * Check register seen from bpf program and register after
+ * uretprobe_regs_trigger call
+ */
+ switch (offset) {
+ /*
+ * These values will be different (not set in uretprobe_regs),
+ * we don't care.
+ */
+ case offsetof(struct pt_regs, orig_rax):
+ case offsetof(struct pt_regs, rip):
+ case offsetof(struct pt_regs, cs):
+ case offsetof(struct pt_regs, rsp):
+ case offsetof(struct pt_regs, ss):
+ break;
+ default:
+ if (!ASSERT_EQ(pp[i], pa[i], "register prog-after value check"))
+ fprintf(stdout, "failed register offset %u\n", offset);
+ }
+ }
+
+cleanup:
+ uprobe_syscall__destroy(skel);
+}
+#else
+static void test_uretprobe_regs_equal(void)
+{
+ test__skip();
+}
+#endif
+
+void test_uprobe_syscall(void)
+{
+ if (test__start_subtest("uretprobe_regs_equal"))
+ test_uretprobe_regs_equal();
+}
diff --git a/tools/testing/selftests/bpf/progs/uprobe_syscall.c b/tools/testing/selftests/bpf/progs/uprobe_syscall.c
new file mode 100644
index 000000000000..8a4fa6c7ef59
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/uprobe_syscall.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <string.h>
+
+struct pt_regs regs;
+
+char _license[] SEC("license") = "GPL";
+
+SEC("uretprobe//proc/self/exe:uretprobe_regs_trigger")
+int uretprobe(struct pt_regs *ctx)
+{
+ __builtin_memcpy(®s, ctx, sizeof(regs));
+ return 0;
+}
--
2.45.1
^ permalink raw reply related
* [PATCHv7 bpf-next 4/9] selftests/x86: Add return uprobe shadow stack test
From: Jiri Olsa @ 2024-05-23 12:11 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240523121149.575616-1-jolsa@kernel.org>
Adding return uprobe test for shadow stack and making sure it's
working properly. Borrowed some of the code from bpf selftests.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../testing/selftests/x86/test_shadow_stack.c | 145 ++++++++++++++++++
1 file changed, 145 insertions(+)
diff --git a/tools/testing/selftests/x86/test_shadow_stack.c b/tools/testing/selftests/x86/test_shadow_stack.c
index 757e6527f67e..e3501b7e2ecc 100644
--- a/tools/testing/selftests/x86/test_shadow_stack.c
+++ b/tools/testing/selftests/x86/test_shadow_stack.c
@@ -34,6 +34,7 @@
#include <sys/ptrace.h>
#include <sys/signal.h>
#include <linux/elf.h>
+#include <linux/perf_event.h>
/*
* Define the ABI defines if needed, so people can run the tests
@@ -681,6 +682,144 @@ int test_32bit(void)
return !segv_triggered;
}
+static int parse_uint_from_file(const char *file, const char *fmt)
+{
+ int err, ret;
+ FILE *f;
+
+ f = fopen(file, "re");
+ if (!f) {
+ err = -errno;
+ printf("failed to open '%s': %d\n", file, err);
+ return err;
+ }
+ err = fscanf(f, fmt, &ret);
+ if (err != 1) {
+ err = err == EOF ? -EIO : -errno;
+ printf("failed to parse '%s': %d\n", file, err);
+ fclose(f);
+ return err;
+ }
+ fclose(f);
+ return ret;
+}
+
+static int determine_uprobe_perf_type(void)
+{
+ const char *file = "/sys/bus/event_source/devices/uprobe/type";
+
+ return parse_uint_from_file(file, "%d\n");
+}
+
+static int determine_uprobe_retprobe_bit(void)
+{
+ const char *file = "/sys/bus/event_source/devices/uprobe/format/retprobe";
+
+ return parse_uint_from_file(file, "config:%d\n");
+}
+
+static ssize_t get_uprobe_offset(const void *addr)
+{
+ size_t start, end, base;
+ char buf[256];
+ bool found = false;
+ FILE *f;
+
+ f = fopen("/proc/self/maps", "r");
+ if (!f)
+ return -errno;
+
+ while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
+ if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
+ found = true;
+ break;
+ }
+ }
+
+ fclose(f);
+
+ if (!found)
+ return -ESRCH;
+
+ return (uintptr_t)addr - start + base;
+}
+
+static __attribute__((noinline)) void uretprobe_trigger(void)
+{
+ asm volatile ("");
+}
+
+/*
+ * This test setups return uprobe, which is sensitive to shadow stack
+ * (crashes without extra fix). After executing the uretprobe we fail
+ * the test if we receive SIGSEGV, no crash means we're good.
+ *
+ * Helper functions above borrowed from bpf selftests.
+ */
+static int test_uretprobe(void)
+{
+ const size_t attr_sz = sizeof(struct perf_event_attr);
+ const char *file = "/proc/self/exe";
+ int bit, fd = 0, type, err = 1;
+ struct perf_event_attr attr;
+ struct sigaction sa = {};
+ ssize_t offset;
+
+ type = determine_uprobe_perf_type();
+ if (type < 0) {
+ if (type == -ENOENT)
+ printf("[SKIP]\tUretprobe test, uprobes are not available\n");
+ return 0;
+ }
+
+ offset = get_uprobe_offset(uretprobe_trigger);
+ if (offset < 0)
+ return 1;
+
+ bit = determine_uprobe_retprobe_bit();
+ if (bit < 0)
+ return 1;
+
+ sa.sa_sigaction = segv_gp_handler;
+ sa.sa_flags = SA_SIGINFO;
+ if (sigaction(SIGSEGV, &sa, NULL))
+ return 1;
+
+ /* Setup return uprobe through perf event interface. */
+ memset(&attr, 0, attr_sz);
+ attr.size = attr_sz;
+ attr.type = type;
+ attr.config = 1 << bit;
+ attr.config1 = (__u64) (unsigned long) file;
+ attr.config2 = offset;
+
+ fd = syscall(__NR_perf_event_open, &attr, 0 /* pid */, -1 /* cpu */,
+ -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
+ if (fd < 0)
+ goto out;
+
+ if (sigsetjmp(jmp_buffer, 1))
+ goto out;
+
+ ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK);
+
+ /*
+ * This either segfaults and goes through sigsetjmp above
+ * or succeeds and we're good.
+ */
+ uretprobe_trigger();
+
+ printf("[OK]\tUretprobe test\n");
+ err = 0;
+
+out:
+ ARCH_PRCTL(ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK);
+ signal(SIGSEGV, SIG_DFL);
+ if (fd)
+ close(fd);
+ return err;
+}
+
void segv_handler_ptrace(int signum, siginfo_t *si, void *uc)
{
/* The SSP adjustment caused a segfault. */
@@ -867,6 +1006,12 @@ int main(int argc, char *argv[])
goto out;
}
+ if (test_uretprobe()) {
+ ret = 1;
+ printf("[FAIL]\turetprobe test\n");
+ goto out;
+ }
+
return ret;
out:
--
2.45.1
^ permalink raw reply related
* [PATCHv7 bpf-next 3/9] uprobe: Add uretprobe syscall to speed up return probe
From: Jiri Olsa @ 2024-05-23 12:11 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240523121149.575616-1-jolsa@kernel.org>
Adding uretprobe syscall instead of trap to speed up return probe.
At the moment the uretprobe setup/path is:
- install entry uprobe
- when the uprobe is hit, it overwrites probed function's return address
on stack with address of the trampoline that contains breakpoint
instruction
- the breakpoint trap code handles the uretprobe consumers execution and
jumps back to original return address
This patch replaces the above trampoline's breakpoint instruction with new
ureprobe syscall call. This syscall does exactly the same job as the trap
with some more extra work:
- syscall trampoline must save original value for rax/r11/rcx registers
on stack - rax is set to syscall number and r11/rcx are changed and
used by syscall instruction
- the syscall code reads the original values of those registers and
restore those values in task's pt_regs area
- only caller from trampoline exposed in '[uprobes]' is allowed,
the process will receive SIGILL signal otherwise
Even with some extra work, using the uretprobes syscall shows speed
improvement (compared to using standard breakpoint):
On Intel (11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz)
current:
uretprobe-nop : 1.498 ± 0.000M/s
uretprobe-push : 1.448 ± 0.001M/s
uretprobe-ret : 0.816 ± 0.001M/s
with the fix:
uretprobe-nop : 1.969 ± 0.002M/s < 31% speed up
uretprobe-push : 1.910 ± 0.000M/s < 31% speed up
uretprobe-ret : 0.934 ± 0.000M/s < 14% speed up
On Amd (AMD Ryzen 7 5700U)
current:
uretprobe-nop : 0.778 ± 0.001M/s
uretprobe-push : 0.744 ± 0.001M/s
uretprobe-ret : 0.540 ± 0.001M/s
with the fix:
uretprobe-nop : 0.860 ± 0.001M/s < 10% speed up
uretprobe-push : 0.818 ± 0.001M/s < 10% speed up
uretprobe-ret : 0.578 ± 0.000M/s < 7% speed up
The performance test spawns a thread that runs loop which triggers
uprobe with attached bpf program that increments the counter that
gets printed in results above.
The uprobe (and uretprobe) kind is determined by which instruction
is being patched with breakpoint instruction. That's also important
for uretprobes, because uprobe is installed for each uretprobe.
The performance test is part of bpf selftests:
tools/testing/selftests/bpf/run_bench_uprobes.sh
Note at the moment uretprobe syscall is supported only for native
64-bit process, compat process still uses standard breakpoint.
Note that when shadow stack is enabled the uretprobe syscall returns
via iret, which is slower than return via sysret, but won't cause the
shadow stack violation.
Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/include/asm/shstk.h | 2 +
arch/x86/kernel/shstk.c | 5 ++
arch/x86/kernel/uprobes.c | 117 +++++++++++++++++++++++++++++++++++
include/linux/uprobes.h | 3 +
kernel/events/uprobes.c | 24 ++++---
5 files changed, 144 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h
index 896909f306e3..4cb77e004615 100644
--- a/arch/x86/include/asm/shstk.h
+++ b/arch/x86/include/asm/shstk.h
@@ -22,6 +22,7 @@ void shstk_free(struct task_struct *p);
int setup_signal_shadow_stack(struct ksignal *ksig);
int restore_signal_shadow_stack(void);
int shstk_update_last_frame(unsigned long val);
+bool shstk_is_enabled(void);
#else
static inline long shstk_prctl(struct task_struct *task, int option,
unsigned long arg2) { return -EINVAL; }
@@ -33,6 +34,7 @@ static inline void shstk_free(struct task_struct *p) {}
static inline int setup_signal_shadow_stack(struct ksignal *ksig) { return 0; }
static inline int restore_signal_shadow_stack(void) { return 0; }
static inline int shstk_update_last_frame(unsigned long val) { return 0; }
+static inline bool shstk_is_enabled(void) { return false; }
#endif /* CONFIG_X86_USER_SHADOW_STACK */
#endif /* __ASSEMBLY__ */
diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
index 9797d4cdb78a..059685612362 100644
--- a/arch/x86/kernel/shstk.c
+++ b/arch/x86/kernel/shstk.c
@@ -588,3 +588,8 @@ int shstk_update_last_frame(unsigned long val)
ssp = get_user_shstk_addr();
return write_user_shstk_64((u64 __user *)ssp, (u64)val);
}
+
+bool shstk_is_enabled(void)
+{
+ return features_enabled(ARCH_SHSTK_SHSTK);
+}
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 6402fb3089d2..5a952c5ea66b 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -12,6 +12,7 @@
#include <linux/ptrace.h>
#include <linux/uprobes.h>
#include <linux/uaccess.h>
+#include <linux/syscalls.h>
#include <linux/kdebug.h>
#include <asm/processor.h>
@@ -308,6 +309,122 @@ static int uprobe_init_insn(struct arch_uprobe *auprobe, struct insn *insn, bool
}
#ifdef CONFIG_X86_64
+
+asm (
+ ".pushsection .rodata\n"
+ ".global uretprobe_trampoline_entry\n"
+ "uretprobe_trampoline_entry:\n"
+ "pushq %rax\n"
+ "pushq %rcx\n"
+ "pushq %r11\n"
+ "movq $" __stringify(__NR_uretprobe) ", %rax\n"
+ "syscall\n"
+ ".global uretprobe_syscall_check\n"
+ "uretprobe_syscall_check:\n"
+ "popq %r11\n"
+ "popq %rcx\n"
+
+ /* The uretprobe syscall replaces stored %rax value with final
+ * return address, so we don't restore %rax in here and just
+ * call ret.
+ */
+ "retq\n"
+ ".global uretprobe_trampoline_end\n"
+ "uretprobe_trampoline_end:\n"
+ ".popsection\n"
+);
+
+extern u8 uretprobe_trampoline_entry[];
+extern u8 uretprobe_trampoline_end[];
+extern u8 uretprobe_syscall_check[];
+
+void *arch_uprobe_trampoline(unsigned long *psize)
+{
+ static uprobe_opcode_t insn = UPROBE_SWBP_INSN;
+ struct pt_regs *regs = task_pt_regs(current);
+
+ /*
+ * At the moment the uretprobe syscall trampoline is supported
+ * only for native 64-bit process, the compat process still uses
+ * standard breakpoint.
+ */
+ if (user_64bit_mode(regs)) {
+ *psize = uretprobe_trampoline_end - uretprobe_trampoline_entry;
+ return uretprobe_trampoline_entry;
+ }
+
+ *psize = UPROBE_SWBP_INSN_SIZE;
+ return &insn;
+}
+
+static unsigned long trampoline_check_ip(void)
+{
+ unsigned long tramp = uprobe_get_trampoline_vaddr();
+
+ return tramp + (uretprobe_syscall_check - uretprobe_trampoline_entry);
+}
+
+SYSCALL_DEFINE0(uretprobe)
+{
+ struct pt_regs *regs = task_pt_regs(current);
+ unsigned long err, ip, sp, r11_cx_ax[3];
+
+ if (regs->ip != trampoline_check_ip())
+ goto sigill;
+
+ err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));
+ if (err)
+ goto sigill;
+
+ /* expose the "right" values of r11/cx/ax/sp to uprobe_consumer/s */
+ regs->r11 = r11_cx_ax[0];
+ regs->cx = r11_cx_ax[1];
+ regs->ax = r11_cx_ax[2];
+ regs->sp += sizeof(r11_cx_ax);
+ regs->orig_ax = -1;
+
+ ip = regs->ip;
+ sp = regs->sp;
+
+ uprobe_handle_trampoline(regs);
+
+ /*
+ * Some of the uprobe consumers has changed sp, we can do nothing,
+ * just return via iret.
+ * .. or shadow stack is enabled, in which case we need to skip
+ * return through the user space stack address.
+ */
+ if (regs->sp != sp || shstk_is_enabled())
+ return regs->ax;
+ regs->sp -= sizeof(r11_cx_ax);
+
+ /* for the case uprobe_consumer has changed r11/cx */
+ r11_cx_ax[0] = regs->r11;
+ r11_cx_ax[1] = regs->cx;
+
+ /*
+ * ax register is passed through as return value, so we can use
+ * its space on stack for ip value and jump to it through the
+ * trampoline's ret instruction
+ */
+ r11_cx_ax[2] = regs->ip;
+ regs->ip = ip;
+
+ err = copy_to_user((void __user *)regs->sp, r11_cx_ax, sizeof(r11_cx_ax));
+ if (err)
+ goto sigill;
+
+ /* ensure sysret, see do_syscall_64() */
+ regs->r11 = regs->flags;
+ regs->cx = regs->ip;
+
+ return regs->ax;
+
+sigill:
+ force_sig(SIGILL);
+ return -1;
+}
+
/*
* If arch_uprobe->insn doesn't use rip-relative addressing, return
* immediately. Otherwise, rewrite the instruction so that it accesses
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index f46e0ca0169c..b503fafb7fb3 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -138,6 +138,9 @@ extern bool arch_uretprobe_is_alive(struct return_instance *ret, enum rp_check c
extern bool arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
extern void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
void *src, unsigned long len);
+extern void uprobe_handle_trampoline(struct pt_regs *regs);
+extern void *arch_uprobe_trampoline(unsigned long *psize);
+extern unsigned long uprobe_get_trampoline_vaddr(void);
#else /* !CONFIG_UPROBES */
struct uprobes_state {
};
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 2c83ba776fc7..2816e65729ac 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1474,11 +1474,20 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
return ret;
}
+void * __weak arch_uprobe_trampoline(unsigned long *psize)
+{
+ static uprobe_opcode_t insn = UPROBE_SWBP_INSN;
+
+ *psize = UPROBE_SWBP_INSN_SIZE;
+ return &insn;
+}
+
static struct xol_area *__create_xol_area(unsigned long vaddr)
{
struct mm_struct *mm = current->mm;
- uprobe_opcode_t insn = UPROBE_SWBP_INSN;
+ unsigned long insns_size;
struct xol_area *area;
+ void *insns;
area = kmalloc(sizeof(*area), GFP_KERNEL);
if (unlikely(!area))
@@ -1502,7 +1511,8 @@ static struct xol_area *__create_xol_area(unsigned long vaddr)
/* Reserve the 1st slot for get_trampoline_vaddr() */
set_bit(0, area->bitmap);
atomic_set(&area->slot_count, 1);
- arch_uprobe_copy_ixol(area->pages[0], 0, &insn, UPROBE_SWBP_INSN_SIZE);
+ insns = arch_uprobe_trampoline(&insns_size);
+ arch_uprobe_copy_ixol(area->pages[0], 0, insns, insns_size);
if (!xol_add_vma(mm, area))
return area;
@@ -1827,7 +1837,7 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
*
* Returns -1 in case the xol_area is not allocated.
*/
-static unsigned long get_trampoline_vaddr(void)
+unsigned long uprobe_get_trampoline_vaddr(void)
{
struct xol_area *area;
unsigned long trampoline_vaddr = -1;
@@ -1878,7 +1888,7 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
if (!ri)
return;
- trampoline_vaddr = get_trampoline_vaddr();
+ trampoline_vaddr = uprobe_get_trampoline_vaddr();
orig_ret_vaddr = arch_uretprobe_hijack_return_addr(trampoline_vaddr, regs);
if (orig_ret_vaddr == -1)
goto fail;
@@ -2123,7 +2133,7 @@ static struct return_instance *find_next_ret_chain(struct return_instance *ri)
return ri;
}
-static void handle_trampoline(struct pt_regs *regs)
+void uprobe_handle_trampoline(struct pt_regs *regs)
{
struct uprobe_task *utask;
struct return_instance *ri, *next;
@@ -2187,8 +2197,8 @@ static void handle_swbp(struct pt_regs *regs)
int is_swbp;
bp_vaddr = uprobe_get_swbp_addr(regs);
- if (bp_vaddr == get_trampoline_vaddr())
- return handle_trampoline(regs);
+ if (bp_vaddr == uprobe_get_trampoline_vaddr())
+ return uprobe_handle_trampoline(regs);
uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
if (!uprobe) {
--
2.45.1
^ permalink raw reply related
* [PATCHv7 bpf-next 2/9] uprobe: Wire up uretprobe system call
From: Jiri Olsa @ 2024-05-23 12:11 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240523121149.575616-1-jolsa@kernel.org>
Wiring up uretprobe system call, which comes in following changes.
We need to do the wiring before, because the uretprobe implementation
needs the syscall number.
Note at the moment uretprobe syscall is supported only for native
64-bit process.
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
include/linux/syscalls.h | 2 ++
include/uapi/asm-generic/unistd.h | 5 ++++-
kernel/sys_ni.c | 2 ++
4 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index cc78226ffc35..47dfea0a827c 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -383,6 +383,7 @@
459 common lsm_get_self_attr sys_lsm_get_self_attr
460 common lsm_set_self_attr sys_lsm_set_self_attr
461 common lsm_list_modules sys_lsm_list_modules
+462 64 uretprobe sys_uretprobe
#
# Due to a historical design error, certain syscalls are numbered differently
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e619ac10cd23..5318e0e76799 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -972,6 +972,8 @@ asmlinkage long sys_lsm_list_modules(u64 *ids, u32 *size, u32 flags);
/* x86 */
asmlinkage long sys_ioperm(unsigned long from, unsigned long num, int on);
+asmlinkage long sys_uretprobe(void);
+
/* pciconfig: alpha, arm, arm64, ia64, sparc */
asmlinkage long sys_pciconfig_read(unsigned long bus, unsigned long dfn,
unsigned long off, unsigned long len,
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 75f00965ab15..8a747cd1d735 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -842,8 +842,11 @@ __SYSCALL(__NR_lsm_set_self_attr, sys_lsm_set_self_attr)
#define __NR_lsm_list_modules 461
__SYSCALL(__NR_lsm_list_modules, sys_lsm_list_modules)
+#define __NR_uretprobe 462
+__SYSCALL(__NR_uretprobe, sys_uretprobe)
+
#undef __NR_syscalls
-#define __NR_syscalls 462
+#define __NR_syscalls 463
/*
* 32 bit systems traditionally used different
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index faad00cce269..be6195e0d078 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -391,3 +391,5 @@ COND_SYSCALL(setuid16);
/* restartable sequence */
COND_SYSCALL(rseq);
+
+COND_SYSCALL(uretprobe);
--
2.45.1
^ permalink raw reply related
* [PATCHv7 bpf-next 1/9] x86/shstk: Make return uprobe work with shadow stack
From: Jiri Olsa @ 2024-05-23 12:11 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240523121149.575616-1-jolsa@kernel.org>
Currently the application with enabled shadow stack will crash
if it sets up return uprobe. The reason is the uretprobe kernel
code changes the user space task's stack, but does not update
shadow stack accordingly.
Adding new functions to update values on shadow stack and using
them in uprobe code to keep shadow stack in sync with uretprobe
changes to user stack.
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Fixes: 488af8ea7131 ("x86/shstk: Wire in shadow stack interface")
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/include/asm/shstk.h | 2 ++
arch/x86/kernel/shstk.c | 11 +++++++++++
arch/x86/kernel/uprobes.c | 7 ++++++-
3 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h
index 42fee8959df7..896909f306e3 100644
--- a/arch/x86/include/asm/shstk.h
+++ b/arch/x86/include/asm/shstk.h
@@ -21,6 +21,7 @@ unsigned long shstk_alloc_thread_stack(struct task_struct *p, unsigned long clon
void shstk_free(struct task_struct *p);
int setup_signal_shadow_stack(struct ksignal *ksig);
int restore_signal_shadow_stack(void);
+int shstk_update_last_frame(unsigned long val);
#else
static inline long shstk_prctl(struct task_struct *task, int option,
unsigned long arg2) { return -EINVAL; }
@@ -31,6 +32,7 @@ static inline unsigned long shstk_alloc_thread_stack(struct task_struct *p,
static inline void shstk_free(struct task_struct *p) {}
static inline int setup_signal_shadow_stack(struct ksignal *ksig) { return 0; }
static inline int restore_signal_shadow_stack(void) { return 0; }
+static inline int shstk_update_last_frame(unsigned long val) { return 0; }
#endif /* CONFIG_X86_USER_SHADOW_STACK */
#endif /* __ASSEMBLY__ */
diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
index 6f1e9883f074..9797d4cdb78a 100644
--- a/arch/x86/kernel/shstk.c
+++ b/arch/x86/kernel/shstk.c
@@ -577,3 +577,14 @@ long shstk_prctl(struct task_struct *task, int option, unsigned long arg2)
return wrss_control(true);
return -EINVAL;
}
+
+int shstk_update_last_frame(unsigned long val)
+{
+ unsigned long ssp;
+
+ if (!features_enabled(ARCH_SHSTK_SHSTK))
+ return 0;
+
+ ssp = get_user_shstk_addr();
+ return write_user_shstk_64((u64 __user *)ssp, (u64)val);
+}
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 6c07f6daaa22..6402fb3089d2 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -1076,8 +1076,13 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs
return orig_ret_vaddr;
nleft = copy_to_user((void __user *)regs->sp, &trampoline_vaddr, rasize);
- if (likely(!nleft))
+ if (likely(!nleft)) {
+ if (shstk_update_last_frame(trampoline_vaddr)) {
+ force_sig(SIGSEGV);
+ return -1;
+ }
return orig_ret_vaddr;
+ }
if (nleft != rasize) {
pr_err("return address clobbered: pid=%d, %%sp=%#lx, %%ip=%#lx\n",
--
2.45.1
^ permalink raw reply related
* [PATCHv7 bpf-next 0/9] uprobe: uretprobe speed up
From: Jiri Olsa @ 2024-05-23 12:11 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
hi,
as part of the effort on speeding up the uprobes [0] coming with
return uprobe optimization by using syscall instead of the trap
on the uretprobe trampoline.
The speed up depends on instruction type that uprobe is installed
and depends on specific HW type, please check patch 1 for details.
Patches 1-8 are based on bpf-next/master, but patch 2 and 3 are
apply-able on linux-trace.git tree probes/for-next branch.
Patch 9 is based on man-pages master.
v7 changes:
- fixes in man page [Alejandro Colomar]
- fixed patch #1 fixes tag [Oleg]
Also available at:
https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
uretprobe_syscall
thanks,
jirka
Notes to check list items in Documentation/process/adding-syscalls.rst:
- System Call Alternatives
New syscall seems like the best way in here, because we need
just to quickly enter kernel with no extra arguments processing,
which we'd need to do if we decided to use another syscall.
- Designing the API: Planning for Extension
The uretprobe syscall is very specific and most likely won't be
extended in the future.
At the moment it does not take any arguments and even if it does
in future, it's allowed to be called only from trampoline prepared
by kernel, so there'll be no broken user.
- Designing the API: Other Considerations
N/A because uretprobe syscall does not return reference to kernel
object.
- Proposing the API
Wiring up of the uretprobe system call is in separate change,
selftests and man page changes are part of the patchset.
- Generic System Call Implementation
There's no CONFIG option for the new functionality because it
keeps the same behaviour from the user POV.
- x86 System Call Implementation
It's 64-bit syscall only.
- Compatibility System Calls (Generic)
N/A uretprobe syscall has no arguments and is not supported
for compat processes.
- Compatibility System Calls (x86)
N/A uretprobe syscall is not supported for compat processes.
- System Calls Returning Elsewhere
N/A.
- Other Details
N/A.
- Testing
Adding new bpf selftests and ran ltp on top of this change.
- Man Page
Attached.
- Do not call System Calls in the Kernel
N/A.
[0] https://lore.kernel.org/bpf/ZeCXHKJ--iYYbmLj@krava/
---
Jiri Olsa (8):
x86/shstk: Make return uprobe work with shadow stack
uprobe: Wire up uretprobe system call
uprobe: Add uretprobe syscall to speed up return probe
selftests/x86: Add return uprobe shadow stack test
selftests/bpf: Add uretprobe syscall test for regs integrity
selftests/bpf: Add uretprobe syscall test for regs changes
selftests/bpf: Add uretprobe syscall call from user space test
selftests/bpf: Add uretprobe shadow stack test
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/x86/include/asm/shstk.h | 4 +
arch/x86/kernel/shstk.c | 16 ++++
arch/x86/kernel/uprobes.c | 124 ++++++++++++++++++++++++++++-
include/linux/syscalls.h | 2 +
include/linux/uprobes.h | 3 +
include/uapi/asm-generic/unistd.h | 5 +-
kernel/events/uprobes.c | 24 ++++--
kernel/sys_ni.c | 2 +
tools/include/linux/compiler.h | 4 +
tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c | 123 ++++++++++++++++++++++++++++-
tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c | 385 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
tools/testing/selftests/bpf/progs/uprobe_syscall.c | 15 ++++
tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c | 17 ++++
tools/testing/selftests/x86/test_shadow_stack.c | 145 ++++++++++++++++++++++++++++++++++
15 files changed, 860 insertions(+), 10 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall.c
create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c
Jiri Olsa (1):
man2: Add uretprobe syscall page
man/man2/uretprobe.2 | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 56 insertions(+)
create mode 100644 man/man2/uretprobe.2
^ permalink raw reply
* Re: [PATCH] uapi: linux/kd.h: include missing header
From: Alexey Gladkov @ 2024-05-23 9:29 UTC (permalink / raw)
To: Dmitry V. Levin; +Cc: Greg Kroah-Hartman, Jiri Slaby, linux-api, linux-kernel
In-Reply-To: <20240523092310.GA31777@altlinux.org>
On Thu, May 23, 2024 at 12:23:10PM +0300, Dmitry V. Levin wrote:
> Given that linux/kd.h now uses macros defined in linux/ioctl.h,
> do not rely on that header being already included by linux/kd.h users
> and include linux/ioctl.h explicitly.
>
> This bug was found by strace CI.
>
> Fixes: 8c467f330059 ("VT: Use macros to define ioctls")
> Signed-off-by: Dmitry V. Levin <ldv@strace.io>
Acked-by: Alexey Gladkov <legion@kernel.org>
> ---
> include/uapi/linux/kd.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
> index 8ddb2219a84b..8fcce939ba25 100644
> --- a/include/uapi/linux/kd.h
> +++ b/include/uapi/linux/kd.h
> @@ -3,6 +3,7 @@
> #define _UAPI_LINUX_KD_H
> #include <linux/types.h>
> #include <linux/compiler.h>
> +#include <linux/ioctl.h>
>
> /* 0x4B is 'K', to avoid collision with termios and vt */
> #define KD_IOCTL_BASE 'K'
> --
> ldv
>
--
Rgrds, legion
^ permalink raw reply
* [PATCH] uapi: linux/kd.h: include missing header
From: Dmitry V. Levin @ 2024-05-23 9:23 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Alexey Gladkov, Jiri Slaby, linux-api, linux-kernel
Given that linux/kd.h now uses macros defined in linux/ioctl.h,
do not rely on that header being already included by linux/kd.h users
and include linux/ioctl.h explicitly.
This bug was found by strace CI.
Fixes: 8c467f330059 ("VT: Use macros to define ioctls")
Signed-off-by: Dmitry V. Levin <ldv@strace.io>
---
include/uapi/linux/kd.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
index 8ddb2219a84b..8fcce939ba25 100644
--- a/include/uapi/linux/kd.h
+++ b/include/uapi/linux/kd.h
@@ -3,6 +3,7 @@
#define _UAPI_LINUX_KD_H
#include <linux/types.h>
#include <linux/compiler.h>
+#include <linux/ioctl.h>
/* 0x4B is 'K', to avoid collision with termios and vt */
#define KD_IOCTL_BASE 'K'
--
ldv
^ permalink raw reply related
* Re: [PATCHv6 9/9] man2: Add uretprobe syscall page
From: Jiri Olsa @ 2024-05-22 11:52 UTC (permalink / raw)
To: Alejandro Colomar
Cc: Jiri Olsa, Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <vqw4ibum2hfnxjkfp7io5ugmwaeok4tynchi3utmzp6xnsmjig@fbjxwmm6u6v3>
On Wed, May 22, 2024 at 12:59:46PM +0200, Alejandro Colomar wrote:
> Hi Jirka,
>
> On Wed, May 22, 2024 at 09:54:58AM GMT, Jiri Olsa wrote:
> > ok, thanks
> >
> > jirka
> >
> >
> > ---
> > diff --git a/man/man2/uretprobe.2 b/man/man2/uretprobe.2
> > new file mode 100644
> > index 000000000000..5b5f340b59b6
> > --- /dev/null
> > +++ b/man/man2/uretprobe.2
> > @@ -0,0 +1,56 @@
> > +.\" Copyright (C) 2024, Jiri Olsa <jolsa@kernel.org>
> > +.\"
> > +.\" SPDX-License-Identifier: Linux-man-pages-copyleft
> > +.\"
> > +.TH uretprobe 2 (date) "Linux man-pages (unreleased)"
> > +.SH NAME
> > +uretprobe \- execute pending return uprobes
> > +.SH SYNOPSIS
> > +.nf
> > +.B int uretprobe(void)
> > +.fi
> > +.SH DESCRIPTION
> > +The
> > +.BR uretprobe ()
> > +system call is an alternative to breakpoint instructions for triggering return
> > +uprobe consumers.
> > +.P
> > +Calls to
> > +.BR uretprobe ()
> > +system call are only made from the user-space trampoline provided by the kernel.
> > +Calls from any other place result in a
> > +.BR SIGILL .
> > +.SH RETURN VALUE
> > +The
> > +.BR uretprobe ()
> > +system call return value is architecture-specific.
> > +.SH ERRORS
> > +.TP
> > +.B SIGILL
> > +The
> > +.BR uretprobe ()
> > +system call was called by user.
>
> Maybe 'a user-space program'?
> Anyway, LGTM. Thanks!
>
> Reviewed-by: Alejandro Colomar <alx@kernel.org>
ok, will change, thanks a lot
jirka
>
> Have a lovely day!
> Alex
>
> > +.SH VERSIONS
> > +Details of the
> > +.BR uretprobe ()
> > +system call behavior vary across systems.
> > +.SH STANDARDS
> > +None.
> > +.SH HISTORY
> > +TBD
> > +.SH NOTES
> > +The
> > +.BR uretprobe ()
> > +system call was initially introduced for the x86_64 architecture
> > +where it was shown to be faster than breakpoint traps.
> > +It might be extended to other architectures.
> > +.P
> > +The
> > +.BR uretprobe ()
> > +system call exists only to allow the invocation of return uprobe consumers.
> > +It should
> > +.B never
> > +be called directly.
> > +Details of the arguments (if any) passed to
> > +.BR uretprobe ()
> > +and the return value are architecture-specific.
>
> --
> <https://www.alejandro-colomar.es/>
^ permalink raw reply
* Re: [PATCHv6 9/9] man2: Add uretprobe syscall page
From: Alejandro Colomar @ 2024-05-22 10:59 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <Zk2k0ttdR7abKSuv@krava>
[-- Attachment #1: Type: text/plain, Size: 2003 bytes --]
Hi Jirka,
On Wed, May 22, 2024 at 09:54:58AM GMT, Jiri Olsa wrote:
> ok, thanks
>
> jirka
>
>
> ---
> diff --git a/man/man2/uretprobe.2 b/man/man2/uretprobe.2
> new file mode 100644
> index 000000000000..5b5f340b59b6
> --- /dev/null
> +++ b/man/man2/uretprobe.2
> @@ -0,0 +1,56 @@
> +.\" Copyright (C) 2024, Jiri Olsa <jolsa@kernel.org>
> +.\"
> +.\" SPDX-License-Identifier: Linux-man-pages-copyleft
> +.\"
> +.TH uretprobe 2 (date) "Linux man-pages (unreleased)"
> +.SH NAME
> +uretprobe \- execute pending return uprobes
> +.SH SYNOPSIS
> +.nf
> +.B int uretprobe(void)
> +.fi
> +.SH DESCRIPTION
> +The
> +.BR uretprobe ()
> +system call is an alternative to breakpoint instructions for triggering return
> +uprobe consumers.
> +.P
> +Calls to
> +.BR uretprobe ()
> +system call are only made from the user-space trampoline provided by the kernel.
> +Calls from any other place result in a
> +.BR SIGILL .
> +.SH RETURN VALUE
> +The
> +.BR uretprobe ()
> +system call return value is architecture-specific.
> +.SH ERRORS
> +.TP
> +.B SIGILL
> +The
> +.BR uretprobe ()
> +system call was called by user.
Maybe 'a user-space program'?
Anyway, LGTM. Thanks!
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Have a lovely day!
Alex
> +.SH VERSIONS
> +Details of the
> +.BR uretprobe ()
> +system call behavior vary across systems.
> +.SH STANDARDS
> +None.
> +.SH HISTORY
> +TBD
> +.SH NOTES
> +The
> +.BR uretprobe ()
> +system call was initially introduced for the x86_64 architecture
> +where it was shown to be faster than breakpoint traps.
> +It might be extended to other architectures.
> +.P
> +The
> +.BR uretprobe ()
> +system call exists only to allow the invocation of return uprobe consumers.
> +It should
> +.B never
> +be called directly.
> +Details of the arguments (if any) passed to
> +.BR uretprobe ()
> +and the return value are architecture-specific.
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCHv6 bpf-next 0/9] uprobe: uretprobe speed up
From: Jiri Olsa @ 2024-05-22 8:55 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Deepak Gupta, Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, LKML,
linux-trace-kernel, Linux API, linux-man, X86 ML, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski,
Edgecombe, Rick P
In-Reply-To: <CAADnVQ+2Q1992e9mRtWOavHfqKsFUxPp4f6MAAJg90TK_KTpew@mail.gmail.com>
On Tue, May 21, 2024 at 01:57:33PM -0700, Alexei Starovoitov wrote:
> On Tue, May 21, 2024 at 1:49 PM Deepak Gupta <debug@rivosinc.com> wrote:
> >
> > On Tue, May 21, 2024 at 12:48:16PM +0200, Jiri Olsa wrote:
> > >hi,
> > >as part of the effort on speeding up the uprobes [0] coming with
> > >return uprobe optimization by using syscall instead of the trap
> > >on the uretprobe trampoline.
> >
> > I understand this provides an optimization on x86. I believe primary reason
> > is syscall is straight-line microcode and short sequence while trap delivery
> > still does all the GDT / IDT and segmentation checks and it makes delivery
> > of the trap slow.
> >
> > So doing syscall improves that. Although it seems x86 is going to get rid of
> > that as part of FRED [1, 2]. And linux kernel support for FRED is already upstream [2].
> > So I am imagining x86 hardware already exists with FRED support.
> >
> > On other architectures, I believe trap delivery for breakpoint instruction
> > is same as syscall instruction.
> >
> > Given that x86 trap delivery is pretty much going following the suit here and
> > intend to make trap delivery cost similar to syscall delivery.
> >
> > Sorry for being buzzkill here but ...
> > Is it worth introducing this syscall which otherwise has no use on other arches
> > and x86 (and x86 kernel) has already taken steps to match trap delivery latency with
> > syscall latency would have similar cost?
> >
> > Did you do any study of this on FRED enabled x86 CPUs?
nope.. interesting, will check, thanks
>
> afaik CPUs with FRED do not exist on the market and it's
> not clear when they will be available.
> And when they finally will be on the shelves
> the overhead of FRED vs int3 would still have to be measured.
> int3 with FRED might still be higher than syscall with FRED.
+1, also it's not really a complicated change and the wiring of the
new syscall to uretprobe is really simple and we could go back to int3
with just one single patch if we see no longer any benefit to it,
but at the moment it provides speed up
jirka
>
> >
> > [1] - https://www.intel.com/content/www/us/en/content-details/780121/flexible-return-and-event-delivery-fred-specification.html
> > [2] - https://docs.kernel.org/arch/x86/x86_64/fred.html
> >
> > >
> > >The speed up depends on instruction type that uprobe is installed
> > >and depends on specific HW type, please check patch 1 for details.
> > >
^ permalink raw reply
* Re: [PATCHv6 9/9] man2: Add uretprobe syscall page
From: Jiri Olsa @ 2024-05-22 7:54 UTC (permalink / raw)
To: Alejandro Colomar
Cc: Jiri Olsa, Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <o5pkz3eenii6p6sm7dl2fsgy4fqqaq2qbn2rbxddhkvaarvwgm@dkjjknb44qp2>
On Tue, May 21, 2024 at 10:54:36PM +0200, Alejandro Colomar wrote:
> Hi Jirka,
>
> On Tue, May 21, 2024 at 10:24:30PM GMT, Jiri Olsa wrote:
> > how about the change below?
>
> Much better. I still have a few comments below. :-)
>
> >
> > thanks,
> > jirka
> >
> >
> > ---
> > diff --git a/man/man2/uretprobe.2 b/man/man2/uretprobe.2
> > new file mode 100644
> > index 000000000000..959b7a47102b
> > --- /dev/null
> > +++ b/man/man2/uretprobe.2
> > @@ -0,0 +1,55 @@
> > +.\" Copyright (C) 2024, Jiri Olsa <jolsa@kernel.org>
> > +.\"
> > +.\" SPDX-License-Identifier: Linux-man-pages-copyleft
> > +.\"
> > +.TH uretprobe 2 (date) "Linux man-pages (unreleased)"
> > +.SH NAME
> > +uretprobe \- execute pending return uprobes
> > +.SH SYNOPSIS
> > +.nf
> > +.B int uretprobe(void)
> > +.fi
> > +.SH DESCRIPTION
> > +The
> > +.BR uretprobe ()
> > +system call is an alternative to breakpoint instructions for triggering return
> > +uprobe consumers.
> > +.P
> > +Calls to
> > +.BR uretprobe ()
> > +system call are only made from the user-space trampoline provided by the kernel.
> > +Calls from any other place result in a
> > +.BR SIGILL .
> > +.SH RETURN VALUE
> > +The
> > +.BR uretprobe ()
> > +system call return value is architecture-specific.
> > +.SH ERRORS
> > +.BR SIGILL
>
> This should be a tagged paragraph, preceeded with '.TP'. See any manual
> page with an ERRORS section for an example.
>
> Also, BR is Bold alternating with Roman, but this is just bold, so it
> should use '.B'.
>
> .TP
> .B SIGILL
ok
>
> > +The
> > +.BR uretprobe ()
> > +system call was called by user.
> > +.SH VERSIONS
> > +Details of the
> > +.BR uretprobe ()
> > +system call behavior vary across systems.
> > +.SH STANDARDS
> > +None.
> > +.SH HISTORY
> > +TBD
> > +.SH NOTES
> > +The
> > +.BR uretprobe ()
> > +system call was initially introduced for the x86_64 architecture where it was shown
>
> We have a strong-ish limit at column 80. Please break after
> 'architecture', which is a clause boundary.
>
ok, thanks
jirka
---
diff --git a/man/man2/uretprobe.2 b/man/man2/uretprobe.2
new file mode 100644
index 000000000000..5b5f340b59b6
--- /dev/null
+++ b/man/man2/uretprobe.2
@@ -0,0 +1,56 @@
+.\" Copyright (C) 2024, Jiri Olsa <jolsa@kernel.org>
+.\"
+.\" SPDX-License-Identifier: Linux-man-pages-copyleft
+.\"
+.TH uretprobe 2 (date) "Linux man-pages (unreleased)"
+.SH NAME
+uretprobe \- execute pending return uprobes
+.SH SYNOPSIS
+.nf
+.B int uretprobe(void)
+.fi
+.SH DESCRIPTION
+The
+.BR uretprobe ()
+system call is an alternative to breakpoint instructions for triggering return
+uprobe consumers.
+.P
+Calls to
+.BR uretprobe ()
+system call are only made from the user-space trampoline provided by the kernel.
+Calls from any other place result in a
+.BR SIGILL .
+.SH RETURN VALUE
+The
+.BR uretprobe ()
+system call return value is architecture-specific.
+.SH ERRORS
+.TP
+.B SIGILL
+The
+.BR uretprobe ()
+system call was called by user.
+.SH VERSIONS
+Details of the
+.BR uretprobe ()
+system call behavior vary across systems.
+.SH STANDARDS
+None.
+.SH HISTORY
+TBD
+.SH NOTES
+The
+.BR uretprobe ()
+system call was initially introduced for the x86_64 architecture
+where it was shown to be faster than breakpoint traps.
+It might be extended to other architectures.
+.P
+The
+.BR uretprobe ()
+system call exists only to allow the invocation of return uprobe consumers.
+It should
+.B never
+be called directly.
+Details of the arguments (if any) passed to
+.BR uretprobe ()
+and the return value are architecture-specific.
^ permalink raw reply related
* Re: [PATCHv6 bpf-next 0/9] uprobe: uretprobe speed up
From: Alexei Starovoitov @ 2024-05-21 20:57 UTC (permalink / raw)
To: Deepak Gupta
Cc: Jiri Olsa, Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, LKML,
linux-trace-kernel, Linux API, linux-man, X86 ML, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski,
Edgecombe, Rick P
In-Reply-To: <Zk0IvZU834RQ7YKp@debug.ba.rivosinc.com>
On Tue, May 21, 2024 at 1:49 PM Deepak Gupta <debug@rivosinc.com> wrote:
>
> On Tue, May 21, 2024 at 12:48:16PM +0200, Jiri Olsa wrote:
> >hi,
> >as part of the effort on speeding up the uprobes [0] coming with
> >return uprobe optimization by using syscall instead of the trap
> >on the uretprobe trampoline.
>
> I understand this provides an optimization on x86. I believe primary reason
> is syscall is straight-line microcode and short sequence while trap delivery
> still does all the GDT / IDT and segmentation checks and it makes delivery
> of the trap slow.
>
> So doing syscall improves that. Although it seems x86 is going to get rid of
> that as part of FRED [1, 2]. And linux kernel support for FRED is already upstream [2].
> So I am imagining x86 hardware already exists with FRED support.
>
> On other architectures, I believe trap delivery for breakpoint instruction
> is same as syscall instruction.
>
> Given that x86 trap delivery is pretty much going following the suit here and
> intend to make trap delivery cost similar to syscall delivery.
>
> Sorry for being buzzkill here but ...
> Is it worth introducing this syscall which otherwise has no use on other arches
> and x86 (and x86 kernel) has already taken steps to match trap delivery latency with
> syscall latency would have similar cost?
>
> Did you do any study of this on FRED enabled x86 CPUs?
afaik CPUs with FRED do not exist on the market and it's
not clear when they will be available.
And when they finally will be on the shelves
the overhead of FRED vs int3 would still have to be measured.
int3 with FRED might still be higher than syscall with FRED.
>
> [1] - https://www.intel.com/content/www/us/en/content-details/780121/flexible-return-and-event-delivery-fred-specification.html
> [2] - https://docs.kernel.org/arch/x86/x86_64/fred.html
>
> >
> >The speed up depends on instruction type that uprobe is installed
> >and depends on specific HW type, please check patch 1 for details.
> >
^ permalink raw reply
* Re: [PATCHv6 9/9] man2: Add uretprobe syscall page
From: Alejandro Colomar @ 2024-05-21 20:54 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <Zk0C_vm3T2L79-_W@krava>
[-- Attachment #1: Type: text/plain, Size: 2321 bytes --]
Hi Jirka,
On Tue, May 21, 2024 at 10:24:30PM GMT, Jiri Olsa wrote:
> how about the change below?
Much better. I still have a few comments below. :-)
>
> thanks,
> jirka
>
>
> ---
> diff --git a/man/man2/uretprobe.2 b/man/man2/uretprobe.2
> new file mode 100644
> index 000000000000..959b7a47102b
> --- /dev/null
> +++ b/man/man2/uretprobe.2
> @@ -0,0 +1,55 @@
> +.\" Copyright (C) 2024, Jiri Olsa <jolsa@kernel.org>
> +.\"
> +.\" SPDX-License-Identifier: Linux-man-pages-copyleft
> +.\"
> +.TH uretprobe 2 (date) "Linux man-pages (unreleased)"
> +.SH NAME
> +uretprobe \- execute pending return uprobes
> +.SH SYNOPSIS
> +.nf
> +.B int uretprobe(void)
> +.fi
> +.SH DESCRIPTION
> +The
> +.BR uretprobe ()
> +system call is an alternative to breakpoint instructions for triggering return
> +uprobe consumers.
> +.P
> +Calls to
> +.BR uretprobe ()
> +system call are only made from the user-space trampoline provided by the kernel.
> +Calls from any other place result in a
> +.BR SIGILL .
> +.SH RETURN VALUE
> +The
> +.BR uretprobe ()
> +system call return value is architecture-specific.
> +.SH ERRORS
> +.BR SIGILL
This should be a tagged paragraph, preceeded with '.TP'. See any manual
page with an ERRORS section for an example.
Also, BR is Bold alternating with Roman, but this is just bold, so it
should use '.B'.
.TP
.B SIGILL
> +The
> +.BR uretprobe ()
> +system call was called by user.
> +.SH VERSIONS
> +Details of the
> +.BR uretprobe ()
> +system call behavior vary across systems.
> +.SH STANDARDS
> +None.
> +.SH HISTORY
> +TBD
> +.SH NOTES
> +The
> +.BR uretprobe ()
> +system call was initially introduced for the x86_64 architecture where it was shown
We have a strong-ish limit at column 80. Please break after
'architecture', which is a clause boundary.
Have a lovely night!
Alex
> +to be faster than breakpoint traps.
> +It might be extended to other architectures.
> +.P
> +The
> +.BR uretprobe ()
> +system call exists only to allow the invocation of return uprobe consumers.
> +It should
> +.B never
> +be called directly.
> +Details of the arguments (if any) passed to
> +.BR uretprobe ()
> +and the return value are architecture-specific.
>
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCHv6 bpf-next 0/9] uprobe: uretprobe speed up
From: Deepak Gupta @ 2024-05-21 20:49 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P
In-Reply-To: <20240521104825.1060966-1-jolsa@kernel.org>
On Tue, May 21, 2024 at 12:48:16PM +0200, Jiri Olsa wrote:
>hi,
>as part of the effort on speeding up the uprobes [0] coming with
>return uprobe optimization by using syscall instead of the trap
>on the uretprobe trampoline.
I understand this provides an optimization on x86. I believe primary reason
is syscall is straight-line microcode and short sequence while trap delivery
still does all the GDT / IDT and segmentation checks and it makes delivery
of the trap slow.
So doing syscall improves that. Although it seems x86 is going to get rid of
that as part of FRED [1, 2]. And linux kernel support for FRED is already upstream [2].
So I am imagining x86 hardware already exists with FRED support.
On other architectures, I believe trap delivery for breakpoint instruction
is same as syscall instruction.
Given that x86 trap delivery is pretty much going following the suit here and
intend to make trap delivery cost similar to syscall delivery.
Sorry for being buzzkill here but ...
Is it worth introducing this syscall which otherwise has no use on other arches
and x86 (and x86 kernel) has already taken steps to match trap delivery latency with
syscall latency would have similar cost?
Did you do any study of this on FRED enabled x86 CPUs?
[1] - https://www.intel.com/content/www/us/en/content-details/780121/flexible-return-and-event-delivery-fred-specification.html
[2] - https://docs.kernel.org/arch/x86/x86_64/fred.html
>
>The speed up depends on instruction type that uprobe is installed
>and depends on specific HW type, please check patch 1 for details.
>
^ 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