nd6: Fix the routing table subscription
ClosedPublic
Actions

Authored by markj on Jul 18 2024, 3:22 PM.

Details

Reviewers

melifaro
bz

Group Reviewers

network

Commits

rGb6d6674fecbd: nd6: Fix the routing table subscription
rG751c889691cc: nd6: Fix the routing table subscription
rG407ef8669f5a: nd6: Fix the routing table subscription
rGa48df53e4249: nd6: Fix the routing table subscription

Summary

The nd6 code listens for RTM_DELETE events so that it can mark the
corresponding default router as inactive in the case where the default
route is deleted. A subsequent RA from the router may then reinstall
the default route.

Commit fedeb08b6a58e broke this for non-multipath routes, as
rib_decompose_notification() only invokes the callback for multipath
routes. Restore the old behaviour. Also ensure that we update the
router only for RTM_DELETE notifications, lost in commit 2259a03020fe0.

Fixes: fedeb08b6a58 ("Introduce scalable route multipath.")
Fixes: 2259a03020fe ("Rework part of routing code to reduce difference to D26449.")
Sponsored by: Klara, Inc.
Sponsored by: Bell Tower Integration

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 58759
Build 55647: arc lint + arc unit

Event Timeline

markj created this revision.Jul 18 2024, 3:22 PM

Herald added subscribers: glebius, ae, imp. · View Herald TranscriptJul 18 2024, 3:22 PM

markj requested review of this revision.Jul 18 2024, 3:22 PM

Harbormaster completed remote builds in B58703: Diff 141075.Jul 18 2024, 3:22 PM

Ah, fast fix. I'm also looking at this issue :)

daniel_belltech.it added a subscriber: daniel_belltech.it.Jul 18 2024, 3:32 PM

franco_opnsense.org added a subscriber: franco_opnsense.org.Jul 18 2024, 7:58 PM

zlei added inline comments.Jul 19 2024, 5:02 PM

sys/netinet6/nd6.c
1636	This should work great for plain nexthop. Shall we check if the `rc->rc_nh_old` is nexthop group, aka via `NH_IS_NHGRP()` ?

Ty for working on that! IIRC the idea of rib_decompose_notification was to provide the same handling for both multipath and non-multipath routes for the callers that care only about specific paths. I'd rather add the relevant callback to the rib_decompose_notification() and not touch nd6_subscription_cb code.

In D46020#1049780, @melifaro wrote:

Ty for working on that! IIRC the idea of rib_decompose_notification was to provide the same handling for both multipath and non-multipath routes for the callers that care only about specific paths. I'd rather add the relevant callback to the rib_decompose_notification() and not touch nd6_subscription_cb code.

To be clear, you are asking me to:

make rib_decompose_notification() work for non-multipath nexthops, i.e., just remove the NH_IS_NHGRP() checks
move rib_decompose_notification() out of the ROUTE_MPATH ifdef
fix up existing callers

If so, sure, I will work on that.

Avoid invoking the callback twice for multipath routes.

Harbormaster completed remote builds in B58759: Diff 141222.Jul 22 2024, 1:51 PM

In D46020#1049951, @markj wrote:

In D46020#1049780, @melifaro wrote:

Ty for working on that! IIRC the idea of rib_decompose_notification was to provide the same handling for both multipath and non-multipath routes for the callers that care only about specific paths. I'd rather add the relevant callback to the rib_decompose_notification() and not touch nd6_subscription_cb code.

To be clear, you are asking me to:

make rib_decompose_notification() work for non-multipath nexthops, i.e., just remove the NH_IS_NHGRP() checks

move rib_decompose_notification() out of the ROUTE_MPATH ifdef

fix up existing callers

?

I took a stab at this and it's more complicated than I thought. I'm not familiar at all with the multipath routing code so would prefer to fix the bug minimally for now. This will also make backports simpler.

bz added a subscriber: bz.Jul 25 2024, 7:17 AM

bz added inline comments.

sys/netinet6/nd6.c
1616	The call stack was too deep for me to conclude rc_nh_old would always be non-NULL here? If you keep removing the NULL check below, can we add a KASSERT to document the assumption (here and below)?

markj added inline comments.Jul 25 2024, 12:56 PM

sys/netinet6/nd6.c
1616	rib_decompose_notification() also assumes that `rc_nh_old` is non-NULL (when the command is `RTM_DELETE`), and that apparently did not cause any problems in the past several years.

I am happy with it for now.
If someone wants to refine it (and write a regression test for it) then I think they can in main afterwards.

This revision is now accepted and ready to land.Jul 25 2024, 1:01 PM

In D46020#1051358, @bz wrote:

I am happy with it for now.
If someone wants to refine it (and write a regression test for it) then I think they can in main afterwards.

I will work on a regression test today. Something like:

create a vnet jail with a SLAAC-enabled interface,
use scapy to inject a RA
verify that a default route is installed
flush v6 routes, verify that the default route is gone
inject a second RA, then verify that the default route is reinstalled

Closed by commit rGa48df53e4249: nd6: Fix the routing table subscription (authored by markj). · Explain WhyJul 25 2024, 2:28 PM

This revision was automatically updated to reflect the committed changes.

markj added a commit: rGa48df53e4249: nd6: Fix the routing table subscription.

In D46020#1051361, @markj wrote:

In D46020#1051358, @bz wrote:

I am happy with it for now.
If someone wants to refine it (and write a regression test for it) then I think they can in main afterwards.

I will work on a regression test today. Something like:

create a vnet jail with a SLAAC-enabled interface,

use scapy to inject a RA

verify that a default route is installed

flush v6 routes, verify that the default route is gone

inject a second RA, then verify that the default route is reinstalled