Page MenuHomeFreeBSD

Complete the fib<4|6>_lookup_nh_<basic|ext> -> fib<4|6>_lookup() transition
ClosedPublic

Authored by neel_neelc.org on Thu, Jun 25, 3:56 AM.

Details

Summary

Complete the fib lookup conversions from fib<4|6>_lookup_nh_<basic|ext> to fib<4|6>_lookup(). This is part of the new routing KPI.

Submitted by: Neel Chauhan <neel AT neelc DOT org>
Suggested by: melifaro@

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

neel_neelc.org created this revision.Thu, Jun 25, 3:56 AM
neel_neelc.org requested review of this revision.Thu, Jun 25, 3:56 AM
ae added inline comments.Thu, Jun 25, 6:41 AM
sys/netinet/if_ether.c
1092 ↗(On Diff #73617)

s/itaddr/isaddr/

sys/netinet/in_mcast.c
1922 ↗(On Diff #73617)

nh != NULL

Also it seem it is called from control code and you need to enter EPOCH.

melifaro added inline comments.Thu, Jun 25, 8:29 AM
sys/netinet6/icmp6.c
2281 ↗(On Diff #73617)

Can you make the assignment separate from the declaration?

sys/netinet6/in6_mcast.c
1845 ↗(On Diff #73617)

I'd put the check into the caller to ensure that passed ifp is still used within epoch.

sys/netpfil/ipfw/ip_fw2.c
472 ↗(On Diff #73617)

why do we have assert here but no assert in verify_path6?
I'd rather remove this one.

sys/netpfil/ipfw/ip_fw_table_algo.c
3818 ↗(On Diff #73617)

IIRC it can be requested from userland table A lookup 1.1.1.1 and I'm not sure whether we enter epoch in this case or not. Have you checked it?

neel_neelc.org marked 6 inline comments as done.Thu, Jun 25, 3:23 PM
neel_neelc.org added inline comments.
sys/netinet/if_ether.c
1092 ↗(On Diff #73617)

itaddr is used earlier in the code.

sys/netinet6/icmp6.c
2281 ↗(On Diff #73617)

Sure.

sys/netpfil/ipfw/ip_fw2.c
472 ↗(On Diff #73617)

OK.

sys/netpfil/ipfw/ip_fw_table_algo.c
3818 ↗(On Diff #73617)

Makes sense.

neel_neelc.org marked 4 inline comments as done.

Here, revised patch. Hope it is okay.

lutz_donnerhacke.de added inline comments.
sys/netinet/if_ether.c
1092 ↗(On Diff #73617)

itaddr is the Target IP in the ARP packet.
isaddr is the Source IP in the ARP packet.

This call is used to determine the "reverse path" and drop "non-local" requests.

Also: maybe worth splitting the review in 2-3 smaller ones to speedup the process?

sys/netinet/in_mcast.c
1920 ↗(On Diff #73638)

As far as I see, none of the ip_ctloutput()->inp_setmoptions()->inp_join_group()->inp_lookup_mcast_ifp()enters epoch, so this assert would fail.

sys/netinet6/icmp6.c
2293 ↗(On Diff #73638)

There is a couple of differences in the KPI.

  1. new KPI has nexthops with addresses embedded.
  2. in case when the nexthop result is "directly-reachable-via ifX" (e.g. no NHF_GATEWAY flag exists) the nexthop address is not equal to the lookup address (not applicable here)
2296 ↗(On Diff #73638)

I guess we want _nexthop_ address here, which should be set first by something like nh_addr = nh->gw6_sa.sin6_addr

sys/netinet6/in6_mcast.c
1880 ↗(On Diff #73638)

Are you sure this is called in epoch?

sys/netinet6/in6_src.c
927 ↗(On Diff #73638)

I guess != should be a bit better here :-)

sys/netpfil/ipfw/ip_fw2.c
472 ↗(On Diff #73638)

no NHR_IFAIF here. The pointer to address interface is returned in the nexthop object in nh_aifp.

815 ↗(On Diff #73638)

Same here, no NHR_IFAIF

sys/netpfil/ipfw/ip_fw_table_algo.c
3823 ↗(On Diff #73638)

Soo, did we check that the control path for table A lookup enter epoch somewhere, so the box doesn't panic?

sys/netpfil/ipfw/nat64/nat64_translate.c
772 ↗(On Diff #73638)

Maybe worth changing function sugnature? The error code is ether 0 or EHOSTUNREACH, so maybe it can return nhop, and the caller can manually set error to EHOSTUNREACH on NULL nhop?

neel_neelc.org marked 10 inline comments as done.Thu, Jun 25, 10:26 PM
neel_neelc.org added inline comments.
sys/netpfil/ipfw/ip_fw_table_algo.c
3823 ↗(On Diff #73638)

I looked at the control path, and to my knowledge it doesn't exist.

neel_neelc.org marked an inline comment as done.

Here's an updated diff.

sys/netpfil/ipfw/ip_fw_table_algo.c
3823 ↗(On Diff #73638)

However, I could be wrong.

sys/netinet/if_ether.c
1092 ↗(On Diff #73617)

Fixed it, sorry. I got confused (blame my ADHD).

melifaro added inline comments.Sat, Jun 27, 6:12 PM
sys/netinet/if_ether.c
1092 ↗(On Diff #73617)

itaddr / isaddr is indeed extremely confusing. I guess at some point in time someone renames it to be somewhat more descriptive.

melifaro added inline comments.Mon, Jun 29, 8:05 AM
sys/netinet6/icmp6.c
2296 ↗(On Diff #73638)

.. and as the nh_addr is the gateway address, it has the scope already embedded, so we don't need this line.

sys/netpfil/ipfw/ip_fw2.c
483 ↗(On Diff #73672)

nh_aifp, as stated above.

820 ↗(On Diff #73672)

same here

sys/netpfil/ipfw/ip_fw_table_algo.c
3823 ↗(On Diff #73638)

Soo, for this to be valid one needs to enter epoch before calling fib4_lookup.

sys/netpfil/ipfw/nat64/nat64_translate.c
777 ↗(On Diff #73672)

style(9) suggests empty line after last variable declaration.

neel_neelc.org marked 5 inline comments as done.

Made the changes.

melifaro accepted this revision.Tue, Jun 30, 10:18 PM

There are still some things outstanding ( epoch entry in multicast code and probably a couple other ones ), but generally looks good.
Thank you for working on this!
I'll commit the change tomorrow.

This revision is now accepted and ready to land.Tue, Jun 30, 10:18 PM