Re: [RFC] bpf: add bpf_link support for BPF_NETFILTER programs

[Florian Westphal <fw@strlen.de>]

Toke Høiland-Jørgensen <toke@kernel.org> wrote:
> > Is BPF_LINK the right place?  Hook gets removed automatically if the calling program
> > exits, afaict this is intended.
> 
> Yes, this is indeed intended for bpf_link. This plays well with
> applications that use the API and stick around (because things get
> cleaned up after them automatically even if they crash, say), but it
> doesn't work so well for programs that don't (which, notably, includes
> command line utilities like 'nft').

Right, but I did not want to create a dependency on nfnetlink or
nftables netlink right from the start.

> For XDP and TC users can choose between bpf_link and netlink for
> attachment and get one of the two semantics (goes away on close or stays
> put). Not sure if it would make sense to do the same for nftables?

For nftables I suspect that, if nft can emit bpf, it would make sense to
pass the bpf descriptor together with nftables netlink, i.e. along with
the normal netlink data.

nftables kernel side would then know to use the bpf prog for the
datapath instead of the interpreter and could even fallback to
interpreter.

But for the raw hook use case that Alexei and Daniel preferred (cf.
initial proposal to call bpf progs from nf_tables interpreter) I think
that there should be no nftables dependency.

I could add a new nfnetlink subtype for nf-bpf if bpf_link is not
appropriate as an alternative.

> > Should a program running in init_netns be allowed to attach hooks in other netns too?
> >
> > I could do what BPF_LINK_TYPE_NETNS is doing and fetch net via
> > get_net_ns_by_fd(attr->link_create.target_fd);
> 
> We don't allow that for any other type of BPF program; the expectation
> is that the entity doing the attachment will move to the right ns first.
> Is there any particular use case for doing something different for
> nftables?

Nope, not at all.  I was just curious.  So no special handling for
init_netns needed, good.

> > For the actual BPF_NETFILTER program type I plan to follow what the bpf
> > flow dissector is doing, i.e. pretend prototype is
> >
> > func(struct __sk_buff *skb)
> >
> > but pass a custom program specific context struct on kernel side.
> > Verifier will rewrite accesses as needed.
> 
> This sounds reasonable, and also promotes code reuse between program
> types (say, you can write some BPF code to parse a packet and reuse it
> between the flow dissector, TC and netfilter).

Ok, thanks.

> > Things like nf_hook_state->in (net_device) could then be exposed via
> > kfuncs.
> 
> Right, so like:
> 
> state = bpf_nf_get_hook_state(ctx); ?
> 
> Sounds OK to me.

Yes, something like that.  Downside is that the nf_hook_state struct
is no longer bound by any abi contract, but as I understood it thats
fine.

> > nf_hook_run_bpf() (c-function that creates the program context and
> > calls the real bpf prog) would be "updated" to use the bpf dispatcher to
> > avoid the indirect call overhead.
> 
> What 'bpf dispatcher' are you referring to here? We have way too many
> things with that name :P

I meant 'DEFINE_BPF_DISPATCHER(nf_user_progs);'

Thanks.