* Re: [PATCH] fs/xattr: unify *at syscalls
From: Christian Brauner @ 2024-05-02 13:02 UTC (permalink / raw)
To: Christian Goettsche
Cc: Christian Brauner, Christian Goettsche, Jan Kara, Alexander Viro,
Jan Kara, Arnd Bergmann, Thomas Gleixner, Kees Cook,
Geert Uytterhoeven, Casey Schaufler, peterz, Sohil Mehta,
Miklos Szeredi, Mark Rutland, linux-fsdevel, linux-kernel,
linux-api
In-Reply-To: <20240430151917.30036-1-cgoettsche@seltendoof.de>
On Tue, 30 Apr 2024 17:19:14 +0200, Christian Göttsche wrote:
> Use the same parameter ordering for all four newly added *xattrat
> syscalls:
>
> dirfd, pathname, at_flags, ...
>
> Also consistently use unsigned int as the type for at_flags.
>
> [...]
Applied to the vfs.xattr branch of the vfs/vfs.git tree.
Patches in the vfs.xattr branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.xattr
[1/1] fs/xattr: unify *at syscalls
https://git.kernel.org/vfs/vfs/c/1d5e73c8c531
^ permalink raw reply
* Re: [PATCH] fs/xattr: unify *at syscalls
From: Christian Brauner @ 2024-05-02 13:04 UTC (permalink / raw)
To: Jan Kara
Cc: cgzones, Jan Kara, Alexander Viro, Arnd Bergmann, Thomas Gleixner,
Kees Cook, Geert Uytterhoeven, Casey Schaufler,
peterz@infradead.org, Sohil Mehta, Miklos Szeredi, Mark Rutland,
linux-fsdevel, linux-kernel, linux-api
In-Reply-To: <20240502103716.avdfm6r3ma2wfxjj@quack3>
On Thu, May 02, 2024 at 12:37:16PM +0200, Jan Kara wrote:
> On Tue 30-04-24 17:19:14, Christian Göttsche wrote:
> > From: Christian Göttsche <cgzones@googlemail.com>
> >
> > Use the same parameter ordering for all four newly added *xattrat
> > syscalls:
> >
> > dirfd, pathname, at_flags, ...
> >
> > Also consistently use unsigned int as the type for at_flags.
> >
> > Suggested-by: Jan Kara <jack@suse.com>
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>
> Thanks! The change looks good to me. Christian, do you plan to fold this
> into the series you've taken to your tree?
Yep, that's the plan.
^ permalink raw reply
* Re: [RFC PATCH 0/1] Add FUTEX_SPIN operation
From: Christian Brauner @ 2024-05-02 13:08 UTC (permalink / raw)
To: Florian Weimer
Cc: André Almeida, Mathieu Desnoyers, Peter Zijlstra,
Thomas Gleixner, linux-kernel, Paul E . McKenney, Boqun Feng,
H . Peter Anvin, Paul Turner, linux-api, 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: <871q6kmra1.fsf@oldenburg.str.redhat.com>
On Thu, May 02, 2024 at 12:39:34PM +0200, Florian Weimer wrote:
> * Christian Brauner:
>
> >> From a glibc perspective, we typically cannot use long-term file
> >> descriptors (that are kept open across function calls) because some
> >> applications do not expect them, or even close them behind our back.
> >
> > Yeah, good point. Note, I suggested it as an extension not as a
> > replacement for the TID. I still think it would be a useful extension in
> > general.
>
> Applications will need a way to determine when it is safe to close the
> pidfd, though. If we automate this in glibc (in the same way we handle
> thread stack deallocation for example), I think we are essentially back
> to square one, except that pidfd collisions are much more likely than
> TID collisions, especially on systems that have adjusted kernel.pid_max.
> (File descriptor allocation is designed to maximize collisions, after
> all.)
(Note that with pidfs (current mainline), pidfds have 64bit unique inode
numbers that are unique for the lifetime of the system. So they can
reliably be compared via statx() and so on.)
^ permalink raw reply
* Re: [PATCHv4 7/7] man2: Add uretprobe syscall page
From: Alejandro Colomar @ 2024-05-02 13:43 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
In-Reply-To: <20240502122313.1579719-8-jolsa@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 2728 bytes --]
Hi Jiri,
On Thu, May 02, 2024 at 02:23:13PM +0200, Jiri Olsa wrote:
> Adding man page for new uretprobe syscall.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> man2/uretprobe.2 | 45 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
> create mode 100644 man2/uretprobe.2
>
> diff --git a/man2/uretprobe.2 b/man2/uretprobe.2
> new file mode 100644
> index 000000000000..08fe6a670430
> --- /dev/null
> +++ b/man2/uretprobe.2
> @@ -0,0 +1,45 @@
> +.\" 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
> +Kernel is using
> +.BR uretprobe()
> +syscall to trigger uprobe return probe consumers instead of using
> +standard breakpoint instruction.
> +
Please use .P instead of a blank. See man-pages(7):
Formatting conventions (general)
Paragraphs should be separated by suitable markers (usually either
.P or .IP). Do not separate paragraphs using blank lines, as this
results in poor rendering in some output formats (such as Post‐
Script and PDF).
> +The uretprobe syscall is not supposed to be called directly by user, it's allowed
s/by user/by the user/
> +to be invoked only through user space trampoline provided by kernel.
s/user space/user-space/
Missing a few 'the' too, here and in the rest of the page.
> +When called from outside of this trampoline, the calling process will receive
> +.BR SIGILL .
> +
> +.SH RETURN VALUE
> +.BR uretprobe()
You're missing a space here:
.BR uretprobe ()
> +return value is specific for given architecture.
> +
> +.SH VERSIONS
> +This syscall is not specified in POSIX,
> +and details of its behavior vary across systems.
> +.SH STANDARDS
> +None.
You could add a HISTORY section.
Have a lovely day!
Alex
> +.SH NOTES
> +.BR uretprobe()
> +syscall is initially introduced on x86-64 architecture, because doing syscall
> +is faster than doing breakpoint trap on it. It might be extended to other
> +architectures.
> +
> +.BR uretprobe()
> +syscall 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 specific for given architecture.
> --
> 2.44.0
>
>
--
<https://www.alejandro-colomar.es/>
A client is hiring kernel driver, mm, and/or crypto developers;
contact me if interested.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 1/1] Remove buffered failover for ext4 and block fops direct writes.
From: Theodore Ts'o @ 2024-05-02 14:01 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jeremy Bongio, linux-ext4, linux-fsdevel, linux-api, linux-block,
Jeremy Bongio
In-Reply-To: <ZjMoYkUsQnd33mXm@infradead.org>
On Wed, May 01, 2024 at 10:45:06PM -0700, Christoph Hellwig wrote:
>
> Please don't combine ext4 and block changes in a single patch. Please
> also explain why you want to change things.
>
> AFAIK this is simply the historic behavior of the old direct I/O code
> that's been around forever. I think the XFS semantics make a lot more
> sense, but people might rely on this one way or another.
I agree that the ext4 and block I/O change should be split into two
separate patches.
As for the rest, we discussed this at the weekly ext4 conference call
last week and at the, I had indicated that this was indeed the
historical Direct I/O behavior. Darrick mentioned that XFS is only
falling back to buffered I/O in one circumstances, which is when there
is direct I/O to a file which is reflinked, which since the
application wouldn't know that this might be the case, falling back to
buffered I/O was the best of not-so-great alternatives.
It might be a good idea if we could agree on a unfied set of standard
semantics for Direct I/O, including what should happen if there is an
I/O error in the middle of a DIO request; should the kernel return a
short write? Should it silently fallback to buffered I/O? Given that
XFS has had a fairly strict "never fall back to buffered" practice,
and there haven't been users screaming bloody murder, perhaps it is
time that we can leave the old historical Direct I/O semantics behind,
and we should just be more strict.
Ext4 can make a decision about what to do on its own, but if we want
to unify behavior across all file systems and all of the direct I/O
implications in the kernels, then this is a discussion that would need
to take place on linux-fsdevel, linux-block, and/or LSF/MM.
With that context, what are folks' thiking about the proposal that we
unify Linux's Direct I/O semantics? I think it would be good if it
was (a) clearly documented, and (b) not be surprising for userspace
application which they switch beteween file systems, or between a file
system and a raw block device. (Which for certain enterprise
database, is mostly only use for benchmarketing, on the back cover of
Business Week, but sometimes there might be users who decide to
squeeze that last 1% of performance by going to a raw block device,
and it might be nice if they see the same behaviour when they make
that change.)
Cheers,
- Ted
^ permalink raw reply
* Re: [RFC PATCH 1/1] Remove buffered failover for ext4 and block fops direct writes.
From: Darrick J. Wong @ 2024-05-02 14:33 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Christoph Hellwig, Jeremy Bongio, linux-ext4, linux-fsdevel,
linux-api, linux-block, Jeremy Bongio
In-Reply-To: <20240502140139.GE1743554@mit.edu>
On Thu, May 02, 2024 at 10:01:39AM -0400, Theodore Ts'o wrote:
> On Wed, May 01, 2024 at 10:45:06PM -0700, Christoph Hellwig wrote:
> >
> > Please don't combine ext4 and block changes in a single patch. Please
> > also explain why you want to change things.
> >
> > AFAIK this is simply the historic behavior of the old direct I/O code
> > that's been around forever. I think the XFS semantics make a lot more
> > sense, but people might rely on this one way or another.
>
> I agree that the ext4 and block I/O change should be split into two
> separate patches.
>
> As for the rest, we discussed this at the weekly ext4 conference call
> last week and at the, I had indicated that this was indeed the
> historical Direct I/O behavior. Darrick mentioned that XFS is only
> falling back to buffered I/O in one circumstances, which is when there
> is direct I/O to a file which is reflinked, which since the
fsblock unaligned directio writes to a reflinked file, specifically.
> application wouldn't know that this might be the case, falling back to
> buffered I/O was the best of not-so-great alternatives.
>
> It might be a good idea if we could agree on a unfied set of standard
> semantics for Direct I/O, including what should happen if there is an
> I/O error in the middle of a DIO request; should the kernel return a
> short write?
Given the attitude of "if you use directio you're supposed to know what
you're doing", I think it's fine to return a short write.
> Should it silently fallback to buffered I/O? Given that
> XFS has had a fairly strict "never fall back to buffered" practice,
> and there haven't been users screaming bloody murder, perhaps it is
> time that we can leave the old historical Direct I/O semantics behind,
> and we should just be more strict.
The other thing I've heard, mostly from willy is that directio could be
done through the pagecache when it is already caching the data. I've
also heard about other operating systems <cough> where the mode could
bleed through to other fds (er...).
> Ext4 can make a decision about what to do on its own, but if we want
> to unify behavior across all file systems and all of the direct I/O
> implications in the kernels, then this is a discussion that would need
> to take place on linux-fsdevel, linux-block, and/or LSF/MM.
>
> With that context, what are folks' thiking about the proposal that we
> unify Linux's Direct I/O semantics? I think it would be good if it
> was (a) clearly documented, and (b) not be surprising for userspace
> application which they switch beteween file systems, or between a file
> system and a raw block device. (Which for certain enterprise
> database, is mostly only use for benchmarketing, on the back cover of
> Business Week, but sometimes there might be users who decide to
> squeeze that last 1% of performance by going to a raw block device,
> and it might be nice if they see the same behaviour when they make
> that change.)
Possibly a good idea but how much of LSFMM do we want to spend
relitigating old {,non-}decisions? ;)
--D
> Cheers,
>
> - Ted
>
^ permalink raw reply
* Re: [PATCHv4 bpf-next 5/7] selftests/bpf: Add uretprobe syscall call from user space test
From: Andrii Nakryiko @ 2024-05-02 16:33 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
In-Reply-To: <20240502122313.1579719-6-jolsa@kernel.org>
On Thu, May 2, 2024 at 5:24 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> 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.
>
> 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..c6fdb8c59ea3 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 (pipe(go))
> + return;
very unlikely to fail, but still, ASSERT_OK() would be in order here
But regardless:
Acked-by: Andrii Nakryiko <andrii@kernel.org>
[...]
^ permalink raw reply
* Re: [PATCHv4 bpf-next 6/7] selftests/bpf: Add uretprobe compat test
From: Andrii Nakryiko @ 2024-05-02 16:35 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
In-Reply-To: <20240502122313.1579719-7-jolsa@kernel.org>
On Thu, May 2, 2024 at 5:24 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding test that adds return uprobe inside 32-bit task
> and verify the return uprobe and attached bpf programs
> get properly executed.
>
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/testing/selftests/bpf/.gitignore | 1 +
> tools/testing/selftests/bpf/Makefile | 7 ++-
> .../selftests/bpf/prog_tests/uprobe_syscall.c | 60 +++++++++++++++++++
> 3 files changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> index f1aebabfb017..69d71223c0dd 100644
> --- a/tools/testing/selftests/bpf/.gitignore
> +++ b/tools/testing/selftests/bpf/.gitignore
> @@ -45,6 +45,7 @@ test_cpp
> /veristat
> /sign-file
> /uprobe_multi
> +/uprobe_compat
> *.ko
> *.tmp
> xskxceiver
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 82247aeef857..a94352162290 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -133,7 +133,7 @@ TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
> xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \
> xdp_features bpf_test_no_cfi.ko
>
> -TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi
> +TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi uprobe_compat
>
> ifneq ($(V),1)
> submake_extras := feature_display=0
> @@ -631,6 +631,7 @@ TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko \
> $(OUTPUT)/xdp_synproxy \
> $(OUTPUT)/sign-file \
> $(OUTPUT)/uprobe_multi \
> + $(OUTPUT)/uprobe_compat \
> ima_setup.sh \
> verify_sig_setup.sh \
> $(wildcard progs/btf_dump_test_case_*.c) \
> @@ -752,6 +753,10 @@ $(OUTPUT)/uprobe_multi: uprobe_multi.c
> $(call msg,BINARY,,$@)
> $(Q)$(CC) $(CFLAGS) -O0 $(LDFLAGS) $^ $(LDLIBS) -o $@
>
> +$(OUTPUT)/uprobe_compat:
> + $(call msg,BINARY,,$@)
> + $(Q)echo "int main() { return 0; }" | $(CC) $(CFLAGS) -xc -m32 -O0 - -o $@
> +
> EXTRA_CLEAN := $(SCRATCH_DIR) $(HOST_SCRATCH_DIR) \
> prog_tests/tests.h map_tests/tests.h verifier/tests.h \
> feature bpftool \
> diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> index c6fdb8c59ea3..bfea9a0368a4 100644
> --- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> @@ -5,6 +5,7 @@
> #ifdef __x86_64__
>
> #include <unistd.h>
> +#include <stdlib.h>
> #include <asm/ptrace.h>
> #include <linux/compiler.h>
> #include <linux/stringify.h>
> @@ -297,6 +298,58 @@ static void test_uretprobe_syscall_call(void)
> close(go[1]);
> close(go[0]);
> }
> +
> +static void test_uretprobe_compat(void)
> +{
> + LIBBPF_OPTS(bpf_uprobe_multi_opts, opts,
> + .retprobe = true,
> + );
> + struct uprobe_syscall_executed *skel;
> + int err, go[2], pid, c, status;
> +
> + if (pipe(go))
> + return;
ASSERT_OK() missing, like in the previous patch
Thanks for switching to pipe() + global variable instead of using trace_pipe.
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> +
> + skel = uprobe_syscall_executed__open_and_load();
> + if (!ASSERT_OK_PTR(skel, "uprobe_syscall_executed__open_and_load"))
> + goto cleanup;
> +
[...]
^ permalink raw reply
* Re: [PATCHv4 bpf-next 0/7] uprobe: uretprobe speed up
From: Andrii Nakryiko @ 2024-05-02 16:43 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
In-Reply-To: <20240502122313.1579719-1-jolsa@kernel.org>
On Thu, May 2, 2024 at 5:23 AM Jiri Olsa <jolsa@kernel.org> 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.
>
> 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-6 are based on bpf-next/master, but path 1 and 2 are
> apply-able on linux-trace.git tree probes/for-next branch.
> Patch 7 is based on man-pages master.
>
> v4 changes:
> - added acks [Oleg,Andrii,Masami]
> - reworded the man page and adding more info to NOTE section [Masami]
> - rewrote bpf tests not to use trace_pipe [Andrii]
> - cc-ed linux-man list
>
> Also available at:
> https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> uretprobe_syscall
>
It looks great to me, thanks! Unfortunately BPF CI build is broken,
probably due to some of the Makefile additions, please investigate and
fix (or we'll need to fix something on BPF CI side), but it looks like
you'll need another revision, unfortunately.
pw-bot: cr
[0] https://github.com/kernel-patches/bpf/actions/runs/8923849088/job/24509002194
But while we are at it.
Masami, Oleg,
What should be the logistics of landing this? Can/should we route this
through the bpf-next tree, given there are lots of BPF-based
selftests? Or you want to take this through
linux-trace/probes/for-next? In the latter case, it's probably better
to apply only the first two patches to probes/for-next and the rest
should still go through the bpf-next tree (otherwise we are running
into conflicts in BPF selftests). Previously we were handling such
cross-tree dependencies by creating a named branch or tag, and merging
it into bpf-next (so that all SHAs are preserved). It's a bunch of
extra work for everyone involved, so the simplest way would be to just
land through bpf-next, of course. But let me know your preferences.
Thanks!
> 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, becase we need
typo (thanks, Gmail): because
> 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 si in separate change,
typo: is
> 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 (6):
> uprobe: Wire up uretprobe system call
> uprobe: Add uretprobe syscall to speed up return probe
> 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 compat test
>
> arch/x86/entry/syscalls/syscall_64.tbl | 1 +
> arch/x86/kernel/uprobes.c | 115 ++++++++++++++++++++++++++++
> 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/.gitignore | 1 +
> tools/testing/selftests/bpf/Makefile | 7 +-
> tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c | 123 ++++++++++++++++++++++++++++-
> tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c | 382 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> tools/testing/selftests/bpf/progs/uprobe_syscall.c | 15 ++++
> tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c | 17 +++++
> 14 files changed, 691 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
>
> man2/uretprobe.2 | 45 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
> create mode 100644 man2/uretprobe.2
^ permalink raw reply
* Re: [PATCHv4 bpf-next 0/7] uprobe: uretprobe speed up
From: Jiri Olsa @ 2024-05-02 20:04 UTC (permalink / raw)
To: Andrii Nakryiko
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
In-Reply-To: <CAEf4BzYxsRMx9M_AiLavTHFpndSmZqOM8QcYhDTbBviSpv1r+A@mail.gmail.com>
On Thu, May 02, 2024 at 09:43:02AM -0700, Andrii Nakryiko wrote:
> On Thu, May 2, 2024 at 5:23 AM Jiri Olsa <jolsa@kernel.org> 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.
> >
> > 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-6 are based on bpf-next/master, but path 1 and 2 are
> > apply-able on linux-trace.git tree probes/for-next branch.
> > Patch 7 is based on man-pages master.
> >
> > v4 changes:
> > - added acks [Oleg,Andrii,Masami]
> > - reworded the man page and adding more info to NOTE section [Masami]
> > - rewrote bpf tests not to use trace_pipe [Andrii]
> > - cc-ed linux-man list
> >
> > Also available at:
> > https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > uretprobe_syscall
> >
>
> It looks great to me, thanks! Unfortunately BPF CI build is broken,
> probably due to some of the Makefile additions, please investigate and
> fix (or we'll need to fix something on BPF CI side), but it looks like
> you'll need another revision, unfortunately.
>
> pw-bot: cr
>
> [0] https://github.com/kernel-patches/bpf/actions/runs/8923849088/job/24509002194
yes, I think it's missing the 32-bit libc for uprobe_compat binary,
probably it needs to be added to github.com:libbpf/ci.git setup-build-env/action.yml ?
hm but I'm not sure how to test it, need to check
>
>
>
> But while we are at it.
>
> Masami, Oleg,
>
> What should be the logistics of landing this? Can/should we route this
> through the bpf-next tree, given there are lots of BPF-based
> selftests? Or you want to take this through
> linux-trace/probes/for-next? In the latter case, it's probably better
> to apply only the first two patches to probes/for-next and the rest
> should still go through the bpf-next tree (otherwise we are running
I think this was the plan, previously mentioned in here:
https://lore.kernel.org/bpf/20240423000943.478ccf1e735a63c6c1b4c66b@kernel.org/
> into conflicts in BPF selftests). Previously we were handling such
> cross-tree dependencies by creating a named branch or tag, and merging
> it into bpf-next (so that all SHAs are preserved). It's a bunch of
> extra work for everyone involved, so the simplest way would be to just
> land through bpf-next, of course. But let me know your preferences.
>
> Thanks!
>
> > 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, becase we need
>
> typo (thanks, Gmail): because
ok
>
> > 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 si in separate change,
>
> typo: is
ok, thanks
jirka
^ permalink raw reply
* Re: [PATCHv4 7/7] man2: Add uretprobe syscall page
From: Jiri Olsa @ 2024-05-02 20:13 UTC (permalink / raw)
To: Alejandro Colomar
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
In-Reply-To: <ZjOYf_g2qRrhDoQD@debian>
On Thu, May 02, 2024 at 03:43:27PM +0200, Alejandro Colomar wrote:
> Hi Jiri,
>
> On Thu, May 02, 2024 at 02:23:13PM +0200, Jiri Olsa wrote:
> > Adding man page for new uretprobe syscall.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > man2/uretprobe.2 | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 45 insertions(+)
> > create mode 100644 man2/uretprobe.2
> >
> > diff --git a/man2/uretprobe.2 b/man2/uretprobe.2
> > new file mode 100644
> > index 000000000000..08fe6a670430
> > --- /dev/null
> > +++ b/man2/uretprobe.2
> > @@ -0,0 +1,45 @@
> > +.\" 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
> > +Kernel is using
> > +.BR uretprobe()
> > +syscall to trigger uprobe return probe consumers instead of using
> > +standard breakpoint instruction.
> > +
>
> Please use .P instead of a blank. See man-pages(7):
>
> Formatting conventions (general)
> Paragraphs should be separated by suitable markers (usually either
> .P or .IP). Do not separate paragraphs using blank lines, as this
> results in poor rendering in some output formats (such as Post‐
> Script and PDF).
ok, will do
>
> > +The uretprobe syscall is not supposed to be called directly by user, it's allowed
>
> s/by user/by the user/
ok
>
> > +to be invoked only through user space trampoline provided by kernel.
>
> s/user space/user-space/
ok
>
> Missing a few 'the' too, here and in the rest of the page.
ok, will check
>
> > +When called from outside of this trampoline, the calling process will receive
> > +.BR SIGILL .
> > +
> > +.SH RETURN VALUE
> > +.BR uretprobe()
>
> You're missing a space here:
>
> .BR uretprobe ()
ok
>
> > +return value is specific for given architecture.
> > +
> > +.SH VERSIONS
> > +This syscall is not specified in POSIX,
> > +and details of its behavior vary across systems.
> > +.SH STANDARDS
> > +None.
>
> You could add a HISTORY section.
ok, IIUC for this syscall it should contain just kernel version where
it got merged, right?
>
> Have a lovely day!
thanks for review,
jirka
^ permalink raw reply
* Re: [PATCHv4 7/7] man2: Add uretprobe syscall page
From: Alejandro Colomar @ 2024-05-02 22:06 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
In-Reply-To: <ZjPz2PWrW2BjXxlw@krava>
[-- Attachment #1: Type: text/plain, Size: 481 bytes --]
Hi Jiri,
On Thu, May 02, 2024 at 10:13:12PM +0200, Jiri Olsa wrote:
> > You could add a HISTORY section.
>
> ok, IIUC for this syscall it should contain just kernel version where
> it got merged, right?
Yep.
>
> >
> > Have a lovely day!
>
> thanks for review,
> jirka
Thanks for the page.
Have a lovely night!
Alex
--
<https://www.alejandro-colomar.es/>
A client is hiring kernel driver, mm, and/or crypto developers;
contact me if interested.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCHv4 bpf-next 2/7] uprobe: Add uretprobe syscall to speed up return probe
From: Peter Zijlstra @ 2024-05-03 11:34 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, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski,
rick.p.edgecombe
In-Reply-To: <20240502122313.1579719-3-jolsa@kernel.org>
On Thu, May 02, 2024 at 02:23:08PM +0200, Jiri Olsa wrote:
> 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
>
Did you consider shadow stacks? IIRC we currently have userspace shadow
stack support available, and that will utterly break all of this.
It would be really nice if the new scheme would consider shadow stacks.
^ permalink raw reply
* Re: [PATCHv4 bpf-next 2/7] uprobe: Add uretprobe syscall to speed up return probe
From: Jiri Olsa @ 2024-05-03 13:04 UTC (permalink / raw)
To: Peter Zijlstra
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, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski,
rick.p.edgecombe
In-Reply-To: <20240503113453.GK40213@noisy.programming.kicks-ass.net>
On Fri, May 03, 2024 at 01:34:53PM +0200, Peter Zijlstra wrote:
> On Thu, May 02, 2024 at 02:23:08PM +0200, Jiri Olsa wrote:
> > 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
> >
>
> Did you consider shadow stacks? IIRC we currently have userspace shadow
> stack support available, and that will utterly break all of this.
nope.. I guess it's the extra ret instruction in the trampoline that would
make it crash?
>
> It would be really nice if the new scheme would consider shadow stacks.
I seem to have the hw with support for user_shstk, let me test that
thanks,
jirka
^ permalink raw reply
* Re: [PATCHv4 bpf-next 2/7] uprobe: Add uretprobe syscall to speed up return probe
From: Edgecombe, Rick P @ 2024-05-03 15:53 UTC (permalink / raw)
To: olsajiri@gmail.com, peterz@infradead.org
Cc: songliubraving@fb.com, luto@kernel.org, mhiramat@kernel.org,
andrii@kernel.org, linux-api@vger.kernel.org,
john.fastabend@gmail.com, linux-kernel@vger.kernel.org,
mingo@redhat.com, rostedt@goodmis.org, ast@kernel.org,
tglx@linutronix.de, yhs@fb.com, linux-man@vger.kernel.org,
oleg@redhat.com, daniel@iogearbox.net,
linux-trace-kernel@vger.kernel.org, bpf@vger.kernel.org,
bp@alien8.de, x86@kernel.org
In-Reply-To: <ZjTg2cunShA6VbpY@krava>
On Fri, 2024-05-03 at 15:04 +0200, Jiri Olsa wrote:
> On Fri, May 03, 2024 at 01:34:53PM +0200, Peter Zijlstra wrote:
> > On Thu, May 02, 2024 at 02:23:08PM +0200, Jiri Olsa wrote:
> > > 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
Hi,
I worked on the x86 shadow stack support.
I didn't know uprobes did anything like this. In hindsight I should have looked
more closely. The current upstream behavior is to overwrite the return address
on the stack?
Stupid uprobes question - what is actually overwriting the return address on the
stack? Is it the kernel? If so perhaps the kernel could just update the shadow
stack at the same time.
> > >
> > > 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
> > >
> >
> > Did you consider shadow stacks? IIRC we currently have userspace shadow
> > stack support available, and that will utterly break all of this.
>
> nope.. I guess it's the extra ret instruction in the trampoline that would
> make it crash?
The original behavior seems problematic for shadow stack IIUC. I'm not sure of
the additional breakage with the new behavior.
Roughly, how shadow stack works is there is an additional protected stack for
the app thread. The HW pushes to from the shadow stack with CALL, and pops from
it with RET. But it also continues to push and pop from the normal stack. On
pop, if the values don't match between the two stacks, an exception is
generated. The whole point is to prevent the app from overwriting its stack
return address to return to random places.
Userspace cannot (normally) write to the shadow stack, but the kernel can do
this or adust the SSP (shadow stack pointer). So in the kernel (for things like
sigreturn) there is an ability to do what is needed. Ptracers also can do things
like this.
^ permalink raw reply
* Re: [PATCHv4 bpf-next 0/7] uprobe: uretprobe speed up
From: Andrii Nakryiko @ 2024-05-03 18:03 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
In-Reply-To: <ZjPx0fncg-8brFBk@krava>
On Thu, May 2, 2024 at 1:04 PM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Thu, May 02, 2024 at 09:43:02AM -0700, Andrii Nakryiko wrote:
> > On Thu, May 2, 2024 at 5:23 AM Jiri Olsa <jolsa@kernel.org> 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.
> > >
> > > 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-6 are based on bpf-next/master, but path 1 and 2 are
> > > apply-able on linux-trace.git tree probes/for-next branch.
> > > Patch 7 is based on man-pages master.
> > >
> > > v4 changes:
> > > - added acks [Oleg,Andrii,Masami]
> > > - reworded the man page and adding more info to NOTE section [Masami]
> > > - rewrote bpf tests not to use trace_pipe [Andrii]
> > > - cc-ed linux-man list
> > >
> > > Also available at:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > > uretprobe_syscall
> > >
> >
> > It looks great to me, thanks! Unfortunately BPF CI build is broken,
> > probably due to some of the Makefile additions, please investigate and
> > fix (or we'll need to fix something on BPF CI side), but it looks like
> > you'll need another revision, unfortunately.
> >
> > pw-bot: cr
> >
> > [0] https://github.com/kernel-patches/bpf/actions/runs/8923849088/job/24509002194
>
> yes, I think it's missing the 32-bit libc for uprobe_compat binary,
> probably it needs to be added to github.com:libbpf/ci.git setup-build-env/action.yml ?
> hm but I'm not sure how to test it, need to check
You can create a custom PR directly against Github repo
(kernel-patches/bpf) and BPF CI will run all the tests on your custom
code. This way you can iterate without spamming the mailing list.
But I'm just wondering if it's worth complicating setup just for
testing this x32 compat mode. So maybe just dropping one of those
patches would be better?
>
> >
> >
> >
> > But while we are at it.
> >
> > Masami, Oleg,
> >
> > What should be the logistics of landing this? Can/should we route this
> > through the bpf-next tree, given there are lots of BPF-based
> > selftests? Or you want to take this through
> > linux-trace/probes/for-next? In the latter case, it's probably better
> > to apply only the first two patches to probes/for-next and the rest
> > should still go through the bpf-next tree (otherwise we are running
>
> I think this was the plan, previously mentioned in here:
> https://lore.kernel.org/bpf/20240423000943.478ccf1e735a63c6c1b4c66b@kernel.org/
>
Ok, then we'll have to land this patch set as two separate ones. It's
fine, let's figure out if you need to do anything for shadow stacks
and try to land it soon.
> > into conflicts in BPF selftests). Previously we were handling such
> > cross-tree dependencies by creating a named branch or tag, and merging
> > it into bpf-next (so that all SHAs are preserved). It's a bunch of
> > extra work for everyone involved, so the simplest way would be to just
> > land through bpf-next, of course. But let me know your preferences.
> >
> > Thanks!
> >
> > > 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, becase we need
> >
> > typo (thanks, Gmail): because
>
> ok
>
> >
> > > 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 si in separate change,
> >
> > typo: is
>
> ok, thanks
>
> jirka
^ permalink raw reply
* Re: [PATCHv4 bpf-next 2/7] uprobe: Add uretprobe syscall to speed up return probe
From: Jiri Olsa @ 2024-05-03 19:18 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: olsajiri@gmail.com, peterz@infradead.org, songliubraving@fb.com,
luto@kernel.org, mhiramat@kernel.org, andrii@kernel.org,
linux-api@vger.kernel.org, john.fastabend@gmail.com,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, ast@kernel.org, tglx@linutronix.de,
yhs@fb.com, linux-man@vger.kernel.org, oleg@redhat.com,
daniel@iogearbox.net, linux-trace-kernel@vger.kernel.org,
bpf@vger.kernel.org, bp@alien8.de, x86@kernel.org
In-Reply-To: <725e2000dc56d55da4097cface4109c17fe5ad1a.camel@intel.com>
On Fri, May 03, 2024 at 03:53:15PM +0000, Edgecombe, Rick P wrote:
> On Fri, 2024-05-03 at 15:04 +0200, Jiri Olsa wrote:
> > On Fri, May 03, 2024 at 01:34:53PM +0200, Peter Zijlstra wrote:
> > > On Thu, May 02, 2024 at 02:23:08PM +0200, Jiri Olsa wrote:
> > > > 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
>
> Hi,
>
> I worked on the x86 shadow stack support.
>
> I didn't know uprobes did anything like this. In hindsight I should have looked
> more closely. The current upstream behavior is to overwrite the return address
> on the stack?
>
> Stupid uprobes question - what is actually overwriting the return address on the
> stack? Is it the kernel? If so perhaps the kernel could just update the shadow
> stack at the same time.
yes, it's in kernel - arch_uretprobe_hijack_return_addr .. so I guess
we need to update the shadow stack with the new return value as well
>
> > > >
> > > > 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
> > > >
> > >
> > > Did you consider shadow stacks? IIRC we currently have userspace shadow
> > > stack support available, and that will utterly break all of this.
> >
> > nope.. I guess it's the extra ret instruction in the trampoline that would
> > make it crash?
>
> The original behavior seems problematic for shadow stack IIUC. I'm not sure of
> the additional breakage with the new behavior.
I can see it's broken also for current uprobes
>
> Roughly, how shadow stack works is there is an additional protected stack for
> the app thread. The HW pushes to from the shadow stack with CALL, and pops from
> it with RET. But it also continues to push and pop from the normal stack. On
> pop, if the values don't match between the two stacks, an exception is
> generated. The whole point is to prevent the app from overwriting its stack
> return address to return to random places.
>
> Userspace cannot (normally) write to the shadow stack, but the kernel can do
> this or adust the SSP (shadow stack pointer). So in the kernel (for things like
> sigreturn) there is an ability to do what is needed. Ptracers also can do things
> like this.
hack below seems to fix it for the current uprobe setup,
we need similar fix for the uretprobe syscall trampoline setup
jirka
---
diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h
index 42fee8959df7..99a0948a3b79 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);
+void uprobe_change_stack(unsigned long addr);
#else
static inline long shstk_prctl(struct task_struct *task, int option,
unsigned long arg2) { return -EINVAL; }
diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
index 59e15dd8d0f8..d2c4dbe5843c 100644
--- a/arch/x86/kernel/shstk.c
+++ b/arch/x86/kernel/shstk.c
@@ -577,3 +577,11 @@ long shstk_prctl(struct task_struct *task, int option, unsigned long arg2)
return wrss_control(true);
return -EINVAL;
}
+
+void uprobe_change_stack(unsigned long addr)
+{
+ unsigned long ssp;
+
+ ssp = get_user_shstk_addr();
+ write_user_shstk_64((u64 __user *)ssp, (u64)addr);
+}
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 81e6ee95784d..88afbeaacb8f 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -348,7 +348,7 @@ void *arch_uprobe_trampoline(unsigned long *psize)
* only for native 64-bit process, the compat process still uses
* standard breakpoint.
*/
- if (user_64bit_mode(regs)) {
+ if (0 && user_64bit_mode(regs)) {
*psize = uretprobe_syscall_end - uretprobe_syscall_entry;
return uretprobe_syscall_entry;
}
@@ -1191,8 +1191,10 @@ 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)) {
+ uprobe_change_stack(trampoline_vaddr);
return orig_ret_vaddr;
+ }
if (nleft != rasize) {
pr_err("return address clobbered: pid=%d, %%sp=%#lx, %%ip=%#lx\n",
^ permalink raw reply related
* Re: [PATCHv4 bpf-next 2/7] uprobe: Add uretprobe syscall to speed up return probe
From: Edgecombe, Rick P @ 2024-05-03 19:38 UTC (permalink / raw)
To: olsajiri@gmail.com
Cc: songliubraving@fb.com, luto@kernel.org, mhiramat@kernel.org,
andrii@kernel.org, linux-api@vger.kernel.org,
john.fastabend@gmail.com, debug@rivosinc.com,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, ast@kernel.org, tglx@linutronix.de,
yhs@fb.com, oleg@redhat.com, linux-man@vger.kernel.org,
daniel@iogearbox.net, peterz@infradead.org,
linux-trace-kernel@vger.kernel.org, bp@alien8.de,
bpf@vger.kernel.org, x86@kernel.org, broonie@kernel.org
In-Reply-To: <ZjU4ganRF1Cbiug6@krava>
+Some more shadow stack folks from other archs. We are discussing how uretprobes
work with shadow stack.
Context:
https://lore.kernel.org/lkml/ZjU4ganRF1Cbiug6@krava/
On Fri, 2024-05-03 at 21:18 +0200, Jiri Olsa wrote:
>
> hack below seems to fix it for the current uprobe setup,
> we need similar fix for the uretprobe syscall trampoline setup
It seems like a reasonable direction.
Security-wise, applications cannot do this on themselves, or it is an otherwise
privileged thing right?
^ permalink raw reply
* Re: [PATCHv4 bpf-next 2/7] uprobe: Add uretprobe syscall to speed up return probe
From: Jiri Olsa @ 2024-05-03 20:17 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: olsajiri@gmail.com, songliubraving@fb.com, luto@kernel.org,
mhiramat@kernel.org, andrii@kernel.org, linux-api@vger.kernel.org,
john.fastabend@gmail.com, debug@rivosinc.com,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, ast@kernel.org, tglx@linutronix.de,
yhs@fb.com, oleg@redhat.com, linux-man@vger.kernel.org,
daniel@iogearbox.net, peterz@infradead.org,
linux-trace-kernel@vger.kernel.org, bp@alien8.de,
bpf@vger.kernel.org, x86@kernel.org, broonie@kernel.org
In-Reply-To: <6c143c648e2eff6c4d4b5e4700d1a8fbcc0f8cbc.camel@intel.com>
On Fri, May 03, 2024 at 07:38:18PM +0000, Edgecombe, Rick P wrote:
> +Some more shadow stack folks from other archs. We are discussing how uretprobes
> work with shadow stack.
>
> Context:
> https://lore.kernel.org/lkml/ZjU4ganRF1Cbiug6@krava/
>
> On Fri, 2024-05-03 at 21:18 +0200, Jiri Olsa wrote:
> >
> > hack below seems to fix it for the current uprobe setup,
> > we need similar fix for the uretprobe syscall trampoline setup
>
> It seems like a reasonable direction.
>
> Security-wise, applications cannot do this on themselves, or it is an otherwise
> privileged thing right?
when uretprobe is created, kernel overwrites the return address on user
stack to point to user space trampoline, so the setup is in kernel hands
with the hack below on top of this patchset I'm no longer seeing shadow
stack app crash on uretprobe.. I'll try to polish it and send out next
week, any suggestions are welcome ;-)
thanks,
jirka
---
diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h
index 42fee8959df7..d374305a6851 100644
--- a/arch/x86/include/asm/shstk.h
+++ b/arch/x86/include/asm/shstk.h
@@ -21,6 +21,8 @@ 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);
+void uprobe_change_stack(unsigned long addr);
+void uprobe_push_stack(unsigned long addr);
#else
static inline long shstk_prctl(struct task_struct *task, int option,
unsigned long arg2) { return -EINVAL; }
diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
index 59e15dd8d0f8..804c446231d9 100644
--- a/arch/x86/kernel/shstk.c
+++ b/arch/x86/kernel/shstk.c
@@ -577,3 +577,24 @@ long shstk_prctl(struct task_struct *task, int option, unsigned long arg2)
return wrss_control(true);
return -EINVAL;
}
+
+void uprobe_change_stack(unsigned long addr)
+{
+ unsigned long ssp;
+
+ ssp = get_user_shstk_addr();
+ write_user_shstk_64((u64 __user *)ssp, (u64)addr);
+}
+
+void uprobe_push_stack(unsigned long addr)
+{
+ unsigned long ssp;
+
+ ssp = get_user_shstk_addr();
+ ssp -= SS_FRAME_SIZE;
+ write_user_shstk_64((u64 __user *)ssp, (u64)addr);
+
+ fpregs_lock_and_load();
+ wrmsrl(MSR_IA32_PL3_SSP, ssp);
+ fpregs_unlock();
+}
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 81e6ee95784d..259457838020 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -416,6 +416,7 @@ SYSCALL_DEFINE0(uretprobe)
regs->r11 = regs->flags;
regs->cx = regs->ip;
+ uprobe_push_stack(r11_cx_ax[2]);
return regs->ax;
sigill:
@@ -1191,8 +1192,10 @@ 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)) {
+ uprobe_change_stack(trampoline_vaddr);
return orig_ret_vaddr;
+ }
if (nleft != rasize) {
pr_err("return address clobbered: pid=%d, %%sp=%#lx, %%ip=%#lx\n",
^ permalink raw reply related
* Re: [PATCHv4 bpf-next 2/7] uprobe: Add uretprobe syscall to speed up return probe
From: Edgecombe, Rick P @ 2024-05-03 20:35 UTC (permalink / raw)
To: olsajiri@gmail.com
Cc: songliubraving@fb.com, luto@kernel.org, mhiramat@kernel.org,
andrii@kernel.org, debug@rivosinc.com, john.fastabend@gmail.com,
linux-api@vger.kernel.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, rostedt@goodmis.org, ast@kernel.org,
tglx@linutronix.de, linux-man@vger.kernel.org, oleg@redhat.com,
yhs@fb.com, daniel@iogearbox.net, peterz@infradead.org,
linux-trace-kernel@vger.kernel.org, bp@alien8.de,
bpf@vger.kernel.org, x86@kernel.org, broonie@kernel.org
In-Reply-To: <ZjVGZeY-_ySqgfER@krava>
On Fri, 2024-05-03 at 22:17 +0200, Jiri Olsa wrote:
> when uretprobe is created, kernel overwrites the return address on user
> stack to point to user space trampoline, so the setup is in kernel hands
I mean for uprobes in general. I'm didn't have any specific ideas in mind, but
in general when we give the kernel more abilities around shadow stack we have to
think if attackers could use it to work around shadow stack protections.
>
> with the hack below on top of this patchset I'm no longer seeing shadow
> stack app crash on uretprobe.. I'll try to polish it and send out next
> week, any suggestions are welcome ;-)
Thanks. Some comments below.
>
> thanks,
> jirka
>
>
> ---
> diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h
> index 42fee8959df7..d374305a6851 100644
> --- a/arch/x86/include/asm/shstk.h
> +++ b/arch/x86/include/asm/shstk.h
> @@ -21,6 +21,8 @@ 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);
> +void uprobe_change_stack(unsigned long addr);
> +void uprobe_push_stack(unsigned long addr);
Maybe name them:
shstk_update_last_frame();
shstk_push_frame();
> #else
> static inline long shstk_prctl(struct task_struct *task, int option,
> unsigned long arg2) { return -EINVAL; }
> diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> index 59e15dd8d0f8..804c446231d9 100644
> --- a/arch/x86/kernel/shstk.c
> +++ b/arch/x86/kernel/shstk.c
> @@ -577,3 +577,24 @@ long shstk_prctl(struct task_struct *task, int option,
> unsigned long arg2)
> return wrss_control(true);
> return -EINVAL;
> }
> +
> +void uprobe_change_stack(unsigned long addr)
> +{
> + unsigned long ssp;
Probably want something like:
if (!features_enabled(ARCH_SHSTK_SHSTK))
return;
So this doesn't try the below if shadow stack is disabled.
> +
> + ssp = get_user_shstk_addr();
> + write_user_shstk_64((u64 __user *)ssp, (u64)addr);
> +}
Can we know that there was a valid return address just before this point on the
stack? Or could it be a sigframe or something?
> +
> +void uprobe_push_stack(unsigned long addr)
> +{
> + unsigned long ssp;
if (!features_enabled(ARCH_SHSTK_SHSTK))
return;
> +
> + ssp = get_user_shstk_addr();
> + ssp -= SS_FRAME_SIZE;
> + write_user_shstk_64((u64 __user *)ssp, (u64)addr);
> +
> + fpregs_lock_and_load();
> + wrmsrl(MSR_IA32_PL3_SSP, ssp);
> + fpregs_unlock();
> +}
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 81e6ee95784d..259457838020 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -416,6 +416,7 @@ SYSCALL_DEFINE0(uretprobe)
> regs->r11 = regs->flags;
> regs->cx = regs->ip;
>
> + uprobe_push_stack(r11_cx_ax[2]);
I'm concerned this could be used to push arbitrary frames to the shadow stack.
Couldn't an attacker do a jump to the point that calls this syscall? Maybe this
is what peterz was raising.
> return regs->ax;
>
> sigill:
> @@ -1191,8 +1192,10 @@ 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)) {
> + uprobe_change_stack(trampoline_vaddr);
> return orig_ret_vaddr;
> + }
>
> if (nleft != rasize) {
> pr_err("return address clobbered: pid=%d, %%sp=%#lx,
> %%ip=%#lx\n",
^ permalink raw reply
* Re: [PATCHv4 bpf-next 0/7] uprobe: uretprobe speed up
From: Jiri Olsa @ 2024-05-03 20:39 UTC (permalink / raw)
To: Andrii Nakryiko
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
In-Reply-To: <CAEf4Bzb-dM+464JvW96KuxwOTfRQA1pxZRWM+pA7AfSWtWwqZw@mail.gmail.com>
On Fri, May 03, 2024 at 11:03:24AM -0700, Andrii Nakryiko wrote:
> On Thu, May 2, 2024 at 1:04 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Thu, May 02, 2024 at 09:43:02AM -0700, Andrii Nakryiko wrote:
> > > On Thu, May 2, 2024 at 5:23 AM Jiri Olsa <jolsa@kernel.org> 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.
> > > >
> > > > 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-6 are based on bpf-next/master, but path 1 and 2 are
> > > > apply-able on linux-trace.git tree probes/for-next branch.
> > > > Patch 7 is based on man-pages master.
> > > >
> > > > v4 changes:
> > > > - added acks [Oleg,Andrii,Masami]
> > > > - reworded the man page and adding more info to NOTE section [Masami]
> > > > - rewrote bpf tests not to use trace_pipe [Andrii]
> > > > - cc-ed linux-man list
> > > >
> > > > Also available at:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > > > uretprobe_syscall
> > > >
> > >
> > > It looks great to me, thanks! Unfortunately BPF CI build is broken,
> > > probably due to some of the Makefile additions, please investigate and
> > > fix (or we'll need to fix something on BPF CI side), but it looks like
> > > you'll need another revision, unfortunately.
> > >
> > > pw-bot: cr
> > >
> > > [0] https://github.com/kernel-patches/bpf/actions/runs/8923849088/job/24509002194
> >
> > yes, I think it's missing the 32-bit libc for uprobe_compat binary,
> > probably it needs to be added to github.com:libbpf/ci.git setup-build-env/action.yml ?
> > hm but I'm not sure how to test it, need to check
>
> You can create a custom PR directly against Github repo
> (kernel-patches/bpf) and BPF CI will run all the tests on your custom
> code. This way you can iterate without spamming the mailing list.
I'm running CI tests like that, but I think I need to change the action
which is in other repo (github.com:libbpf/ci.git)
>
> But I'm just wondering if it's worth complicating setup just for
> testing this x32 compat mode. So maybe just dropping one of those
> patches would be better?
well, we had compat process crashing on uretprobe because of this change,
so I rather keep the test.. or it can go in later on when the CI stuff is
figured out.. I got busy with the shadow stack issue today, will check on
the CI PR next week
jirka
^ permalink raw reply
* Re: [PATCHv4 bpf-next 2/7] uprobe: Add uretprobe syscall to speed up return probe
From: Deepak Gupta @ 2024-05-03 23:01 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: olsajiri@gmail.com, songliubraving@fb.com, luto@kernel.org,
mhiramat@kernel.org, andrii@kernel.org, linux-api@vger.kernel.org,
john.fastabend@gmail.com, linux-kernel@vger.kernel.org,
mingo@redhat.com, rostedt@goodmis.org, ast@kernel.org,
tglx@linutronix.de, yhs@fb.com, oleg@redhat.com,
linux-man@vger.kernel.org, daniel@iogearbox.net,
peterz@infradead.org, linux-trace-kernel@vger.kernel.org,
bp@alien8.de, bpf@vger.kernel.org, x86@kernel.org,
broonie@kernel.org
In-Reply-To: <6c143c648e2eff6c4d4b5e4700d1a8fbcc0f8cbc.camel@intel.com>
On Fri, May 03, 2024 at 07:38:18PM +0000, Edgecombe, Rick P wrote:
>+Some more shadow stack folks from other archs. We are discussing how uretprobes
>work with shadow stack.
>
>Context:
>https://lore.kernel.org/lkml/ZjU4ganRF1Cbiug6@krava/
Thanks Rick.
Yeah I didn't give enough attention to uprobes either.
Although now that I think for RISC-V shadow stack, it shouldn't be an issue.
On RISC-V return addresses don't get pushed as part of call instruction.
There is a distinct instruction "shadow stack push of return address" in prolog.
Similarly in epilog there is distinct instruction "shadow stack pop and check with
link register".
On RISC-V, uretprobe would install a uprobe on function start and when it's hit.
It'll replace pt_regs->ra = trampoline_handler. As function will resume, trampoline
addr will get pushed and popped. Although trampoline_handler would have to be enlightened
to eventually return to original return site.
>
>On Fri, 2024-05-03 at 21:18 +0200, Jiri Olsa wrote:
>>
>> hack below seems to fix it for the current uprobe setup,
>> we need similar fix for the uretprobe syscall trampoline setup
>
>It seems like a reasonable direction.
>
>Security-wise, applications cannot do this on themselves, or it is an otherwise
>privileged thing right?
>
>
^ permalink raw reply
* Re: [PATCH v6 3/3] openat2: add OA2_CRED_INHERIT flag
From: Donald Buczek @ 2024-05-04 20:38 UTC (permalink / raw)
To: Stas Sergeev, linux-kernel
Cc: Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240427112451.1609471-4-stsp2@yandex.ru>
On 4/27/24 13:24, Stas Sergeev wrote:
> This flag performs the open operation with the fs credentials
> (fsuid, fsgid, group_info) that were in effect when dir_fd was opened.
> dir_fd must be opened with O_CRED_ALLOW, or EPERM is returned.
>
> Selftests are added to check for these properties as well as for
> the invalid flag combinations.
>
> This allows the process to pre-open some directories and then
> change eUID (and all other UIDs/GIDs) to a less-privileged user,
> retaining the ability to open/create files within these directories.
>
> Design goal:
> The idea is to provide a very light-weight sandboxing, where the
> process, without the use of any heavy-weight techniques like chroot
> within namespaces, can restrict the access to the set of pre-opened
> directories.
> This patch is just a first step to such sandboxing. If things go
> well, in the future the same extension can be added to more syscalls.
> These should include at least unlinkat(), renameat2() and the
> not-yet-upstreamed setxattrat().
>
> Security considerations:
> - Only the bare minimal set of credentials is overridden:
> fsuid, fsgid and group_info. The rest, for example capabilities,
> are not overridden to avoid unneeded security risks.
> - To avoid sandboxing escape, this patch makes sure the restricted
> lookup modes are used. Namely, RESOLVE_BENEATH or RESOLVE_IN_ROOT.
> - Magic /proc symlinks are discarded, as suggested by
> Andy Lutomirski <luto@kernel.org>> - O_CRED_ALLOW fds cannot be passed via unix socket and are always
> closed on exec() to prevent "unsuspecting userspace" from not being
> able to fully drop privs.
What about hard links?
== snip ==
#include <sys/types.h>
#include <sys/stat.h>
#include <stdio.h>
#include <errno.h>
#include <stdlib.h>
#include <stdarg.h>
#include <fcntl.h>
#include <unistd.h>
#include <linux/openat2.h>
#define O_CRED_ALLOW 0x2000000
#define OA2_CRED_INHERIT (1UL << 28)
#define SYS_openat2 437
long openat2(int dirfd, const char *pathname, struct open_how *how, size_t size) {
return syscall(SYS_openat2, dirfd, pathname, how, size);
}
__attribute__ ((noreturn, format(printf, 1, 2)))
static void die(const char *restrict fmt, ...) {
va_list ap;
va_start(ap, fmt);
vfprintf(stderr, fmt, ap);
va_end(ap);
_exit(1);
}
int main() {
unlink("/tmp/d/test.dat");
unlink("/tmp/d/hostname");
if (rmdir("/tmp/d") != 0 && errno != ENOENT)
die("/tmp/d: %m\n");
umask(0);
if (mkdir("/tmp/d", 0777) != 0)
die("/tmp/d: %m\n");
int dirfd = open("/tmp/d", O_RDONLY + O_CRED_ALLOW);
if (dirfd == -1)
die("/tmp/d: %m\n");
if (setuid(1000) != 0)
die("setuid: %m\n");
if (link("/etc/hostname", "/tmp/d/hostname") == -1)
die ("/etc/hostname: %m\n");
if(openat(dirfd, "hostname", O_RDWR) != -1)
die("/tmp/d/hostname could be opened by uid 1000");
{ struct open_how how = { .flags = O_RDWR + OA2_CRED_INHERIT, .resolve = RESOLVE_BENEATH };
if (openat2(dirfd, "hostname", &how, sizeof(how)) == -1)
die("hostname: %m\n");
printf("able to open /etc/hostname RDWR \n");
}
}
== snip ==
buczek@dose:~$ gcc -O0 -Wall -Wextra -Werror -g -o test test.c
buczek@dose:~$ sudo ./test
able to open /etc/hostname RDWR
buczek@dose:~$
--
Donald Buczek
buczek@molgen.mpg.de
Tel: +49 30 8413 1433
^ permalink raw reply
* Re: [PATCH v6 3/3] openat2: add OA2_CRED_INHERIT flag
From: stsp @ 2024-05-04 21:11 UTC (permalink / raw)
To: Donald Buczek, linux-kernel
Cc: Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
In-Reply-To: <bf4a737a-0c5b-4349-886d-4013683818ce@molgen.mpg.de>
04.05.2024 23:38, Donald Buczek пишет:
> On 4/27/24 13:24, Stas Sergeev wrote:
>> This flag performs the open operation with the fs credentials
>> (fsuid, fsgid, group_info) that were in effect when dir_fd was opened.
>> dir_fd must be opened with O_CRED_ALLOW, or EPERM is returned.
>>
>> Selftests are added to check for these properties as well as for
>> the invalid flag combinations.
>>
>> This allows the process to pre-open some directories and then
>> change eUID (and all other UIDs/GIDs) to a less-privileged user,
>> retaining the ability to open/create files within these directories.
>>
>> Design goal:
>> The idea is to provide a very light-weight sandboxing, where the
>> process, without the use of any heavy-weight techniques like chroot
>> within namespaces, can restrict the access to the set of pre-opened
>> directories.
>> This patch is just a first step to such sandboxing. If things go
>> well, in the future the same extension can be added to more syscalls.
>> These should include at least unlinkat(), renameat2() and the
>> not-yet-upstreamed setxattrat().
>>
>> Security considerations:
>> - Only the bare minimal set of credentials is overridden:
>> fsuid, fsgid and group_info. The rest, for example capabilities,
>> are not overridden to avoid unneeded security risks.
>> - To avoid sandboxing escape, this patch makes sure the restricted
>> lookup modes are used. Namely, RESOLVE_BENEATH or RESOLVE_IN_ROOT.
>> - Magic /proc symlinks are discarded, as suggested by
>> Andy Lutomirski <luto@kernel.org>> - O_CRED_ALLOW fds cannot be
>> passed via unix socket and are always
>> closed on exec() to prevent "unsuspecting userspace" from not being
>> able to fully drop privs.
>
> What about hard links?
Well, you set umask to 0 in your example.
If you didn't do that, the dir wouldn't have
0777 perms, and the hard link would not
be created.
But yes, that demonstrates the unsafe
usage scenario, i.e. unsafe directory perms
immediately lead to a security hole.
Maybe O_CRED_ALLOW should check for
safe perms, or maybe it shouldn't... So far
there are no signs of this patch to ever be
accepted, so I am not sure if more complexity
needs to be added to it.
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Aleksa Sarai @ 2024-05-06 7:13 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stas Sergeev, Serge E. Hallyn, linux-kernel, Stefan Metzmacher,
Eric Biederman, Alexander Viro, Andy Lutomirski,
Christian Brauner, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <CALCETrUL3zXAX94CpcQYwj1omwO+=-1Li+J7Bw2kpAw4d7nsyw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5369 bytes --]
On 2024-04-28, Andy Lutomirski <luto@amacapital.net> wrote:
> > On Apr 26, 2024, at 6:39 AM, Stas Sergeev <stsp2@yandex.ru> wrote:
> > This patch-set implements the OA2_CRED_INHERIT flag for openat2() syscall.
> > It is needed to perform an open operation with the creds that were in
> > effect when the dir_fd was opened, if the dir was opened with O_CRED_ALLOW
> > flag. This allows the process to pre-open some dirs and switch eUID
> > (and other UIDs/GIDs) to the less-privileged user, while still retaining
> > the possibility to open/create files within the pre-opened directory set.
> >
>
> I’ve been contemplating this, and I want to propose a different solution.
>
> First, the problem Stas is solving is quite narrow and doesn’t
> actually need kernel support: if I want to write a user program that
> sandboxes itself, I have at least three solutions already. I can make
> a userns and a mountns; I can use landlock; and I can have a separate
> process that brokers filesystem access using SCM_RIGHTS.
>
> But what if I want to run a container, where the container can access
> a specific host directory, and the contained application is not aware
> of the exact technology being used? I recently started using
> containers in anger in a production setting, and “anger” was
> definitely the right word: binding part of a filesystem in is
> *miserable*. Getting the DAC rules right is nasty. LSMs are worse.
> Podman’s “bind,relabel” feature is IMO utterly disgusting. I think I
> actually gave up on making one of my use cases work on a Fedora
> system.
>
> Here’s what I wanted to do, logically, in production: pick a host
> directory, pick a host *principal* (UID, GID, label, etc), and have
> the *entire container* access the directory as that principal. This is
> what happens automatically if I run the whole container as a userns
> with only a single UID mapped, but I don’t really want to do that for
> a whole variety and of reasons.
>
> So maybe reimagining Stas’ feature a bit can actually solve this
> problem. Instead of a special dirfd, what if there was a special
> subtree (in the sense of open_tree) that captures a set of creds and
> does all opens inside the subtree using those creds?
>
> This isn’t a fully formed proposal, but I *think* it should be
> generally fairly safe for even an unprivileged user to clone a subtree
> with a specific flag set to do this. Maybe a capability would be
> needed (CAP_CAPTURE_CREDS?), but it would be nice to allow delegating
> this to a daemon if a privilege is needed, and getting the API right
> might be a bit tricky.
Tying this to an actual mount rather than a file handle sounds like a
more plausible proposal than OA2_CRED_INHERIT, but it just seems that
this is going to re-create all of the work that went into id-mapped
mounts but with the extra-special step of making the generic VFS
permissions no longer work normally (unless the idea is that everything
would pretend to be owned by current_fsuid()?).
IMHO it also isn't enough to just make open work, you need to make all
operations work (which leads to a non-trivial amount of
filesystem-specific handling), which is just idmapped mounts. A lot of
work was put into making sure that is safe, and collapsing owners seems
like it will cause a lot of headaches.
I also find it somewhat amusing that this proposal is to basically give
up on multi-user permissions for this one directory tree because it's
too annoying to deal with. In that case, isn't chmod 777 a simpler
solution? (I'm being a bit flippant, of course there is a difference,
but the net result is that all users in the container would have the
same permissions with all of the fun issues that implies.)
In short, AFAICS idmapped mounts pretty much solve this problem (minus
the ability to collapse users, which I suspect is not a good idea in
general)?
> Then two different things could be done:
>
> 1. The subtree could be used unmounted or via /proc magic links. This
> would be for programs that are aware of this interface.
>
> 2. The subtree could be mounted, and accessed through the mount would
> use the captured creds.
>
> (Hmm. What would a new open_tree() pointing at this special subtree do?)
>
>
> With all this done, if userspace wired it up, a container user could
> do something like:
>
> —bind-capture-creds source=dest
>
> And the contained program would access source *as the user who started
> the container*, and this would just work without relabeling or
> fiddling with owner uids or gids or ACLs, and it would continue to
> work even if the container has multiple dynamically allocated subuids
> mapped (e.g. one for “root” and one for the actual application).
>
> Bonus points for the ability to revoke the creds in an already opened
> subtree. Or even for the creds to automatically revoke themselves when
> the opener exits (or maybe when a specific cred-pinning fd goes away).
>
> (This should work for single files as well as for directories.)
>
> New LSM hooks or extensions of existing hooks might be needed to make
> LSMs comfortable with this.
>
> What do you all think?
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ 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