netpfil: Introduce PFIL_FWD flag

[kp]

Summary

Forwarded packets passed through PFIL_OUT, which made it
difficult for firewalls to figure out if they were forwarding or
producing packets.
This in turn is an issue for pf for IPv6 fragment handling: it needs to
call ip6_output() or ip6_forward() to handle the fragments. Figuring out
which was difficult (and until now, incorrect).
Having pfil distinguish the two removes an ugly piece of code from pf.

Introduce a flags variable in the netpfil callbacks, which has PFIL_FWD
set for forwarded packets. This allows pf to reliably work out if a packet
is forwarded or not.

[ae]

Can you please update the patch with additional context according to https://wiki.freebsd.org/Phabricator#Create_a_Revision_via_Web_Interface

[eri]

While this is needed i do not agree that the modifications on the stack and packet filters should be so hackish.

[kp]

More context. No changes to the diff.

[kp]

In D13715#288702, @eri wrote:

While this is needed i do not agree that the modifications on the stack and packet filters should be so hackish.

What specifically do you not like? The 'if (dir == PFIL_FWD) dir = PFIL_OUT;' additions to the other pfil users?
The alternative to that would be to try to hide this in pfil, so that pf (and others who want PFIL_FWD) could tell pfil it understands this, and keep sending PFIL_OUT for PFIL_FWD to the others. That would get rid of the couple of if statements in the other pfil users, at the price of extra complexity in pfil.

[kp]

Based around a suggestion from Kyle Evans (who also did all of the work), introduce a flags variable to the pfil callbacks. Keep using PFIL_OUT for forwarded packets, but set the PFIL_FWD flag for them. This allows pf to work out if a packet is being forwarded or not, with essentially no changes to other netpfil consumers.

[kevans]

Sorry, removal of manpages was unintentional- caches, grrrr.

[kevans]

Having had time to review it again, I think it looks good. This iteration exposes it as a flag to describes the path the packet's taken, rather than exposing it as the direction the packet is going in and having places where it was necessary to then mask that fact by flipping dir back to OUT where paths didn't yet know about FWD

As discussed via e-mail, we can figure out later if we want to allow hooks to only be invoked for FWD'd packets and not all OUT- this would be a straightforward addition without many invasive changes.

[eri]

While i have not much time lately to spend on this, i still think this is the wrong way of doing things since it just creates confusion.
pf(4) has already knows about mbuf_tag(9) and uses it. I would strongly suggest using them until a proper _FWD hook comes to life and allows removing all the 'hacks' in pf(4) and possibly elsewhere.

[kp]

In D13715#295449, @eri wrote:

While i have not much time lately to spend on this, i still think this is the wrong way of doing things since it just creates confusion.

I'm not sure I see how this would create confusion. This merely presents more information about the packet, and where the netpfil hook being called from.

pf(4) has already knows about mbuf_tag(9) and uses it. I would strongly suggest using them until a proper _FWD hook comes to life and allows removing all the 'hacks' in pf(4) and possibly elsewhere.

My gut reaction (i.e. without benchmarks) is that there'd be a performance penalty for tagging all forwarded packets.

[kevans]

In D13715#295452, @kristof wrote:

In D13715#295449, @eri wrote:

pf(4) has already knows about mbuf_tag(9) and uses it. I would strongly suggest using them until a proper _FWD hook comes to life and allows removing all the 'hacks' in pf(4) and possibly elsewhere.

My gut reaction (i.e. without benchmarks) is that there'd be a performance penalty for tagging all forwarded packets.

On the other hand, we have this PACKET_TAG_IPFORWARD that already exists [1] and that ipfw already "understands" [2] (in the sense that it assumes that one could exist on an outgoing packet and it will update it accordingly if it does). As far as I can see, ipfw is the only thing that will actually tag an mbuf with it.

I have to wonder if it wouldn't be advantageous and cleaner to use this tag. We could then do something like:

Delete [3], add the tag just prior to [4] if fwd_tag == NULL. Set the tag up just prior to [5] and [6] as well. If I read correctly, all instances of this are vaguely aware of this tag already and will consume it after hooks are ran if some mbuf flags are set (only by ipfw, at the moment)- we could do a minor rewrite in these places to check for the forward tag, honor it if the proper nexthop flag is set, and drop it afterwards if we located the tag.

Cost should be relatively low because we keep it prepended to the mbuf tags, and then we nuke it once we're done with it so it should have minimal impact on things outside of this context in which we're running the hooks. We'd then merely be extending the concept that already exists for allowing pfil hooks to tag an mbuf for forwarding to also indicate to the mbuf that we intend to forward. It should have no impact on existing hooks at that point.

[1] http://src.illumos.org/source/xref/freebsd-head/sys/sys/mbuf.h#1095
[2] http://src.illumos.org/source/xref/freebsd-head/sys/netpfil/ipfw/ip_fw_pfil.c#194

