svn commit: r273855 - head/sys/netinet6
Author: ae Date: Thu Oct 30 10:59:57 2014 New Revision: 273855 URL: https://svnweb.freebsd.org/changeset/base/273855 Log: Fix mbuf leak in IPv6 multicast code. When multicast capable interface goes away, it leaves multicast groups, this leads to generate MLD reports, but MLD code does deffered send and MLD reports are queued in the in6_multi's in6m_scq ifq. The problem is that in6_multi structures are freed when interface leaves multicast groups and thread that does deffered send will not take these queued packets. PR: 194577 MFC after:1 week Sponsored by: Yandex LLC Modified: head/sys/netinet6/in6_mcast.c Modified: head/sys/netinet6/in6_mcast.c == --- head/sys/netinet6/in6_mcast.c Thu Oct 30 10:56:38 2014 (r273854) +++ head/sys/netinet6/in6_mcast.c Thu Oct 30 10:59:57 2014 (r273855) @@ -526,6 +526,9 @@ in6m_release_locked(struct in6_multi *in in6m_purge(inm); + /* Free state-change requests that might be queued. */ + _IF_DRAIN(inm-in6m_scq); + free(inm, M_IP6MADDR); if_delmulti_ifma(ifma); ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r273855 - head/sys/netinet6
Hello,
This is a really inconvenient time for me (I am up against a deadline)
but I am not 100% comfortable with this change.
On Thu, 30 Oct 2014, at 10:59, Andrey V. Elsukov wrote:
Log:
Fix mbuf leak in IPv6 multicast code.
When multicast capable interface goes away, it leaves multicast groups,
this leads to generate MLD reports, but MLD code does deffered send and
MLD reports are queued in the in6_multi's in6m_scq ifq. The problem is
that in6_multi structures are freed when interface leaves multicast
groups
and thread that does deffered send will not take these queued packets.
A few comments:
1) Stylistic -- a change of this kind should probably be part of
inm_purge() itself because it modifies state which is private to the
group membership.
2) Logical -- The patch forces pending (queued) state change record
fragments to be freed when the parent interface is taken down.
Unfortunately, those are pending for a reason; there has been a state
change, and MLD needs to communicate it upstream to on-link routers (and
snooping switches).
So - there is a risk with this approach that upstream MLD listener (e.g.
router, switch) will be inconsistent, at least until the next General
Query.
A better approach might be to force an INCLUDE {} to be sent for the
group (there are other parts of the code which try to do this for
similar events), but if the parent interface has already been taken
down, all bets are off. As of writing - The FreeBSD networking stack
doesn't provide any hints either way about the nature of the teardown.
Perhaps need to be a distinction between use cases where a hasty
teardown is unlikely to have operational impact (e.g. an ephemeral VPN
session between two nodes which is point-to-point in nature, and not
generally used for forwarding traffic), and cases where it may impact
other users (e.g. an MLD membership residing on a wireless interface,
which might result in unwanted multicast traffic being relayed to an
802.11 ESS).
--
BMS (sent via webmail)
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r273855 - head/sys/netinet6
On 30.10.2014 14:34, Bruce Simpson wrote: Hello, This is a really inconvenient time for me (I am up against a deadline) but I am not 100% comfortable with this change. On Thu, 30 Oct 2014, at 10:59, Andrey V. Elsukov wrote: Log: Fix mbuf leak in IPv6 multicast code. When multicast capable interface goes away, it leaves multicast groups, this leads to generate MLD reports, but MLD code does deffered send and MLD reports are queued in the in6_multi's in6m_scq ifq. The problem is that in6_multi structures are freed when interface leaves multicast groups and thread that does deffered send will not take these queued packets. A few comments: 1) Stylistic -- a change of this kind should probably be part of inm_purge() itself because it modifies state which is private to the group membership. 2) Logical -- The patch forces pending (queued) state change record fragments to be freed when the parent interface is taken down. Unfortunately, those are pending for a reason; there has been a state change, and MLD needs to communicate it upstream to on-link routers (and snooping switches). So - there is a risk with this approach that upstream MLD listener (e.g. router, switch) will be inconsistent, at least until the next General Query. I'm not quite sure, but I think that the leak happened only when interface disappeared. In case when system just leaves the group, MLD code takes reference to in6_multi and releases it when fasttimo handler dispatches packets. When interface is disappearing, in6_ifdetach() calls in6_purgemaddrs(), where all references to in6_multi released again. Now in6m_release_locked() will drain queue when in6_multi has no more references. -- WBR, Andrey V. Elsukov ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r273855 - head/sys/netinet6
Btw - this is also a problem in the wifi stack. When you destroy an interface, do you want to make sure the STA going away frames go out and are ACKed before you finish tearing down the interface? We don't really have any framework / standards in place for how to put tearing down an interface on hold whilst we notify things and wait for packets to finish transmitting, so we don't bother. (I had the same issue with net80211 / ath and transmit buffers hanging around after I destroyed a VAP. They came from the disassociate packet being generated when the interface was being torn down.) -adrian On 30 October 2014 07:43, Andrey V. Elsukov a...@freebsd.org wrote: On 30.10.2014 14:34, Bruce Simpson wrote: Hello, This is a really inconvenient time for me (I am up against a deadline) but I am not 100% comfortable with this change. On Thu, 30 Oct 2014, at 10:59, Andrey V. Elsukov wrote: Log: Fix mbuf leak in IPv6 multicast code. When multicast capable interface goes away, it leaves multicast groups, this leads to generate MLD reports, but MLD code does deffered send and MLD reports are queued in the in6_multi's in6m_scq ifq. The problem is that in6_multi structures are freed when interface leaves multicast groups and thread that does deffered send will not take these queued packets. A few comments: 1) Stylistic -- a change of this kind should probably be part of inm_purge() itself because it modifies state which is private to the group membership. 2) Logical -- The patch forces pending (queued) state change record fragments to be freed when the parent interface is taken down. Unfortunately, those are pending for a reason; there has been a state change, and MLD needs to communicate it upstream to on-link routers (and snooping switches). So - there is a risk with this approach that upstream MLD listener (e.g. router, switch) will be inconsistent, at least until the next General Query. I'm not quite sure, but I think that the leak happened only when interface disappeared. In case when system just leaves the group, MLD code takes reference to in6_multi and releases it when fasttimo handler dispatches packets. When interface is disappearing, in6_ifdetach() calls in6_purgemaddrs(), where all references to in6_multi released again. Now in6m_release_locked() will drain queue when in6_multi has no more references. -- WBR, Andrey V. Elsukov ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
