On Thu, May 04, 2023 at 08:43:19AM +0200, Alexander Bluhm wrote:
> To make ND6 mp-safe, I have to guarantee the life time of ln =
> rt->rt_llinfo. This call to nd6_llinfo_settimer(ln) looks strange.
It reads like two distinct cases folded into one overly clever block.
> The complicated logic can be replaced with what we have in ARP.
> Digging through the histroy shows a lot of refactoring that seems
> to make rt_expire handling here obsolete. Just initialize it to
> 0. And I doubt that event this is necessary. But let's stick to
> the ARP code for now.
Zeroing it does not hurt and clarifies intent, I'd do that for now.
Eventually, rt_expire should go completely.
> Cloning and local routes should never expire. If RTF_LLINFO is
> set, ln should not be NULL. So nd6_llinfo_settimer() was not reached
> in this case.
That matches nd6_llinfo_settimer(), check which you could extend:
KASSERT(!ISSET(ln->ln_rt->rt_flags, RTF_LOCAL));
>
> While there, remove obsolete comment and #if 0 code that never worked.
>
> ok?
>
> bluhm
>
> Index: netinet6/nd6.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6.c,v
> retrieving revision 1.274
> diff -u -p -r1.274 nd6.c
> --- netinet6/nd6.c 3 May 2023 11:43:31 -0000 1.274
> +++ netinet6/nd6.c 3 May 2023 22:02:28 -0000
> @@ -760,40 +760,12 @@ nd6_rtrequest(struct ifnet *ifp, int req
>
> switch (req) {
> case RTM_ADD:
> - if ((rt->rt_flags & RTF_CLONING) ||
> - ((rt->rt_flags & (RTF_LLINFO | RTF_LOCAL)) && ln == NULL)) {
> - if (ln != NULL)
> - nd6_llinfo_settimer(ln, 0);
> - if ((rt->rt_flags & RTF_CLONING) != 0)
> - break;
Looks like this could logically be split into...
if (rt->rt_flags & RTF_CLONING) {
nd6_llinfo_settimer(ln, 0);
break;
}
if ((rt->rt_flags & (RTF_LLINFO | RTF_LOCAL)) && ln == NULL))
if ((rt->rt_flags & RTF_CLONING) != 0)
break;
> + if (rt->rt_flags & RTF_CLONING) {
> + rt->rt_expire = 0;
> + break;
> }
... which is where your diff arrives at for cloning routes, aptly
disabling timers.
> - /*
> - * In IPv4 code, we try to announce new RTF_ANNOUNCE entry here.
> - * We don't do that here since llinfo is not ready yet.
> - *
> - * There are also couple of other things to be discussed:
> - * - unsolicited NA code needs improvement beforehand
> - * - RFC2461 says we MAY send multicast unsolicited NA
> - * (7.2.6 paragraph 4), however, it also says that we
> - * SHOULD provide a mechanism to prevent multicast NA storm.
> - * we don't have anything like it right now.
> - * note that the mechanism needs a mutual agreement
> - * between proxies, which means that we need to implement
> - * a new protocol, or a new kludge.
> - * - from RFC2461 6.2.4, host MUST NOT send an unsolicited NA.
> - * we need to check ip6forwarding before sending it.
> - * (or should we allow proxy ND configuration only for
> - * routers? there's no mention about proxy ND from hosts)
> - */
> -#if 0
> - /* XXX it does not work */
> - if (rt->rt_flags & RTF_ANNOUNCE)
> - nd6_na_output(ifp,
> - &satosin6(rt_key(rt))->sin6_addr,
> - &satosin6(rt_key(rt))->sin6_addr,
> - ip6_forwarding ? ND_NA_FLAG_ROUTER : 0,
> - 1, NULL);
> -#endif
(twenty year old noise...)
> + if ((rt->rt_flags & RTF_LOCAL) && rt->rt_llinfo == NULL)
> + rt->rt_expire = 0;
As nd6_llinfo_settimer() asserts, local routes must not expire, hence you
disable that here as well.
This leaves the llinfo case, which keeps falling through to resolve.
It all makes sense to me, but I wouldn't mind another pair of eyes on this
crippled code.
> /* FALLTHROUGH */
> case RTM_RESOLVE:
> if (gate->sa_family != AF_LINK ||
>
