On Mon, Apr 03, 2023 at 02:12:45PM +0200, Alexander Bluhm wrote:
> Hi,
>
> The arp mbuf queue la_mq has its own mutex, la_hold_total uses
> atomic operations. So they don't need the global arp mutex.
>
> Pull them out of arp_mtx blocks to make clear what the scope of
> arp_mtx protection is.
>
> ok?
>
According `arp_mtx' mutex(9) description, "llinfo_arp live time,
rt_llinfo and RTF_LLINFO are protected by arp_mtx". So why unlocked `la'
dereference within arpinvalidate() is safe?
The mq_init(&la->la_mq, ...) movement hunks are good to me.
> bluhm
>
> Index: netinet/if_ether.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/if_ether.c,v
> retrieving revision 1.258
> diff -u -p -r1.258 if_ether.c
> --- netinet/if_ether.c 8 Mar 2023 04:43:08 -0000 1.258
> +++ netinet/if_ether.c 3 Apr 2023 11:47:57 -0000
> @@ -203,6 +203,7 @@ arp_rtrequest(struct ifnet *ifp, int req
> log(LOG_DEBUG, "%s: pool get failed\n", __func__);
> break;
> }
> + mq_init(&la->la_mq, LA_HOLD_QUEUE, IPL_SOFTNET);
>
> mtx_enter(&arp_mtx);
> if (rt->rt_llinfo != NULL) {
> @@ -211,7 +212,6 @@ arp_rtrequest(struct ifnet *ifp, int req
> pool_put(&arp_pool, la);
> break;
> }
> - mq_init(&la->la_mq, LA_HOLD_QUEUE, IPL_SOFTNET);
> rt->rt_llinfo = (caddr_t)la;
> la->la_rt = rt;
> rt->rt_flags |= RTF_LLINFO;
> @@ -233,9 +233,9 @@ arp_rtrequest(struct ifnet *ifp, int req
> LIST_REMOVE(la, la_list);
> rt->rt_llinfo = NULL;
> rt->rt_flags &= ~RTF_LLINFO;
> - atomic_sub_int(&la_hold_total, mq_purge(&la->la_mq));
> mtx_leave(&arp_mtx);
>
> + atomic_sub_int(&la_hold_total, mq_purge(&la->la_mq));
> pool_put(&arp_pool, la);
> break;
>
> @@ -755,10 +755,11 @@ arpinvalidate(struct rtentry *rt)
> mtx_leave(&arp_mtx);
> return;
> }
> - atomic_sub_int(&la_hold_total, mq_purge(&la->la_mq));
> sdl->sdl_alen = 0;
> la->la_asked = 0;
> mtx_leave(&arp_mtx);
> +
> + atomic_sub_int(&la_hold_total, mq_purge(&la->la_mq));
> }
>
> /*
>
