Skip to content

Commit

Permalink
MFC r343395:
Browse files Browse the repository at this point in the history 
Fix refcounting leaks in IPv6 MLD code leading to loss of IPv6
connectivity.

Looking at past changes in this area like r337866, some refcounting
bugs have been introduced, one by one. For example like calling
in6m_disconnect() and in6m_rele_locked() in mld_v1_process_group_timer()
where previously no disconnect nor refcount decrement was done.
Calling in6m_disconnect() when it shouldn't causes IPv6 solitation to no
longer work, because all the multicast addresses receiving the solitation
messages are now deleted from the network interface.

This patch reverts some recent changes while improving the MLD
refcounting and concurrency model after the MLD code was converted
to using EPOCH(9).

List changes:
- All CK_STAILQ_FOREACH() macros are now properly enclosed into
  EPOCH(9) sections. This simplifies assertion of locking inside
  in6m_ifmultiaddr_get_inm().
- Corrected bad use of in6m_disconnect() leading to loss of IPv6
  connectivity for MLD v1.
- Factored out checks for valid inm structure into
  in6m_ifmultiaddr_get_inm().

PR:			233535
Differential Revision:	https://reviews.freebsd.org/D18887
Reviewed by:		bz (net)
Tested by:		ae
Sponsored by:		Mellanox Technologies
  • Loading branch information
@hselasky
hselasky committed Feb 1, 2019
1 parent 913106d commit 69483a2
Show file tree
Hide file tree
Showing 5 changed files with 126 additions and 147 deletions.
29 changes: 4 additions & 25 deletions sys/netinet6/in6_ifattach.c
View file
Expand Up @@ -852,36 +852,15 @@ in6_tmpaddrtimer(void *arg)
static void
in6_purgemaddrs(struct ifnet *ifp)
{
struct in6_multi_head purgeinms;
struct in6_multi *inm;
struct ifmultiaddr *ifma, *next;
struct in6_multi_head inmh;

SLIST_INIT(&purgeinms);
SLIST_INIT(&inmh);
IN6_MULTI_LOCK();
IN6_MULTI_LIST_LOCK();
IF_ADDR_WLOCK(ifp);
/*
* Extract list of in6_multi associated with the detaching ifp
* which the PF_INET6 layer is about to release.
*/
restart:
CK_STAILQ_FOREACH_SAFE(ifma, &ifp->if_multiaddrs, ifma_link, next) {
if (ifma->ifma_addr->sa_family != AF_INET6 ||
ifma->ifma_protospec == NULL)
continue;
inm = (struct in6_multi *)ifma->ifma_protospec;
in6m_disconnect(inm);
in6m_rele_locked(&purgeinms, inm);
if (__predict_false(ifma6_restart)) {
ifma6_restart = false;
goto restart;
}
}
IF_ADDR_WUNLOCK(ifp);
mld_ifdetach(ifp);
mld_ifdetach(ifp, &inmh);
IN6_MULTI_LIST_UNLOCK();
IN6_MULTI_UNLOCK();
in6m_release_list_deferred(&purgeinms);
in6m_release_list_deferred(&inmh);

/*
* Make sure all multicast deletions invoking if_ioctl() are
Expand Down
60 changes: 28 additions & 32 deletions sys/netinet6/in6_mcast.c
View file
Expand Up @@ -190,7 +190,6 @@ static SYSCTL_NODE(_net_inet6_ip6_mcast, OID_AUTO, filters,
CTLFLAG_RD | CTLFLAG_MPSAFE, sysctl_ip6_mcast_filters,
"Per-interface stack-wide source filters");

int ifma6_restart = 0;
#ifdef KTR
/*
* Inline function which wraps assertions for a valid ifp.
Expand Down Expand Up @@ -405,6 +404,7 @@ static int
in6_getmulti(struct ifnet *ifp, const struct in6_addr *group,
struct in6_multi **pinm)
{
struct epoch_tracker et;
struct sockaddr_in6 gsin6;
struct ifmultiaddr *ifma;
struct in6_multi *inm;
Expand All @@ -420,7 +420,10 @@ in6_getmulti(struct ifnet *ifp, const struct in6_addr *group,
IN6_MULTI_LOCK_ASSERT();
IN6_MULTI_LIST_LOCK();
IF_ADDR_WLOCK(ifp);
NET_EPOCH_ENTER(et);
inm = in6m_lookup_locked(ifp, group);
NET_EPOCH_EXIT(et);

if (inm != NULL) {
/*
* If we already joined this group, just bump the
Expand Down Expand Up @@ -593,18 +596,20 @@ in6m_release_wait(void)
}

void
in6m_disconnect(struct in6_multi *inm)
in6m_disconnect_locked(struct in6_multi_head *inmh, struct in6_multi *inm)
{
struct ifnet *ifp;
struct ifaddr *ifa;
struct in6_ifaddr *ifa6;
struct in6_multi_mship *imm, *imm_tmp;
struct ifmultiaddr *ifma, *ll_ifma;

ifp = inm->in6m_ifp;
IN6_MULTI_LIST_LOCK_ASSERT();

ifp = inm->in6m_ifp;
if (ifp == NULL)
return;
return; /* already called */

inm->in6m_ifp = NULL;
IF_ADDR_WLOCK_ASSERT(ifp);
ifma = inm->in6m_ifma;
Expand All @@ -623,7 +628,6 @@ in6m_disconnect(struct in6_multi *inm)
MPASS(ll_ifma->ifma_llifma == NULL);
MPASS(ll_ifma->ifma_ifp == ifp);
if (--ll_ifma->ifma_refcount == 0) {
ifma6_restart = true;
if (ll_ifma->ifma_flags & IFMA_F_ENQUEUED) {
CK_STAILQ_REMOVE(&ifp->if_multiaddrs, ll_ifma, ifmultiaddr, ifma_link);
ll_ifma->ifma_flags &= ~IFMA_F_ENQUEUED;
Expand All @@ -641,28 +645,12 @@ in6m_disconnect(struct in6_multi *inm)
if (inm == imm->i6mm_maddr) {
LIST_REMOVE(imm, i6mm_chain);
free(imm, M_IP6MADDR);
in6m_rele_locked(inmh, inm);
}
}
}
}

