* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Kees Cook @ 2025-01-18 20:21 UTC (permalink / raw)
To: Eyal Birger
Cc: luto, wad, oleg, ldv, mhiramat, andrii, jolsa, alexei.starovoitov,
olsajiri, cyphar, songliubraving, yhs, john.fastabend, peterz,
tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <20250117005539.325887-1-eyal.birger@gmail.com>
On Thu, Jan 16, 2025 at 04:55:39PM -0800, Eyal Birger wrote:
> Since uretprobe is a "kernel implementation detail" system call which is
> not used by userspace application code directly, it is impractical and
> there's very little point in forcing all userspace applications to
> explicitly allow it in order to avoid crashing tracked processes.
How is this any different from sigreturn, rt_sigreturn, or
restart_syscall? These are all handled explicitly by userspace filters
already, and I don't see why uretprobe should be any different. Docker
has had plenty of experience with fixing their seccomp filters for new
syscalls. For example, many times already a given libc will suddenly
start using a new syscall when it sees its available, etc.
Basically, this is a Docker issue, not a kernel issue. Seccomp is
behaving correctly. I don't want to start making syscalls invisible
without an extremely good reason. If _anything_ should be invisible, it
is restart_syscall (which actually IS invisible under certain
architectures).
-Kees
--
Kees Cook
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Oleg Nesterov @ 2025-01-18 15:05 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Masami Hiramatsu, Eyal Birger, kees, luto, wad, andrii, jolsa,
alexei.starovoitov, olsajiri, cyphar, songliubraving, yhs,
john.fastabend, peterz, tglx, bp, daniel, ast, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable, Dmitry V. Levin
In-Reply-To: <CAEf4BzYhcG8waFMFoQS5dFWVkQGP6ed_0mwGTK4quN5+6-8XuA@mail.gmail.com>
On 01/17, Andrii Nakryiko wrote:
>
> On Fri, Jan 17, 2025 at 6:10 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > We can, and this is what I tried to suggest from the very beginning.
> > But I agree with Eyal who decided to send the most trivial fix for
> > -stable, we can add the helper later.
> >
> > I don't think it should live in uprobes.h and I'd prefer something
> > like arch_seccomp_ignored(int) but I won't insist.
>
> yep, I think this is the way, keeping it as a general category. Should
> we also put rt_sigreturn there explicitly as well? Also, wouldn't it
> be better to have it as a non-arch-specific function for something
> like rt_sigreturn where defining it per each arch is cumbersome, and
> have the default implementation also call into an arch-specific
> function?
I personally don't think we should exclude rt_sigreturn. and I guess
we can't do it in a arch-agnostic way, think of __NR_ia32_sigreturn.
However. These are all good questions that need a separate discussion.
Plus the SECCOMP_RET_TRACE/strace issue raised by Dmitry. And probably
even more.
But IMO it would be better to push the trivial (and urgent) fix to
-stable first, then discuss the possible cleanups/improvements.
What do you think?
Oleg.
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Oleg Nesterov @ 2025-01-18 14:13 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Eugene Syromyatnikov, Mike Frysinger, Renzo Davoli,
Davide Berardi, strace-devel, linux-kernel, linux-api
In-Reply-To: <20250117162255.GA15597@strace.io>
On 01/17, Dmitry V. Levin wrote:
>
(reordered)
> struct ptrace_syscall_info has members of type __u64, and it currently
> ends with "__u32 ret_data". So depending on the alignment, the structure
> either has extra 4 trailing padding bytes, or it doesn't.
Ah, I didn't realize that the last member is __u32, so I completely
misunderstood your "it depends on the alignment of __u64" note.
> For example, on x86_64 sizeof(struct ptrace_syscall_info) is currently 88,
> while on x86 it is 84.
Not good, but too late to complain...
OK, I see your point now and I won't argue with approach you outlined in your
previous email
size_t min_size = offsetofend(struct ptrace_syscall_info, seccomp.ret_data);
size_t copy_size = min(sizeof(info), user_size);
if (copy_size < min_size)
return -EINVAL;
if (copy_from_user(&info, datavp, copy_size))
return -EFAULT;
-------------------------------------------------------------------------------
Thats said... Can't resist,
> An absolutely artificial example: let's say we're adding an optional
> 64-bit field "artificial" to ptrace_syscall_info.seccomp, this means
> sizeof(ptrace_syscall_info) grows by 8 bytes. When userspace wants
> to set this optional field, it sets a bit in ptrace_syscall_info.flags,
> this tells the kernel to look into this new "artificial" field.
> When userspace is not interested in setting new optional fields,
> it just keeps ptrace_syscall_info.flags == 0. Remember, however, that
> by adding the new optional field sizeof(ptrace_syscall_info) grew by 8 bytes.
>
> What we need is to make sure that an older kernel that has no idea of this
> new field would still accept the bigger size, so that userspace would be
> able to continue doing its
> ptrace(PTRACE_SET_SYSCALL_INFO, pid, sizeof(info), &info)
> despite of potential growth of sizeof(info) until it actually starts using
> new optional fields.
This is clear, but personally I don't really like this pattern... Consider
void set_syscall_info(int unlikely_condition)
{
struct ptrace_syscall_info info;
fill_info(&info);
if (unlikely_condition) {
info.flags = USE_ARTIFICIAL;
info.artificial = 1;
}
assert(ptrace(PTRACE_SET_SYSCALL_INFO, sizeof(info), &info) == 0);
}
Now this application (running on the older kernel) can fail or not, depending
on "unlikely_condition". To me it would be better to always fail in this case.
That is why I tried to suggest to use "user_size" as a version number.
Currently we have PTRACE_SYSCALL_INFO_SIZE_VER0, when we add the new
"artificial" member we will have PTRACE_SYSCALL_INFO_SIZE_VER1. Granted,
this way set_syscall_info() can't use sizeof(info), it should do
ptrace(PTRACE_SET_SYSCALL_INFO, PTRACE_SYSCALL_INFO_SIZE_VER1, info);
and the kernel needs more checks, but this is what I had in mind when I said
that the 1st version can just require "user_size == PTRACE_SYSCALL_INFO_SIZE_VER0".
But I won't insist, I do not pretend I understand the user-space needs.
Thanks!
Oleg.
^ permalink raw reply
* Re: [PATCH RFC v3 02/10] sched_getattr: port to copy_struct_to_user
From: Xi Ruoyao @ 2025-01-18 13:02 UTC (permalink / raw)
To: Christian Brauner, Florian Weimer, Aleksa Sarai, Ingo Molnar
Cc: Peter Zijlstra, Juri Lelli, Vincent Guittot, Dietmar Eggemann,
Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider,
Alexander Viro, Jan Kara, Arnd Bergmann, Shuah Khan, Kees Cook,
Mark Rutland, linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, libc-alpha
In-Reply-To: <20241211-gemsen-zuarbeiten-ae8d062ec251@brauner>
On Wed, 2024-12-11 at 11:23 +0100, Christian Brauner wrote:
> On Tue, Dec 10, 2024 at 07:14:07PM +0100, Florian Weimer wrote:
> > * Aleksa Sarai:
> >
> > > sched_getattr(2) doesn't care about trailing non-zero bytes in the
> > > (ksize > usize) case, so just use copy_struct_to_user() without checking
> > > ignored_trailing.
> >
> > I think this is what causes glibc's misc/tst-sched_setattr test to fail
> > on recent kernels. The previous non-modifying behavior was documented
> > in the manual page:
> >
> > If the caller-provided attr buffer is larger than the kernel's
> > sched_attr structure, the additional bytes in the user-space
> > structure are not touched.
> >
> > I can just drop this part of the test if the kernel deems both behaviors
> > valid.
> I think in general both behaviors are valid but I would consider zeroing
> the unknown parts of the provided buffer to be the safer option. And all
> newer extensible struct system calls do that.
Florian,
So should we drop the test before Glibc-2.41 release? I'm seeing the
failure during my machine test.
--
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Andrii Nakryiko @ 2025-01-17 19:34 UTC (permalink / raw)
To: Eyal Birger
Cc: Oleg Nesterov, Masami Hiramatsu, kees, luto, wad, andrii, jolsa,
alexei.starovoitov, olsajiri, cyphar, songliubraving, yhs,
john.fastabend, peterz, tglx, bp, daniel, ast, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <CAHsH6GvgqXgd3F_Nqf-f-tOigtmOACXFukSm+Wpi561xf2vCAA@mail.gmail.com>
On Fri, Jan 17, 2025 at 11:24 AM Eyal Birger <eyal.birger@gmail.com> wrote:
>
> On Fri, Jan 17, 2025 at 9:51 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Fri, Jan 17, 2025 at 6:10 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > On 01/17, Masami Hiramatsu wrote:
> > > >
> > > > On Fri, 17 Jan 2025 02:39:28 +0100
> > > > Oleg Nesterov <oleg@redhat.com> wrote:
> > > >
> > > > > A note for the seccomp maintainers...
> > > > >
> > > > > I don't know what do you think, but I agree in advance that the very fact this
> > > > > patch adds "#ifdef CONFIG_X86_64" into __secure_computing() doesn't look nice.
> > > > >
> > > >
> > > > Indeed. in_ia32_syscall() depends arch/x86 too.
> > > > We can add an inline function like;
> > > >
> > > > ``` uprobes.h
> > > > static inline bool is_uprobe_syscall(int syscall)
> > > > {
> > >
> > > We can, and this is what I tried to suggest from the very beginning.
> > > But I agree with Eyal who decided to send the most trivial fix for
> > > -stable, we can add the helper later.
> > >
> > > I don't think it should live in uprobes.h and I'd prefer something
> > > like arch_seccomp_ignored(int) but I won't insist.
> >
> > yep, I think this is the way, keeping it as a general category. Should
> > we also put rt_sigreturn there explicitly as well? Also, wouldn't it
> > be better to have it as a non-arch-specific function for something
> > like rt_sigreturn where defining it per each arch is cumbersome, and
> > have the default implementation also call into an arch-specific
> > function?
>
> I like the more generic approach and keeping CONFIG_X86 out of seccomp,
> and more generic than uprobes, however, I'm not sure where a common part
> to place it which includes arch/x86/include/asm/syscall.h would be. And
> as mentioned before, this would make this bugfix more complex to backport.
>
> For that reason I wouldn't refactor handling rt_sigreturn as part of
> this fix.
>
SGTM, it can always be improved later, if necessary
> Thanks!
> Eyal.
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Eyal Birger @ 2025-01-17 19:24 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Oleg Nesterov, Masami Hiramatsu, kees, luto, wad, andrii, jolsa,
alexei.starovoitov, olsajiri, cyphar, songliubraving, yhs,
john.fastabend, peterz, tglx, bp, daniel, ast, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <CAEf4BzYhcG8waFMFoQS5dFWVkQGP6ed_0mwGTK4quN5+6-8XuA@mail.gmail.com>
On Fri, Jan 17, 2025 at 9:51 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jan 17, 2025 at 6:10 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > On 01/17, Masami Hiramatsu wrote:
> > >
> > > On Fri, 17 Jan 2025 02:39:28 +0100
> > > Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > > A note for the seccomp maintainers...
> > > >
> > > > I don't know what do you think, but I agree in advance that the very fact this
> > > > patch adds "#ifdef CONFIG_X86_64" into __secure_computing() doesn't look nice.
> > > >
> > >
> > > Indeed. in_ia32_syscall() depends arch/x86 too.
> > > We can add an inline function like;
> > >
> > > ``` uprobes.h
> > > static inline bool is_uprobe_syscall(int syscall)
> > > {
> >
> > We can, and this is what I tried to suggest from the very beginning.
> > But I agree with Eyal who decided to send the most trivial fix for
> > -stable, we can add the helper later.
> >
> > I don't think it should live in uprobes.h and I'd prefer something
> > like arch_seccomp_ignored(int) but I won't insist.
>
> yep, I think this is the way, keeping it as a general category. Should
> we also put rt_sigreturn there explicitly as well? Also, wouldn't it
> be better to have it as a non-arch-specific function for something
> like rt_sigreturn where defining it per each arch is cumbersome, and
> have the default implementation also call into an arch-specific
> function?
I like the more generic approach and keeping CONFIG_X86 out of seccomp,
and more generic than uprobes, however, I'm not sure where a common part
to place it which includes arch/x86/include/asm/syscall.h would be. And
as mentioned before, this would make this bugfix more complex to backport.
For that reason I wouldn't refactor handling rt_sigreturn as part of
this fix.
Thanks!
Eyal.
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Eyal Birger @ 2025-01-17 18:52 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: kees, luto, wad, oleg, mhiramat, andrii, jolsa,
alexei.starovoitov, olsajiri, cyphar, songliubraving, yhs,
john.fastabend, peterz, tglx, bp, daniel, ast, andrii.nakryiko,
rostedt, rafi, shmulik.ladkani, bpf, linux-api,
linux-trace-kernel, x86, linux-kernel, stable
In-Reply-To: <20250117183416.GA16831@strace.io>
On Fri, Jan 17, 2025 at 10:34 AM Dmitry V. Levin <ldv@strace.io> wrote:
>
> On Thu, Jan 16, 2025 at 04:55:39PM -0800, Eyal Birger wrote:
> > When attaching uretprobes to processes running inside docker, the attached
> > process is segfaulted when encountering the retprobe.
> >
> > The reason is that now that uretprobe is a system call the default seccomp
> > filters in docker block it as they only allow a specific set of known
> > syscalls. This is true for other userspace applications which use seccomp
> > to control their syscall surface.
> >
> > Since uretprobe is a "kernel implementation detail" system call which is
> > not used by userspace application code directly, it is impractical and
> > there's very little point in forcing all userspace applications to
> > explicitly allow it in order to avoid crashing tracked processes.
> >
> > Pass this systemcall through seccomp without depending on configuration.
> >
> > Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
> > Reported-by: Rafael Buchbinder <rafi@rbk.io>
> > Link: https://lore.kernel.org/lkml/CAHsH6Gs3Eh8DFU0wq58c_LF8A4_+o6z456J7BidmcVY2AqOnHQ@mail.gmail.com/
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
> > ---
> >
> > The following reproduction script synthetically demonstrates the problem:
> >
> > cat > /tmp/x.c << EOF
> >
> > char *syscalls[] = {
> > "write",
> > "exit_group",
> > "fstat",
> > };
> >
> > __attribute__((noinline)) int probed(void)
> > {
> > printf("Probed\n");
> > return 1;
> > }
> >
> > void apply_seccomp_filter(char **syscalls, int num_syscalls)
> > {
> > scmp_filter_ctx ctx;
> >
> > ctx = seccomp_init(SCMP_ACT_KILL);
> > for (int i = 0; i < num_syscalls; i++) {
> > seccomp_rule_add(ctx, SCMP_ACT_ALLOW,
> > seccomp_syscall_resolve_name(syscalls[i]), 0);
> > }
> > seccomp_load(ctx);
> > seccomp_release(ctx);
> > }
> >
> > int main(int argc, char *argv[])
> > {
> > int num_syscalls = sizeof(syscalls) / sizeof(syscalls[0]);
> >
> > apply_seccomp_filter(syscalls, num_syscalls);
> >
> > probed();
> >
> > return 0;
> > }
> > EOF
> >
> > cat > /tmp/trace.bt << EOF
> > uretprobe:/tmp/x:probed
> > {
> > printf("ret=%d\n", retval);
> > }
> > EOF
> >
> > gcc -o /tmp/x /tmp/x.c -lseccomp
> >
> > /usr/bin/bpftrace /tmp/trace.bt &
> >
> > sleep 5 # wait for uretprobe attach
> > /tmp/x
> >
> > pkill bpftrace
> >
> > rm /tmp/x /tmp/x.c /tmp/trace.bt
> > ---
> > kernel/seccomp.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > index 385d48293a5f..10a55c9b5c18 100644
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -1359,6 +1359,11 @@ int __secure_computing(const struct seccomp_data *sd)
> > this_syscall = sd ? sd->nr :
> > syscall_get_nr(current, current_pt_regs());
> >
> > +#ifdef CONFIG_X86_64
> > + if (unlikely(this_syscall == __NR_uretprobe) && !in_ia32_syscall())
> > + return 0;
> > +#endif
> > +
> > switch (mode) {
> > case SECCOMP_MODE_STRICT:
> > __secure_computing_strict(this_syscall); /* may call do_exit */
>
> This seems to be a hot fix to bypass some SECCOMP_RET_ERRNO filters.
It's a little broader than just SECCOMP_RET_ERRNO, but yes, this is a
hotfix to avoid filtering this system call in seccomp.
The rationale is that this is not a userspace created system call - the
kernel uses it to instrument the function - and the fact that it's a
system call is just an implementation detail. Ideally, userspace wouldn't
need to know or care about it.
> However, this way it bypasses seccomp completely, including
> SECCOMP_RET_TRACE, making it invisible to strace --seccomp,
> and I wonder why do you want that.
It's a good question. I could move this check to both "strict" seccomp and
after the BPF verdict is received, but before it's applied, but I fear this
would make the fix more error prone, and way harder to backmerge. So I'm
wondering whether supporting strace --seccomp-bpf for this particular
syscall is a priority.
Eyal.
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Dmitry V. Levin @ 2025-01-17 18:34 UTC (permalink / raw)
To: Eyal Birger
Cc: kees, luto, wad, oleg, mhiramat, andrii, jolsa,
alexei.starovoitov, olsajiri, cyphar, songliubraving, yhs,
john.fastabend, peterz, tglx, bp, daniel, ast, andrii.nakryiko,
rostedt, rafi, shmulik.ladkani, bpf, linux-api,
linux-trace-kernel, x86, linux-kernel, stable
In-Reply-To: <20250117005539.325887-1-eyal.birger@gmail.com>
On Thu, Jan 16, 2025 at 04:55:39PM -0800, Eyal Birger wrote:
> When attaching uretprobes to processes running inside docker, the attached
> process is segfaulted when encountering the retprobe.
>
> The reason is that now that uretprobe is a system call the default seccomp
> filters in docker block it as they only allow a specific set of known
> syscalls. This is true for other userspace applications which use seccomp
> to control their syscall surface.
>
> Since uretprobe is a "kernel implementation detail" system call which is
> not used by userspace application code directly, it is impractical and
> there's very little point in forcing all userspace applications to
> explicitly allow it in order to avoid crashing tracked processes.
>
> Pass this systemcall through seccomp without depending on configuration.
>
> Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
> Reported-by: Rafael Buchbinder <rafi@rbk.io>
> Link: https://lore.kernel.org/lkml/CAHsH6Gs3Eh8DFU0wq58c_LF8A4_+o6z456J7BidmcVY2AqOnHQ@mail.gmail.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
> ---
>
> The following reproduction script synthetically demonstrates the problem:
>
> cat > /tmp/x.c << EOF
>
> char *syscalls[] = {
> "write",
> "exit_group",
> "fstat",
> };
>
> __attribute__((noinline)) int probed(void)
> {
> printf("Probed\n");
> return 1;
> }
>
> void apply_seccomp_filter(char **syscalls, int num_syscalls)
> {
> scmp_filter_ctx ctx;
>
> ctx = seccomp_init(SCMP_ACT_KILL);
> for (int i = 0; i < num_syscalls; i++) {
> seccomp_rule_add(ctx, SCMP_ACT_ALLOW,
> seccomp_syscall_resolve_name(syscalls[i]), 0);
> }
> seccomp_load(ctx);
> seccomp_release(ctx);
> }
>
> int main(int argc, char *argv[])
> {
> int num_syscalls = sizeof(syscalls) / sizeof(syscalls[0]);
>
> apply_seccomp_filter(syscalls, num_syscalls);
>
> probed();
>
> return 0;
> }
> EOF
>
> cat > /tmp/trace.bt << EOF
> uretprobe:/tmp/x:probed
> {
> printf("ret=%d\n", retval);
> }
> EOF
>
> gcc -o /tmp/x /tmp/x.c -lseccomp
>
> /usr/bin/bpftrace /tmp/trace.bt &
>
> sleep 5 # wait for uretprobe attach
> /tmp/x
>
> pkill bpftrace
>
> rm /tmp/x /tmp/x.c /tmp/trace.bt
> ---
> kernel/seccomp.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 385d48293a5f..10a55c9b5c18 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -1359,6 +1359,11 @@ int __secure_computing(const struct seccomp_data *sd)
> this_syscall = sd ? sd->nr :
> syscall_get_nr(current, current_pt_regs());
>
> +#ifdef CONFIG_X86_64
> + if (unlikely(this_syscall == __NR_uretprobe) && !in_ia32_syscall())
> + return 0;
> +#endif
> +
> switch (mode) {
> case SECCOMP_MODE_STRICT:
> __secure_computing_strict(this_syscall); /* may call do_exit */
This seems to be a hot fix to bypass some SECCOMP_RET_ERRNO filters.
However, this way it bypasses seccomp completely, including
SECCOMP_RET_TRACE, making it invisible to strace --seccomp,
and I wonder why do you want that.
--
ldv
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Andrii Nakryiko @ 2025-01-17 17:53 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Oleg Nesterov, Jiri Olsa, Aleksa Sarai, Eyal Birger, mhiramat,
linux-kernel, linux-trace-kernel, BPF-dev-list, Song Liu,
Yonghong Song, John Fastabend, tglx, bp, x86, linux-api,
Andrii Nakryiko, Daniel Borkmann, Alexei Starovoitov,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <20250117114130.GB8603@noisy.programming.kicks-ass.net>
On Fri, Jan 17, 2025 at 3:41 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Jan 14, 2025 at 01:45:11PM -0800, Andrii Nakryiko wrote:
>
> > someday sys_uretprobe will be old as well
>
> And then we'll delete it because everybody that cares about performance
> will have FRED on ;-)
with "then" sufficiently far into the future, sure :)
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Andrii Nakryiko @ 2025-01-17 17:51 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Masami Hiramatsu, Eyal Birger, kees, luto, wad, andrii, jolsa,
alexei.starovoitov, olsajiri, cyphar, songliubraving, yhs,
john.fastabend, peterz, tglx, bp, daniel, ast, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <20250117140924.GA21203@redhat.com>
On Fri, Jan 17, 2025 at 6:10 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 01/17, Masami Hiramatsu wrote:
> >
> > On Fri, 17 Jan 2025 02:39:28 +0100
> > Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > A note for the seccomp maintainers...
> > >
> > > I don't know what do you think, but I agree in advance that the very fact this
> > > patch adds "#ifdef CONFIG_X86_64" into __secure_computing() doesn't look nice.
> > >
> >
> > Indeed. in_ia32_syscall() depends arch/x86 too.
> > We can add an inline function like;
> >
> > ``` uprobes.h
> > static inline bool is_uprobe_syscall(int syscall)
> > {
>
> We can, and this is what I tried to suggest from the very beginning.
> But I agree with Eyal who decided to send the most trivial fix for
> -stable, we can add the helper later.
>
> I don't think it should live in uprobes.h and I'd prefer something
> like arch_seccomp_ignored(int) but I won't insist.
yep, I think this is the way, keeping it as a general category. Should
we also put rt_sigreturn there explicitly as well? Also, wouldn't it
be better to have it as a non-arch-specific function for something
like rt_sigreturn where defining it per each arch is cumbersome, and
have the default implementation also call into an arch-specific
function?
>
> > // arch_is_uprobe_syscall check can be replaced by Kconfig,
> > // something like CONFIG_ARCH_URETPROBE_SYSCALL.
>
> Or sysctl or both. This is another issue.
>
> > ``` arch/x86/include/asm/uprobes.h
> > #define arch_is_uprobe_syscall(syscall) \
> > (IS_ENABLED(CONFIG_X86_64) && syscall == __NR_uretprobe && !in_ia32_syscall())
> > ```
>
> This won't compile if IS_ENABLED(CONFIG_X86_64) == false, __NR_uretprobe
> will be undefined.
>
> > > The problem is that we need a simple patch for -stable which fixes the real
> > > problem. We can cleanup this logic later, I think.
> >
> > Hmm, at least we should make it is_uprobe_syscall() in uprobes.h so that
> > do not pollute the seccomp subsystem with #ifdef.
>
> See above. But I won't insist.
>
> Oleg.
>
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2025-01-17 16:22 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Eugene Syromyatnikov, Mike Frysinger, Renzo Davoli,
Davide Berardi, strace-devel, linux-kernel, linux-api
In-Reply-To: <20250117153258.GC21203@redhat.com>
On Fri, Jan 17, 2025 at 04:32:59PM +0100, Oleg Nesterov wrote:
> Dmitry,
>
> You certainly understand the user-space needs much better than me.
> I am just trying to understand your point.
>
> On 01/17, Dmitry V. Levin wrote:
> >
> > We should accept larger user_size from the very beginning, so that in case
> > the structure grows in the future, the userspace that sicks to the current
> > set of supported features would be still able to work with older kernels.
>
> This is what I can't understand, perhaps I have a blind spot here ;)
>
> Could you provide an example (even absolutely artificial) of possible extension
> which can help me to understand?
An absolutely artificial example: let's say we're adding an optional
64-bit field "artificial" to ptrace_syscall_info.seccomp, this means
sizeof(ptrace_syscall_info) grows by 8 bytes. When userspace wants
to set this optional field, it sets a bit in ptrace_syscall_info.flags,
this tells the kernel to look into this new "artificial" field.
When userspace is not interested in setting new optional fields,
it just keeps ptrace_syscall_info.flags == 0. Remember, however, that
by adding the new optional field sizeof(ptrace_syscall_info) grew by 8 bytes.
What we need is to make sure that an older kernel that has no idea of this
new field would still accept the bigger size, so that userspace would be
able to continue doing its
ptrace(PTRACE_SET_SYSCALL_INFO, pid, sizeof(info), &info)
despite of potential growth of sizeof(info) until it actually starts using
new optional fields.
> > We cannot just use sizeof(info) because it depends on the alignment of
> > __u64.
>
> Hmm why? I thought that the kernel already depends on the "natural" alignment?
> And if we can't, then PTRACE_SYSCALL_INFO_SIZE_VER0 added by this patch makes
> no sense?
struct ptrace_syscall_info has members of type __u64, and it currently
ends with "__u32 ret_data". So depending on the alignment, the structure
either has extra 4 trailing padding bytes, or it doesn't.
For example, on x86_64 sizeof(struct ptrace_syscall_info) is currently 88,
while on x86 it is 84.
--
ldv
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Oleg Nesterov @ 2025-01-17 15:32 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Eugene Syromyatnikov, Mike Frysinger, Renzo Davoli,
Davide Berardi, strace-devel, linux-kernel, linux-api
In-Reply-To: <20250117150627.GA15109@strace.io>
Dmitry,
You certainly understand the user-space needs much better than me.
I am just trying to understand your point.
On 01/17, Dmitry V. Levin wrote:
>
> We should accept larger user_size from the very beginning, so that in case
> the structure grows in the future, the userspace that sicks to the current
> set of supported features would be still able to work with older kernels.
This is what I can't understand, perhaps I have a blind spot here ;)
Could you provide an example (even absolutely artificial) of possible extension
which can help me to understand?
> We cannot just use sizeof(info) because it depends on the alignment of
> __u64.
Hmm why? I thought that the kernel already depends on the "natural" alignment?
And if we can't, then PTRACE_SYSCALL_INFO_SIZE_VER0 added by this patch makes
no sense?
Sorry I guess I must have missed something, I am sick today.
> Also, I don't think we need to fill with zeroes the trailing
> padding bytes of the structure as we are not going to use them in any way.
At least we seem to agree here ;)
Oleg.
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2025-01-17 15:06 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Eugene Syromyatnikov, Mike Frysinger, Renzo Davoli,
Davide Berardi, strace-devel, linux-kernel, linux-api
In-Reply-To: <20250117144556.GB21203@redhat.com>
On Fri, Jan 17, 2025 at 03:45:57PM +0100, Oleg Nesterov wrote:
> On 01/16, Dmitry V. Levin wrote:
> >
> > The idea is to use "op" to specify the operation, and "flags" to specify
> > future extensions to the operation.
>
> OK,
>
> > That is, the zero check implied by copy_struct_from_user() is not really
> > needed here since the compatibility is tracked by "op" and "flags":
>
> OK, but then why this patch uses copy_struct_from_user() ?
>
> Why can't we simply do
>
> if (user_size != PTRACE_SYSCALL_INFO_SIZE_VER0)
> return -EINVAL;
>
> if (copy_from_user(..., user_size))
> return EFAULT;
>
> now, until we add the extensions ?
We should accept larger user_size from the very beginning, so that in case
the structure grows in the future, the userspace that sicks to the current
set of supported features would be still able to work with older kernels.
I think we can do the following:
/*
* ptrace_syscall_info.seccomp is the largest member in the union,
* and ret_data is the last field there.
* min_size can be less than sizeof(info) due to alignment.
*/
size_t min_size = offsetofend(struct ptrace_syscall_info, seccomp.ret_data);
size_t copy_size = min(sizeof(info), user_size);
if (copy_size < min_size)
return -EINVAL;
if (copy_from_user(&info, datavp, copy_size))
return -EFAULT;
We cannot just use sizeof(info) because it depends on the alignment of
__u64. Also, I don't think we need to fill with zeroes the trailing
padding bytes of the structure as we are not going to use them in any way.
--
ldv
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Oleg Nesterov @ 2025-01-17 14:45 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Eugene Syromyatnikov, Mike Frysinger, Renzo Davoli,
Davide Berardi, strace-devel, linux-kernel, linux-api
In-Reply-To: <20250116160403.GA3554@strace.io>
On 01/16, Dmitry V. Levin wrote:
>
> The idea is to use "op" to specify the operation, and "flags" to specify
> future extensions to the operation.
OK,
> That is, the zero check implied by copy_struct_from_user() is not really
> needed here since the compatibility is tracked by "op" and "flags":
OK, but then why this patch uses copy_struct_from_user() ?
Why can't we simply do
if (user_size != PTRACE_SYSCALL_INFO_SIZE_VER0)
return -EINVAL;
if (copy_from_user(..., user_size))
return EFAULT;
now, until we add the extensions ?
Oleg.
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Oleg Nesterov @ 2025-01-17 14:09 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Eyal Birger, kees, luto, wad, andrii, jolsa, alexei.starovoitov,
olsajiri, cyphar, songliubraving, yhs, john.fastabend, peterz,
tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <20250117170229.f1e1a9f03a8547d31cd875db@kernel.org>
On 01/17, Masami Hiramatsu wrote:
>
> On Fri, 17 Jan 2025 02:39:28 +0100
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > A note for the seccomp maintainers...
> >
> > I don't know what do you think, but I agree in advance that the very fact this
> > patch adds "#ifdef CONFIG_X86_64" into __secure_computing() doesn't look nice.
> >
>
> Indeed. in_ia32_syscall() depends arch/x86 too.
> We can add an inline function like;
>
> ``` uprobes.h
> static inline bool is_uprobe_syscall(int syscall)
> {
We can, and this is what I tried to suggest from the very beginning.
But I agree with Eyal who decided to send the most trivial fix for
-stable, we can add the helper later.
I don't think it should live in uprobes.h and I'd prefer something
like arch_seccomp_ignored(int) but I won't insist.
> // arch_is_uprobe_syscall check can be replaced by Kconfig,
> // something like CONFIG_ARCH_URETPROBE_SYSCALL.
Or sysctl or both. This is another issue.
> ``` arch/x86/include/asm/uprobes.h
> #define arch_is_uprobe_syscall(syscall) \
> (IS_ENABLED(CONFIG_X86_64) && syscall == __NR_uretprobe && !in_ia32_syscall())
> ```
This won't compile if IS_ENABLED(CONFIG_X86_64) == false, __NR_uretprobe
will be undefined.
> > The problem is that we need a simple patch for -stable which fixes the real
> > problem. We can cleanup this logic later, I think.
>
> Hmm, at least we should make it is_uprobe_syscall() in uprobes.h so that
> do not pollute the seccomp subsystem with #ifdef.
See above. But I won't insist.
Oleg.
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Eyal Birger @ 2025-01-17 13:36 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Oleg Nesterov, kees, luto, wad, andrii, jolsa, alexei.starovoitov,
olsajiri, cyphar, songliubraving, yhs, john.fastabend, peterz,
tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <20250117170229.f1e1a9f03a8547d31cd875db@kernel.org>
On Fri, Jan 17, 2025 at 12:02 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Fri, 17 Jan 2025 02:39:28 +0100
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > On 01/16, Eyal Birger wrote:
> > >
> > > Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
> > > Reported-by: Rafael Buchbinder <rafi@rbk.io>
> > > Link: https://lore.kernel.org/lkml/CAHsH6Gs3Eh8DFU0wq58c_LF8A4_+o6z456J7BidmcVY2AqOnHQ@mail.gmail.com/
> > > Cc: stable@vger.kernel.org
> > ...
> > > @@ -1359,6 +1359,11 @@ int __secure_computing(const struct seccomp_data *sd)
> > > this_syscall = sd ? sd->nr :
> > > syscall_get_nr(current, current_pt_regs());
> > >
> > > +#ifdef CONFIG_X86_64
> > > + if (unlikely(this_syscall == __NR_uretprobe) && !in_ia32_syscall())
> > > + return 0;
> > > +#endif
> >
> > Acked-by: Oleg Nesterov <oleg@redhat.com>
> >
> >
> > A note for the seccomp maintainers...
> >
> > I don't know what do you think, but I agree in advance that the very fact this
> > patch adds "#ifdef CONFIG_X86_64" into __secure_computing() doesn't look nice.
> >
>
> Indeed. in_ia32_syscall() depends arch/x86 too.
> We can add an inline function like;
>
> ``` uprobes.h
> static inline bool is_uprobe_syscall(int syscall)
> {
> // arch_is_uprobe_syscall check can be replaced by Kconfig,
> // something like CONFIG_ARCH_URETPROBE_SYSCALL.
> #ifdef arch_is_uprobe_syscall
> return arch_is_uprobe_syscall(syscall)
> #else
> return false;
> #endif
> }
> ```
> and
> ``` arch/x86/include/asm/uprobes.h
> #define arch_is_uprobe_syscall(syscall) \
> (IS_ENABLED(CONFIG_X86_64) && syscall == __NR_uretprobe && !in_ia32_syscall())
> ```
Notice it'll need to be enclosed in ifdef CONFIG_X86_64 as __NR_uretprobe
isn't defined otherwise so the IS_ENABLED isn't needed.
>
> > The problem is that we need a simple patch for -stable which fixes the real
> > problem. We can cleanup this logic later, I think.
>
> Hmm, at least we should make it is_uprobe_syscall() in uprobes.h so that
> do not pollute the seccomp subsystem with #ifdef.
I like this approach.
Notice it does add a couple of includes that weren't there before:
- arch/x86/include/asm/uprobes.h would include asm/unistd.h
- seccomp.c would include linux/uprobes.h
So it's a less "trivial" patch... If that's ok I can repost with these
changes.
Eyal.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Peter Zijlstra @ 2025-01-17 11:41 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Oleg Nesterov, Jiri Olsa, Aleksa Sarai, Eyal Birger, mhiramat,
linux-kernel, linux-trace-kernel, BPF-dev-list, Song Liu,
Yonghong Song, John Fastabend, tglx, bp, x86, linux-api,
Andrii Nakryiko, Daniel Borkmann, Alexei Starovoitov,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <CAEf4BzaRCzWMVvyGC_T52djF7q65yM8=AdBEMOPUU8edG-PLxg@mail.gmail.com>
On Tue, Jan 14, 2025 at 01:45:11PM -0800, Andrii Nakryiko wrote:
> someday sys_uretprobe will be old as well
And then we'll delete it because everybody that cares about performance
will have FRED on ;-)
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Masami Hiramatsu @ 2025-01-17 8:02 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Eyal Birger, kees, luto, wad, mhiramat, andrii, jolsa,
alexei.starovoitov, olsajiri, cyphar, songliubraving, yhs,
john.fastabend, peterz, tglx, bp, daniel, ast, andrii.nakryiko,
rostedt, rafi, shmulik.ladkani, bpf, linux-api,
linux-trace-kernel, x86, linux-kernel, stable
In-Reply-To: <20250117013927.GB2610@redhat.com>
On Fri, 17 Jan 2025 02:39:28 +0100
Oleg Nesterov <oleg@redhat.com> wrote:
> On 01/16, Eyal Birger wrote:
> >
> > Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
> > Reported-by: Rafael Buchbinder <rafi@rbk.io>
> > Link: https://lore.kernel.org/lkml/CAHsH6Gs3Eh8DFU0wq58c_LF8A4_+o6z456J7BidmcVY2AqOnHQ@mail.gmail.com/
> > Cc: stable@vger.kernel.org
> ...
> > @@ -1359,6 +1359,11 @@ int __secure_computing(const struct seccomp_data *sd)
> > this_syscall = sd ? sd->nr :
> > syscall_get_nr(current, current_pt_regs());
> >
> > +#ifdef CONFIG_X86_64
> > + if (unlikely(this_syscall == __NR_uretprobe) && !in_ia32_syscall())
> > + return 0;
> > +#endif
>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
>
>
> A note for the seccomp maintainers...
>
> I don't know what do you think, but I agree in advance that the very fact this
> patch adds "#ifdef CONFIG_X86_64" into __secure_computing() doesn't look nice.
>
Indeed. in_ia32_syscall() depends arch/x86 too.
We can add an inline function like;
``` uprobes.h
static inline bool is_uprobe_syscall(int syscall)
{
// arch_is_uprobe_syscall check can be replaced by Kconfig,
// something like CONFIG_ARCH_URETPROBE_SYSCALL.
#ifdef arch_is_uprobe_syscall
return arch_is_uprobe_syscall(syscall)
#else
return false;
#endif
}
```
and
``` arch/x86/include/asm/uprobes.h
#define arch_is_uprobe_syscall(syscall) \
(IS_ENABLED(CONFIG_X86_64) && syscall == __NR_uretprobe && !in_ia32_syscall())
```
> The problem is that we need a simple patch for -stable which fixes the real
> problem. We can cleanup this logic later, I think.
Hmm, at least we should make it is_uprobe_syscall() in uprobes.h so that
do not pollute the seccomp subsystem with #ifdef.
Thank you,
>
> Oleg.
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH 1/2] f2fs: register inodes which is able to donate pages
From: Christoph Hellwig @ 2025-01-17 7:56 UTC (permalink / raw)
To: Jaegeuk Kim
Cc: Christoph Hellwig, linux-kernel, linux-f2fs-devel, Al Viro,
Jan Kara, Christian Brauner, linux-fsdevel, linux-api, linux-man,
linux-mm
In-Reply-To: <Z4k_nKT3V1xuhXGc@google.com>
On Thu, Jan 16, 2025 at 05:19:24PM +0000, Jaegeuk Kim wrote:
> > mean a invalidate_inode_pages2_range. Which is a strange use of the
> > word. what are the use cases? Why is this queued up to a thread and
> > not done inline? Why is this in f2fs and not in common code.
>
> The idea is let apps register some file ranges for page donation and admin
> recliam such pages all togehter if they expect to see memory pressure soon.
> We can rely on LRU, but this is more user-given trigger. I'm not sure whether
> there's a need in general, hence, wanted to put it in f2fs first to get more
> concrete use-cases beyond this Android case.
Well, that's certainly not a file system feature. Please build this
as generic infrastucture and send it to the linux-mm list.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Oleg Nesterov @ 2025-01-17 1:57 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Jiri Olsa, Aleksa Sarai, Eyal Birger, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <20250117102307.cf919a0e7e59e3df0ddbcd3c@kernel.org>
On 01/17, Masami Hiramatsu wrote:
>
> On Tue, 14 Jan 2025 15:21:29 +0100
> Jiri Olsa <olsajiri@gmail.com> wrote:
>
> > On Tue, Jan 14, 2025 at 12:21:07PM +0100, Oleg Nesterov wrote:
> > >
> > > But please not that the uretprobed function can return any value
> > > including -ENOSYS, and this is what sys_uretprobe() has to return.
> >
> > right, uretprobe syscall returns value of the uretprobed function,
> > so we can't use any reserved value
>
> We can make uretprobe (entry) fail if the return value is one of
> errno or NULL, because it *knows* what the return value here.
I fail to understand... Could you spell please?
But whatever you meant, I don't think it is right, sorry.. please
correct me.
"it *knows*". Who it? How can it know??? I'd say "it" can't know,
but probably I missed your point.
Not to mention it doesn't really matter. It is not safe to even try
to do
movq $__NR_uretprobe, %rax
syscall
if the probed task has seccomp filters.
Oleg.
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Oleg Nesterov @ 2025-01-17 1:39 UTC (permalink / raw)
To: Eyal Birger
Cc: kees, luto, wad, mhiramat, andrii, jolsa, alexei.starovoitov,
olsajiri, cyphar, songliubraving, yhs, john.fastabend, peterz,
tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <20250117005539.325887-1-eyal.birger@gmail.com>
On 01/16, Eyal Birger wrote:
>
> Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
> Reported-by: Rafael Buchbinder <rafi@rbk.io>
> Link: https://lore.kernel.org/lkml/CAHsH6Gs3Eh8DFU0wq58c_LF8A4_+o6z456J7BidmcVY2AqOnHQ@mail.gmail.com/
> Cc: stable@vger.kernel.org
...
> @@ -1359,6 +1359,11 @@ int __secure_computing(const struct seccomp_data *sd)
> this_syscall = sd ? sd->nr :
> syscall_get_nr(current, current_pt_regs());
>
> +#ifdef CONFIG_X86_64
> + if (unlikely(this_syscall == __NR_uretprobe) && !in_ia32_syscall())
> + return 0;
> +#endif
Acked-by: Oleg Nesterov <oleg@redhat.com>
A note for the seccomp maintainers...
I don't know what do you think, but I agree in advance that the very fact this
patch adds "#ifdef CONFIG_X86_64" into __secure_computing() doesn't look nice.
The problem is that we need a simple patch for -stable which fixes the real
problem. We can cleanup this logic later, I think.
Oleg.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Masami Hiramatsu @ 2025-01-17 1:23 UTC (permalink / raw)
To: Jiri Olsa
Cc: Oleg Nesterov, Masami Hiramatsu, Aleksa Sarai, Eyal Birger,
linux-kernel, linux-trace-kernel, BPF-dev-list, Song Liu,
Yonghong Song, John Fastabend, peterz, tglx, bp, x86, linux-api,
Andrii Nakryiko, Daniel Borkmann, Alexei Starovoitov,
Andrii Nakryiko, rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <Z4Zy6W6z3ICp6SdJ@krava>
On Tue, 14 Jan 2025 15:21:29 +0100
Jiri Olsa <olsajiri@gmail.com> wrote:
> On Tue, Jan 14, 2025 at 12:21:07PM +0100, Oleg Nesterov wrote:
> > On 01/14, Masami Hiramatsu wrote:
> > >
> > > On Tue, 14 Jan 2025 10:22:20 +0100
> > > Jiri Olsa <olsajiri@gmail.com> wrote:
> > >
> > > > @@ -418,6 +439,9 @@ SYSCALL_DEFINE0(uretprobe)
> > > > regs->r11 = regs->flags;
> > > > regs->cx = regs->ip;
> > > >
> > > > + /* zero rbx to signal trampoline that uretprobe syscall was executed */
> > > > + regs->bx = 0;
> > >
> > > Can we just return -ENOSYS as like as other syscall instead of
> > > using rbx as a side channel?
> > > We can carefully check the return address is not -ERRNO when set up
> > > and reserve the -ENOSYS for this use case.
> >
> > Not sure I understand...
> >
> > But please not that the uretprobed function can return any value
> > including -ENOSYS, and this is what sys_uretprobe() has to return.
>
> right, uretprobe syscall returns value of the uretprobed function,
> so we can't use any reserved value
We can make uretprobe (entry) fail if the return value is one of
errno or NULL, because it *knows* what the return value here.
Thank you,
>
> jirka
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Eyal Birger @ 2025-01-17 0:55 UTC (permalink / raw)
To: kees, luto, wad, oleg, mhiramat, andrii, jolsa
Cc: alexei.starovoitov, olsajiri, cyphar, songliubraving, yhs,
john.fastabend, peterz, tglx, bp, daniel, ast, andrii.nakryiko,
rostedt, rafi, shmulik.ladkani, bpf, linux-api,
linux-trace-kernel, x86, linux-kernel, Eyal Birger, stable
When attaching uretprobes to processes running inside docker, the attached
process is segfaulted when encountering the retprobe.
The reason is that now that uretprobe is a system call the default seccomp
filters in docker block it as they only allow a specific set of known
syscalls. This is true for other userspace applications which use seccomp
to control their syscall surface.
Since uretprobe is a "kernel implementation detail" system call which is
not used by userspace application code directly, it is impractical and
there's very little point in forcing all userspace applications to
explicitly allow it in order to avoid crashing tracked processes.
Pass this systemcall through seccomp without depending on configuration.
Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
Reported-by: Rafael Buchbinder <rafi@rbk.io>
Link: https://lore.kernel.org/lkml/CAHsH6Gs3Eh8DFU0wq58c_LF8A4_+o6z456J7BidmcVY2AqOnHQ@mail.gmail.com/
Cc: stable@vger.kernel.org
Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
---
The following reproduction script synthetically demonstrates the problem:
cat > /tmp/x.c << EOF
char *syscalls[] = {
"write",
"exit_group",
"fstat",
};
__attribute__((noinline)) int probed(void)
{
printf("Probed\n");
return 1;
}
void apply_seccomp_filter(char **syscalls, int num_syscalls)
{
scmp_filter_ctx ctx;
ctx = seccomp_init(SCMP_ACT_KILL);
for (int i = 0; i < num_syscalls; i++) {
seccomp_rule_add(ctx, SCMP_ACT_ALLOW,
seccomp_syscall_resolve_name(syscalls[i]), 0);
}
seccomp_load(ctx);
seccomp_release(ctx);
}
int main(int argc, char *argv[])
{
int num_syscalls = sizeof(syscalls) / sizeof(syscalls[0]);
apply_seccomp_filter(syscalls, num_syscalls);
probed();
return 0;
}
EOF
cat > /tmp/trace.bt << EOF
uretprobe:/tmp/x:probed
{
printf("ret=%d\n", retval);
}
EOF
gcc -o /tmp/x /tmp/x.c -lseccomp
/usr/bin/bpftrace /tmp/trace.bt &
sleep 5 # wait for uretprobe attach
/tmp/x
pkill bpftrace
rm /tmp/x /tmp/x.c /tmp/trace.bt
---
kernel/seccomp.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 385d48293a5f..10a55c9b5c18 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1359,6 +1359,11 @@ int __secure_computing(const struct seccomp_data *sd)
this_syscall = sd ? sd->nr :
syscall_get_nr(current, current_pt_regs());
+#ifdef CONFIG_X86_64
+ if (unlikely(this_syscall == __NR_uretprobe) && !in_ia32_syscall())
+ return 0;
+#endif
+
switch (mode) {
case SECCOMP_MODE_STRICT:
__secure_computing_strict(this_syscall); /* may call do_exit */
--
2.43.0
^ permalink raw reply related
* Re: Crash when attaching uretprobes to processes running in Docker
From: Oleg Nesterov @ 2025-01-17 0:48 UTC (permalink / raw)
To: Eyal Birger
Cc: Alexei Starovoitov, Jiri Olsa, Aleksa Sarai, Masami Hiramatsu,
linux-kernel, linux-trace-kernel, BPF-dev-list, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov, X86 ML, Linux API, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <CAHsH6GshxQxw2euH_v+cvoCk=17GLQXgogUUWzLmo2P=gO9oLg@mail.gmail.com>
On 01/16, Eyal Birger wrote:
>
> Will do. If it's ok I'll put you in a "Suggested-by" tag (or
> co-developed - as you see fit).
Heh ;) thanks Eyal but I don't care at all. This doesn't matter.
What does matter is that you have found/investigated the problem.
So if you agree with this change, just send the patch and I'll
send my acked-by in reply.
Thanks ;)
Oleg.
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Charlie Jenkins @ 2025-01-16 21:47 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Oleg Nesterov, Eugene Syromyatnikov, Mike Frysinger, Renzo Davoli,
Davide Berardi, Celeste Liu, strace-devel, linux-kernel,
linux-api
In-Reply-To: <Z4l1L4OLoN7-lTkD@ghost>
On Thu, Jan 16, 2025 at 01:07:59PM -0800, Charlie Jenkins wrote:
> On Thu, Jan 16, 2025 at 10:33:28AM +0200, Dmitry V. Levin wrote:
> > On Wed, Jan 15, 2025 at 05:55:31PM -0800, Charlie Jenkins wrote:
> > > On Mon, Jan 13, 2025 at 07:12:08PM +0200, Dmitry V. Levin wrote:
> > [...]
> > > > + /* Changing the type of the system call stop is not supported. */
> > > > + if (ptrace_get_syscall_info_op(child) != info.op)
> > >
> > > Since this isn't supported anyway, would it make sense to set the
> > > info.op to ptrace_get_syscall_info_op(child) like is done for
> > > get_syscall_info? The usecase I see for this is simplifying when the
> > > user doesn't call PTRACE_GET_SYSCALL_INFO before calling
> > > PTRACE_SET_SYSCALL_INFO.
> >
> > struct ptrace_syscall_info.op is a field that specifies how to interpret
> > the union fields of the structure, so if "op" is ignored, then the
> > kernel would infer the meaning of the structure specified by the userspace
> > tracer from the kernel state of the tracee. This looks a bit too
> > error-prone to allow. For example, nothing good is expected to happen
> > if syscall entry information is applied in a syscall exit stop.
>
> Yes that's a good point.
>
> >
> > The tracer is not obliged to call PTRACE_GET_SYSCALL_INFO to set
> > struct ptrace_syscall_info.op. If the tracer keeps track of ptrace stops
> > by other means, it can assign the right value by itself.
> >
> > And, btw, the comment should say "is not currently supported",
> > I'll update it in the next iteration.
> >
> > An idea mentioned in prior discussions was that it would make sense to
> > specify syscall return value along with skipping the syscall in seccomp stop,
> > and this would require a different value for "op" field, but
> > I decided not to introduce this extra complexity yet.
>
> Makes sense, thank you!
>
> - Charlie
I am no longer convinced that we need Celeste's patch that solves this
problem on riscv [1]. That patch is necessary without this change, but
PTRACE_SET_SYSCALL_INFO seems like a cleaner solution.
Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
Tested-by: Charlie Jenkins <charlie@rivosinc.com>
- Charlie
[1] https://lore.kernel.org/lkml/20250115-13cc73c36c7bb3b9f046f614@orel/T/
>
> >
> >
> > --
> > ldv
^ 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