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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jan 30, 2023 at 07:01:15PM +0100, Florian Westphal wrote:
> Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
> > > Is BPF_LINK the right place?  Hook gets removed automatically if the calling program
> > > exits, afaict this is intended.

Yes. bpf_link is the right model.
I'd also allow more than one BPF_NETFILTER prog at the hook.
When Daniel respins his tc bpf_link set there will be a way to do that
for tc and hopefully soon for xdp.
For netfilter hook we can use the same approach.

> > 
> > 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.

Let's start with bpf_link and figure out netlink path when appropriate.

> > > 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.

I'd steer clear from new abi-s.
Don't look at uapi __sk_buff model. It's not a great example to follow.
Just pass kernel nf_hook_state into bpf prog and let program deal
with changes to it via CORE.
The prog will get a defition of 'struct nf_hook_state' from vmlinux.h
or via private 'struct nf_hook_state___flavor' with few fields defined
that prog wants to use. CORE will deal with offset adjustments.
That's a lot less kernel code. No need for asm style ctx rewrites.
Just see how much kernel code we already burned on *convert_ctx_access().
We cannot remove this tech debt due to uapi.
When you pass struct nf_hook_state directly none of it is needed.



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux