> On 6 Feb 2021, at 15:23, Alexander Bluhm <alexander.bl...@gmx.net> wrote: > > On Sat, Feb 06, 2021 at 05:04:20PM +1000, David Gwynne wrote: >> refcnt_init starts counting at 1, while the existing code starts at 0. Do >> the crashes stop because we never fully release all the references and >> never free it now? > > You are absolutely right. I was too optimistic. > > Correct diff is below. It does not fix anything. Only advantage > is that carp does not access interface group internals. > > bluhm
I’m not sure it should be atomic. It seems groups require their own lock and this lock should be held while we perform if_addgroup() and if_delgroup(). However if_creategroup() should set `ifg_refcnt’ to 1 and carp(4) should not touch groups internals. > > Index: net/if.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/net/if.c,v > retrieving revision 1.626 > diff -u -p -r1.626 if.c > --- net/if.c 1 Feb 2021 07:43:33 -0000 1.626 > +++ net/if.c 6 Feb 2021 12:16:20 -0000 > @@ -2601,7 +2601,7 @@ if_creategroup(const char *groupname) > return (NULL); > > strlcpy(ifg->ifg_group, groupname, sizeof(ifg->ifg_group)); > - ifg->ifg_refcnt = 0; > + refcnt_init(&ifg->ifg_refcnt); > ifg->ifg_carp_demoted = 0; > TAILQ_INIT(&ifg->ifg_members); > #if NPF > 0 > @@ -2642,13 +2642,17 @@ if_addgroup(struct ifnet *ifp, const cha > if (!strcmp(ifg->ifg_group, groupname)) > break; > > - if (ifg == NULL && (ifg = if_creategroup(groupname)) == NULL) { > + if (ifg == NULL) > + ifg = if_creategroup(groupname); > + else > + refcnt_take(&ifg->ifg_refcnt); > + > + if (ifg == NULL) { > free(ifgl, M_TEMP, sizeof(*ifgl)); > free(ifgm, M_TEMP, sizeof(*ifgm)); > return (ENOMEM); > } > > - ifg->ifg_refcnt++; > ifgl->ifgl_group = ifg; > ifgm->ifgm_ifp = ifp; > > @@ -2692,7 +2696,7 @@ if_delgroup(struct ifnet *ifp, const cha > pfi_group_change(groupname); > #endif > > - if (--ifgl->ifgl_group->ifg_refcnt == 0) { > + if (refcnt_rele(&ifgl->ifgl_group->ifg_refcnt)) { > TAILQ_REMOVE(&ifg_head, ifgl->ifgl_group, ifg_next); > #if NPF > 0 > pfi_detach_ifgroup(ifgl->ifgl_group); > Index: net/if_var.h > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_var.h,v > retrieving revision 1.112 > diff -u -p -r1.112 if_var.h > --- net/if_var.h 29 Jul 2020 12:09:31 -0000 1.112 > +++ net/if_var.h 6 Feb 2021 12:11:35 -0000 > @@ -263,7 +263,7 @@ struct ifmaddr { > > struct ifg_group { > char ifg_group[IFNAMSIZ]; > - u_int ifg_refcnt; > + struct refcnt ifg_refcnt; > caddr_t ifg_pf_kif; > int ifg_carp_demoted; > TAILQ_HEAD(, ifg_member) ifg_members; > Index: netinet/ip_carp.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_carp.c,v > retrieving revision 1.351 > diff -u -p -r1.351 ip_carp.c > --- netinet/ip_carp.c 21 Jan 2021 13:18:07 -0000 1.351 > +++ netinet/ip_carp.c 6 Feb 2021 12:11:35 -0000 > @@ -786,10 +786,7 @@ carp_sysctl(int *name, u_int namelen, vo > void > carpattach(int n) > { > - struct ifg_group *ifg; > - > - if ((ifg = if_creategroup("carp")) != NULL) > - ifg->ifg_refcnt++; /* keep around even if empty */ > + if_creategroup("carp"); /* keep around even if empty */ > if_clone_attach(&carp_cloner); > carpcounters = counters_alloc(carps_ncounters); > } >