pf|ipfw|netinet6?: shared IP forwarding

[franco_opnsense.org]

Summary

This complements the if_output calls in the pf(4) code that escape further
processing by deferring the forwarding execution to the network stack
using on/off style sysctls for both IPv4 and IPv6.

Also see: https://lists.freebsd.org/pipermail/freebsd-current/2016-November/063815.html

[ae]

sys/netinet/ip_output.c

1432

I think return error code is better than magic value. How about (ENOBUFS)?

1437

I think it would be good add KASSERT(dst->sin_family == AF_INET) for consistency.

1448

"Values in return statements should be enclosed in parentheses." (c) style(9) :-)

1458

ENOENT/ESRCH

sys/netinet6/ip6_output.c

550

This check looks incorrect.

559

and this.

3131

We can return EINVAL here due to wrong sin6_scope_id in dst. Or return ENXIO like sa6_embedscope() does.

[franco_opnsense.org]

• requested changes

[ae]

Looks good to me.

[ae]

Forgot to mention, you also need to update ip_fastfwd.c and ip6_fastfwd.c :)

[ae]

Also it there are few another places where M_IP[6]_NEXTHOP flag is used.

[franco_opnsense.org]

• complete conversion

[franco_opnsense.org]

sys/netinet/ip_input.c

608 ↗

(On Diff #23175)

This is an equivalent transformation, but maybe the second call can be omitted, as it doesn't read data and we are kind of enforcing consistency now with the functions itself?

sys/netinet6/ip6_fastfwd.c

241 ↗

(On Diff #23175)

error here, should be else if

245 ↗

(On Diff #23175)

Logic was very intertwined, ended up copying this block to make it easier to follow.

sys/netinet6/ip6_forward.c

528 ↗

(On Diff #23175)

convoluted logic, worked around it...

sys/netinet6/ip6_input.c

785 ↗

(On Diff #23175)

ip6_get_fwdtag() could also be omitted here.

[franco_opnsense.org]

• use else if

[franco_opnsense.org]

sys/netinet6/ip6_forward.c

530 ↗

(On Diff #23176)

what do do here? dst was tained cannot abort like before

[ae]

sys/netinet/ip_input.c

608 ↗

(On Diff #23175)

I think IP_HAS_NEXTHOP() is enough to check.

sys/netinet6/ip6_forward.c

530 ↗

(On Diff #23176)

goto freecopy;

sys/netinet6/ip6_input.c

785 ↗

(On Diff #23175)

yes

[ae]

sys/netinet6/ip6_forward.c

530 ↗

(On Diff #23176)

and m_freem(m)

[franco_opnsense.org]

• omit empty reads for ip[6]_get_fwdtag
• error out safely on failure

[franco_opnsense.org]

sys/netinet6/ip6_forward.c

530 ↗

(On Diff #23176)

better safe than sorry: one for freecopy, one for bad

[eri]

Can't this be made part of a single API where the protocol family is passed so it can be reused and extended easily?
Imagine layer2 forwarding can use this as well.....

[ae]

In D8877#183862, @eri wrote:

Can't this be made part of a single API where the protocol family is passed so it can be reused and extended easily?
Imagine layer2 forwarding can use this as well.....

When it will appear, we can just add link_[get,set,flush]_fwdtag() functions. Internally this can be implemented as call to generic functions with AF argument. But currently such implementation just has the best readability, than one function with bunch of #ifdefs, IMHO. Also currently we don't have NEXTHOP flag for layer2.

[franco_opnsense.org]

• correctly build against GENERIC

[mjg]

what's the fate of this patch?

[franco_opnsense.org]

In D8877#754326, @mjg wrote:

what's the fate of this patch?

There's an updated patch for stable/13 available here https://github.com/opnsense/src/commit/e3c5dc9e31 + https://github.com/opnsense/src/commit/730eb40ce9 with several extensions and fixes since 2016. The concept seems to work for the bulk of OPNsense users -- it's enabled by default there for a number of years now.

I think that @ae wanted to add versioning to the forward tags to avoid kernel module breakage of some sort for out of tree stuff but in my previous day job capacity I wasn't able to spend more time on it.

All this really needs now is a src committer. I can rebase this on main and break it down into multiple pieces if required.

[franco_opnsense.org]

Update the downstream code and adapt for main