Date: Mon, 23 Jun 2014 10:44:58 -0600 From: Alan Somers <asomers@freebsd.org> To: Gleb Smirnoff <glebius@freebsd.org> Cc: "freebsd-net@freebsd.org" <freebsd-net@freebsd.org>, Navdeep Parhar <navdeep@chelsio.com> Subject: Re: ifaddr refcount problem Message-ID: <CAOtMX2hGpvhK5TkBUfXvXisu5E2zen=h6MEv_Bxecn=aKsnnnQ@mail.gmail.com> In-Reply-To: <20140623085229.GQ28199@FreeBSD.org> References: <53A48849.8080504@chelsio.com> <20140623085229.GQ28199@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Jun 23, 2014 at 2:52 AM, Gleb Smirnoff <glebius@freebsd.org> wrote:
> Navdeep,
>
> On Fri, Jun 20, 2014 at 12:15:21PM -0700, Navdeep Parhar wrote:
> N> Revision 264905 and 266860 that followed it seem to leak ifaddr
> N> references. ifa_ifwithdstaddr and ifa_ifwithnet both install a
> N> reference on the ifaddr returned to the caller but ip_output does not
> N> release it, eventually leading to a panic when the refcount wraps over
> N> to 0 and the ifaddr is freed while it is still on various lists.
> N>
> N> I'm using this patch for now. Thoughts?
> N>
> N> Regards,
> N> Navdeep
> N>
> N>
> N> diff -r 6dfcecd314af sys/netinet/ip_output.c
> N> --- a/sys/netinet/ip_output.c Fri Jun 20 10:33:22 2014 -0700
> N> +++ b/sys/netinet/ip_output.c Fri Jun 20 12:07:12 2014 -0700
> N> @@ -243,6 +243,7 @@ again:
> N> ifp = ia->ia_ifp;
> N> ip->ip_ttl = 1;
> N> isbroadcast = 1;
> N> + ifa_free((void *)ia);
> N> } else if (flags & IP_ROUTETOIF) {
> N> if ((ia = ifatoia(ifa_ifwithdstaddr(sintosa(dst)))) == NULL &&
> N> (ia = ifatoia(ifa_ifwithnet(sintosa(dst), 0))) == NULL) {
> N> @@ -253,6 +254,7 @@ again:
> N> ifp = ia->ia_ifp;
> N> ip->ip_ttl = 1;
> N> isbroadcast = in_broadcast(dst->sin_addr, ifp);
> N> + ifa_free((void *)ia);
> N> } else if (IN_MULTICAST(ntohl(ip->ip_dst.s_addr)) &&
> N> imo != NULL && imo->imo_multicast_ifp != NULL) {
> N> /*
>
> The patch shouldn't use void * casts, but instead specify explicit member:
>
> ifa_free(&ia->ia_ifa);
>
> Apart from that it, the patch looks entirely correct and plugging a leak.
> Thanks!
I still don't see how this patch would work without breaking stuff
like the statistics collection at line 673 of ip_output.c. If we call
ifa_free immediately after choosing our ifp, then ia won't be
available at lines 630 or 673, and ip_output will never record
statistics, right?
-Alan
>
> --
> Totus tuus, Glebius.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAOtMX2hGpvhK5TkBUfXvXisu5E2zen=h6MEv_Bxecn=aKsnnnQ>