net: add pfil_run_hooks_{in,out}

[mjg]

Summary

Reduces the branchfest by taking advantage of knowing the direction of traffic at compilation time and that PFIL_MEMPTR is not passed.

[melifaro]

sys/net/pfil.h

205

Is this still needed?

[mjg]

sys/net/pfil.h

205

this is used a lot to avoid calling the routine. perhaps you are asking if it can be patched to check for the list being empty?

ultimately that's beyond the scope of this patch

205

fwiw i don't think firewalls should be using this interface to begin with, but that's for another time

[melifaro]

sys/net/pfil.h

205

Sorry, let me rephrase. Originally this structure existed as pfil_head was private to pfil.c . Given this patch exposes it to everyone, _pfil_head becomes unnecessary

[mjg]

• remove _pfil_hooks

[glebius]

I totally support the idea in general, but have some requests on the implementation. Note that there no pfil(9) consumer in ports, so our hands are untied. Last time I changed the KPI, there were not a single issue. Here is the plan:

• Keep pfil_head structure private as is.
• Remove pfil_run_hooks, add pfil_run_hooks_in() and pfil_run_hooks_out().
• For now we can require that PFIL_MEMPTR can be set only for pfil_run_hooks_in().

[mjg]

In D36454#828134, @glebius wrote:

I totally support the idea in general, but have some requests on the implementation. Note that there no pfil(9) consumer in ports, so our hands are untied. Last time I changed the KPI, there were not a single issue. Here is the plan:

• Keep pfil_head structure private as is.

This makes it impossible to make the callers just select the right list at compilation time. At best pfil_run_hooks_in/out can select it on its own and call an __always_inline func. I don't think this buys much.

• Remove pfil_run_hooks, add pfil_run_hooks_in() and pfil_run_hooks_out().
• For now we can require that PFIL_MEMPTR can be set only for pfil_run_hooks_in().

Most of the point was to not deal with existence of the PFIL_MEMPTR flag to begin with -- it is the primary source of the branchfest and is not used by most common consumers.

[glebius]

The PFIL_MEMPTR is actually the high-performance flag. If we are down to optimizing branches, we shouldn't ignore a feature that does much more heavy optimization. Let's separate then in and out lists (heads in pfil slang). This will require ip_init() and ip6_init() call pfil_head_register() twice, but swapping the flag. For the device level filtering points (those that use PFIL_MEMPTR) it is going to be zero change, as they do only in filtering already.

I can come up with the patch for you, if you agree.

[mjg]

All the places I patched to use the new routines are used a lot and don't pass the flag in question, yet with the current code they pay for its existence -- once by checking for it to begin with, and later by checking for realloc.

So I don't understand why you insist on making them keep checking for it. Note that whatever consumers of the flag are out there, remain unaffected.

[glebius]

Ok, can we then please achieve your goal without exporting external structure and using cpp?

In pfil.h just:

int     pfil_run_hooks_in(pfil_chain_t *, pfil_packet_t, struct ifnet *, int,  struct inpcb *inp);
int     pfil_run_hooks_out(pfil_chain_t *, pfil_packet_t, struct ifnet *, int,  struct inpcb *inp);

In pfil.c:

static int
pfil_run_hooks_simple(pfil_chain_t *pch, pfil_packet_t p, struct ifnet *ifp,
    int flags, struct inpcb *inp)
{
...
}
int
pfil_run_hooks_in(head, packet, ifp, inp)
{
        return (pfil_run_hooks_simple(&(head)->head_in, packet, ifp, PFIL_IN, inp));
}
int
pfil_run_hooks_out(head, packet, ifp, inp)
{
        return (pfil_run_hooks_simple(&(head)->head_out, packet, ifp, PFIL_OUT, inp));
}

You can use static inline int for the pfil_run_hooks_simple declaration if you really want.

Please also assert !PFIL_MEMPTR in the prologue of pfil_run_hooks_simple.

[mjg]

• add back consumer conversion

[glebius]

Thanks! Since you already started to decompose the single entry point of pfil_run_hooks(), I think, I'm going to provide a specific call from a driver level, too. I will post review for you, so that you confirm that it is an improvement, that removes branching.

[mjg]

you mean a dedicated set for memptr users which also avoids branching on in/out? i'm on it :)

[glebius]

While you haven't yet committed, maybe discuss short and meaningful function names for the new KPI? That will also be in one namespace. You need two functions that take mbuf and have direction encoded in the function name. I need one function that takes memory pointer and is always with in direction. We might want complement function that takes memory pointer with out semantic in future, but we definitely don't need it now. The existing pfil_run_hooks() seems to be no longer used.

So what about, your functions:

pfil_mbuf_in()
pfil_mbuf_out()

My function:

pfil_mem_in()

I'm open to other suggestions.

[glebius]

Do you agree that historic pfil terminology of hooks for filters and heads for filtering points is hard to understand and we should shift away from it?

[mjg]

I don't mind the names.

I would argue the entire mechanism should be scrapped at least for the cases i'm patching in this diff -- if firewall stacking was removed, this could just call the right routine (if any) and better yet, if the firewall is compiled in, it can just call it in place without even an indirect func call.

[kp]

In D36454#828831, @mjg wrote:

I would argue the entire mechanism should be scrapped at least for the cases i'm patching in this diff -- if firewall stacking was removed, this could just call the right routine (if any) and better yet, if the firewall is compiled in, it can just call it in place without even an indirect func call.

Fwiw, while there are occasional users who stack pf and ipfw I've always considered (and loudly informed them) this to be an unsupported use case. There are far too many subtle issues that occur when two firewalls are active.
Moreover, the main reasons I've heard from users to do so (using pf for filtering and dummynet for scheduling) no longer apply as pf can now also use dummynet directly.

pfSense used to run in this configuration but no longer does. I don't know if opnsense does, if we're considering removing the ability to stack firewalls we should check with them first. (Although they're on stable/13, and any removal of this feature would naturally be 14+ only).

[glebius]

There is a very valid use case, and I'm talking about my work, where we compile firewall in statically and then unhook it from ip_input/ip_output and enable only on the driver level. Given that interface level filtering is the way packet filtering is done on most router equipment (Cisco, Juniper, etc.), I would say even argue that the default hooks are a legacy, rather than a standard.

[mjg]

• rename

[mjg]

In D36454#828858, @glebius wrote:

There is a very valid use case, and I'm talking about my work, where we compile firewall in statically and then unhook it from ip_input/ip_output and enable only on the driver level. Given that interface level filtering is the way packet filtering is done on most router equipment (Cisco, Juniper, etc.), I would say even argue that the default hooks are a legacy, rather than a standard.

This only strengthens what I wrote. For your use case, you could have an ifdef or something to not even provide the call site for ip_input/ip_output.

[glebius]

I don't have strong opinion on stacking. Original idea of pfils was that filter could be not only firewalls, but e.g. traffic counters. None exists, however, AFAIK.