void
in6m_release_deferred(struct in6_multi *inm)
{
struct in6_multi_head tmp;

IN6_MULTI_LIST_LOCK_ASSERT();
KASSERT(inm->in6m_refcount > 0, ("refcount == %d inm: %p", inm->in6m_refcount, inm));
if (--inm->in6m_refcount == 0) {
MPASS(inm->in6m_ifp == NULL);
SLIST_INIT(&tmp);
inm->in6m_ifma->ifma_protospec = NULL;
MPASS(inm->in6m_ifma->ifma_llifma == NULL);
SLIST_INSERT_HEAD(&tmp, inm, in6m_nrele);
in6m_release_list_deferred(&tmp);
}
}

static void
in6m_release_task(void *arg __unused)
{
Expand Down Expand Up @@ -1262,6 +1250,7 @@ in6_joingroup_locked(struct ifnet *ifp, const struct in6_addr *mcaddr,
/*const*/ struct in6_mfilter *imf, struct in6_multi **pinm,
const int delay)
{
struct in6_multi_head inmh;
struct in6_mfilter timf;
struct in6_multi *inm;
struct ifmultiaddr *ifma;
Expand Down Expand Up @@ -1323,6 +1312,7 @@ in6_joingroup_locked(struct ifnet *ifp, const struct in6_addr *mcaddr,
}

out_in6m_release:
SLIST_INIT(&inmh);
if (error) {
CTR2(KTR_MLD, "%s: dropping ref on %p", __func__, inm);
IF_ADDR_RLOCK(ifp);
Expand All @@ -1332,13 +1322,14 @@ in6_joingroup_locked(struct ifnet *ifp, const struct in6_addr *mcaddr,
break;
}
}
in6m_disconnect(inm);
in6m_release_deferred(inm);
in6m_disconnect_locked(&inmh, inm);
in6m_rele_locked(&inmh, inm);
IF_ADDR_RUNLOCK(ifp);
} else {
*pinm = inm;
}
IN6_MULTI_LIST_UNLOCK();
in6m_release_list_deferred(&inmh);
return (error);
}

Expand Down Expand Up @@ -1372,6 +1363,7 @@ in6_leavegroup(struct in6_multi *inm, /*const*/ struct in6_mfilter *imf)
int
in6_leavegroup_locked(struct in6_multi *inm, /*const*/ struct in6_mfilter *imf)
{
struct in6_multi_head inmh;
struct in6_mfilter timf;
struct ifnet *ifp;
int error;
Expand Down Expand Up @@ -1421,13 +1413,15 @@ in6_leavegroup_locked(struct in6_multi *inm, /*const*/ struct in6_mfilter *imf)
CTR2(KTR_MLD, "%s: dropping ref on %p", __func__, inm);
if (ifp)
IF_ADDR_WLOCK(ifp);
if (inm->in6m_refcount == 1 && inm->in6m_ifp != NULL)
in6m_disconnect(inm);
in6m_release_deferred(inm);

SLIST_INIT(&inmh);
if (inm->in6m_refcount == 1)
in6m_disconnect_locked(&inmh, inm);
in6m_rele_locked(&inmh, inm);
if (ifp)
IF_ADDR_WUNLOCK(ifp);
IN6_MULTI_LIST_UNLOCK();

in6m_release_list_deferred(&inmh);
return (error);
}

