--- if.c.orig 2026-09-10 02:34:29.419130783 +0000 +++ if.c.fixed 2026-09-10 02:51:59.901641114 +0000 @@ -131,7 +131,6 @@ const struct ifnet_array *); static struct ifnet_array *ifnet_array_del(struct ifnet *, const struct ifnet_array *); -static struct ifg_group *if_creategroup(const char *); static int if_destroygroup(struct ifg_group *); static int if_delgroup_locked(struct ifnet *, const char *); static int if_getgroups(struct ifgroupreq *, struct ifnet *); @@ -1102,22 +1101,23 @@ /* * Create an empty interface group. */ +/* + * NOTE: ifgroup_lock must be held exclusively when calling this. + */ static struct ifg_group * -if_creategroup(const char *groupname) +if_creategroup_locked(const char *groupname) { struct ifg_group *ifg; + KKASSERT(lockstatus(&ifgroup_lock, curthread) == LK_EXCLUSIVE); + ifg = kmalloc(sizeof(*ifg), M_IFNET, M_WAITOK); strlcpy(ifg->ifg_group, groupname, sizeof(ifg->ifg_group)); ifg->ifg_refcnt = 0; ifg->ifg_carp_demoted = 0; TAILQ_INIT(&ifg->ifg_members); - ifgroup_lockmgr(LK_EXCLUSIVE); TAILQ_INSERT_TAIL(&ifg_head, ifg, ifg_next); - ifgroup_lockmgr(LK_RELEASE); - - EVENTHANDLER_INVOKE(group_attach_event, ifg); return (ifg); } @@ -1148,21 +1148,32 @@ int if_addgroup(struct ifnet *ifp, const char *groupname) { - struct ifg_list *ifgl; + struct ifg_list *ifgl, *ifgl_tmp; struct ifg_group *ifg; struct ifg_member *ifgm; + int created = 0; + int error; if (groupname[0] && groupname[strlen(groupname) - 1] >= '0' && groupname[strlen(groupname) - 1] <= '9') return (EINVAL); - ifgroup_lockmgr(LK_SHARED); + ifgl = kmalloc(sizeof(*ifgl), M_IFNET, M_WAITOK); + ifgm = kmalloc(sizeof(*ifgm), M_IFNET, M_WAITOK); - TAILQ_FOREACH(ifgl, &ifp->if_groups, ifgl_next) { - if (strcmp(ifgl->ifgl_group->ifg_group, groupname) == 0) { - ifgroup_lockmgr(LK_RELEASE); - return (EEXIST); + ifgroup_lockmgr(LK_EXCLUSIVE); + + /* + * NOTE: Group lookup, creation, and the refcount bump are + * performed atomically w.r.t. other if_addgroup() calls and + * w.r.t. if_delgroup_locked() -- ifg_refcnt is modified by + * both, so the increment must be covered by the ifgroup lock. + */ + TAILQ_FOREACH(ifgl_tmp, &ifp->if_groups, ifgl_next) { + if (strcmp(ifgl_tmp->ifgl_group->ifg_group, groupname) == 0) { + error = EEXIST; + goto failed; } } @@ -1170,26 +1181,31 @@ if (strcmp(ifg->ifg_group, groupname) == 0) break; } + if (ifg == NULL) { + ifg = if_creategroup_locked(groupname); + created = 1; + } - ifgroup_lockmgr(LK_RELEASE); - - if (ifg == NULL) - ifg = if_creategroup(groupname); - - ifgl = kmalloc(sizeof(*ifgl), M_IFNET, M_WAITOK); - ifgm = kmalloc(sizeof(*ifgm), M_IFNET, M_WAITOK); ifgl->ifgl_group = ifg; ifgm->ifgm_ifp = ifp; ifg->ifg_refcnt++; - ifgroup_lockmgr(LK_EXCLUSIVE); TAILQ_INSERT_TAIL(&ifg->ifg_members, ifgm, ifgm_next); TAILQ_INSERT_TAIL(&ifp->if_groups, ifgl, ifgl_next); + ifgroup_lockmgr(LK_RELEASE); + if (created) + EVENTHANDLER_INVOKE(group_attach_event, ifg); EVENTHANDLER_INVOKE(group_change_event, groupname); return (0); + +failed: + ifgroup_lockmgr(LK_RELEASE); + kfree(ifgl, M_IFNET); + kfree(ifgm, M_IFNET); + return (error); } /* @@ -1348,6 +1364,36 @@ return (ENOMEM); ifgroup_lockmgr(LK_SHARED); + + /* + * The group may have been destroyed (or destroyed and + * recreated) while we were not holding the lock. Look it up + * again rather than dereferencing the potentially stale + * pointer found above. + */ + TAILQ_FOREACH(ifg, &ifg_head, ifg_next) { + if (strcmp(ifg->ifg_group, ifgr->ifgr_name) == 0) + break; + } + if (ifg == NULL) { + ifgroup_lockmgr(LK_RELEASE); + kfree(ifgrq, M_TEMP); + return (ENOENT); + } + + /* + * Membership may also have changed; recompute the length and + * re-check it against the caller's buffer. + */ + len = 0; + TAILQ_FOREACH(ifgm, &ifg->ifg_members, ifgm_next) + len += sizeof(struct ifg_req); + if (ifgr->ifgr_len != len) { + ifgroup_lockmgr(LK_RELEASE); + kfree(ifgrq, M_TEMP); + return (EINVAL); + } + p = ifgrq; TAILQ_FOREACH(ifgm, &ifg->ifg_members, ifgm_next) { if (len < sizeof(struct ifg_req)) {