Message-ID: <eeeaac99-9053-90c2-aa33-cc1ecb1ae9ca@isovalent.com>
Date:   Fri, 14 Apr 2023 14:20:11 +0100
From:   Quentin Monnet <quentin@...valent.com>
To:     Florian Westphal <fw@...len.de>
Cc:     netdev@...r.kernel.org, netfilter-devel@...r.kernel.org,
        bpf@...r.kernel.org, dxu@...uu.xyz, qde@...cy.de
Subject: Re: [PATCH bpf-next v2 5/6] tools: bpftool: print netfilter link info

2023-04-14 12:41 UTC+0200 ~ Florian Westphal <fw@...len.de>
> Quentin Monnet <quentin@...valent.com> wrote:
>> On Thu, 13 Apr 2023 at 14:36, Florian Westphal <fw@...len.de> wrote:
>>>
>>> Dump protocol family, hook and priority value:
>>> $ bpftool link
>>> 2: type 10  prog 20
>>
>> Could you please update link_type_name in libbpf (libbpf.c) so that we
>> display "netfilter" here instead of "type 10"?
> 
> Done.

Thanks!

I'm just thinking we could also maybe print something nicer for the pf
and the hook, "NF_INET_LOCAL_IN" would be more user-friendly than "hook 1"?

> 
>>> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
>>> index 3823100b7934..c93febc4c75f 100644
>>> --- a/tools/include/uapi/linux/bpf.h
>>> +++ b/tools/include/uapi/linux/bpf.h
>>> @@ -986,6 +986,7 @@ enum bpf_prog_type {
>>>         BPF_PROG_TYPE_LSM,
>>>         BPF_PROG_TYPE_SK_LOOKUP,
>>>         BPF_PROG_TYPE_SYSCALL, /* a program that can execute syscalls */
>>> +       BPF_PROG_TYPE_NETFILTER,
>>
>> If netfilter programs could be loaded with bpftool, we'd need to
>> update bpftool's docs. But I don't think this is the case, right?
> 
> bpftool prog load nftest.o /sys/fs/bpf/nftest
> 
> will work, but the program isn't attached anywhere.

Let's maybe not document it, then. It may still be useful to check
whether a program load, but users would definitely expect the program to
remain loaded after bpftool invocation has completed. Or alternatively,
we could document, but print a warning.

> 
>> don't currently have a way to pass the pf, hooknum, priority and flags
>> necessary to load the program with "bpftool prog load" so it would
>> fail?
> 
> I don't know how to make it work to actually attach it, because
> the hook is unregistered when the link fd is closed.
> 
> So either bpftool would have to fork and auto-daemon (maybe
> unexpected...) or wait/block until CTRL-C.
> 
> This also needs new libbpf api AFAICS because existing bpf_link
> are specific to the program type, so I'd have to add something like:
> 
> struct bpf_link *
> bpf_program__attach_netfilter(const struct bpf_program *prog,
> 			      const struct bpf_netfilter_opts *opts)
> 
> Advice welcome.

OK, yes we'd need something like this if we wanted to load and attach
from bpftool. If you already have the tooling elsewhere, it's maybe not
necessary to add it here. Depends if you want users to be able to attach
netfilter programs with bpftool or even libbpf.

There are other program types that are not supported for
loading/attaching with bpftool (the bpftool-prog man page is not always
correct in that regard, I think).

I'd say let's keep this out of the current patchset anyway. If we have a
use case for attaching via libbpf/bpftool we can do this as a follow-up.

> 
>> Have you considered listing netfilter programs in the output of
>> "bpftool net" as well? Given that they're related to networking, it
>> would maybe make sense to have them listed alongside XDP, TC, and flow
>> dissector programs?
> 
> I could print the same output that 'bpf link' already shows.
> 
> Not sure on the real distinction between those two here.

There would probably be some overlap (to say the least), yes.

> 
> When should I use 'bpftool link' and when 'bpftool net', and what info
> and features should either of these provide for netfilter programs?

That's a good question. I thought I'd check how we handle it for XDP for
"bpftool net" vs. "bpftool link", but I realised this link type (and
some others) were never added to the switch/case you update in
bpftool/link.c, and we're not printing any particular information about
them beyond type and associated program id. Conversely, I'd have to
check whether we print XDP programs using links in "bpftool net". Maybe
some things to improve here. Anyway.

The way I see it, "bpftool net" should provide a more structured
overview of the different programs affecting networking, in particular
for JSON. The idea would be to display all BPF programs that can affect
packet processing. See what we have for XDP for example:


    # bpftool net -p
    [{
            "xdp": [{
                    "devname": "eni88np1",
                    "ifindex": 12,
                    "multi_attachments": [{
                            "mode": "driver",
                            "id": 1238
                        },{
                            "mode": "offload",
                            "id": 1239
                        }
                    ]
                }
            ],
            "tc": [{
                    "devname": "eni88np1",
                    "ifindex": 12,
                    "kind": "clsact/ingress",
                    "name": "sample_ret0.o:[.text]",
                    "id": 1241
                },{
                    "devname": "eni88np1",
                    "ifindex": 12,
                    "kind": "clsact/ingress",
                    "name": "sample_ret0.o:[.text]",
                    "id": 1240
                }
            ],
            "flow_dissector": [
                "id": 1434
            ]
        }
    ]

This gives us all the info about XDP programs at once, grouped by device
when relevant. By contrast, listing them in "bpftool link" would likely
only show one at a time, in an uncorrelated manner. Similarly, we could
have netfilter sorted by pf then hook in "bpftool net". If there's more
relevant info that we get from program info and not from the netfilter
link, this would also be a good place to have it (but not sure there's
any info we're missing from "bpftool link"?).

But given that the info will be close, or identical, if not for the JSON
structure, I don't mean to impose this to you - it's also OK to just
skip "bpftool net" for now if you prefer.

Quentin

Powered by blists - more mailing lists