From: Jiri Olsa <olsajiri@gmail.com>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Jiri Olsa <olsajiri@gmail.com>,
Masami Hiramatsu <mhiramat@kernel.org>,
Andrii Nakryiko <andrii.nakryiko@gmail.com>,
Steven Rostedt <rostedt@goodmis.org>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org,
bpf@vger.kernel.org, Song Liu <songliubraving@fb.com>,
Yonghong Song <yhs@fb.com>,
John Fastabend <john.fastabend@gmail.com>,
Peter Zijlstra <peterz@infradead.org>,
Thomas Gleixner <tglx@linutronix.de>,
"Borislav Petkov (AMD)" <bp@alien8.de>,
x86@kernel.org, linux-api@vger.kernel.org
Subject: Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe
Date: Mon, 8 Apr 2024 18:02:13 +0200 [thread overview]
Message-ID: <ZhQVBYQYr5ph33Uu@krava> (raw)
In-Reply-To: <20240405110230.GA22839@redhat.com>
On Fri, Apr 05, 2024 at 01:02:30PM +0200, Oleg Nesterov wrote:
> On 04/05, Jiri Olsa wrote:
> >
> > On Fri, Apr 05, 2024 at 10:22:03AM +0900, Masami Hiramatsu wrote:
> > >
> > > I think this expects setjmp/longjmp as below
> > >
> > > foo() { <- retprobe1
> > > setjmp()
> > > bar() { <- retprobe2
> > > longjmp()
> > > }
> > > } <- return to trampoline
> > >
> > > In this case, we need to skip retprobe2's instance.
>
> Yes,
>
> > > My concern is, if we can not find appropriate return instance, what happen?
> > > e.g.
> > >
> > > foo() { <-- retprobe1
> > > bar() { # sp is decremented
> > > sys_uretprobe() <-- ??
> > > }
> > > }
> > >
> > > It seems sys_uretprobe() will handle retprobe1 at that point instead of
> > > SIGILL.
> >
> > yes, and I think it's fine, you get the consumer called in wrong place,
> > but it's your fault and kernel won't crash
>
> Agreed.
>
> With or without this patch userpace can also do
>
> foo() { <-- retprobe1
> bar() {
> jump to xol_area
> }
> }
>
> handle_trampoline() will handle retprobe1.
>
> > this can be fixed by checking the syscall is called from the trampoline
> > and prevent handle_trampoline call if it's not
>
> Yes, but I still do not think this makes a lot of sense. But I won't argue.
>
> And what should sys_uretprobe() do if it is not called from the trampoline?
> I'd prefer force_sig(SIGILL) to punish the abuser ;) OK, OK, EINVAL.
so the similar behaviour with int3 ends up with immediate SIGTRAP
and not invoking pending uretprobe consumers, like:
- setup uretprobe for foo
- foo() {
executes int 3 -> sends SIGTRAP
}
because the int3 handler checks if it got executed from the uretprobe's
trampoline.. if not it treats that int3 as regular trap
while for uretprobe syscall we have at the moment following behaviour:
- setup uretprobe for foo
- foo() {
uretprobe_syscall -> executes foo's uretprobe consumers
}
- at some point we get to the 'ret' instruction that jump into uretprobe
trampoline and the uretprobe_syscall won't find pending uretprobe and
will send SIGILL
so I think we should mimic int3 behaviour and:
- setup uretprobe for foo
- foo() {
uretprobe_syscall -> check if we got executed from uretprobe's
trampoline and send SIGILL if that's not the case
I think it's better to have the offending process killed right away,
rather than having more undefined behaviour, waiting for final 'ret'
instruction that jumps to uretprobe trampoline and causes SIGILL
>
> I agree very much with Andrii,
>
> sigreturn() exists only to allow the implementation of signal handlers. It should never be
> called directly. Details of the arguments (if any) passed to sigreturn() vary depending on
> the architecture.
>
> this is how sys_uretprobe() should be treated/documented.
yes, will include man page patch in new version
jirka
>
> sigreturn() can be "improved" too. Say, it could validate sigcontext->ip
> and return -EINVAL if this addr is not valid. But why?
>
> Oleg.
>
next prev parent reply other threads:[~2024-04-08 16:02 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20240402093302.2416467-1-jolsa@kernel.org>
[not found] ` <20240402093302.2416467-2-jolsa@kernel.org>
[not found] ` <20240403100708.233575a8ac2a5bac2192d180@kernel.org>
[not found] ` <Zg0lvUIB4WdRUGw_@krava>
[not found] ` <20240403230937.c3bd47ee47c102cd89713ee8@kernel.org>
[not found] ` <CAEf4BzZ2RFfz8PNgJ4ENZ0us4uX=DWhYFimXdtWms-VvGXOjgQ@mail.gmail.com>
[not found] ` <20240404095829.ec5db177f29cd29e849169fa@kernel.org>
[not found] ` <CAEf4BzYH60TwvBipHWB_kUqZZ6D-iUVnnFsBv06imRikK3o-bg@mail.gmail.com>
2024-04-04 15:54 ` [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe Masami Hiramatsu
2024-04-04 16:11 ` Oleg Nesterov
2024-04-05 1:22 ` Masami Hiramatsu
2024-04-05 8:56 ` Jiri Olsa
2024-04-05 11:02 ` Oleg Nesterov
2024-04-06 3:05 ` Masami Hiramatsu
2024-04-06 17:55 ` Oleg Nesterov
2024-04-08 3:54 ` Masami Hiramatsu
2024-04-08 16:02 ` Jiri Olsa [this message]
2024-04-08 16:22 ` Oleg Nesterov
2024-04-09 12:06 ` Jiri Olsa
2024-04-09 0:34 ` Masami Hiramatsu
2024-04-09 7:57 ` Jiri Olsa
2024-04-08 3:16 ` Masami Hiramatsu
[not found] ` <Zg6V8y2-OP_9at2l@krava>
2024-04-04 16:06 ` Masami Hiramatsu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZhQVBYQYr5ph33Uu@krava \
--to=olsajiri@gmail.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bp@alien8.de \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=john.fastabend@gmail.com \
--cc=linux-api@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-trace-kernel@vger.kernel.org \
--cc=mhiramat@kernel.org \
--cc=oleg@redhat.com \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=songliubraving@fb.com \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).