Date: Fri, 20 Jun 2014 16:20:45 -0600 From: Alan Somers <asomers@freebsd.org> To: Navdeep Parhar <navdeep@chelsio.com> Cc: "freebsd-net@freebsd.org" <freebsd-net@freebsd.org> Subject: Re: ifaddr refcount problem Message-ID: <CAOtMX2gH39O_3y1tpzcGw3ups_ujr%2Bzv2hkmPpEw5_xRcxSvQQ@mail.gmail.com> In-Reply-To: <53A48849.8080504@chelsio.com> References: <53A48849.8080504@chelsio.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Jun 20, 2014 at 1:15 PM, Navdeep Parhar <navdeep@chelsio.com> wrote:
> Revision 264905 and 266860 that followed it seem to leak ifaddr
> references. ifa_ifwithdstaddr and ifa_ifwithnet both install a
> reference on the ifaddr returned to the caller but ip_output does not
> release it, eventually leading to a panic when the refcount wraps over
> to 0 and the ifaddr is freed while it is still on various lists.
Are you referring to the ifa_ref() calls at lines 1674, 1745, 1756,
and 1795 of sys/net/if.c? Those long predate 264905. Why do you
think that 264905 introduced the bug? If anything, I guess that it
was introduced by change 262747, which removed ifa_free() from the
done: block of ip_output().
>
> I'm using this patch for now. Thoughts?
>
> Regards,
> Navdeep
>
>
> diff -r 6dfcecd314af sys/netinet/ip_output.c
> --- a/sys/netinet/ip_output.c Fri Jun 20 10:33:22 2014 -0700
> +++ b/sys/netinet/ip_output.c Fri Jun 20 12:07:12 2014 -0700
> @@ -243,6 +243,7 @@ again:
> ifp = ia->ia_ifp;
> ip->ip_ttl = 1;
> isbroadcast = 1;
> + ifa_free((void *)ia);
> } else if (flags & IP_ROUTETOIF) {
> if ((ia = ifatoia(ifa_ifwithdstaddr(sintosa(dst)))) == NULL &&
> (ia = ifatoia(ifa_ifwithnet(sintosa(dst), 0))) == NULL) {
> @@ -253,6 +254,7 @@ again:
> ifp = ia->ia_ifp;
> ip->ip_ttl = 1;
> isbroadcast = in_broadcast(dst->sin_addr, ifp);
> + ifa_free((void *)ia);
> } else if (IN_MULTICAST(ntohl(ip->ip_dst.s_addr)) &&
> imo != NULL && imo->imo_multicast_ifp != NULL) {
> /*
I don't think this is quite right. ip_output() references ia at lines
366, 433, 630-637, 674, and 675. All of those references have NULL
checks, so your patch won't cause a use-after-free bug, but I think
that the patch might cause statistics collection to fail, and it might
cause the IP source address to not be set.
Do you have a test case that can reproduce the panic?
-Alan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAOtMX2gH39O_3y1tpzcGw3ups_ujr%2Bzv2hkmPpEw5_xRcxSvQQ>