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@
Details
Details
- Reviewers
ae melifaro - Group Reviewers
network - Commits
- rS362900: Complete conversions from fib<4|6>_lookup_nh_<basic|ext> to fib<4|6>_lookup().
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| 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? |
| 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? |
| sys/netinet/if_ether.c | ||
|---|---|---|
| 1092 ↗ | (On Diff #73617) | itaddr is the Target IP in the ARP packet. This call is used to determine the "reverse path" and drop "non-local" requests. |
Comment Actions
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.
|
| 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? |
| 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. |
| 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). |
| 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. |
| 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. |
Comment Actions
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.
| head/sys/netinet/in_mcast.c | ||
|---|---|---|
| 2838 | This is too wrong. If you look inside inp_join_group(), code there:
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. | |
| head/sys/netinet/in_mcast.c | ||
|---|---|---|
| 2838 | would moving the epoch enter/exit calls inside the IN_MULTI_LOCK/UNLOCK in inp_join_group be a reasonable thing to try? | |