On Thu, Apr 27, 2023 at 2:10 AM Florian Westphal <fw@xxxxxxxxx> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
> > > @@ -1560,6 +1562,12 @@ union bpf_attr {
> > > */
> > > __u64 cookie;
> > > } tracing;
> > > + struct {
> > > + __u32 pf;
> > > + __u32 hooknum;
> >
> > catching up on stuff a bit...
> >
> > enum nf_inet_hooks {
> > NF_INET_PRE_ROUTING,
> > NF_INET_LOCAL_IN,
> > NF_INET_FORWARD,
> > NF_INET_LOCAL_OUT,
> > NF_INET_POST_ROUTING,
> > NF_INET_NUMHOOKS,
> > NF_INET_INGRESS = NF_INET_NUMHOOKS,
> > };
> >
> > So it seems like this "hook number" is more like "hook type", is my
> > understanding correct?
>
> What is 'hook type'?
I meant that it's not some dynamically generated number that could
change from the system to system, it's a fixed set of point in which
this BPF program can be triggered. The distinction I was trying to
make that it's actually different in nature compared to, say, ifindex,
as it is fixed by the kernel.
>
> > If so, wouldn't it be cleaner and more uniform
> > with, say, cgroup network hooks to provide hook type as
> > expected_attach_type? It would also allow to have a nicer interface in
> > libbpf, by specifying that as part of SEC():
> >
> > SEC("netfilter/pre_routing"), SEC("netfilter/local_in"), etc...
>
> I don't understand how that would help.
> Attachment needs a priority and a family (ipv4, arp, etc.).
>
> If we allow netdev type we'll also need an ifindex.
> Daniel Xu work will need to pass extra arguments ("please enable ip
> defrag").
Ok, that's fine, if you think it doesn't make sense to pre-declare
that a given BPF program is supposed to be run only in, say,
PRE_ROUTING, then it's fine. We do declare this for other programs
(e.g., cgroup_skb/egress vs cgroup_skb/ingress), so it felt like this
might be a similar case.
>
> > Also, it seems like you actually didn't wire NETFILTER link support in
> > libbpf completely. See bpf_link_create under tools/lib/bpf/bpf.c, it
> > has to handle this new type of link as well. Existing tests seem a bit
> > bare-bones for SEC("netfilter"), would it be possible to add something
> > that will demonstrate it a bit better and will be actually executed at
> > runtime and validated?
>
> I can have a look.
It probably makes sense to add bpf_program__attach_netfilter() API as
well which will return `struct bpf_link *`. Right now libbpf support
for NETFILTER is very incomplete.
