Live-Patching Archive on lore.kernel.org
 help / color / Atom feed
From: Petr Mladek <pmladek@suse.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Jann Horn <jannh@google.com>,
	bpf@vger.kernel.org, live-patching@vger.kernel.org,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Steven Rostedt <rostedt@goodmis.org>,
	KP Singh <kpsingh@chromium.org>,
	Andy Lutomirski <luto@amacapital.net>,
	kernel list <linux-kernel@vger.kernel.org>,
	the arch/x86 maintainers <x86@kernel.org>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	Ingo Molnar <mingo@redhat.com>
Subject: Re: BPF tracing trampoline synchronization between update/freeing and execution?
Date: Tue, 7 Jan 2020 10:03:19 +0100
Message-ID: <20200107090319.ggggnpkqfqdmldfy@pathway.suse.cz> (raw)
In-Reply-To: <20200107082842.5w6zjgxy56wiftmm@pathway.suse.cz>

On Tue 2020-01-07 09:28:42, Petr Mladek wrote:
> On Mon 2020-01-06 17:56:54, Peter Zijlstra wrote:
> > On Mon, Jan 06, 2020 at 05:39:30PM +0100, Jann Horn wrote:
> > > Hi!
> > > 
> > > I was chatting with kpsingh about BPF trampolines, and I noticed that
> > > it looks like BPF trampolines (as of current bpf-next/master) seem to
> > > be missing synchronization between trampoline code updates and
> > > trampoline execution. Or maybe I'm missing something?
> > > 
> > > If I understand correctly, trampolines are executed directly from the
> > > fentry placeholders at the start of arbitrary kernel functions, so
> > > they can run without any locks held. So for example, if task A starts
> > > executing a trampoline on entry to sys_open(), then gets preempted in
> > > the middle of the trampoline, and then task B quickly calls
> > > BPF_RAW_TRACEPOINT_OPEN twice, and then task A continues execution,
> > > task A will end up executing the middle of newly-written machine code,
> > > which can probably end up crashing the kernel somehow?
> > > 
> > > I think that at least to synchronize trampoline text freeing with
> > > concurrent trampoline execution, it is necessary to do something
> > > similar to what the livepatching code does with klp_check_stack(), and
> > > then either use a callback from the scheduler to periodically re-check
> > > tasks that were in the trampoline or let the trampoline tail-call into
> > > a cleanup helper that is part of normal kernel text. And you'd
> > > probably have to gate BPF trampolines on
> > > CONFIG_HAVE_RELIABLE_STACKTRACE.
> > 
> > ftrace uses synchronize_rcu_tasks() to flip between trampolines iirc.
> 
> ftrace calls also schedule_on_each_cpu(ftrace_sync) to handle
> situations where RCU is not watching, see rcu_is_watching().
> 
> The following is called in ftrace_shutdown():
> 
> 	schedule_on_each_cpu(ftrace_sync);
> 
> 	if (IS_ENABLED(CONFIG_PREEMPTION))
> 		synchronize_rcu_tasks();
> 
> 	arch_ftrace_trampoline_free(ops);

Just to be sure. IMHO, the above should be enough to decide when
a ftrace-like trampoline could be freed. But it is not enough
to decide whether any task is still in the middle of the BPF
program that the trampoline jumped to.

You will likely need something more complicated to decide when it is safe
to free the BPF program itself. The stack check probably won't help.
I guess that the stack will be marked as unsafe in BPF program because
there will be no data for ORC unwinder. But some simple reference
counting might do the job.

Best Regards,
Petr

      reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-06 16:39 Jann Horn
2020-01-06 16:56 ` Peter Zijlstra
2020-01-06 22:29   ` Alexei Starovoitov
2020-01-07  8:28   ` Petr Mladek
2020-01-07  9:03     ` Petr Mladek [this message]

Reply instructions:

You may reply publically 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=20200107090319.ggggnpkqfqdmldfy@pathway.suse.cz \
    --to=pmladek@suse.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=jannh@google.com \
    --cc=jpoimboe@redhat.com \
    --cc=kpsingh@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=x86@kernel.org \
    /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

Live-Patching Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/live-patching/0 live-patching/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 live-patching live-patching/ https://lore.kernel.org/live-patching \
		live-patching@vger.kernel.org
	public-inbox-index live-patching

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.live-patching


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git