Re: static and dynamic linking. Was: [PATCH bpf-next v3 1/5] bpf: Support chain calling multiple BPF

From: Lorenz Bauer <lmb@cloudflare.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: "Toke Høiland-Jørgensen" <toke@redhat.com>,
	"Edward Cree" <ecree@solarflare.com>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Martin KaFai Lau" <kafai@fb.com>,
	"Song Liu" <songliubraving@fb.com>, "Yonghong Song" <yhs@fb.com>,
	"Marek Majkowski" <marek@cloudflare.com>,
	"Alan Maguire" <alan.maguire@oracle.com>,
	"Jesper Dangaard Brouer" <brouer@redhat.com>,
	"David Miller" <davem@davemloft.net>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>
Subject: Re: static and dynamic linking. Was: [PATCH bpf-next v3 1/5] bpf: Support chain calling multiple BPF
Date: Mon, 18 Nov 2019 13:29:00 +0000	[thread overview]
Message-ID: <CACAyw9-Sx8jBY2HiO0idLk_HE5R5R229w5qEqVkaQ92YUKM0kg@mail.gmail.com> (raw)
In-Reply-To: <20191115230229.qd6plwnvrmcm4pfo@ast-mbp.dhcp.thefacebook.com>

On Fri, 15 Nov 2019 at 23:02, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> imo all bpf attach api-s that are not FD-based are fragile and error prone
> operationally. We've seen people adding a ton of TC ingress progs because of
> bugs. Then there were issues with roolet being removed due to bugs. The issues
> with overriding wrong entries in prog_array. When multiple teams working on
> common infra having globally visible and unprotected state is dangerous. imo
> XDP and TC have to move to FD based api. When application holds the 'link fd'
> that attached program will not be detached by anything else accidentally.
> The operation 'attach rootlet XDP prog to netdev eth0' should return FD.
> While that FD is held by application (or pinned in bpffs) nothing should be
> able to override XDP prog on that eth0. We don't have such api yet, but I think
> it's necessary.

Ok, I wasn't aware you're planning this. Having a separate fd for the
link resolves
my concerns, since now the lifetime of the program and the link are independent.

Re: the rootlet example, the API would be load (with attach_prog_fd) followed by
override / attach (without arguments?) which then returns a "link fd"?

> Same thing with replacing rootlet's placeholder subprogram with
> fw1. When fw1's application links fw1 prog into rootlet nothing should be able
> to break that attachment. But if fw1 application crashes that fw1 prog will be
> auto-detached from rootlet. The admin can ssh into the box and kill fw1. The
> packets will flow into rootlet and will flow into dummy placeholder. No
> cleanups to worry about.

Nice!

> > I'd much prefer if the API didn't require attach_prog_fd and id at
> > load time, and
> > rather have an explicit replace_sub_prog(prog_fd, btf_id, sub_prog_fd).
>
> The verifier has to see the target prog and its full BTF at load time. The
> fentry prog needs target prog's BTF. XDP replacement prog needs target prog's
> BTF too. So prog_fd+btf_id has to be passed at load time. I think
> tgt_prog->refcnt++ should be done at load time too. The ugly alternative would
> be to do tgt_prog->refcnt++ before verification. Then after verification
> duplicate tgt_prog BTF, store it somewhere, and do tgr_prog->refcnt--. Then
> later during attach/replace compare saved BTF with tgt_prog's BTF. That's imo a
> ton of unncessary work for the kernel.

I've not looked at the fentry patch set, so I don't understand the
technical reasons why
having prog_fd at load time is necessary, I'm just not a fan of the
implied UAPI.
I'll take a look, hopefully I'll understand the trade off better afterwards.

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com