pflow: import from OpenBSD

[kp]

Summary

pflow is a pseudo device to export flow accounting data over UDP.
It's compatible with netflow version 5 and IPFIX (10).

The data is extracted from the pf state table. States are exported once
they are removed.

Obtained from: OpenBSD
Sponsored by: Rubicon Communications, LLC ("Netgate")

[glebius]

I really dislike OpenBSD style of creating an network interface for everything that isn't an interface. Interfaces are either real NICs, or aggregates, or tunnels. A device node that implements a certain protocol is not an interface. This all started with carp(4) and created a lot of confusion a while ago. pfsync(4) and pflog(4) are also on my hate list, by at least they don't cause that much pain as carp did. My guess is that the original reason for this style was difficulty to code something new into original BSD in_control() without breaking things, so they decided to create interfaces, because interface immediately gives you if_ioctl - a single clean entry point to your configuration, without any side effects in in_control(). But there are other side effects with creating a bogus entry on the interface list.

Can we please rework this part? pflow is definitely a valuable addition to our pf.

[kp]

In D43106#982806, @glebius wrote:

I really dislike OpenBSD style of creating an network interface for everything that isn't an interface. Interfaces are either real NICs, or aggregates, or tunnels. A device node that implements a certain protocol is not an interface. This all started with carp(4) and created a lot of confusion a while ago. pfsync(4) and pflog(4) are also on my hate list, by at least they don't cause that much pain as carp did. My guess is that the original reason for this style was difficulty to code something new into original BSD in_control() without breaking things, so they decided to create interfaces, because interface immediately gives you if_ioctl - a single clean entry point to your configuration, without any side effects in in_control(). But there are other side effects with creating a bogus entry on the interface list.

Can we please rework this part? pflow is definitely a valuable addition to our pf.

"Potentially". I'm not sure what a better interface would look like.
At a minimum we need to cope with 'pflow.ko is not loaded', 'multiple netflow destinations' and 'configuration & status updates'.

I'd also argue (but it's not a 100% "no" based on this) that there are upsides to being close-ish to the openbsd approach, so we can more easily pick up their changes.

[glebius]

In D43106#982808, @kp wrote:

I'd also argue (but it's not a 100% "no" based on this) that there are upsides to being close-ish to the openbsd approach, so we can more easily pick up their changes.

There are two options here: leave it still running over ioctl(2) but without ifnet. Just the entry to pflowioctl() would be different. Other option is use netlink, that would be bigger of course.

[bz]

How did usbdump() solve this?

[glebius]

Just looked around at how often we dereference struct ifnet - very little:

• in the clone creation/destruction, that disappears
• in the empty pflow_output(), that disappears
• for stupid IFF_UP checks in several functions
• Finally, in the pflowioctl() the only function that should remain, but entered differently

Sum: should be easy.

I also see little value in staying as close to OpenBSD as possible. You already added VIMAGE support. You probably want to change '++' to counter(9). Declare functions as static. Maybe add more assertions. You may want to move epoch entry/exit into pflow code instead of pf_ioctl.c code as it is done in the current patch, so that this actions are executed only when we are sending packets on killing states. Other changes definitely will come up.

P.S. You may want to get rid of caddr_t and u_intXX_t, as well.

[glebius]

In D43106#982821, @bz wrote:

How did usbdump() solve this?

It did not solve! USB dumping is another ifnet abuse, but for a different and more clear reason. Without any guessing it is clear that it is abusing ifnet because there is no way to create bpf(4) node in a kernel without creating ifnet. Warner will soon come up with a proper CAM/ATA/etc dumper and hopefully somebody will convert usbdump to it.

[glebius]

P.P.S. For example the only reason for this check

if (ifp->if_afdata[AF_INET6] == NULL)

spread around netinet6 is the existence of IFT_PFSYNC, IFT_PFLOG, IFT_USB.

[melifaro]

Thank you for adding it! It's certainly nice to have an integrated solution for PF. Conceptually I'm all up for it.
For the control path implementation I'll second Glebius concern - it's pretty ugly to build a cloner, a bunch of interfaces to allow to use (non-extendable) interface ioctls..
I'd really suggest doing gentlink family with CREATE|UPDATE/DELETE/GET interface, which will be easily extendable. It may even be less code that it is now.

sys/netpfil/pf/if_pflow.c

97 ↗

(On Diff #131560)

Do all those need to be non-static?

858 ↗

(On Diff #131560)

Q: where is it called?

sys/netpfil/pf/pf_ioctl.c

6012

Q: pf_clear_states has the "assert epoch" semantics. Do we really want to do it differently here?

[kp]

In D43106#984447, @melifaro wrote:

Thank you for adding it! It's certainly nice to have an integrated solution for PF. Conceptually I'm all up for it.
For the control path implementation I'll second Glebius concern - it's pretty ugly to build a cloner, a bunch of interfaces to allow to use (non-extendable) interface ioctls..
I'd really suggest doing gentlink family with CREATE|UPDATE/DELETE/GET interface, which will be easily extendable. It may even be less code that it is now.

That's in progress, but it'll be a week or two before I post an updated version.

sys/netpfil/pf/if_pflow.c

97 ↗

(On Diff #131560)

No, I should mark them all as static.

858 ↗

(On Diff #131560)

It's not called yet in this commit, but it'll get assigned to a function pointer in the same way we do for pflog and pfsync.

It'll get called from pf_detach_state().

sys/netpfil/pf/pf_ioctl.c

6012

Good point. I'll align that.

[kp]

• netlink interface rather than cloned interfaces

This is a first draft. I've not updated the man pages, for example (which I'll
get to when the rest of this patch series is updated).

[kp]

• rebase
• polish
• update man pages

This should be ready for review now.

[glebius]

sys/net/if_types.h

257 ↗

(On Diff #132455)

Looks like this is no longer needed! Thanks a lot for your work. I'll take a more thorough look tomorrow.

[kp]

Remove IFT_PFLOW

[melifaro]

Thank you for updating the interface to netlink!
Generally LGTM, the only concern I have is about using sockaddrs inside netlink..

sbin/pflowctl/pflowctl.c

224

Q: why do we want sockaddrs?

425

Do we expect multiple data replies? Any data replies?

447

bool flag = *buf == ‘[‘;

452

Nit: can cp be loop-local?

475

Why not do c99 init?

hints = { .ai_family = , … };

sys/netpfil/pf/pflow.c

1182

Can’t we just determine family based on the PFLOWNL_ADDR_IP value length?
(And generally - why do we try to pass sockaddrs through netlink? It just seem to add complexity and I’m not sure what’s the benefit

[kp]

sbin/pflowctl/pflowctl.c

224

We want a port number as well as an IPv4 or IPv6 address (which implies the need to know the address family as well).

425

No, we just want to check for errors here.

I can rework that to use snl_read_reply_code() instead.

sys/netpfil/pf/pflow.c

1182

I used sockaddr mostly because that's what the OpenBSD ioctl code used.

We do need both port number and IP(v4|v6) address, but we could potentially simplify a bit by having a single field for the IP address.
We could also make a generic version of serialise/deserialise a struct sockaddr_storage and use that. There have been other cases where that could be useful (e.g. if_ovpn, if we ever convert it to netlink).

[kp]

Review remarks

[kp]

@glebius Did you want some more time to look at this too?

I'd like to start landing this patch series in the next few days.