Date: Sat, 22 Jul 2017 12:11:31 +0300 From: "Andrey V. Elsukov" <bu7cher@yandex.ru> To: Daniel Bilik <ddb@neosystem.org>, freebsd-net@freebsd.org Subject: Re: mbuf clusters leak in netinet6 Message-ID: <5dadd0d0-d5ce-3a2c-7ad6-1c0a39a4a0e7@yandex.ru> In-Reply-To: <20170721232112.82f6e78b76057312183be937@neosystem.cz> References: <20170721232112.82f6e78b76057312183be937@neosystem.cz>
next in thread | previous in thread | raw e-mail | index | archive | help
22.07.17 0:21, Daniel Bilik пишет:
> Hi.
>
> (Please keep me in cc, I'm not subscribed to the list.)
>
> After deploying ndproxy[1] on a few 10-stable hosts, some of them have
> experienced mbuf clusters exhaustion. Initial analysis showed that after
> loading ndproxy.ko, "current" values of "mbuf clusters" and "mbuf+clusters
> out of packet secondary zone" (from netstat -m output) keep continuously
> increasing and never decrease. More thorough inspection of ndproxy source
> code pointed me at function packet() in ndpacket.c[2], to the very last
> "return 1". With this line changed to "return 0", mbuf clusters do not
> increase anymore, ie. it fixes the issue. As the leak does not come from
> "return" itself, I suspect "the proper solution" is to modify code in
> the upper layer to not leak anything on any returned value. If I read it
> right, the upper layer in this case is function ip6_input() in
> sys/netinet6/ip6_input.c[3], specifically pfil_run_hooks() call at line
> 765. I guess it should be changed like this to avoid the leak:
>
> --- ip6_input.c.orig 2017-07-21 22:42:17.780594000 +0200
> +++ ip6_input.c 2017-07-21 22:45:28.981497000 +0200
> @@ -620,8 +620,11 @@
> goto passin;
>
> if (pfil_run_hooks(&V_inet6_pfil_hook, &m,
> - m->m_pkthdr.rcvif, PFIL_IN, NULL))
> + m->m_pkthdr.rcvif, PFIL_IN, NULL)) {
> + if (m)
> + m_free(m);
> return;
> + }
> if (m == NULL) /* consumed by filter */
> return;
> ip6 = mtod(m, struct ip6_hdr *);
>
> I haven't actually tested this modification. I prefer to know your
> opinions first before trying to panic production hosts running hundreds of
> miles from me. ;-) Thanks.
Freeing mbuf is under pfil hook responsibility, if it returns nonzero
value it must call m_freem(). So, it is bug in the ndpacket.c.
https://github.com/AlexandreFenyo/ndproxy/blob/master/ndpacket.c
Also, ip6_output() always consumes mbuf, it is wrong to call m_freem()
after calling ip6_output(), even when it returns error.
--
WBR, Andrey V. Elsukov
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5dadd0d0-d5ce-3a2c-7ad6-1c0a39a4a0e7>