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
This commit is contained in:
Robert Watson 2009-06-24 10:32:44 +00:00
parent 9087bd7925
commit 6c7ffe9340
5 changed files with 149 additions and 94 deletions

@ -261,14 +261,17 @@ ether_output(struct ifnet *ifp, struct mbuf *m,
if ((aa = at_ifawithnet((struct sockaddr_at *)dst)) == NULL) if ((aa = at_ifawithnet((struct sockaddr_at *)dst)) == NULL)
senderr(EHOSTUNREACH); /* XXX */ 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); return (0);
}
/* /*
* In the phase 2 case, need to prepend an mbuf for the llc header. * In the phase 2 case, need to prepend an mbuf for the llc header.
*/ */
if ( aa->aa_flags & AFA_PHASE2 ) { if ( aa->aa_flags & AFA_PHASE2 ) {
struct llc llc; struct llc llc;
ifa_free(&aa->aa_ifa);
M_PREPEND(m, LLC_SNAPFRAMELEN, M_DONTWAIT); M_PREPEND(m, LLC_SNAPFRAMELEN, M_DONTWAIT);
if (m == NULL) if (m == NULL)
senderr(ENOBUFS); senderr(ENOBUFS);
@ -280,6 +283,7 @@ ether_output(struct ifnet *ifp, struct mbuf *m,
type = htons(m->m_pkthdr.len); type = htons(m->m_pkthdr.len);
hlen = LLC_SNAPFRAMELEN + ETHER_HDR_LEN; hlen = LLC_SNAPFRAMELEN + ETHER_HDR_LEN;
} else { } else {
ifa_free(&aa->aa_ifa);
type = htons(ETHERTYPE_AT); type = htons(ETHERTYPE_AT);
} }
break; break;

@ -222,6 +222,7 @@ fddi_output(ifp, m, dst, ro)
} else { } else {
type = htons(ETHERTYPE_AT); type = htons(ETHERTYPE_AT);
} }
ifa_free(&aa->aa_ifa);
break; break;
} }
#endif /* NETATALK */ #endif /* NETATALK */

