netdev - Re: [PATCH bpf-next v5 1/7] bpf: add bpf_link support for BPF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 28 Apr 2023 14:18:35 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Quentin Deslandes <qde@...cy.de>
Cc:     Florian Westphal <fw@...len.de>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, netfilter-devel@...r.kernel.org,
        dxu@...uu.xyz
Subject: Re: [PATCH bpf-next v5 1/7] bpf: add bpf_link support for
 BPF_NETFILTER programs

On Fri, Apr 28, 2023 at 9:54 AM Quentin Deslandes <qde@...cy.de> wrote:
>
> On 28/04/2023 00:21, Andrii Nakryiko wrote:
> > On Thu, Apr 27, 2023 at 2:10 AM Florian Westphal <fw@...len.de> wrote:
> >>
> >> Andrii Nakryiko <andrii.nakryiko@...il.com> 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.
>
> Doesn't this ties the program to a specific hook then? Let's say you
> have a program counting the number of packets from a specific IP, and
> would you be able to attach it to both LOCAL_IN and FORWARD without
> modifying it?

By default, yes (but you can work around that). From your and
Florian's replies it follows that these are not like
expected_attach_type, if I understand correctly. So I'm fine with
having them as attach argument, not part of program type and attach
type.

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