Page MenuHomeFreeBSD

Use fib4_lookup_nh_ext() in ip_output().
ClosedPublic

Authored by glebius on Apr 2 2019, 9:26 PM.

Details

Summary

Existense of PCB route cachine doesn't allow us to use new fast route
lookup KPI in ip_output() like it is already used in ip_forward().

However, when there is no PCB provided we can use it. Typical scenario
is a DNS server that would use sendto(2) over a not connected UDP socket.

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

glebius created this revision.Apr 2 2019, 9:26 PM
melifaro added inline comments.
sys/netinet/ip_output.c
306 ↗(On Diff #55755)

Nitpick: if we're revisiting this fancy condition, would it be possible to consider replace its occurrence with something like

if (RTE_IS_INVALID(ro->ro_rt) || dst->sin_family != AF_INET ...)

?

glebius added inline comments.Apr 3 2019, 8:20 PM
sys/netinet/ip_output.c
306 ↗(On Diff #55755)

Do we have RTE_IS_INVALID? But we always first need to check that ro->ro_rt isn't NULL anyway.

glebius updated this revision to Diff 55787.Apr 3 2019, 8:21 PM

More comments & fix a bug with broadcast.

glebius updated this revision to Diff 55789.Apr 3 2019, 8:51 PM
  • Copy RTF_HOST to NHF_HOST.
  • Export in_ifaddr as nh_ia. This allows to fix ifa packets/bytes accounting
melifaro added inline comments.Apr 4 2019, 1:46 AM
sys/netinet/in_fib.h
46 ↗(On Diff #55789)

Would it be possible to move it to a spare field?

sys/netinet/ip_output.c
306 ↗(On Diff #55755)

No, we don't. I was suggesting adding it as we have these checks repeated at least twice in the ip_output()

443 ↗(On Diff #55789)

Seem to always fallback to the in_ifaddr_broadcast() check.

Originally RTF_BROADCAST flag was added 22 years ago in r15652 to avoid calling in_ifaddr_broadcast(). We had route cloning back then, so it was a fair assumption - if the lookup result was a host route (and it almost always was), then we could trust its broadcast flag, as it was set in the in_addroute() after verificaton.

Currently we don't have route cloning and the only way of having RTF_BROADCAST set on the route is to explicitly add host route to the broadcast address:

route add -host 10.0.0.255 -iface vtnet0

assuming 10.0.0.0/24 belongs to vtnet0

Given that, the check here and in line 400 (if (ro->ro_rt->rt_flags & RTF_HOST)) will aways failback to in_ifaddr_broadcast().

Maybe it's worth just eliminating this condition and always check for in_ifaddr_broadcast().

glebius added inline comments.Apr 4 2019, 10:27 PM
sys/netinet/ip_output.c
443 ↗(On Diff #55789)

But as long as we allow adding such routes manually, we should support them, shouldn't we?

glebius updated this revision to Diff 55818.Apr 4 2019, 10:28 PM
  • Remove spare, since nh_ia was added.
glebius marked an inline comment as done.Apr 4 2019, 10:28 PM
melifaro accepted this revision.Apr 8 2019, 3:22 PM

LGTM. The last one: are you planning to implement the same functionality in ip6_output()? :-)

This revision is now accepted and ready to land.Apr 8 2019, 3:22 PM

Yes, as a separate change.

This revision was automatically updated to reflect the committed changes.