On Fri, May 12, 2023 at 12:18:12AM +0200, Alexander Bluhm wrote:
> Hi,
>
> I would like to remove the kernel lock from nd6 resolve and use nd6
> mutex instead.
>
> Access rt_llinfo and check for NULL without checking RTF_LLINFO
> flag before. They are changed togehter with the arp or nd6 mutex.
It is the same change, but I'd commit ARP separately (you don't change
any locking semantics there).
> Access rt_llinfo either with nd6 mutex or exclusive netlock.
Can you leave a comment at the read-only ioctl wrt. exclusive net lock?
> Remove some needless NULL initializations.
>
> In nd6 resolve replace kernel lock with nd6 mutex.
I prefert the direct llinfo_* check, simpler and more obvious.
>
> ok?
>
> bluhm
>
> Index: netinet/if_ether.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/if_ether.c,v
> retrieving revision 1.264
> diff -u -p -r1.264 if_ether.c
> --- netinet/if_ether.c 7 May 2023 16:23:23 -0000 1.264
> +++ netinet/if_ether.c 11 May 2023 19:00:19 -0000
> @@ -388,10 +388,8 @@ arpresolve(struct ifnet *ifp, struct rte
> rt->rt_expire - arpt_keep / 8 < uptime) {
>
> mtx_enter(&arp_mtx);
> - if (ISSET(rt->rt_flags, RTF_LLINFO)) {
> - la = (struct llinfo_arp *)rt->rt_llinfo;
> - KASSERT(la != NULL);
> -
> + la = (struct llinfo_arp *)rt->rt_llinfo;
> + if (la != NULL) {
> if (la->la_refreshed + 30 < uptime) {
> la->la_refreshed = uptime;
> refresh = 1;
> @@ -412,12 +410,11 @@ arpresolve(struct ifnet *ifp, struct rte
> goto bad;
>
> mtx_enter(&arp_mtx);
> - if (!ISSET(rt->rt_flags, RTF_LLINFO)) {
> + la = (struct llinfo_arp *)rt->rt_llinfo;
> + if (la == NULL) {
> mtx_leave(&arp_mtx);
> goto bad;
> }
> - la = (struct llinfo_arp *)rt->rt_llinfo;
> - KASSERT(la != NULL);
>
> /*
> * There is an arptab entry, but no ethernet address
> Index: netinet6/nd6.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6.c,v
> retrieving revision 1.278
> diff -u -p -r1.278 nd6.c
> --- netinet6/nd6.c 8 May 2023 13:14:21 -0000 1.278
> +++ netinet6/nd6.c 11 May 2023 20:39:58 -0000
> @@ -306,7 +306,7 @@ nd6_llinfo_timer(struct rtentry *rt)
> struct sockaddr_in6 *dst = satosin6(rt_key(rt));
> struct ifnet *ifp;
>
> - NET_ASSERT_LOCKED();
> + NET_ASSERT_LOCKED_EXCLUSIVE();
>
> if ((ifp = if_get(rt->rt_ifidx)) == NULL)
> return 1;
> @@ -527,6 +527,7 @@ nd6_lookup(const struct in6_addr *addr6,
> if (rt == NULL) {
> if (create && ifp) {
> struct rt_addrinfo info;
> + struct llinfo_nd6 *ln;
> struct ifaddr *ifa;
> int error;
>
> @@ -556,11 +557,11 @@ nd6_lookup(const struct in6_addr *addr6,
> rtableid);
> if (error)
> return (NULL);
> - if (rt->rt_llinfo != NULL) {
> - struct llinfo_nd6 *ln =
> - (struct llinfo_nd6 *)rt->rt_llinfo;
> + mtx_enter(&nd6_mtx);
> + ln = (struct llinfo_nd6 *)rt->rt_llinfo;
> + if (ln != NULL)
> ln->ln_state = ND6_LLINFO_NOSTATE;
> - }
> + mtx_leave(&nd6_mtx);
You now grab a mutex while holding the exclusive net lock in some callers,
e.g. nd6_na_input -> NET_ASSERT_LOCKED_EXCLUSIVE -> nd6_lookup -> nd6_mtx,
is that intended?
What about the end of nd6_lookup(), your diff does not add a mutex there:
/*
* Validation for the entry.
* Note that the check for rt_llinfo is necessary because a cloned
* route from a parent route that has the L flag (e.g. the default
* route to a p2p interface) may have the flag, too, while the
* destination is not actually a neighbor.
*/
if ((rt->rt_flags & RTF_GATEWAY) || (rt->rt_flags & RTF_LLINFO) == 0 ||
rt->rt_gateway->sa_family != AF_LINK || rt->rt_llinfo == NULL ||
(ifp != NULL && rt->rt_ifidx != ifp->if_index)) {
if (create) {
char addr[INET6_ADDRSTRLEN];
nd6log((LOG_DEBUG, "%s: failed to lookup %s (if=%s)\n",
__func__,
inet_ntop(AF_INET6, addr6, addr, sizeof(addr)),
ifp ? ifp->if_xname : "unspec"));
}
rtfree(rt);
return (NULL);
}
return (rt);
}
> } else
> return (NULL);
> }
> @@ -666,7 +667,7 @@ nd6_free(struct rtentry *rt)
> struct in6_addr in6 = satosin6(rt_key(rt))->sin6_addr;
> struct ifnet *ifp;
>
> - NET_ASSERT_LOCKED();
> + NET_ASSERT_LOCKED_EXCLUSIVE();
>
> ifp = if_get(rt->rt_ifidx);
>
> @@ -706,6 +707,8 @@ nd6_nud_hint(struct rtentry *rt)
> struct llinfo_nd6 *ln;
> struct ifnet *ifp;
>
> + NET_ASSERT_LOCKED_EXCLUSIVE();
> +
> ifp = if_get(rt->rt_ifidx);
> if (ifp == NULL)
> return;
> @@ -741,7 +744,7 @@ void
> nd6_rtrequest(struct ifnet *ifp, int req, struct rtentry *rt)
> {
> struct sockaddr *gate = rt->rt_gateway;
> - struct llinfo_nd6 *ln = (struct llinfo_nd6 *)rt->rt_llinfo;
> + struct llinfo_nd6 *ln;
> struct ifaddr *ifa;
> struct in6_ifaddr *ifa6;
>
> @@ -977,7 +980,7 @@ nd6_ioctl(u_long cmd, caddr_t data, stru
> struct in6_addr nb_addr = nbi->addr; /* make local for safety */
> time_t expire;
>
> - NET_LOCK_SHARED();
> + NET_LOCK();
> /*
> * XXX: KAME specific hack for scoped addresses
> * XXXX: for other scopes than link-local?
> @@ -994,7 +997,7 @@ nd6_ioctl(u_long cmd, caddr_t data, stru
> if (rt == NULL ||
> (ln = (struct llinfo_nd6 *)rt->rt_llinfo) == NULL) {
> rtfree(rt);
> - NET_UNLOCK_SHARED();
> + NET_UNLOCK();
> return (EINVAL);
> }
> expire = ln->ln_rt->rt_expire;
> @@ -1009,7 +1012,7 @@ nd6_ioctl(u_long cmd, caddr_t data, stru
> nbi->expire = expire;
>
> rtfree(rt);
> - NET_UNLOCK_SHARED();
> + NET_UNLOCK();
> return (0);
> }
> }
> @@ -1027,15 +1030,17 @@ void
> nd6_cache_lladdr(struct ifnet *ifp, const struct in6_addr *from, char
> *lladdr,
> int lladdrlen, int type, int code)
> {
> - struct rtentry *rt = NULL;
> - struct llinfo_nd6 *ln = NULL;
> + struct rtentry *rt;
> + struct llinfo_nd6 *ln;
> int is_newentry;
> - struct sockaddr_dl *sdl = NULL;
> + struct sockaddr_dl *sdl;
> int do_update;
> int olladdr;
> int llchange;
> int newstate = 0;
>
> + NET_ASSERT_LOCKED_EXCLUSIVE();
> +
> if (!ifp)
> panic("%s: ifp == NULL", __func__);
> if (!from)
> @@ -1257,7 +1262,7 @@ nd6_resolve(struct ifnet *ifp, struct rt
> {
> struct sockaddr_dl *sdl;
> struct rtentry *rt;
> - struct llinfo_nd6 *ln = NULL;
> + struct llinfo_nd6 *ln;
> struct in6_addr saddr6;
> time_t uptime;
> int solicit = 0;
> @@ -1295,23 +1300,20 @@ nd6_resolve(struct ifnet *ifp, struct rt
> goto bad;
> }
>
> - KERNEL_LOCK();
> - if (!ISSET(rt->rt_flags, RTF_LLINFO)) {
> - KERNEL_UNLOCK();
> + mtx_enter(&nd6_mtx);
> + ln = (struct llinfo_nd6 *)rt->rt_llinfo;
> + if (ln == NULL) {
> + mtx_leave(&nd6_mtx);
> goto bad;
> }
> - ln = (struct llinfo_nd6 *)rt->rt_llinfo;
> - KASSERT(ln != NULL);
>
> /*
> * Move this entry to the head of the queue so that it is less likely
> * for this entry to be a target of forced garbage collection (see
> * nd6_rtrequest()).
> */
> - mtx_enter(&nd6_mtx);
> TAILQ_REMOVE(&nd6_list, ln, ln_list);
> TAILQ_INSERT_HEAD(&nd6_list, ln, ln_list);
> - mtx_leave(&nd6_mtx);
>
> /*
> * The first time we send a packet to a neighbor whose entry is
> @@ -1332,7 +1334,7 @@ nd6_resolve(struct ifnet *ifp, struct rt
> * send the packet.
> */
> if (ln->ln_state > ND6_LLINFO_INCOMPLETE) {
> - KERNEL_UNLOCK();
> + mtx_leave(&nd6_mtx);
>
> sdl = satosdl(rt->rt_gateway);
> if (sdl->sdl_alen != ETHER_ADDR_LEN) {
> @@ -1378,7 +1380,7 @@ nd6_resolve(struct ifnet *ifp, struct rt
> saddr6 = ln->ln_saddr6;
> solicit = 1;
> }
> - KERNEL_UNLOCK();
> + mtx_leave(&nd6_mtx);
>
> if (solicit)
> nd6_ns_output(ifp, NULL, &satosin6(dst)->sin6_addr, &saddr6, 0);
>