@ -142,9 +142,12 @@ aarptimer(void *ignored)
/* /*
* Search through the network addresses to find one that includes the given * Search through the network addresses to find one that includes the given
* network. Remember to take netranges into consideration. * 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 * struct at_ifaddr *
at_ifawithnet(struct sockaddr_at *sat) at_ifawithnet_locked(struct sockaddr_at *sat)
{ {
struct at_ifaddr *aa; struct at_ifaddr *aa;
struct sockaddr_at *sat2; struct sockaddr_at *sat2;
@ -163,6 +166,19 @@ at_ifawithnet(struct sockaddr_at *sat)
return (aa); 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 static void
aarpwhohas(struct ifnet *ifp, struct sockaddr_at *sat) aarpwhohas(struct ifnet *ifp, struct sockaddr_at *sat)
{ {
@ -201,9 +217,8 @@ aarpwhohas(struct ifnet *ifp, struct sockaddr_at *sat)
* same address as we're looking for. If the net is phase 2, * same address as we're looking for. If the net is phase 2,
* generate an 802.2 and SNAP header. * generate an 802.2 and SNAP header.
*/ */
AT_IFADDR_RLOCK(); aa = at_ifawithnet(sat);
if ((aa = at_ifawithnet(sat)) == NULL) { if (aa == NULL) {
AT_IFADDR_RUNLOCK();
m_freem(m); m_freem(m);
return; return;
} }
@ -217,7 +232,7 @@ aarpwhohas(struct ifnet *ifp, struct sockaddr_at *sat)
sizeof(struct ether_aarp)); sizeof(struct ether_aarp));
M_PREPEND(m, sizeof(struct llc), M_DONTWAIT); M_PREPEND(m, sizeof(struct llc), M_DONTWAIT);
if (m == NULL) { if (m == NULL) {
AT_IFADDR_RUNLOCK(); ifa_free(&aa->aa_ifa);
return; return;
} }
llc = mtod(m, struct llc *); llc = mtod(m, struct llc *);
@ -244,7 +259,7 @@ aarpwhohas(struct ifnet *ifp, struct sockaddr_at *sat)
printf("aarp: sending request for %u.%u\n", printf("aarp: sending request for %u.%u\n",
ntohs(AA_SAT(aa)->sat_addr.s_net), AA_SAT(aa)->sat_addr.s_node); ntohs(AA_SAT(aa)->sat_addr.s_net), AA_SAT(aa)->sat_addr.s_node);
#endif /* NETATALKDEBUG */ #endif /* NETATALKDEBUG */
AT_IFADDR_RUNLOCK(); ifa_free(&aa->aa_ifa);
sa.sa_len = sizeof(struct sockaddr); sa.sa_len = sizeof(struct sockaddr);
sa.sa_family = AF_UNSPEC; sa.sa_family = AF_UNSPEC;
@ -261,7 +276,7 @@ aarpresolve(struct ifnet *ifp, struct mbuf *m, struct sockaddr_at *destsat,
AT_IFADDR_RLOCK(); AT_IFADDR_RLOCK();
if (at_broadcast(destsat)) { if (at_broadcast(destsat)) {
m->m_flags |= M_BCAST; m->m_flags |= M_BCAST;
if ((aa = at_ifawithnet(destsat)) == NULL) { if ((aa = at_ifawithnet_locked(destsat)) == NULL) {
AT_IFADDR_RUNLOCK(); AT_IFADDR_RUNLOCK();
m_freem(m); m_freem(m);
return (0); return (0);
@ -379,14 +394,11 @@ at_aarpinput(struct ifnet *ifp, struct mbuf *m)
sat.sat_len = sizeof(struct sockaddr_at); sat.sat_len = sizeof(struct sockaddr_at);
sat.sat_family = AF_APPLETALK; sat.sat_family = AF_APPLETALK;
sat.sat_addr.s_net = net; sat.sat_addr.s_net = net;
AT_IFADDR_RLOCK(); aa = at_ifawithnet(&sat);
if ((aa = at_ifawithnet(&sat)) == NULL) { if (aa == NULL) {
AT_IFADDR_RUNLOCK();
m_freem(m); m_freem(m);
return; return;
} }
ifa_ref(&aa->aa_ifa);
AT_IFADDR_RUNLOCK();
bcopy(ea->aarp_spnet, &spa.s_net, sizeof(spa.s_net)); bcopy(ea->aarp_spnet, &spa.s_net, sizeof(spa.s_net));
bcopy(ea->aarp_tpnet, &tpa.s_net, sizeof(tpa.s_net)); bcopy(ea->aarp_tpnet, &tpa.s_net, sizeof(tpa.s_net));
} else { } else {

@ -79,28 +79,32 @@ at_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp,
struct sockaddr_at *sat; struct sockaddr_at *sat;
struct netrange *nr; struct netrange *nr;
struct at_aliasreq *ifra = (struct at_aliasreq *)data; struct at_aliasreq *ifra = (struct at_aliasreq *)data;
struct at_ifaddr *aa0; struct at_ifaddr *aa_temp;
struct at_ifaddr *aa = NULL; struct at_ifaddr *aa;
struct ifaddr *ifa, *ifa0; struct ifaddr *ifa, *ifa0;
int error; int error;
/* /*
* If we have an ifp, then find the matching at_ifaddr if it exists * 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) { if (ifp != NULL) {
for (aa = at_ifaddr_list; aa != NULL; aa = aa->aa_next) { for (aa = at_ifaddr_list; aa != NULL; aa = aa->aa_next) {
if (aa->aa_ifp == ifp) if (aa->aa_ifp == ifp)
break; break;
} }
} }
if (aa != NULL)
ifa_ref(&aa->aa_ifa);
AT_IFADDR_RUNLOCK();
/* /*
* In this first switch table we are basically getting ready for * In this first switch table we are basically getting ready for
* the second one, by getting the atalk-specific things set up * the second one, by getting the atalk-specific things set up
* so that they start to look more similar to other protocols etc. * so that they start to look more similar to other protocols etc.
*/ */
error = 0;
switch (cmd) { switch (cmd) {
case SIOCAIFADDR: case SIOCAIFADDR:
case SIOCDIFADDR: case SIOCDIFADDR:
@ -111,19 +115,27 @@ at_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp,
* the first address on the NEXT interface! * the first address on the NEXT interface!
*/ */
if (ifra->ifra_addr.sat_family == AF_APPLETALK) { 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 && if (aa->aa_ifp == ifp &&
sateqaddr(&aa->aa_addr, &ifra->ifra_addr)) sateqaddr(&aa->aa_addr, &ifra->ifra_addr))
break; 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, * If we a retrying to delete an addres but didn't find such,
* then rewurn with an error * then rewurn with an error
*/ */
if (cmd == SIOCDIFADDR && aa == NULL) { if (cmd == SIOCDIFADDR && aa == NULL) {
AT_IFADDR_WUNLOCK(); error = EADDRNOTAVAIL;
return (EADDRNOTAVAIL); goto out;
} }
/*FALLTHROUGH*/ /*FALLTHROUGH*/
@ -134,34 +146,50 @@ at_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp,
* XXXRW: Layering? * XXXRW: Layering?
*/ */
if (priv_check(td, PRIV_NET_ADDIFADDR)) { if (priv_check(td, PRIV_NET_ADDIFADDR)) {
AT_IFADDR_WUNLOCK(); error = EPERM;
return (EPERM); goto out;
} }
sat = satosat(&ifr->ifr_addr); sat = satosat(&ifr->ifr_addr);
nr = (struct netrange *)sat->sat_zero; nr = (struct netrange *)sat->sat_zero;
if (nr->nr_phase == 1) { if (nr->nr_phase == 1) {
struct at_ifaddr *oaa;
/* /*
* Look for a phase 1 address on this interface. * Look for a phase 1 address on this interface.
* This may leave aa pointing to the first address on * This may leave aa pointing to the first address on
* the NEXT interface! * 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 && if (aa->aa_ifp == ifp &&
(aa->aa_flags & AFA_PHASE2) == 0) (aa->aa_flags & AFA_PHASE2) == 0)
break; 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 */ } else { /* default to phase 2 */
struct at_ifaddr *oaa;
/* /*
* Look for a phase 2 address on this interface. * Look for a phase 2 address on this interface.
* This may leave aa pointing to the first address on * This may leave aa pointing to the first address on
* the NEXT interface! * 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 & if (aa->aa_ifp == ifp && (aa->aa_flags &
AFA_PHASE2)) AFA_PHASE2))
break; 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) if (ifp == NULL)
@ -172,45 +200,20 @@ at_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp,
* allocate a fresh one. * allocate a fresh one.
*/ */
if (aa == NULL) { if (aa == NULL) {
aa0 = malloc(sizeof(struct at_ifaddr), M_IFADDR, aa = malloc(sizeof(struct at_ifaddr), M_IFADDR,
M_NOWAIT | M_ZERO); M_NOWAIT | M_ZERO);
if (aa0 == NULL) { if (aa == NULL) {
AT_IFADDR_WUNLOCK(); error = ENOBUFS;
return (ENOBUFS); goto out;
} }
callout_init(&aa0->aa_callout, CALLOUT_MPSAFE); callout_init(&aa->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;
/*
* Find the end of the interface's addresses
* and link our new one on the end
*/
ifa = (struct ifaddr *)aa; ifa = (struct ifaddr *)aa;
ifa_init(ifa); ifa_init(ifa);
/* /*
* As the at_ifaddr contains the actual sockaddrs, * 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. * correctly.
*/ */
ifa->ifa_addr = (struct sockaddr *)&aa->aa_addr; ifa->ifa_addr = (struct sockaddr *)&aa->aa_addr;
@ -225,10 +228,35 @@ at_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp,
else else
aa->aa_flags |= AFA_PHASE2; 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 * and link it all together
*/ */
aa->aa_ifp = ifp; aa->aa_ifp = ifp;
ifa_ref(&aa->aa_ifa); /* if_addrhead */
IF_ADDR_LOCK(ifp); IF_ADDR_LOCK(ifp);
TAILQ_INSERT_TAIL(&ifp->if_addrhead, ifa, ifa_link); TAILQ_INSERT_TAIL(&ifp->if_addrhead, ifa, ifa_link);
IF_ADDR_UNLOCK(ifp); IF_ADDR_UNLOCK(ifp);
@ -236,15 +264,8 @@ at_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp,
/* /*
* If we DID find one then we clobber any routes * If we DID find one then we clobber any routes
* dependent on it.. * 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_scrub(ifp, aa);
AT_IFADDR_WLOCK();
ifa_free(&aa->aa_ifa);
} }
break; break;
@ -252,29 +273,45 @@ at_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp,
sat = satosat(&ifr->ifr_addr); sat = satosat(&ifr->ifr_addr);
nr = (struct netrange *)sat->sat_zero; nr = (struct netrange *)sat->sat_zero;
if (nr->nr_phase == 1) { if (nr->nr_phase == 1) {
struct at_ifaddr *oaa;
/* /*
* If the request is specifying phase 1, then * If the request is specifying phase 1, then
* only look at a phase one address * 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 && if (aa->aa_ifp == ifp &&
(aa->aa_flags & AFA_PHASE2) == 0) (aa->aa_flags & AFA_PHASE2) == 0)
break; 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 { } else {
struct at_ifaddr *oaa;
/* /*
* default to phase 2 * 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 & if (aa->aa_ifp == ifp && (aa->aa_flags &
AFA_PHASE2)) AFA_PHASE2))
break; 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) { if (aa == NULL) {
AT_IFADDR_WUNLOCK(); error = EADDRNOTAVAIL;
return (EADDRNOTAVAIL); goto out;
} }
break; break;
} }
@ -301,30 +338,24 @@ at_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp,
aa->aa_firstnet; aa->aa_firstnet;
((struct netrange *)&sat->sat_zero)->nr_lastnet = ((struct netrange *)&sat->sat_zero)->nr_lastnet =
aa->aa_lastnet; aa->aa_lastnet;
AT_IFADDR_WUNLOCK();
break; break;
case SIOCSIFADDR: case SIOCSIFADDR:
ifa_ref(&aa->aa_ifa);
AT_IFADDR_WUNLOCK();
error = at_ifinit(ifp, aa, error = at_ifinit(ifp, aa,
(struct sockaddr_at *)&ifr->ifr_addr); (struct sockaddr_at *)&ifr->ifr_addr);
ifa_free(&aa->aa_ifa); goto out;
return (error);
case SIOCAIFADDR: case SIOCAIFADDR:
if (sateqaddr(&ifra->ifra_addr, &aa->aa_addr)) { if (sateqaddr(&ifra->ifra_addr, &aa->aa_addr)) {
AT_IFADDR_WUNLOCK(); error = 0;
return (0); goto out;
} }
ifa_ref(&aa->aa_ifa);
AT_IFADDR_WUNLOCK();
error = at_ifinit(ifp, aa, error = at_ifinit(ifp, aa,
(struct sockaddr_at *)&ifr->ifr_addr); (struct sockaddr_at *)&ifr->ifr_addr);
ifa_free(&aa->aa_ifa); goto out;
return (error);
case SIOCDIFADDR: case SIOCDIFADDR:
/* /*
* remove the ifaddr from the interface * remove the ifaddr from the interface
*/ */
@ -332,41 +363,46 @@ at_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp,
IF_ADDR_LOCK(ifp); IF_ADDR_LOCK(ifp);
TAILQ_REMOVE(&ifp->if_addrhead, ifa0, ifa_link); TAILQ_REMOVE(&ifp->if_addrhead, ifa0, ifa_link);
IF_ADDR_UNLOCK(ifp); IF_ADDR_UNLOCK(ifp);
ifa_free(ifa0); /* if_addrhead */
/* /*
* Now remove the at_ifaddr from the parallel structure * Now remove the at_ifaddr from the parallel structure
* as well, or we'd be in deep trouble * 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; at_ifaddr_list = aa->aa_next;
} else { } else {
while (aa->aa_next && (aa->aa_next != aa0)) while (aa_temp->aa_next && (aa_temp->aa_next != aa))
aa = aa->aa_next; 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) if (aa_temp->aa_next)
aa->aa_next = aa0->aa_next; aa_temp->aa_next = aa->aa_next;
else else
panic("at_control"); panic("at_control");
} }
AT_IFADDR_WUNLOCK(); AT_IFADDR_WUNLOCK();
ifa_free(ifa0); /* at_ifaddr_list */
/* aa = aa_temp;
* Now reclaim the reference.
*/
ifa_free(ifa0);
break; break;
default: default:
AT_IFADDR_WUNLOCK(); if (ifp == NULL || ifp->if_ioctl == NULL) {
if (ifp == NULL || ifp->if_ioctl == NULL) error = EOPNOTSUPP;
return (EOPNOTSUPP); goto out;
return ((*ifp->if_ioctl)(ifp, cmd, data)); }
error = ((*ifp->if_ioctl)(ifp, cmd, data));
} }
return (0);
out:
if (aa != NULL)
ifa_free(&aa->aa_ifa);
return (error);
} }
/* /*

@ -55,6 +55,8 @@ u_short at_cksum(struct mbuf *m, int skip);
int at_control(struct socket *so, u_long cmd, caddr_t data, int at_control(struct socket *so, u_long cmd, caddr_t data,
struct ifnet *ifp, struct thread *td); struct ifnet *ifp, struct thread *td);
struct at_ifaddr *at_ifawithnet(struct sockaddr_at *); struct at_ifaddr *at_ifawithnet(struct sockaddr_at *);
struct at_ifaddr *at_ifawithnet_locked(struct sockaddr_at *sat);
int at_inithead(void**, int); int at_inithead(void**, int);
void ddp_init(void); void ddp_init(void);
int ddp_output(struct mbuf *m, struct socket *so); int ddp_output(struct mbuf *m, struct socket *so);