Date: Sat, 23 Jul 2005 00:00:09 GMT From: Robert Watson <rwatson@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 80817 for review Message-ID: <200507230000.j6N009R2050416@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=80817 Change 80817 by rwatson@rwatson_zoo on 2005/07/22 23:59:30 Pass M_ZERO to MALLOC() when allocating multicast addresses. This isn't strictly necessary, but will avoid confusing when looking at the structure later in a debugger. Since we now allow M_NOWAIT to be used throughout the multicast code, rearrange the if_addmulti() code to no longer use the "detect race and recover" approach, but instead detect allocation failure and abort. This simplifies the logic quite a bit, and avoids having lots of memory floating around that may or may not need to be freed. Affected files ... .. //depot/projects/netsmp/src/sys/net/if.c#5 edit Differences ... ==== //depot/projects/netsmp/src/sys/net/if.c#5 (text+ko) ==== @@ -1834,9 +1834,10 @@ IF_ADDR_LOCK_ASSERT(ifp); - TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) + TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) { if (sa_equal(ifma->ifma_addr, sa)) break; + } return ifma; } @@ -1868,10 +1869,8 @@ struct ifmultiaddr *ifma; struct sockaddr *dupsa; - KASSERT(ifp != NULL, ("if_allocmulti: NULL ifp")); - KASSERT(sa != NULL, ("if_allocmulti: NULL sa")); - - MALLOC(ifma, struct ifmultiaddr *, sizeof *ifma, M_IFMADDR, mflags); + MALLOC(ifma, struct ifmultiaddr *, sizeof *ifma, M_IFMADDR, mflags | + M_ZERO); if (ifma == NULL) return (NULL); @@ -1883,23 +1882,24 @@ bcopy(sa, dupsa, sa->sa_len); ifma->ifma_addr = dupsa; - if (llsa != NULL) { - MALLOC(dupsa, struct sockaddr *, llsa->sa_len, M_IFMADDR, - mflags); - if (dupsa == NULL) { - FREE(ifma->ifma_addr, M_IFMADDR); - FREE(ifma, M_IFMADDR); - return (NULL); - } - bcopy(llsa, dupsa, llsa->sa_len); - ifma->ifma_lladdr = llsa; - } else - ifma->ifma_lladdr = NULL; - ifma->ifma_ifp = ifp; ifma->ifma_refcount = 1; ifma->ifma_protospec = NULL; + if (llsa == NULL) { + ifma->ifma_lladdr = NULL; + return (ifma); + } + + MALLOC(dupsa, struct sockaddr *, llsa->sa_len, M_IFMADDR, mflags); + if (dupsa == NULL) { + FREE(ifma->ifma_addr, M_IFMADDR); + FREE(ifma, M_IFMADDR); + return (NULL); + } + bcopy(llsa, dupsa, llsa->sa_len); + ifma->ifma_lladdr = dupsa; + return (ifma); } @@ -1946,15 +1946,13 @@ if_addmulti(struct ifnet *ifp, struct sockaddr *sa, struct ifmultiaddr **retifma) { - struct ifmultiaddr *ifma, *ll_ifma, *new_ifma, *new_ll_ifma; + struct ifmultiaddr *ifma, *ll_ifma; struct sockaddr *llsa; int error; /* * If the address is already present, return a new reference to it; - * otherwise, allocate storage and set up a new address. Since we - * release the interface lock, we have to check for races in which - * another thread may also have set up the same address. + * otherwise, allocate storage and set up a new address. */ IF_ADDR_LOCK(ifp); ifma = if_findmulti(ifp, sa); @@ -1965,69 +1963,59 @@ IF_ADDR_UNLOCK(ifp); return (0); } - IF_ADDR_UNLOCK(ifp); /* - * The address isn't already present; perform a link layer - * resolution if it's not already a link layer address, and allocate - * and set up the address. As of this point, if llsa is non-NULL, - * we must free the sockaddr if we don't need it. + * The address isn't already present; resolve the protocol address + * into a link layer address, and then look that up, bump its + * refcount or allocate an ifma for that also. If 'llsa' was + * returned, we will need to free it later. */ + llsa = NULL; + ll_ifma = NULL; if (ifp->if_resolvemulti != NULL) { error = ifp->if_resolvemulti(ifp, &llsa, sa); if (error) - return error; - } else - llsa = NULL; - - new_ifma = if_allocmulti(ifp, sa, llsa, M_NOWAIT); - if (new_ifma == NULL) { - if (llsa != NULL) - free(llsa, M_IFMADDR); - return (ENOMEM); + goto unlock_out; } - if (llsa != NULL) { - new_ll_ifma = if_allocmulti(ifp, llsa, NULL, M_NOWAIT); - if (new_ll_ifma == NULL) { - if_freemulti(new_ifma); - if (llsa != NULL) - free(llsa, M_IFMADDR); - return (ENOMEM); - } - } else - new_ll_ifma = NULL; /* gcc */ /* - * Now check to see if we lost the race, and continue inserting if - * not. Note that we need to separately consider the link layer - * and protocol layer multicast addresses. + * Allocate the new address. Don't hook it up yet, as we may also + * need to allocate a link layer multicast address. */ - IF_ADDR_LOCK(ifp); - ifma = if_findmulti(ifp, sa); - if (llsa != NULL) - ll_ifma = if_findmulti(ifp, llsa); - else - ll_ifma = NULL; /* gcc */ - - if (ifma != NULL) { - if_freemulti(new_ifma); - ifma->ifma_refcount++; - } else { - ifma = new_ifma; - TAILQ_INSERT_HEAD(&ifp->if_multiaddrs, ifma, ifma_link); + ifma = if_allocmulti(ifp, sa, llsa, M_NOWAIT); + if (ifma == NULL) { + error = ENOMEM; + goto free_llsa_out; } + /* + * If a link layer address is found, we'll need to see if it's + * already present in the address list, or allocate is as well. + * When this block finishes, the link layer address will be on the + * list. + */ if (llsa != NULL) { - if (ll_ifma != NULL) { - if_freemulti(new_ll_ifma); - ll_ifma->ifma_refcount++; - } else { - ll_ifma = new_ll_ifma; + ll_ifma = if_findmulti(ifp, llsa); + if (ll_ifma == NULL) { + ll_ifma = if_allocmulti(ifp, llsa, NULL, M_NOWAIT); + if (ll_ifma == NULL) { + if_freemulti(ifma); + error = ENOMEM; + goto free_llsa_out; + } TAILQ_INSERT_HEAD(&ifp->if_multiaddrs, ll_ifma, ifma_link); - } + } else + ll_ifma->ifma_refcount++; } + /* + * We now have a new multicast address, ifma, and possibly a new or + * referenced link layer address. Add the primary address to the + * ifnet address list. + */ + TAILQ_INSERT_HEAD(&ifp->if_multiaddrs, ifma, ifma_link); + if (retifma != NULL) *retifma = ifma; @@ -2052,7 +2040,16 @@ if (llsa != NULL) FREE(llsa, M_IFMADDR); + return (0); + +free_llsa_out: + if (llsa != NULL) + FREE(llsa, M_IFMADDR); + +unlock_out: + IF_ADDR_UNLOCK(ifp); + return (error); } /* @@ -2078,7 +2075,10 @@ } sa = ifma->ifma_lladdr; - ll_ifma = if_findmulti(ifp, sa); + if (sa != NULL) + ll_ifma = if_findmulti(ifp, sa); + else + ll_ifma = NULL; /* * XXXRW: How come we don't announce ll_ifma?
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200507230000.j6N009R2050416>