Hi,
Mutex arp_mtx protects the llinfo_arp la_... fields. So kernel
lock is only needed for changing the route rt_flags.
Of course there is a race between checking and setting rt_flags.
But the other checks of the RTF_REJECT flags were without kernel
lock before. This does not cause trouble, the worst thing that may
happen is to wait another exprire time for ARP retry. My diff does
not make it worse, reading rt_flags and rt_expire is done without
lock anyway.
The kernel lock is needed to change rt_flags. Testing without
KERNEL_LOCK() caused crashes.
ok?
bluhm
Index: netinet/if_ether.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/if_ether.c,v
retrieving revision 1.262
diff -u -p -r1.262 if_ether.c
--- netinet/if_ether.c 12 Apr 2023 16:14:42 -0000 1.262
+++ netinet/if_ether.c 24 Apr 2023 14:44:51 -0000
@@ -339,7 +339,7 @@ arpresolve(struct ifnet *ifp, struct rte
struct rtentry *rt = NULL;
char addr[INET_ADDRSTRLEN];
time_t uptime;
- int refresh = 0;
+ int refresh = 0, reject = 0;
if (m->m_flags & M_BCAST) { /* broadcast */
memcpy(desten, etherbroadcastaddr, sizeof(etherbroadcastaddr));
@@ -411,16 +411,9 @@ arpresolve(struct ifnet *ifp, struct rte
if (ifp->if_flags & (IFF_NOARP|IFF_STATICARP))
goto bad;
- KERNEL_LOCK();
- /*
- * Re-check since we grab the kernel lock after the first check.
- * rtrequest_delete() can be called with shared netlock. From
- * there arp_rtrequest() is reached which touches RTF_LLINFO
- * and rt_llinfo. As this is called with kernel lock we grab the
- * kernel lock here and are safe. XXXSMP
- */
+ mtx_enter(&arp_mtx);
if (!ISSET(rt->rt_flags, RTF_LLINFO)) {
- KERNEL_UNLOCK();
+ mtx_leave(&arp_mtx);
goto bad;
}
la = (struct llinfo_arp *)rt->rt_llinfo;
@@ -451,13 +444,13 @@ arpresolve(struct ifnet *ifp, struct rte
}
#endif
if (rt->rt_expire) {
- rt->rt_flags &= ~RTF_REJECT;
+ reject = ~RTF_REJECT;
if (la->la_asked == 0 || rt->rt_expire != uptime) {
rt->rt_expire = uptime;
if (la->la_asked++ < arp_maxtries)
refresh = 1;
else {
- rt->rt_flags |= RTF_REJECT;
+ reject = RTF_REJECT;
rt->rt_expire += arpt_down;
la->la_asked = 0;
la->la_refreshed = 0;
@@ -466,8 +459,18 @@ arpresolve(struct ifnet *ifp, struct rte
}
}
}
+ mtx_leave(&arp_mtx);
- KERNEL_UNLOCK();
+ if (reject == RTF_REJECT && !ISSET(rt->rt_flags, RTF_REJECT)) {
+ KERNEL_LOCK();
+ SET(rt->rt_flags, RTF_REJECT);
+ KERNEL_UNLOCK();
+ }
+ if (reject == ~RTF_REJECT && ISSET(rt->rt_flags, RTF_REJECT)) {
+ KERNEL_LOCK();
+ CLR(rt->rt_flags, RTF_REJECT);
+ KERNEL_UNLOCK();
+ }
if (refresh)
arprequest(ifp, &satosin(rt->rt_ifa->ifa_addr)->sin_addr.s_addr,
&satosin(dst)->sin_addr.s_addr, ac->ac_enaddr);
