On Fri, 15 Nov 2019 at 23:02, Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> 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