Expand Down Expand Up @@ -1941,6 +1935,7 @@ in6p_lookup_mcast_ifp(const struct inpcb *in6p,
static int
in6p_join_group(struct inpcb *inp, struct sockopt *sopt)
{
struct in6_multi_head inmh;
struct group_source_req gsr;
sockunion_t *gsa, *ssa;
struct ifnet *ifp;
Expand All @@ -1951,6 +1946,7 @@ in6p_join_group(struct inpcb *inp, struct sockopt *sopt)
size_t idx;
int error, is_new;

SLIST_INIT(&inmh);
ifp = NULL;
imf = NULL;
lims = NULL;
Expand Down Expand Up @@ -2227,7 +2223,7 @@ in6p_join_group(struct inpcb *inp, struct sockopt *sopt)
inm = imo->im6o_membership[idx];
if (inm != NULL) {
IN6_MULTI_LIST_LOCK();
in6m_release_deferred(inm);
in6m_rele_locked(&inmh, inm);
IN6_MULTI_LIST_UNLOCK();
}
imo->im6o_membership[idx] = NULL;
Expand All @@ -2236,6 +2232,7 @@ in6p_join_group(struct inpcb *inp, struct sockopt *sopt)

out_in6p_locked:
INP_WUNLOCK(inp);
in6m_release_list_deferred(&inmh);
return (error);
}

Expand Down Expand Up @@ -2879,10 +2876,9 @@ sysctl_ip6_mcast_filters(SYSCTL_HANDLER_ARGS)
IN6_MULTI_LIST_LOCK();
IF_ADDR_RLOCK(ifp);
CK_STAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) {
if (ifma->ifma_addr->sa_family != AF_INET6 ||
ifma->ifma_protospec == NULL)
inm = in6m_ifmultiaddr_get_inm(ifma);
if (inm == NULL)
continue;
inm = (struct in6_multi *)ifma->ifma_protospec;
if (!IN6_ARE_ADDR_EQUAL(&inm->in6m_addr, &mcaddr))
continue;
fmode = inm->in6m_st[1].iss_fmode;
Expand Down
40 changes: 24 additions & 16 deletions sys/netinet6/in6_var.h
View file
Expand Up @@ -645,6 +645,7 @@ struct in6_multi {
/* New fields for MLDv2 follow. */
struct mld_ifsoftc *in6m_mli; /* MLD info */
SLIST_ENTRY(in6_multi) in6m_nrele; /* to-be-released by MLD */
SLIST_ENTRY(in6_multi) in6m_defer; /* deferred MLDv1 */
struct ip6_msource_tree in6m_srcs; /* tree of sources */
u_long in6m_nsrc; /* # of tree entries */

Expand All @@ -670,8 +671,8 @@ struct in6_multi {
} in6m_st[2]; /* state at t0, t1 */
};

void in6m_disconnect(struct in6_multi *inm);
extern int ifma6_restart;
void in6m_disconnect_locked(struct in6_multi_head *inmh, struct in6_multi *inm);

/*
* Helper function to derive the filter mode on a source entry
* from its internal counters. Predicates are:
Expand Down Expand Up @@ -713,32 +714,40 @@ extern struct sx in6_multi_sx;
#define IN6_MULTI_LOCK_ASSERT() sx_assert(&in6_multi_sx, SA_XLOCKED)
#define IN6_MULTI_UNLOCK_ASSERT() sx_assert(&in6_multi_sx, SA_XUNLOCKED)

/*
* Get the in6_multi pointer from a ifmultiaddr.
* Returns NULL if ifmultiaddr is no longer valid.
*/
static __inline struct in6_multi *
in6m_ifmultiaddr_get_inm(struct ifmultiaddr *ifma)
{

NET_EPOCH_ASSERT();

return ((ifma->ifma_addr->sa_family != AF_INET6 ||
(ifma->ifma_flags & IFMA_F_ENQUEUED) == 0) ? NULL :
ifma->ifma_protospec);
}

/*
* Look up an in6_multi record for an IPv6 multicast address
* on the interface ifp.
* If no record found, return NULL.
*
* SMPng: The IN6_MULTI_LOCK and IF_ADDR_LOCK on ifp must be held.
*/
static __inline struct in6_multi *
in6m_lookup_locked(struct ifnet *ifp, const struct in6_addr *mcaddr)
{
struct ifmultiaddr *ifma;
struct in6_multi *inm;

inm = NULL;
CK_STAILQ_FOREACH(ifma, &((ifp)->if_multiaddrs), ifma_link) {
if (ifma->ifma_addr->sa_family == AF_INET6) {
inm = (struct in6_multi *)ifma->ifma_protospec;
if (inm == NULL)
continue;
if (IN6_ARE_ADDR_EQUAL(&inm->in6m_addr, mcaddr))
break;
inm = NULL;
}
CK_STAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) {
inm = in6m_ifmultiaddr_get_inm(ifma);
if (inm == NULL)
continue;
if (IN6_ARE_ADDR_EQUAL(&inm->in6m_addr, mcaddr))
return (inm);
}
return (inm);
return (NULL);
}

/*
Expand Down Expand Up @@ -808,7 +817,6 @@ void in6m_clear_recorded(struct in6_multi *);
void in6m_commit(struct in6_multi *);
void in6m_print(const struct in6_multi *);
int in6m_record_source(struct in6_multi *, const struct in6_addr *);
void in6m_release_deferred(struct in6_multi *);
void in6m_release_list_deferred(struct in6_multi_head *);
void in6m_release_wait(void);
void ip6_freemoptions(struct ip6_moptions *);
Expand Down

0 comments on commit 69483a2

Please sign in to comment.