Re: nd6 remove kernel lock
On Fri, May 12, 2023 at 11:43:42AM +, Klemens Nanni wrote:
> On Fri, May 12, 2023 at 12:18:12AM +0200, Alexander Bluhm wrote:
> > Access rt_llinfo either with nd6 mutex or exclusive netlock.
>
> Can you leave a comment at the read-only ioctl wrt. exclusive net lock?
Even better. nd6_lookup() must be mp-safe as it is called by
ip6_forward6() via nd6_is_addr_neighbor(). Just put the mutex
around rt_llinfo in nd6_ioctl() and keep the netlock shared.
ok?
bluhm
Index: netinet6/nd6.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6.c,v
retrieving revision 1.279
diff -u -p -r1.279 nd6.c
--- netinet6/nd6.c 12 May 2023 12:42:16 - 1.279
+++ netinet6/nd6.c 13 May 2023 14:28:57 -
@@ -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;
@@ -557,9 +557,11 @@ nd6_lookup(const struct in6_addr *addr6,
rtableid);
if (error)
return (NULL);
+ mtx_enter(_mtx);
ln = (struct llinfo_nd6 *)rt->rt_llinfo;
if (ln != NULL)
ln->ln_state = ND6_LLINFO_NOSTATE;
+ mtx_leave(_mtx);
} else
return (NULL);
}
@@ -665,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);
@@ -705,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;
@@ -990,8 +994,10 @@ nd6_ioctl(u_long cmd, caddr_t data, stru
}
rt = nd6_lookup(_addr, 0, ifp, ifp->if_rdomain);
+ mtx_enter(_mtx);
if (rt == NULL ||
(ln = (struct llinfo_nd6 *)rt->rt_llinfo) == NULL) {
+ mtx_leave(_mtx);
rtfree(rt);
NET_UNLOCK_SHARED();
return (EINVAL);
@@ -1006,6 +1012,7 @@ nd6_ioctl(u_long cmd, caddr_t data, stru
nbi->asked = ln->ln_asked;
nbi->isrouter = ln->ln_router;
nbi->expire = expire;
+ mtx_leave(_mtx);
rtfree(rt);
NET_UNLOCK_SHARED();
@@ -1035,6 +1042,8 @@ nd6_cache_lladdr(struct ifnet *ifp, cons
int llchange;
int newstate = 0;
+ NET_ASSERT_LOCKED_EXCLUSIVE();
+
if (!ifp)
panic("%s: ifp == NULL", __func__);
if (!from)
@@ -1294,23 +1303,20 @@ nd6_resolve(struct ifnet *ifp, struct rt
goto bad;
}
- KERNEL_LOCK();
- if (!ISSET(rt->rt_flags, RTF_LLINFO)) {
- KERNEL_UNLOCK();
+ mtx_enter(_mtx);
+ ln = (struct llinfo_nd6 *)rt->rt_llinfo;
+ if (ln == NULL) {
+ mtx_leave(_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(_mtx);
TAILQ_REMOVE(_list, ln, ln_list);
TAILQ_INSERT_HEAD(_list, ln, ln_list);
- mtx_leave(_mtx);
/*
* The first time we send a packet to a neighbor whose entry is
@@ -1331,7 +1337,7 @@ nd6_resolve(struct ifnet *ifp, struct rt
* send the packet.
*/
if (ln->ln_state > ND6_LLINFO_INCOMPLETE) {
- KERNEL_UNLOCK();
+ mtx_leave(_mtx);
sdl = satosdl(rt->rt_gateway);
if (sdl->sdl_alen != ETHER_ADDR_LEN) {
@@ -1377,7 +1383,7 @@ nd6_resolve(struct ifnet *ifp, struct rt
saddr6 = ln->ln_saddr6;
solicit = 1;
}
- KERNEL_UNLOCK();
+ mtx_leave(_mtx);
if (solicit)
nd6_ns_output(ifp, NULL, (dst)->sin6_addr, , 0);
Re: nd6 remove kernel lock
> On 12 May 2023, at 15:07, Alexander Bluhm wrote:
>
> On Fri, May 12, 2023 at 11:43:42AM +, Klemens Nanni wrote:
>>> 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).
>
> I had prepared a smaller diff already. Here is the part that does
> not touch the locking. Just some cleanup to get ARP and ND6 in
> sync.
>
> Let's start with that and discuss locking separately.
>
> ok?
>
ok mvs
> 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.c7 May 2023 16:23:23 - 1.264
> +++ netinet/if_ether.c12 May 2023 11:15:07 -
> @@ -388,10 +388,8 @@ arpresolve(struct ifnet *ifp, struct rte
> rt->rt_expire - arpt_keep / 8 < uptime) {
>
> mtx_enter(_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(_mtx);
> - if (!ISSET(rt->rt_flags, RTF_LLINFO)) {
> + la = (struct llinfo_arp *)rt->rt_llinfo;
> + if (la == NULL) {
> mtx_leave(_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.c8 May 2023 13:14:21 - 1.278
> +++ netinet6/nd6.c12 May 2023 11:58:54 -
> @@ -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,9 @@ 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;
> + ln = (struct llinfo_nd6 *)rt->rt_llinfo;
> + if (ln != NULL)
> ln->ln_state = ND6_LLINFO_NOSTATE;
> - }
> } else
> return (NULL);
> }
> @@ -741,7 +740,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;
>
> @@ -1027,10 +1026,10 @@ 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;
> @@ -1257,7 +1256,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;
>
Re: nd6 remove kernel lock
On Fri, May 12, 2023 at 11:43:42AM +, Klemens Nanni wrote:
> > 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).
I had prepared a smaller diff already. Here is the part that does
not touch the locking. Just some cleanup to get ARP and ND6 in
sync.
Let's start with that and discuss locking separately.
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 - 1.264
+++ netinet/if_ether.c 12 May 2023 11:15:07 -
@@ -388,10 +388,8 @@ arpresolve(struct ifnet *ifp, struct rte
rt->rt_expire - arpt_keep / 8 < uptime) {
mtx_enter(_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(_mtx);
- if (!ISSET(rt->rt_flags, RTF_LLINFO)) {
+ la = (struct llinfo_arp *)rt->rt_llinfo;
+ if (la == NULL) {
mtx_leave(_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 - 1.278
+++ netinet6/nd6.c 12 May 2023 11:58:54 -
@@ -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,9 @@ 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;
+ ln = (struct llinfo_nd6 *)rt->rt_llinfo;
+ if (ln != NULL)
ln->ln_state = ND6_LLINFO_NOSTATE;
- }
} else
return (NULL);
}
@@ -741,7 +740,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;
@@ -1027,10 +1026,10 @@ 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;
@@ -1257,7 +1256,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;
Re: nd6 remove kernel lock
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.c7 May 2023 16:23:23 - 1.264
> +++ netinet/if_ether.c11 May 2023 19:00:19 -
> @@ -388,10 +388,8 @@ arpresolve(struct ifnet *ifp, struct rte
> rt->rt_expire - arpt_keep / 8 < uptime) {
>
> mtx_enter(_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(_mtx);
> - if (!ISSET(rt->rt_flags, RTF_LLINFO)) {
> + la = (struct llinfo_arp *)rt->rt_llinfo;
> + if (la == NULL) {
> mtx_leave(_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.c8 May 2023 13:14:21 - 1.278
> +++ netinet6/nd6.c11 May 2023 20:39:58 -
> @@ -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(_mtx);
> + ln = (struct llinfo_nd6 *)rt->rt_llinfo;
> + if (ln != NULL)
> ln->ln_state = ND6_LLINFO_NOSTATE;
> - }
> + mtx_leave(_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);
