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: <20220720114652.3020467-1-asavkov@redhat.com> <20220720114652.3020467-2-asavkov@redhat.com>
 <CAADnVQ+mt1iEsXUGBeL-dgXRoRwPxoz+G=aRcZTkhx2AA10R-A@mail.gmail.com> <YtolJfvSGjSSwbc3@sparkplug.usersys.redhat.com>
In-Reply-To: <YtolJfvSGjSSwbc3@sparkplug.usersys.redhat.com>
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date:   Thu, 21 Jul 2022 21:32:51 -0700
Message-ID: <CAADnVQLyCc7reM1By+TYBaNGh1SBpVqyNyT+WJXOooCqX_w2GA@mail.gmail.com>
Subject: Re: [PATCH bpf-next 1/4] bpf: add BPF_F_DESTRUCTIVE flag for BPF_PROG_LOAD
To:     Artem Savkov <asavkov@redhat.com>
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>, bpf <bpf@vger.kernel.org>,
        Network Development <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Daniel Vacek <dvacek@redhat.com>,
        Jiri Olsa <olsajiri@gmail.com>, Song Liu <song@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jul 21, 2022 at 9:18 PM Artem Savkov <asavkov@redhat.com> wrote:
>
> On Thu, Jul 21, 2022 at 07:02:07AM -0700, Alexei Starovoitov wrote:
> > On Wed, Jul 20, 2022 at 4:47 AM Artem Savkov <asavkov@redhat.com> wrote:
> > >
> > > +/* If BPF_F_DESTRUCTIVE is used in BPF_PROG_LOAD command, the loaded program
> > > + * will be able to perform destructive operations such as calling bpf_panic()
> > > + * helper.
> > > + */
> > > +#define BPF_F_DESTRUCTIVE      (1U << 6)
> >
> > I don't understand what value this flag provides.
> >
> > bpf prog won't be using kexec accidentally.
> > Requiring user space to also pass this flag seems pointless.
>
> bpf program likely won't. But I think it is not uncommon for people to
> run bpftrace scripts they fetched off the internet to run them without
> fully reading the code. So the idea was to provide intermediate tools
> like that with a common way to confirm user's intent without
> implementing their own guards around dangerous calls.
> If that is not a good enough of a reason to add the flag I can drop it.

The intent makes sense, but bpftrace will set the flag silently.
Since bpftrace compiles the prog it knows what helpers are being
called, so it will have to pass that extra flag automatically anyway.
You can argue that bpftrace needs to require a mandatory cmdline flag
from users to run such scripts, but even if you convince the bpftrace
community to do that everybody else might just ignore that request.
Any tool (even libbpf) can scan the insns and provide flags.

Long ago we added the 'kern_version' field to the prog_load command.
The intent was to tie bpf prog with kernel version.
Soon enough people started querying the kernel and put that
version in there ignoring SEC("version") that bpf prog had.
It took years to clean that up.
BPF_F_DESTRUCTIVE flag looks similar to me.
Good intent, but unlikely to achieve the goal.

Do you have other ideas to achieve the goal:
'cannot run destructive prog by accident' ?

If we had an UI it would be a question 'are you sure? please type: yes'.

I hate to propose the following, since it will delay your patch
for a long time, but maybe we should only allow signed bpf programs
to be destructive?