On Tue, Apr 25, 2023 at 11:44:34AM +0200, Alexander Bluhm wrote:
> 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.
This is progress in the right direction, I think.
The XXXSMP comment is incorrect and confusing.
>
> The kernel lock is needed to change rt_flags. Testing without
> KERNEL_LOCK() caused crashes.
Still not nice, reducing kernel lock scope helps.
With this diff in, we couldn't break the kernel with further unlock diffs.
A clearer version of this diff would use two new bools `expired' and `reject'
rather than a ternary `reject', but that can be polished and retested later.
OK kn
>
> 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);
>
