Re: [RFC PATCH bpf-next 3/4] bpf: add bpf_panic() helper

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Artem Savkov <asavkov@redhat.com>
Cc: Song Liu <song@kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>, bpf <bpf@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>,
	Andrea Arcangeli <aarcange@redhat.com>,
	dvacek@redhat.com
Subject: Re: [RFC PATCH bpf-next 3/4] bpf: add bpf_panic() helper
Date: Wed, 13 Jul 2022 15:20:22 -0700	[thread overview]
Message-ID: <CAADnVQ+6aN5nMwaTjoa9ddnT6rakgwb9oPhtdWSsgyaHP8kZ6Q@mail.gmail.com> (raw)
In-Reply-To: <Ys7JL9Ih3546Eynf@wtfbox.lan>

On Wed, Jul 13, 2022 at 6:31 AM Artem Savkov <asavkov@redhat.com> wrote:
>
> On Tue, Jul 12, 2022 at 11:08:54AM -0700, Alexei Starovoitov wrote:
> > On Tue, Jul 12, 2022 at 10:53 AM Song Liu <song@kernel.org> wrote:
> > >
> > > >
> > > > +BPF_CALL_1(bpf_panic, const char *, msg)
> > > > +{
> > > > +       panic(msg);
> > >
> > > I think we should also check
> > >
> > >    capable(CAP_SYS_BOOT) && destructive_ebpf_enabled()
> > >
> > > here. Or at least, destructive_ebpf_enabled(). Otherwise, we
> > > may trigger panic after the sysctl is disabled.
> > >
> > > In general, I don't think sysctl is a good API, as it is global, and
> > > the user can easily forget to turn it back off. If possible, I would
> > > rather avoid adding new BPF related sysctls.
> >
> > +1. New syscal isn't warranted here.
> > Just CAP_SYS_BOOT would be enough here.
>
> Point taken, I'll remove sysctl knob in any further versions.
>
> > Also full blown panic() seems unnecessary.
> > If the motivation is to get a memory dump then crash_kexec() helper
> > would be more suitable.
> > If the goal is to reboot the system then the wrapper of sys_reboot()
> > is better.
> > Unfortunately the cover letter lacks these details.
>
> The main goal is to get the memory dump, so crash_kexec() should be enough.
> However panic() is a bit more versatile and it's consequences are configurable
> to some extent. Are there any downsides to using it?

versatile? In what sense? That it does a lot more than kexec?
That's a disadvantage.
We should provide bpf with minimal building blocks and let
bpf program decide what to do.
If dmesg (that is part of panic) is useful it should be its
own kfunc.
If halt is necessary -> separate kfunc as well.
reboot -> another kfunc.

Also panic() is not guaranteed to do kexec and just
panic is not what you stated is the goal of the helper.

>
> > Why this destructive action cannot be delegated to user space?
>
> Going through userspace adds delays and makes it impossible to hit "exactly
> the right moment" thus making it unusable in most cases.

What would be an example of that?
kexec is not instant either.

> I'll add this to the cover letter.
>
> > btw, we should avoid adding new uapi helpers in most cases.
> > Ideally all of them should be added as new kfunc-s, because they're
> > unstable and we can rip them out later if our judgement call
> > turns out to be problematic for whatever reason.
>
> Ok, I'll look into doing it this way.
>
> --
> Regards,
>   Artem
>