[3] http://src.illumos.org/source/xref/freebsd-head/sys/netinet6/ip6_fastfwd.c#183
[4] http://src.illumos.org/source/xref/freebsd-head/sys/netinet6/ip6_fastfwd.c#203
[5] http://src.illumos.org/source/xref/freebsd-head/sys/netinet/ip_fastfwd.c#308
[6] http://src.illumos.org/source/xref/freebsd-head/sys/netinet6/ip6_forward.c#328

[kevans]

I guess the "cost should be relatively low" comment is kind of wrong. OUT hooks that care about whether it's forwarded or not will take a hit if it's not forwarded and the tag is not present, but I have no idea how heavy this would be- I have no notion of how heavily used mbuf tags are. =)

[kevans]

(Apologies; last comment on this matter)

I think this is what I'm proposing: https://people.freebsd.org/~kevans/netpfil-proposal.diff

Note that I ran out of time to verify that the mbuf tags in pf would still be intact after any scrubbing that gets done, so it may well be that my assumption is wrong and the tag goes away.

[ae]

I'm against the last proposal. It is not costless to tag each forwarded packet and then remove the tag. This will seriously hit the performance.

[eri]

In D13715#295452, @kristof wrote:

In D13715#295449, @eri wrote:

While i have not much time lately to spend on this, i still think this is the wrong way of doing things since it just creates confusion.

I'm not sure I see how this would create confusion. This merely presents more information about the packet, and where the netpfil hook being called from.

pf(4) already has a loop detection mechanism that i created to support divert(9) and dummunet(9), not sure if the later ever made it into FreeBSD.

pf(4) has already knows about mbuf_tag(9) and uses it. I would strongly suggest using them until a proper _FWD hook comes to life and allows removing all the 'hacks' in pf(4) and possibly elsewhere.

My gut reaction (i.e. without benchmarks) is that there'd be a performance penalty for tagging all forwarded packets.

Are you optimising pf(4) for forwarding performance or are you trying to solve an issue?!
These are two different goals that should not be tackled together.

The long story should be, pf(4) should forget how it implements today route-to/reply-to and LOOPED mbuf should have a dedicated flag as M_IP_NETHOP has.
I choose M_IP_NEXTHOP specifically because it is an optimisation around mbuf_tags(9) used in ipfw(9) fwd implementation for not hurting normal forwarding performance but also allowing the feature to co-exist without a kernel recompile.
There are proto flags in mbuf specifically for such purpose just grab one that is unused and move on.

[kp]

In D13715#296702, @eri wrote:

In D13715#295452, @kristof wrote:

In D13715#295449, @eri wrote:

While i have not much time lately to spend on this, i still think this is the wrong way of doing things since it just creates confusion.

I'm not sure I see how this would create confusion. This merely presents more information about the packet, and where the netpfil hook being called from.

pf(4) already has a loop detection mechanism that i created to support divert(9) and dummunet(9), not sure if the later ever made it into FreeBSD.

Now I'm confused. This isn't about loop detection. This is about detecting if a PFIL_OUT packet is being forwarded or output.

pf(4) has already knows about mbuf_tag(9) and uses it. I would strongly suggest using them until a proper _FWD hook comes to life and allows removing all the 'hacks' in pf(4) and possibly elsewhere.

My gut reaction (i.e. without benchmarks) is that there'd be a performance penalty for tagging all forwarded packets.

Are you optimising pf(4) for forwarding performance or are you trying to solve an issue?!
These are two different goals that should not be tackled together.

I'd like to fix the issue where pf can't reliably figure out if it should call ip6_forward() or ip6_output(). I'd like to do so without negatively affecting the general forwarding performance (with or without pf).

The long story should be, pf(4) should forget how it implements today route-to/reply-to and LOOPED mbuf should have a dedicated flag as M_IP_NETHOP has.
I choose M_IP_NEXTHOP specifically because it is an optimisation around mbuf_tags(9) used in ipfw(9) fwd implementation for not hurting normal forwarding performance but also allowing the feature to co-exist without a kernel recompile.
There are proto flags in mbuf specifically for such purpose just grab one that is unused and move on.

This isn't about route-to or reply-to.

[ae]

In D13715#296744, @kristof wrote:

I'd like to fix the issue where pf can't reliably figure out if it should call ip6_forward() or ip6_output(). I'd like to do so without negatively affecting the general forwarding performance (with or without pf).

I think someone from pfSense or openSense already reworked PF to work "inplace" like ipfw does, i.e. it returns mbuf back to the function from where pfil was invoked.

[kp]

In D13715#296796, @ae wrote:

I think someone from pfSense or openSense already reworked PF to work "inplace" like ipfw does, i.e. it returns mbuf back to the function from where pfil was invoked.

I believe you're mistaken. I've just checked their public code and openSense just does what base FreeBSD does, while pfSense is on a much older version and doesn't handle v6 fragmentation at all.

This would be the best solution, but it's nontrivial to teach the rest of the network stack that netpfil could return multiple packets.

[kp]

Keep the old hooks so other pfil users don't need to change. Allow pf to use the new style of hook, with the flags argument.

[ae]

Looks good to me.