Date: Wed, 24 Jun 2009 10:32:44 +0000 (UTC) From: Robert Watson <rwatson@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r194819 - in head/sys: net netatalk Message-ID: <200906241032.n5OAWimu009142@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: rwatson Date: Wed Jun 24 10:32:44 2009 New Revision: 194819 URL: http://svn.freebsd.org/changeset/base/194819 Log: Break at_ifawithnet() into two variants: - at_ifawithnet(), which acquires an locks it needs and returns an at_ifaddr reference. - at_ifawithnet_locked(), which relies on the caller locking at_ifaddr_list, and returns a pointer rather than a reference. Update various consumers to prefer one or the other, including ether and fddi output, to properly release at_ifaddr references. Rework at_control() to manage locking and references in a manner identical to in_control(). MFC after: 6 weeks Modified: head/sys/net/if_ethersubr.c head/sys/net/if_fddisubr.c head/sys/netatalk/aarp.c head/sys/netatalk/at_control.c head/sys/netatalk/at_extern.h Modified: head/sys/net/if_ethersubr.c ============================================================================== --- head/sys/net/if_ethersubr.c Wed Jun 24 10:28:30 2009 (r194818) +++ head/sys/net/if_ethersubr.c Wed Jun 24 10:32:44 2009 (r194819) @@ -261,14 +261,17 @@ ether_output(struct ifnet *ifp, struct m if ((aa = at_ifawithnet((struct sockaddr_at *)dst)) == NULL) senderr(EHOSTUNREACH); /* XXX */ - if (!aarpresolve(ifp, m, (struct sockaddr_at *)dst, edst)) + if (!aarpresolve(ifp, m, (struct sockaddr_at *)dst, edst)) { + ifa_free(&aa->aa_ifa); return (0); + } /* * In the phase 2 case, need to prepend an mbuf for the llc header. */ if ( aa->aa_flags & AFA_PHASE2 ) { struct llc llc; + ifa_free(&aa->aa_ifa); M_PREPEND(m, LLC_SNAPFRAMELEN, M_DONTWAIT); if (m == NULL) senderr(ENOBUFS); @@ -280,6 +283,7 @@ ether_output(struct ifnet *ifp, struct m type = htons(m->m_pkthdr.len); hlen = LLC_SNAPFRAMELEN + ETHER_HDR_LEN; } else { + ifa_free(&aa->aa_ifa); type = htons(ETHERTYPE_AT); } break; Modified: head/sys/net/if_fddisubr.c ============================================================================== --- head/sys/net/if_fddisubr.c Wed Jun 24 10:28:30 2009 (r194818) +++ head/sys/net/if_fddisubr.c Wed Jun 24 10:32:44 2009 (r194819) @@ -222,6 +222,7 @@ fddi_output(ifp, m, dst, ro) } else { type = htons(ETHERTYPE_AT); } + ifa_free(&aa->aa_ifa); break; } #endif /* NETATALK */ Modified: head/sys/netatalk/aarp.c ============================================================================== --- head/sys/netatalk/aarp.c Wed Jun 24 10:28:30 2009 (r194818) +++ head/sys/netatalk/aarp.c Wed Jun 24 10:32:44 2009 (r194819) @@ -142,9 +142,12 @@ aarptimer(void *ignored) /* * Search through the network addresses to find one that includes the given * network. Remember to take netranges into consideration. + * + * The _locked variant relies on the caller holding the at_ifaddr lock; the + * unlocked variant returns a reference that the caller must dispose of. */ struct at_ifaddr * -at_ifawithnet(struct sockaddr_at *sat) +at_ifawithnet_locked(struct sockaddr_at *sat) { struct at_ifaddr *aa; struct sockaddr_at *sat2; @@ -163,6 +166,19 @@ at_ifawithnet(struct sockaddr_at *sat) return (aa); } +struct at_ifaddr * +at_ifawithnet(struct sockaddr_at *sat) +{ + struct at_ifaddr *aa; + + AT_IFADDR_RLOCK(); + aa = at_ifawithnet_locked(sat); + if (aa != NULL) + ifa_ref(&aa->aa_ifa); + AT_IFADDR_RUNLOCK(); + return (aa); +} + static void aarpwhohas(struct ifnet *ifp, struct sockaddr_at *sat) { @@ -201,9 +217,8 @@ aarpwhohas(struct ifnet *ifp, struct soc * same address as we're looking for. If the net is phase 2, * generate an 802.2 and SNAP header. */ - AT_IFADDR_RLOCK(); - if ((aa = at_ifawithnet(sat)) == NULL) { - AT_IFADDR_RUNLOCK(); + aa = at_ifawithnet(sat); + if (aa == NULL) { m_freem(m); return; } @@ -217,7 +232,7 @@ aarpwhohas(struct ifnet *ifp, struct soc sizeof(struct ether_aarp)); M_PREPEND(m, sizeof(struct llc), M_DONTWAIT); if (m == NULL) { - AT_IFADDR_RUNLOCK(); + ifa_free(&aa->aa_ifa); return; } llc = mtod(m, struct llc *); @@ -244,7 +259,7 @@ aarpwhohas(struct ifnet *ifp, struct soc printf("aarp: sending request for %u.%u\n", ntohs(AA_SAT(aa)->sat_addr.s_net), AA_SAT(aa)->sat_addr.s_node); #endif /* NETATALKDEBUG */ - AT_IFADDR_RUNLOCK(); + ifa_free(&aa->aa_ifa); sa.sa_len = sizeof(struct sockaddr); sa.sa_family = AF_UNSPEC; @@ -261,7 +276,7 @@ aarpresolve(struct ifnet *ifp, struct mb AT_IFADDR_RLOCK(); if (at_broadcast(destsat)) { m->m_flags |= M_BCAST; - if ((aa = at_ifawithnet(destsat)) == NULL) { + if ((aa = at_ifawithnet_locked(destsat)) == NULL) { AT_IFADDR_RUNLOCK(); m_freem(m); return (0); @@ -379,14 +394,11 @@ at_aarpinput(struct ifnet *ifp, struct m sat.sat_len = sizeof(struct sockaddr_at); sat.sat_family = AF_APPLETALK; sat.sat_addr.s_net = net; - AT_IFADDR_RLOCK(); - if ((aa = at_ifawithnet(&sat)) == NULL) { - AT_IFADDR_RUNLOCK(); + aa = at_ifawithnet(&sat); + if (aa == NULL) { m_freem(m); return; } - ifa_ref(&aa->aa_ifa); - AT_IFADDR_RUNLOCK(); bcopy(ea->aarp_spnet, &spa.s_net, sizeof(spa.s_net)); bcopy(ea->aarp_tpnet, &tpa.s_net, sizeof(tpa.s_net)); } else { Modified: head/sys/netatalk/at_control.c ============================================================================== --- head/sys/netatalk/at_control.c Wed Jun 24 10:28:30 2009 (r194818) +++ head/sys/netatalk/at_control.c Wed Jun 24 10:32:44 2009 (r194819) @@ -79,28 +79,32 @@ at_control(struct socket *so, u_long cmd struct sockaddr_at *sat; struct netrange *nr; struct at_aliasreq *ifra = (struct at_aliasreq *)data; - struct at_ifaddr *aa0; - struct at_ifaddr *aa = NULL; + struct at_ifaddr *aa_temp; + struct at_ifaddr *aa; struct ifaddr *ifa, *ifa0; int error; /* * If we have an ifp, then find the matching at_ifaddr if it exists */ - AT_IFADDR_WLOCK(); + aa = NULL; + AT_IFADDR_RLOCK(); if (ifp != NULL) { for (aa = at_ifaddr_list; aa != NULL; aa = aa->aa_next) { if (aa->aa_ifp == ifp) break; } } + if (aa != NULL) + ifa_ref(&aa->aa_ifa); + AT_IFADDR_RUNLOCK(); /* * In this first switch table we are basically getting ready for * the second one, by getting the atalk-specific things set up * so that they start to look more similar to other protocols etc. */ - + error = 0; switch (cmd) { case SIOCAIFADDR: case SIOCDIFADDR: @@ -111,19 +115,27 @@ at_control(struct socket *so, u_long cmd * the first address on the NEXT interface! */ if (ifra->ifra_addr.sat_family == AF_APPLETALK) { - for (; aa; aa = aa->aa_next) { + struct at_ifaddr *oaa; + + AT_IFADDR_RLOCK(); + for (oaa = aa; aa; aa = aa->aa_next) { if (aa->aa_ifp == ifp && sateqaddr(&aa->aa_addr, &ifra->ifra_addr)) break; } + if (oaa != NULL && oaa != aa) + ifa_free(&oaa->aa_ifa); + if (aa != NULL && oaa != aa) + ifa_ref(&aa->aa_ifa); + AT_IFADDR_RUNLOCK(); } /* * If we a retrying to delete an addres but didn't find such, * then rewurn with an error */ if (cmd == SIOCDIFADDR && aa == NULL) { - AT_IFADDR_WUNLOCK(); - return (EADDRNOTAVAIL); + error = EADDRNOTAVAIL; + goto out; } /*FALLTHROUGH*/ @@ -134,34 +146,50 @@ at_control(struct socket *so, u_long cmd * XXXRW: Layering? */ if (priv_check(td, PRIV_NET_ADDIFADDR)) { - AT_IFADDR_WUNLOCK(); - return (EPERM); + error = EPERM; + goto out; } sat = satosat(&ifr->ifr_addr); nr = (struct netrange *)sat->sat_zero; if (nr->nr_phase == 1) { + struct at_ifaddr *oaa; + /* * Look for a phase 1 address on this interface. * This may leave aa pointing to the first address on * the NEXT interface! */ - for (; aa; aa = aa->aa_next) { + AT_IFADDR_RLOCK(); + for (oaa = aa; aa; aa = aa->aa_next) { if (aa->aa_ifp == ifp && (aa->aa_flags & AFA_PHASE2) == 0) break; } + if (oaa != NULL && oaa != aa) + ifa_free(&oaa->aa_ifa); + if (aa != NULL && oaa != aa) + ifa_ref(&aa->aa_ifa); + AT_IFADDR_RUNLOCK(); } else { /* default to phase 2 */ + struct at_ifaddr *oaa; + /* * Look for a phase 2 address on this interface. * This may leave aa pointing to the first address on * the NEXT interface! */ - for (; aa; aa = aa->aa_next) { + AT_IFADDR_RLOCK(); + for (oaa = aa; aa; aa = aa->aa_next) { if (aa->aa_ifp == ifp && (aa->aa_flags & AFA_PHASE2)) break; } + if (oaa != NULL && oaa != aa) + ifa_free(&oaa->aa_ifa); + if (aa != NULL && oaa != aa) + ifa_ref(&aa->aa_ifa); + AT_IFADDR_RUNLOCK(); } if (ifp == NULL) @@ -172,45 +200,20 @@ at_control(struct socket *so, u_long cmd * allocate a fresh one. */ if (aa == NULL) { - aa0 = malloc(sizeof(struct at_ifaddr), M_IFADDR, + aa = malloc(sizeof(struct at_ifaddr), M_IFADDR, M_NOWAIT | M_ZERO); - if (aa0 == NULL) { - AT_IFADDR_WUNLOCK(); - return (ENOBUFS); + if (aa == NULL) { + error = ENOBUFS; + goto out; } - callout_init(&aa0->aa_callout, CALLOUT_MPSAFE); - if ((aa = at_ifaddr_list) != NULL) { - /* - * Don't let the loopback be first, since the - * first address is the machine's default - * address for binding. If it is, stick - * ourself in front, otherwise go to the back - * of the list. - */ - if (at_ifaddr_list->aa_ifp->if_flags & - IFF_LOOPBACK) { - aa = aa0; - aa->aa_next = at_ifaddr_list; - at_ifaddr_list = aa; - } else { - for (; aa->aa_next; aa = aa->aa_next) - ; - aa->aa_next = aa0; - } - } else - at_ifaddr_list = aa0; - aa = aa0; + callout_init(&aa->aa_callout, CALLOUT_MPSAFE); - /* - * Find the end of the interface's addresses - * and link our new one on the end - */ ifa = (struct ifaddr *)aa; ifa_init(ifa); /* * As the at_ifaddr contains the actual sockaddrs, - * and the ifaddr itself, link them al together + * and the ifaddr itself, link them all together * correctly. */ ifa->ifa_addr = (struct sockaddr *)&aa->aa_addr; @@ -225,10 +228,35 @@ at_control(struct socket *so, u_long cmd else aa->aa_flags |= AFA_PHASE2; + ifa_ref(&aa->aa_ifa); /* at_ifaddr_list */ + AT_IFADDR_WLOCK(); + if ((aa_temp = at_ifaddr_list) != NULL) { + /* + * Don't let the loopback be first, since the + * first address is the machine's default + * address for binding. If it is, stick + * ourself in front, otherwise go to the back + * of the list. + */ + if (at_ifaddr_list->aa_ifp->if_flags & + IFF_LOOPBACK) { + aa->aa_next = at_ifaddr_list; + at_ifaddr_list = aa; + } else { + for (; aa_temp->aa_next; aa_temp = + aa_temp->aa_next) + ; + aa_temp->aa_next = aa; + } + } else + at_ifaddr_list = aa; + AT_IFADDR_WUNLOCK(); + /* * and link it all together */ aa->aa_ifp = ifp; + ifa_ref(&aa->aa_ifa); /* if_addrhead */ IF_ADDR_LOCK(ifp); TAILQ_INSERT_TAIL(&ifp->if_addrhead, ifa, ifa_link); IF_ADDR_UNLOCK(ifp); @@ -236,15 +264,8 @@ at_control(struct socket *so, u_long cmd /* * If we DID find one then we clobber any routes * dependent on it.. - * - * XXXRW: While we ref the ifaddr, there are - * potential races here still. */ - ifa_ref(&aa->aa_ifa); - AT_IFADDR_WUNLOCK(); at_scrub(ifp, aa); - AT_IFADDR_WLOCK(); - ifa_free(&aa->aa_ifa); } break; @@ -252,29 +273,45 @@ at_control(struct socket *so, u_long cmd sat = satosat(&ifr->ifr_addr); nr = (struct netrange *)sat->sat_zero; if (nr->nr_phase == 1) { + struct at_ifaddr *oaa; + /* * If the request is specifying phase 1, then * only look at a phase one address */ - for (; aa; aa = aa->aa_next) { + AT_IFADDR_RUNLOCK(); + for (oaa = aa; aa; aa = aa->aa_next) { if (aa->aa_ifp == ifp && (aa->aa_flags & AFA_PHASE2) == 0) break; } + if (oaa != NULL && oaa != aa) + ifa_free(&oaa->aa_ifa); + if (aa != NULL && oaa != aa) + ifa_ref(&aa->aa_ifa); + AT_IFADDR_RLOCK(); } else { + struct at_ifaddr *oaa; + /* * default to phase 2 */ - for (; aa; aa = aa->aa_next) { + AT_IFADDR_RLOCK(); + for (oaa = aa; aa; aa = aa->aa_next) { if (aa->aa_ifp == ifp && (aa->aa_flags & AFA_PHASE2)) break; } + if (oaa != NULL && oaa != aa) + ifa_free(&oaa->aa_ifa); + if (aa != NULL && oaa != aa) + ifa_ref(&aa->aa_ifa); + AT_IFADDR_RUNLOCK(); } if (aa == NULL) { - AT_IFADDR_WUNLOCK(); - return (EADDRNOTAVAIL); + error = EADDRNOTAVAIL; + goto out; } break; } @@ -301,30 +338,24 @@ at_control(struct socket *so, u_long cmd aa->aa_firstnet; ((struct netrange *)&sat->sat_zero)->nr_lastnet = aa->aa_lastnet; - AT_IFADDR_WUNLOCK(); break; case SIOCSIFADDR: - ifa_ref(&aa->aa_ifa); - AT_IFADDR_WUNLOCK(); error = at_ifinit(ifp, aa, (struct sockaddr_at *)&ifr->ifr_addr); - ifa_free(&aa->aa_ifa); - return (error); + goto out; case SIOCAIFADDR: if (sateqaddr(&ifra->ifra_addr, &aa->aa_addr)) { - AT_IFADDR_WUNLOCK(); - return (0); + error = 0; + goto out; } - ifa_ref(&aa->aa_ifa); - AT_IFADDR_WUNLOCK(); error = at_ifinit(ifp, aa, (struct sockaddr_at *)&ifr->ifr_addr); - ifa_free(&aa->aa_ifa); - return (error); + goto out; case SIOCDIFADDR: + /* * remove the ifaddr from the interface */ @@ -332,41 +363,46 @@ at_control(struct socket *so, u_long cmd IF_ADDR_LOCK(ifp); TAILQ_REMOVE(&ifp->if_addrhead, ifa0, ifa_link); IF_ADDR_UNLOCK(ifp); + ifa_free(ifa0); /* if_addrhead */ /* * Now remove the at_ifaddr from the parallel structure * as well, or we'd be in deep trouble */ - aa0 = aa; - if (aa0 == (aa = at_ifaddr_list)) { + + AT_IFADDR_WLOCK(); + if (aa == (aa_temp = at_ifaddr_list)) { at_ifaddr_list = aa->aa_next; } else { - while (aa->aa_next && (aa->aa_next != aa0)) - aa = aa->aa_next; + while (aa_temp->aa_next && (aa_temp->aa_next != aa)) + aa_temp = aa_temp->aa_next; /* - * if we found it, remove it, otherwise we screwed up. + * if we found it, remove it, otherwise we + * screwed up. */ - if (aa->aa_next) - aa->aa_next = aa0->aa_next; + if (aa_temp->aa_next) + aa_temp->aa_next = aa->aa_next; else panic("at_control"); } AT_IFADDR_WUNLOCK(); - - /* - * Now reclaim the reference. - */ - ifa_free(ifa0); + ifa_free(ifa0); /* at_ifaddr_list */ + aa = aa_temp; break; default: - AT_IFADDR_WUNLOCK(); - if (ifp == NULL || ifp->if_ioctl == NULL) - return (EOPNOTSUPP); - return ((*ifp->if_ioctl)(ifp, cmd, data)); + if (ifp == NULL || ifp->if_ioctl == NULL) { + error = EOPNOTSUPP; + goto out; + } + error = ((*ifp->if_ioctl)(ifp, cmd, data)); } - return (0); + +out: + if (aa != NULL) + ifa_free(&aa->aa_ifa); + return (error); } /* Modified: head/sys/netatalk/at_extern.h ============================================================================== --- head/sys/netatalk/at_extern.h Wed Jun 24 10:28:30 2009 (r194818) +++ head/sys/netatalk/at_extern.h Wed Jun 24 10:32:44 2009 (r194819) @@ -55,6 +55,8 @@ u_short at_cksum(struct mbuf *m, int s int at_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp, struct thread *td); struct at_ifaddr *at_ifawithnet(struct sockaddr_at *); +struct at_ifaddr *at_ifawithnet_locked(struct sockaddr_at *sat); + int at_inithead(void**, int); void ddp_init(void); int ddp_output(struct mbuf *m, struct socket *so);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200906241032.n5OAWimu009142>