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

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

 



On 28/04/2023 00:21, Andrii Nakryiko wrote:
> 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.

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?

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




[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