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@xxxxxxxxxx> 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?
> 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.
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
