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

[nc]

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@

[ae]

sys/netinet/if_ether.c

1092

s/itaddr/isaddr/

sys/netinet/in_mcast.c

1922

nh != NULL

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

[melifaro]

sys/netinet6/icmp6.c

2281

Can you make the assignment separate from the declaration?

sys/netinet6/in6_mcast.c

1845

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

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

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?

[nc]

sys/netinet/if_ether.c

1092

itaddr is used earlier in the code.

sys/netinet6/icmp6.c

2281

Sure.

sys/netpfil/ipfw/ip_fw2.c

472

OK.

sys/netpfil/ipfw/ip_fw_table_algo.c

3818

Makes sense.

[nc]

Here, revised patch. Hope it is okay.

[donner]

sys/netinet/if_ether.c

1092

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.

[melifaro]

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

sys/netinet/in_mcast.c

1920–1923

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

2292

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)

2295

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

1881

Are you sure this is called in epoch?

sys/netinet6/in6_src.c

927

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

sys/netpfil/ipfw/ip_fw2.c

472–474

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

816–817

Same here, no NHR_IFAIF

sys/netpfil/ipfw/ip_fw_table_algo.c

3824

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

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?

[nc]

sys/netpfil/ipfw/ip_fw_table_algo.c

3824

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

[nc]

Here's an updated diff.

[nc]

sys/netpfil/ipfw/ip_fw_table_algo.c

3824

However, I could be wrong.

[nc]

sys/netinet/if_ether.c

1092

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

[melifaro]

sys/netinet/if_ether.c

1092

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

[melifaro]

sys/netinet6/icmp6.c

2295

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

484

nh_aifp, as stated above.

821

same here

sys/netpfil/ipfw/ip_fw_table_algo.c

3824

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

sys/netpfil/ipfw/nat64/nat64_translate.c

775

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

[nc]

Made the changes.

[melifaro]

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.

[kib]

head/sys/netinet/in_mcast.c

2838 ↗

(On Diff #74031)

This is too wrong.

If you look inside inp_join_group(), code there:

1. does copyin(9)
2. locks sx

which cannot work while inside an epoch.

I do not understand multicast code, so cannot fix it. It might be that it is enough to push epoch inside inp_join_group() after all copyins, and leave it before cleanup at the very end of the function.

See https://people.freebsd.org/~pho/stress/log/kostik1311.txt as an example.

[hsw_bitmark.com]

head/sys/netinet/in_mcast.c

2838 ↗

(On Diff #74031)

would moving the epoch enter/exit calls inside the IN_MULTI_LOCK/UNLOCK in inp_join_group be a reasonable thing to try?