From: bluhm Date: Mon, 29 Aug 2022 07:51:45 +0000 (+0000) Subject: Use struct refcnt for interface address reference counting. X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=18a44669273a480c6bde7a39e70e368af32c272f;p=openbsd Use struct refcnt for interface address reference counting. There was a crash due to use after free of the ifa although it is ref counted. As ifa_refcnt was a simple integer increment, there may be a path where multiple CPUs access it concurrently. So change to struct refcnt which is MP safe and provides dt(4) leak debugging. Link level address for IPsec enc(4) and various MPLS interfaces is special. There ifa is part of struct sc. Use refcount anyway and add a panic to detect use after free. bug report stsp@; OK mvs@ --- diff --git a/sys/dev/dt/dt_prov_static.c b/sys/dev/dt/dt_prov_static.c index d608373df56..611269fb390 100644 --- a/sys/dev/dt/dt_prov_static.c +++ b/sys/dev/dt/dt_prov_static.c @@ -1,4 +1,4 @@ -/* $OpenBSD: dt_prov_static.c,v 1.14 2022/06/28 09:32:27 bluhm Exp $ */ +/* $OpenBSD: dt_prov_static.c,v 1.15 2022/08/29 07:51:45 bluhm Exp $ */ /* * Copyright (c) 2019 Martin Pieuchot @@ -88,9 +88,10 @@ DT_STATIC_PROBE0(smr, wakeup); DT_STATIC_PROBE2(smr, thread, "uint64_t", "uint64_t"); /* - * reference counting + * reference counting, keep in sync with sys/refcnt.h */ DT_STATIC_PROBE0(refcnt, none); +DT_STATIC_PROBE3(refcnt, ifaddr, "void *", "int", "int"); DT_STATIC_PROBE3(refcnt, inpcb, "void *", "int", "int"); DT_STATIC_PROBE3(refcnt, tdb, "void *", "int", "int"); @@ -135,6 +136,7 @@ struct dt_probe *const dtps_static[] = { &_DT_STATIC_P(smr, thread), /* refcnt */ &_DT_STATIC_P(refcnt, none), + &_DT_STATIC_P(refcnt, ifaddr), &_DT_STATIC_P(refcnt, inpcb), &_DT_STATIC_P(refcnt, tdb), }; diff --git a/sys/net/if_enc.c b/sys/net/if_enc.c index 4645f3af89f..e835a8b31e3 100644 --- a/sys/net/if_enc.c +++ b/sys/net/if_enc.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if_enc.c,v 1.78 2020/12/28 14:28:50 kn Exp $ */ +/* $OpenBSD: if_enc.c,v 1.79 2022/08/29 07:51:45 bluhm Exp $ */ /* * Copyright (c) 2010 Reyk Floeter @@ -100,6 +100,7 @@ enc_clone_create(struct if_clone *ifc, int unit) * and empty ifa of type AF_LINK for this purpose. */ if_alloc_sadl(ifp); + refcnt_init_trace(&sc->sc_ifa.ifa_refcnt, DT_REFCNT_IDX_IFADDR); sc->sc_ifa.ifa_ifp = ifp; sc->sc_ifa.ifa_addr = sdltosa(ifp->if_sadl); sc->sc_ifa.ifa_netmask = NULL; @@ -152,6 +153,10 @@ enc_clone_destroy(struct ifnet *ifp) NET_UNLOCK(); if_detach(ifp); + if (refcnt_rele(&sc->sc_ifa.ifa_refcnt) == 0) { + panic("%s: ifa refcnt has %u refs", __func__, + sc->sc_ifa.ifa_refcnt.r_refs); + } free(sc, M_DEVBUF, sizeof(*sc)); return (0); diff --git a/sys/net/if_mpe.c b/sys/net/if_mpe.c index 8232b2f8209..7b84d24d5d7 100644 --- a/sys/net/if_mpe.c +++ b/sys/net/if_mpe.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if_mpe.c,v 1.101 2021/11/08 04:50:54 dlg Exp $ */ +/* $OpenBSD: if_mpe.c,v 1.102 2022/08/29 07:51:45 bluhm Exp $ */ /* * Copyright (c) 2008 Pierre-Yves Ritschard @@ -128,6 +128,7 @@ mpe_clone_create(struct if_clone *ifc, int unit) sc->sc_txhprio = 0; sc->sc_rxhprio = IF_HDRPRIO_PACKET; sc->sc_rdomain = 0; + refcnt_init_trace(&sc->sc_ifa.ifa_refcnt, DT_REFCNT_IDX_IFADDR); sc->sc_ifa.ifa_ifp = ifp; sc->sc_ifa.ifa_addr = sdltosa(ifp->if_sadl); sc->sc_smpls.smpls_len = sizeof(sc->sc_smpls); @@ -154,6 +155,10 @@ mpe_clone_destroy(struct ifnet *ifp) ifq_barrier(&ifp->if_snd); if_detach(ifp); + if (refcnt_rele(&sc->sc_ifa.ifa_refcnt) == 0) { + panic("%s: ifa refcnt has %u refs", __func__, + sc->sc_ifa.ifa_refcnt.r_refs); + } free(sc, M_DEVBUF, sizeof *sc); return (0); } diff --git a/sys/net/if_mpip.c b/sys/net/if_mpip.c index a8daeeea314..4fefa7580f2 100644 --- a/sys/net/if_mpip.c +++ b/sys/net/if_mpip.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if_mpip.c,v 1.15 2021/03/26 19:00:21 kn Exp $ */ +/* $OpenBSD: if_mpip.c,v 1.16 2022/08/29 07:51:45 bluhm Exp $ */ /* * Copyright (c) 2015 Rafael Zalamena @@ -128,6 +128,7 @@ mpip_clone_create(struct if_clone *ifc, int unit) bpfattach(&ifp->if_bpf, ifp, DLT_LOOP, sizeof(uint32_t)); #endif + refcnt_init_trace(&sc->sc_ifa.ifa_refcnt, DT_REFCNT_IDX_IFADDR); sc->sc_ifa.ifa_ifp = ifp; sc->sc_ifa.ifa_addr = sdltosa(ifp->if_sadl); @@ -152,7 +153,10 @@ mpip_clone_destroy(struct ifnet *ifp) ifq_barrier(&ifp->if_snd); if_detach(ifp); - + if (refcnt_rele(&sc->sc_ifa.ifa_refcnt) == 0) { + panic("%s: ifa refcnt has %u refs", __func__, + sc->sc_ifa.ifa_refcnt.r_refs); + } free(sc->sc_neighbor, M_DEVBUF, sizeof(*sc->sc_neighbor)); free(sc, M_DEVBUF, sizeof(*sc)); diff --git a/sys/net/if_mpw.c b/sys/net/if_mpw.c index f17693dc766..2106c3e6819 100644 --- a/sys/net/if_mpw.c +++ b/sys/net/if_mpw.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if_mpw.c,v 1.62 2021/03/26 19:00:21 kn Exp $ */ +/* $OpenBSD: if_mpw.c,v 1.63 2022/08/29 07:51:45 bluhm Exp $ */ /* * Copyright (c) 2015 Rafael Zalamena @@ -122,6 +122,7 @@ mpw_clone_create(struct if_clone *ifc, int unit) sc->sc_txhprio = 0; sc->sc_rxhprio = IF_HDRPRIO_PACKET; sc->sc_rdomain = 0; + refcnt_init_trace(&sc->sc_ifa.ifa_refcnt, DT_REFCNT_IDX_IFADDR); sc->sc_ifa.ifa_ifp = ifp; sc->sc_ifa.ifa_addr = sdltosa(ifp->if_sadl); sc->sc_smpls.smpls_len = sizeof(sc->sc_smpls); @@ -149,7 +150,10 @@ mpw_clone_destroy(struct ifnet *ifp) ether_ifdetach(ifp); if_detach(ifp); - + if (refcnt_rele(&sc->sc_ifa.ifa_refcnt) == 0) { + panic("%s: ifa refcnt has %u refs", __func__, + sc->sc_ifa.ifa_refcnt.r_refs); + } free(sc->sc_neighbor, M_DEVBUF, sizeof(*sc->sc_neighbor)); free(sc, M_DEVBUF, sizeof(*sc)); diff --git a/sys/net/if_pppx.c b/sys/net/if_pppx.c index fd099d81ff0..953524b3a31 100644 --- a/sys/net/if_pppx.c +++ b/sys/net/if_pppx.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if_pppx.c,v 1.121 2022/07/25 08:29:26 mvs Exp $ */ +/* $OpenBSD: if_pppx.c,v 1.122 2022/08/29 07:51:45 bluhm Exp $ */ /* * Copyright (c) 2010 Claudio Jeker @@ -659,6 +659,7 @@ pppx_add_session(struct pppx_dev *pxd, struct pipex_session_req *req) ifaddr.sin_addr = req->pr_ip_srcaddr; ia = malloc(sizeof (*ia), M_IFADDR, M_WAITOK | M_ZERO); + refcnt_init_trace(&ia->ia_ifa.ifa_refcnt, DT_REFCNT_IDX_IFADDR); ia->ia_addr.sin_family = AF_INET; ia->ia_addr.sin_len = sizeof(struct sockaddr_in); diff --git a/sys/net/if_var.h b/sys/net/if_var.h index 400afd319b0..394398c5c5c 100644 --- a/sys/net/if_var.h +++ b/sys/net/if_var.h @@ -1,4 +1,4 @@ -/* $OpenBSD: if_var.h,v 1.114 2021/02/20 04:55:52 dlg Exp $ */ +/* $OpenBSD: if_var.h,v 1.115 2022/08/29 07:51:45 bluhm Exp $ */ /* $NetBSD: if.h,v 1.23 1996/05/07 02:40:27 thorpej Exp $ */ /* @@ -242,7 +242,7 @@ struct ifaddr { struct ifnet *ifa_ifp; /* back-pointer to interface */ TAILQ_ENTRY(ifaddr) ifa_list; /* list of addresses for interface */ u_int ifa_flags; /* interface flags, see below */ - u_int ifa_refcnt; /* number of `rt_ifa` references */ + struct refcnt ifa_refcnt; /* number of `rt_ifa` references */ int ifa_metric; /* cost of going out this interface */ }; @@ -333,6 +333,7 @@ int p2p_bpf_mtap(caddr_t, const struct mbuf *, u_int); struct ifaddr *ifa_ifwithaddr(struct sockaddr *, u_int); struct ifaddr *ifa_ifwithdstaddr(struct sockaddr *, u_int); struct ifaddr *ifaof_ifpforaddr(struct sockaddr *, struct ifnet *); +struct ifaddr *ifaref(struct ifaddr *); void ifafree(struct ifaddr *); int if_isconnected(const struct ifnet *, unsigned int); diff --git a/sys/net/route.c b/sys/net/route.c index 7d1aeeca26a..6f5cb089aec 100644 --- a/sys/net/route.c +++ b/sys/net/route.c @@ -1,4 +1,4 @@ -/* $OpenBSD: route.c,v 1.413 2022/07/28 22:19:09 bluhm Exp $ */ +/* $OpenBSD: route.c,v 1.414 2022/08/29 07:51:45 bluhm Exp $ */ /* $NetBSD: route.c,v 1.14 1996/02/13 22:00:46 christos Exp $ */ /* @@ -146,7 +146,6 @@ extern unsigned int rtmap_limit; struct cpumem * rtcounters; int rttrash; /* routes not in table but not freed */ -int ifatrash; /* ifas not in ifp list but not free */ struct pool rtentry_pool; /* pool for rtentry structures */ struct pool rttimer_pool; /* pool for rttimer structures */ @@ -512,16 +511,19 @@ rtfree(struct rtentry *rt) pool_put(&rtentry_pool, rt); } +struct ifaddr * +ifaref(struct ifaddr *ifa) +{ + refcnt_take(&ifa->ifa_refcnt); + return ifa; +} + void ifafree(struct ifaddr *ifa) { - if (ifa == NULL) - panic("ifafree"); - if (ifa->ifa_refcnt == 0) { - ifatrash--; - free(ifa, M_IFADDR, 0); - } else - ifa->ifa_refcnt--; + if (refcnt_rele(&ifa->ifa_refcnt) == 0) + return; + free(ifa, M_IFADDR, 0); } /* @@ -901,8 +903,7 @@ rtrequest(int req, struct rt_addrinfo *info, u_int8_t prio, rt_mpls_clear(rt); #endif - ifa->ifa_refcnt++; - rt->rt_ifa = ifa; + rt->rt_ifa = ifaref(ifa); rt->rt_ifidx = ifp->if_index; /* * Copy metrics and a back pointer from the cloned @@ -1857,8 +1858,8 @@ db_print_ifa(struct ifaddr *ifa) db_print_sa(ifa->ifa_dstaddr); db_printf(" ifa_mask="); db_print_sa(ifa->ifa_netmask); - db_printf(" flags=0x%x, refcnt=%d, metric=%d\n", - ifa->ifa_flags, ifa->ifa_refcnt, ifa->ifa_metric); + db_printf(" flags=0x%x, refcnt=%u, metric=%d\n", + ifa->ifa_flags, ifa->ifa_refcnt.r_refs, ifa->ifa_metric); } /* diff --git a/sys/net/rtsock.c b/sys/net/rtsock.c index 5e54c1c94fd..c7476a5b359 100644 --- a/sys/net/rtsock.c +++ b/sys/net/rtsock.c @@ -1,4 +1,4 @@ -/* $OpenBSD: rtsock.c,v 1.346 2022/08/28 21:35:12 mvs Exp $ */ +/* $OpenBSD: rtsock.c,v 1.347 2022/08/29 07:51:45 bluhm Exp $ */ /* $NetBSD: rtsock.c,v 1.18 1996/03/29 00:32:10 cgd Exp $ */ /* @@ -1145,8 +1145,7 @@ rtm_output(struct rt_msghdr *rtm, struct rtentry **prt, ifp->if_rtrequest(ifp, RTM_DELETE, rt); ifafree(rt->rt_ifa); - ifa->ifa_refcnt++; - rt->rt_ifa = ifa; + rt->rt_ifa = ifaref(ifa); rt->rt_ifidx = ifa->ifa_ifp->if_index; /* recheck link state after ifp change */ rt_if_linkstate_change(rt, ifa->ifa_ifp, diff --git a/sys/netinet/in.c b/sys/netinet/in.c index 2ff41a56f9a..70bfd7e5299 100644 --- a/sys/netinet/in.c +++ b/sys/netinet/in.c @@ -1,4 +1,4 @@ -/* $OpenBSD: in.c,v 1.175 2022/08/06 15:57:59 bluhm Exp $ */ +/* $OpenBSD: in.c,v 1.176 2022/08/29 07:51:45 bluhm Exp $ */ /* $NetBSD: in.c,v 1.26 1996/02/13 23:41:39 christos Exp $ */ /* @@ -379,6 +379,7 @@ in_ioctl_set_ifaddr(u_long cmd, caddr_t data, struct ifnet *ifp, } if (ia == NULL) { ia = malloc(sizeof *ia, M_IFADDR, M_WAITOK | M_ZERO); + refcnt_init_trace(&ia->ia_ifa.ifa_refcnt, DT_REFCNT_IDX_IFADDR); ia->ia_addr.sin_family = AF_INET; ia->ia_addr.sin_len = sizeof(ia->ia_addr); ia->ia_ifa.ifa_addr = sintosa(&ia->ia_addr); @@ -475,6 +476,8 @@ in_ioctl_change_ifaddr(u_long cmd, caddr_t data, struct ifnet *ifp, if (ia == NULL) { ia = malloc(sizeof *ia, M_IFADDR, M_WAITOK | M_ZERO); + refcnt_init_trace(&ia->ia_ifa.ifa_refcnt, + DT_REFCNT_IDX_IFADDR); ia->ia_addr.sin_family = AF_INET; ia->ia_addr.sin_len = sizeof(ia->ia_addr); ia->ia_ifa.ifa_addr = sintosa(&ia->ia_addr); @@ -745,7 +748,6 @@ in_purgeaddr(struct ifaddr *ifa) { struct ifnet *ifp = ifa->ifa_ifp; struct in_ifaddr *ia = ifatoia(ifa); - extern int ifatrash; NET_ASSERT_LOCKED(); @@ -760,7 +762,6 @@ in_purgeaddr(struct ifaddr *ifa) ia->ia_allhosts = NULL; } - ifatrash++; ia->ia_ifp = NULL; ifafree(&ia->ia_ifa); } diff --git a/sys/netinet6/in6.c b/sys/netinet6/in6.c index 20ff9213535..65ad1687e3c 100644 --- a/sys/netinet6/in6.c +++ b/sys/netinet6/in6.c @@ -1,4 +1,4 @@ -/* $OpenBSD: in6.c,v 1.247 2022/08/06 15:57:59 bluhm Exp $ */ +/* $OpenBSD: in6.c,v 1.248 2022/08/29 07:51:45 bluhm Exp $ */ /* $KAME: in6.c,v 1.372 2004/06/14 08:14:21 itojun Exp $ */ /* @@ -647,6 +647,8 @@ in6_update_ifa(struct ifnet *ifp, struct in6_aliasreq *ifra, if (ia6 == NULL) { hostIsNew = 1; ia6 = malloc(sizeof(*ia6), M_IFADDR, M_WAITOK | M_ZERO); + refcnt_init_trace(&ia6->ia_ifa.ifa_refcnt, + DT_REFCNT_IDX_IFADDR); LIST_INIT(&ia6->ia6_memberships); /* Initialize the address and masks, and put time stamp */ ia6->ia_ifa.ifa_addr = sin6tosa(&ia6->ia_addr); @@ -938,7 +940,6 @@ void in6_unlink_ifa(struct in6_ifaddr *ia6, struct ifnet *ifp) { struct ifaddr *ifa = &ia6->ia_ifa; - extern int ifatrash; int plen; NET_ASSERT_LOCKED(); @@ -953,7 +954,6 @@ in6_unlink_ifa(struct in6_ifaddr *ia6, struct ifnet *ifp) rt_ifa_purge(ifa); ifa_del(ifp, ifa); - ifatrash++; ia6->ia_ifp = NULL; ifafree(&ia6->ia_ifa); } diff --git a/sys/netinet6/nd6_nbr.c b/sys/netinet6/nd6_nbr.c index 57433b47d41..3701ae49cf4 100644 --- a/sys/netinet6/nd6_nbr.c +++ b/sys/netinet6/nd6_nbr.c @@ -1,4 +1,4 @@ -/* $OpenBSD: nd6_nbr.c,v 1.132 2022/08/08 17:47:59 kn Exp $ */ +/* $OpenBSD: nd6_nbr.c,v 1.133 2022/08/29 07:51:45 bluhm Exp $ */ /* $KAME: nd6_nbr.c,v 1.61 2001/02/10 16:06:14 jinmei Exp $ */ /* @@ -1124,8 +1124,7 @@ nd6_dad_start(struct ifaddr *ifa) * first packet to be sent from the interface after interface * (re)initialization. */ - dp->dad_ifa = ifa; - ifa->ifa_refcnt++; /* just for safety */ + dp->dad_ifa = ifaref(ifa); dp->dad_count = ip6_dad_count; dp->dad_ns_icount = dp->dad_na_icount = 0; dp->dad_ns_ocount = dp->dad_ns_tcount = 0; diff --git a/sys/sys/refcnt.h b/sys/sys/refcnt.h index bb6e9686136..c6a44d6f85f 100644 --- a/sys/sys/refcnt.h +++ b/sys/sys/refcnt.h @@ -1,4 +1,4 @@ -/* $OpenBSD: refcnt.h,v 1.7 2022/06/28 09:32:28 bluhm Exp $ */ +/* $OpenBSD: refcnt.h,v 1.8 2022/08/29 07:51:45 bluhm Exp $ */ /* * Copyright (c) 2015 David Gwynne @@ -43,8 +43,10 @@ void refcnt_finalize(struct refcnt *, const char *); int refcnt_shared(struct refcnt *); unsigned int refcnt_read(struct refcnt *); -#define DT_REFCNT_IDX_INPCB 1 -#define DT_REFCNT_IDX_TDB 2 +/* sorted alphabetically, keep in sync with dev/dt/dt_prov_static.c */ +#define DT_REFCNT_IDX_IFADDR 1 +#define DT_REFCNT_IDX_INPCB 2 +#define DT_REFCNT_IDX_TDB 3 #endif /* _KERNEL */