Page MenuHomeFreeBSD
Differential D8877

pf|ipfw|netinet6?: shared IP forwarding
Needs ReviewPublic
Actions

Authored by franco_opnsense.org on Dec 21 2016, 8:33 AM.
Tags
None
Referenced Files
F116974632: D8877.id23178.diff
Mon, May 12, 5:30 PM
Unknown Object (File)
Sat, May 10, 12:55 AM
Unknown Object (File)
Fri, May 9, 9:05 PM
Unknown Object (File)
Mon, May 5, 12:45 AM
Unknown Object (File)
Fri, May 2, 9:37 PM
Unknown Object (File)
Sun, Apr 27, 8:07 AM
Unknown Object (File)
Fri, Apr 25, 2:53 AM
Unknown Object (File)
Thu, Apr 24, 1:26 PM
View All Files
Subscribers
donner
eri
farrokhi
glebius
imp
mjg
vegeta_tuxpowered.net

Details

Reviewers
melifaro
ae
Group Reviewers
network
transport
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

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 6341
Build 6580: arc lint + arc unit

Event Timeline

franco_opnsense.org updated this revision to Diff 23172.Dec 21 2016, 8:33 AM
franco_opnsense.org retitled this revision from to netinet[6]: KPI for opaque fwd_tag handling for PFIL consumers.
franco_opnsense.org updated this object.
franco_opnsense.org edited the test plan for this revision. (Show Details)
Herald added a reviewer: transport. · View Herald TranscriptDec 21 2016, 8:33 AM
franco_opnsense.org added a reviewer: ae.Dec 21 2016, 8:34 AM
ae added inline comments.Dec 21 2016, 11:35 AM
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.

ae added reviewers: network, melifaro.Dec 21 2016, 11:36 AM
franco_opnsense.org updated this revision to Diff 23174.Dec 21 2016, 12:06 PM
  • requested changes
Herald edited edge metadata. · View Herald TranscriptDec 21 2016, 12:06 PM
ae accepted this revision.Dec 21 2016, 12:22 PM
ae edited edge metadata.

Looks good to me.

This revision is now accepted and ready to land.Dec 21 2016, 12:22 PM
ae requested changes to this revision.Dec 21 2016, 12:45 PM
ae edited edge metadata.

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

This revision now requires changes to proceed.Dec 21 2016, 12:45 PM
ae added a comment.Dec 21 2016, 1:06 PM

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

franco_opnsense.org updated this revision to Diff 23175.Dec 21 2016, 1:47 PM
franco_opnsense.org edited edge metadata.
franco_opnsense.org marked 7 inline comments as done.
  • complete conversion
franco_opnsense.org added inline comments.Dec 21 2016, 1:53 PM
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 updated this revision to Diff 23176.Dec 21 2016, 1:58 PM
franco_opnsense.org edited edge metadata.
  • use else if
franco_opnsense.org marked an inline comment as done.Dec 21 2016, 2:04 PM
franco_opnsense.org added inline comments.
sys/netinet6/ip6_forward.c
530 ↗(On Diff #23176)

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

ae added inline comments.Dec 21 2016, 2:16 PM
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 added inline comments.Dec 21 2016, 2:18 PM
sys/netinet6/ip6_forward.c
530 ↗(On Diff #23176)

and m_freem(m)

franco_opnsense.org updated this revision to Diff 23178.Dec 21 2016, 2:39 PM
  • omit empty reads for ip[6]_get_fwdtag
  • error out safely on failure
franco_opnsense.org marked 8 inline comments as done.Dec 21 2016, 2:41 PM
franco_opnsense.org added inline comments.
sys/netinet6/ip6_forward.c
530 ↗(On Diff #23176)

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

ae accepted this revision.Dec 21 2016, 2:45 PM
ae edited edge metadata.
This revision is now accepted and ready to land.Dec 21 2016, 2:45 PM
eri added a subscriber: eri.Dec 22 2016, 3:52 AM

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 added a comment.Dec 22 2016, 7:21 AM
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 updated this revision to Diff 23266.Dec 26 2016, 10:21 AM
franco_opnsense.org edited edge metadata.
franco_opnsense.org marked an inline comment as done.
  • correctly build against GENERIC
This revision now requires review to proceed.Dec 26 2016, 10:21 AM
mjg added a subscriber: mjg.Dec 9 2021, 9:51 PM

what's the fate of this patch?

This revision is now accepted and ready to land.Dec 9 2021, 9:51 PM
Herald added subscribers: glebius, donner. · View Herald TranscriptDec 9 2021, 9:51 PM
franco_opnsense.org added a comment.Dec 10 2021, 7:36 AM
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 mentioned this in D41517: Draft: pf: Switch pf_route() to PACKET_TAG_IPFORWARD tag.Oct 9 2023, 6:51 AM
franco_opnsense.org updated this revision to Diff 139199.May 29 2024, 6:51 PM

Update the downstream code and adapt for main

This revision now requires review to proceed.May 29 2024, 6:51 PM
Herald added subscribers: vegeta_tuxpowered.net, farrokhi, imp. · View Herald TranscriptMay 29 2024, 6:51 PM
Harbormaster completed remote builds in B57957: Diff 139199.May 29 2024, 6:51 PM
franco_opnsense.org retitled this revision from netinet[6]: KPI for opaque fwd_tag handling for PFIL consumers to pf|ipfw|netinet6?: shared IP forwarding.May 29 2024, 6:55 PM
franco_opnsense.org edited the summary of this revision. (Show Details)
franco_opnsense.org edited the summary of this revision. (Show Details)May 29 2024, 7:01 PM
franco_opnsense.org mentioned this in D48163: ip: Defer checks for an unspecified dstaddr until after pfil hooks.Dec 23 2024, 7:14 AM

Revision Contents
Changeset List

PathSize
sys/
netinet/
88 lines
5 lines
48 lines
18 lines
netinet6/
106 lines
5 lines
18 lines
netpfil/
ipfw/
51 lines

Diff 23174

View Options

sys/netinet/ip_output.c

Loading...
View Options

sys/netinet/ip_var.h

Loading...
View Options

sys/netinet/tcp_input.c

Loading...
View Options

sys/netinet/udp_usrreq.c

Loading...
View Options

sys/netinet6/ip6_output.c

Loading...
View Options

sys/netinet6/ip6_var.h

Loading...
View Options

sys/netinet6/udp6_usrreq.c

Loading...
View Options

sys/netpfil/ipfw/ip_fw_pfil.c

Loading...