Page MenuHomeFreeBSD

netinet[6]: KPI for opaque fwd_tag handling for PFIL consumers
Needs ReviewPublic

Authored by franco_opnsense.org on Dec 21 2016, 8:33 AM.

Details

Reviewers
melifaro
ae
Group Reviewers
network
transport
Summary

This adds a KPI suggested by ae@ to handle the get, set and flush
of the IP forwarding tag information currently used by UDP, TCP,
and IPFW.

A missing piece in this is PF, which uses low level code to forward
its packets, escaping the PFIL chain. This is because the current
IP forwarding does not support selecting an arbitrary interface to
forward to.

This proposal adds the possiblity to pass an interface index, but
does not currently implement it. As such, the changes are fully
compatible with the current approach used by IPFW.

On top of this, a few minor issues were addressed:

o IPFW is now able to overwrite the forwarding tag for PFIL in direction
o TPC forwarding now flushes its tag like UDP does
o An mbuf tag leak in IPFW was plugged

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

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6383
Build 6616: arc lint + arc unit

Event Timeline

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

  • requested changes
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 edited edge metadata.
franco_opnsense.org marked 7 inline comments as done.
  • complete conversion
sys/netinet/ip_input.c
608–615

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
251

error here, should be else if

255

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

sys/netinet6/ip6_forward.c
528

convoluted logic, worked around it...

sys/netinet6/ip6_input.c
785

ip6_get_fwdtag() could also be omitted here.

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

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–615

I think IP_HAS_NEXTHOP() is enough to check.

sys/netinet6/ip6_forward.c
530

goto freecopy;

sys/netinet6/ip6_input.c
785

yes

ae added inline comments.Dec 21 2016, 2:18 PM
sys/netinet6/ip6_forward.c
530

and m_freem(m)

  • 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

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