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.

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.

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.

How did usbdump() solve this?

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.

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.

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.

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.

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.

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

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