nd6: Fix the routing table subscription

[markj]

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

[zlei]

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

[zlei]

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() ?

[melifaro]

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.

[markj]

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.

[markj]

Avoid invoking the callback twice for multipath routes.

[markj]

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]

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]

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.

[bz]

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.

[markj]

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

[markj]

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

https://reviews.freebsd.org/D46136