Page MenuHomeFreeBSD

Simplify ifa/ifp refcounting in the routing stack.
ClosedPublic

Authored by melifaro on Feb 21 2021, 2:29 PM.
Tags
None
Referenced Files
Unknown Object (File)
Oct 1 2024, 1:27 PM
Unknown Object (File)
Sep 26 2024, 5:22 PM
Unknown Object (File)
Sep 26 2024, 5:21 PM
Unknown Object (File)
Sep 26 2024, 5:21 PM
Unknown Object (File)
Sep 26 2024, 5:21 PM
Unknown Object (File)
Sep 23 2024, 9:58 AM
Unknown Object (File)
Sep 17 2024, 12:09 AM
Unknown Object (File)
Sep 17 2024, 12:09 AM
Subscribers

Details

Summary

The routing stack control depends on quite a tree of functions to determine the proper attributes of a route such as a source address (ifa) or transmit ifp of a route.

When actually inserting a route, stack needs to ensure that ifa and ifp points to the entities that are still valid.
Validity means slightly more than just pointer validity - stack need guarantee that the provided objects are not scheduled for deletion.

Currently, callers either ignore it (most ifp parts, historically) or try to use refcounting (ifa parts). Even in case of ifa refcounting it's not always implemented correctly. For example, some codepaths inside rt_getifa_fib() are referencing ifa while not holding any locks, resulting in the referencing scheduled-for-deletion ifa.

Instead of trying to fix all of the callers by enforcing proper refcounting, switch to a different model.
As the rib_action() already requires epoch, do not require any stability guarantees other than the epoch-provided one.
Use newly-added conditional versions of the refcounting functions (ifa_try_ref(), if_try_ref()) and fail if any of these fails.

This addresses quite a number of corner cases in the route modification handling AND makes it simpler.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 37242
Build 34131: arc lint + arc unit

Event Timeline

melifaro added a reviewer: network.

I like, that the ugly "reference" paramenter is gone. It was never used.

sys/net/route/nhop_ctl.c
541–555

Can you merge that into a single

if (ng->...sent == NULL || !ref...deps(ng)) {
   if (ng->...send)
      counter..free();

to avoid code duplication?

Or something like

if (!ng->...sent) {
   error = ENOMEN;
   goto fail;
}
sys/net/route/nhop_ctl.c
541–555

Yeah. Tried that, but it doesn't look any better, at least in my view.

There are only 2 lines that is actually the same in these blocks. Merging two blocks into one adds too many conditions (how do we easily distinguish between ENOMEM and EAGAIN?).

I'd rather leave them as is - at least it's easy to read.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 22 2021, 11:38 PM
This revision was automatically updated to reflect the committed changes.