Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220711083220.2175036-1-asavkov@redhat.com> <20220711083220.2175036-4-asavkov@redhat.com>
 <CAPhsuW7xTRpLf1kyj5ejH0fV_aHCMQjUwn-uhWeNytXedh4+TQ@mail.gmail.com>
In-Reply-To: <CAPhsuW7xTRpLf1kyj5ejH0fV_aHCMQjUwn-uhWeNytXedh4+TQ@mail.gmail.com>
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date:   Tue, 12 Jul 2022 11:08:54 -0700
Message-ID: <CAADnVQ+ju04JAqyEbA_7oVj9uBAuL-fUP1FBr_OTygGf915RfQ@mail.gmail.com>
Subject: Re: [RFC PATCH bpf-next 3/4] bpf: add bpf_panic() helper
To:     Song Liu <song@kernel.org>
Cc:     Artem Savkov <asavkov@redhat.com>,
        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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.

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.
Why this destructive action cannot be delegated to user space?

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.