Date: Tue, 24 Jun 2014 08:43:40 -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: <CAOtMX2gV%2BnTLs-%2BpFWWETLKW2Dtrtxb-Z30Dq65Fctwy4pBfPA@mail.gmail.com> In-Reply-To: <20140624090847.GB28199@glebius.int.ru> References: <53A48849.8080504@chelsio.com> <20140623085229.GQ28199@FreeBSD.org> <CAOtMX2hGpvhK5TkBUfXvXisu5E2zen=h6MEv_Bxecn=aKsnnnQ@mail.gmail.com> <20140624090847.GB28199@glebius.int.ru>
next in thread | previous in thread | raw e-mail | index | archive | help
--bcaec53d55f5594ee104fc95fcf8
Content-Type: text/plain; charset=UTF-8
On Tue, Jun 24, 2014 at 3:08 AM, Gleb Smirnoff <glebius@freebsd.org> wrote:
> On Mon, Jun 23, 2014 at 10:44:58AM -0600, Alan Somers wrote:
> A> > On Fri, Jun 20, 2014 at 12:15:21PM -0700, Navdeep Parhar wrote:
> A> > N> Revision 264905 and 266860 that followed it seem to leak ifaddr
> A> > N> references. ifa_ifwithdstaddr and ifa_ifwithnet both install a
> A> > N> reference on the ifaddr returned to the caller but ip_output does not
> A> > N> release it, eventually leading to a panic when the refcount wraps over
> A> > N> to 0 and the ifaddr is freed while it is still on various lists.
> A> > N>
> A> > N> I'm using this patch for now. Thoughts?
> A> > N>
> A> > N> Regards,
> A> > N> Navdeep
> A> > N>
> A> > N>
> A> > N> diff -r 6dfcecd314af sys/netinet/ip_output.c
> A> > N> --- a/sys/netinet/ip_output.c Fri Jun 20 10:33:22 2014 -0700
> A> > N> +++ b/sys/netinet/ip_output.c Fri Jun 20 12:07:12 2014 -0700
> A> > N> @@ -243,6 +243,7 @@ again:
> A> > N> ifp = ia->ia_ifp;
> A> > N> ip->ip_ttl = 1;
> A> > N> isbroadcast = 1;
> A> > N> + ifa_free((void *)ia);
> A> > N> } else if (flags & IP_ROUTETOIF) {
> A> > N> if ((ia = ifatoia(ifa_ifwithdstaddr(sintosa(dst)))) == NULL &&
> A> > N> (ia = ifatoia(ifa_ifwithnet(sintosa(dst), 0))) == NULL) {
> A> > N> @@ -253,6 +254,7 @@ again:
> A> > N> ifp = ia->ia_ifp;
> A> > N> ip->ip_ttl = 1;
> A> > N> isbroadcast = in_broadcast(dst->sin_addr, ifp);
> A> > N> + ifa_free((void *)ia);
> A> > N> } else if (IN_MULTICAST(ntohl(ip->ip_dst.s_addr)) &&
> A> > N> imo != NULL && imo->imo_multicast_ifp != NULL) {
> A> > N> /*
> A> >
> A> > The patch shouldn't use void * casts, but instead specify explicit member:
> A> >
> A> > ifa_free(&ia->ia_ifa);
> A> >
> A> > Apart from that it, the patch looks entirely correct and plugging a leak.
> A> > Thanks!
> A>
> A> I still don't see how this patch would work without breaking stuff
> A> like the statistics collection at line 673 of ip_output.c. If we call
> A> ifa_free immediately after choosing our ifp, then ia won't be
> A> available at lines 630 or 673, and ip_output will never record
> A> statistics, right?
>
> You are right, thanks.
>
> In case of IP_SENDONES/IP_ROUTETOIF we should hold the reference to ia
> throughout the function and free it at the end.
>
> Suggested patch, not tested.
That looks better. But I think there is one more possibility for a
leak. For multicast packets, IFP_TO_IA at line 263 will call
ifa_ref(), unless the the interface has no address assigned. How
about this patch instead? Also, not tested.
>
> --
> Totus tuus, Glebius.
--bcaec53d55f5594ee104fc95fcf8
Content-Type: text/plain; charset=US-ASCII; name="ip_output_v3.diff"
Content-Disposition: attachment; filename="ip_output_v3.diff"
Content-Transfer-Encoding: base64
X-Attachment-Id: f_hwtbpicn1
SW5kZXg6IHN5cy9uZXRpbmV0L2lwX291dHB1dC5jCj09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIHN5cy9uZXRpbmV0
L2lwX291dHB1dC5jCShyZXZpc2lvbiAyNjc4MDEpCisrKyBzeXMvbmV0aW5ldC9pcF9vdXRwdXQu
Ywkod29ya2luZyBjb3B5KQpAQCAtMTM2LDYgKzEzNiw3IEBACiAJc3RydWN0IHJ0ZW50cnkgKnJ0
ZTsJLyogY2FjaGUgZm9yIHJvLT5yb19ydCAqLwogCXN0cnVjdCBpbl9hZGRyIG9kc3Q7CiAJc3Ry
dWN0IG1fdGFnICpmd2RfdGFnID0gTlVMTDsKKwlpbnQgaGF2ZV9pYV9yZWYgPSAwOwogI2lmZGVm
IElQU0VDCiAJaW50IG5vX3JvdXRlX2J1dF9jaGVja19zcGQgPSAwOwogI2VuZGlmCkBAIC0yMzgs
NiArMjM5LDcgQEAKIAkJCWVycm9yID0gRU5FVFVOUkVBQ0g7CiAJCQlnb3RvIGJhZDsKIAkJfQor
CQloYXZlX2lhX3JlZiA9IDE7CiAJCWlwLT5pcF9kc3Quc19hZGRyID0gSU5BRERSX0JST0FEQ0FT
VDsKIAkJZHN0LT5zaW5fYWRkciA9IGlwLT5pcF9kc3Q7CiAJCWlmcCA9IGlhLT5pYV9pZnA7CkBA
IC0yNTAsNiArMjUyLDcgQEAKIAkJCWVycm9yID0gRU5FVFVOUkVBQ0g7CiAJCQlnb3RvIGJhZDsK
IAkJfQorCQloYXZlX2lhX3JlZiA9IDE7CiAJCWlmcCA9IGlhLT5pYV9pZnA7CiAJCWlwLT5pcF90
dGwgPSAxOwogCQlpc2Jyb2FkY2FzdCA9IGluX2Jyb2FkY2FzdChkc3QtPnNpbl9hZGRyLCBpZnAp
OwpAQCAtMjYxLDYgKzI2NCw3IEBACiAJCSAqLwogCQlpZnAgPSBpbW8tPmltb19tdWx0aWNhc3Rf
aWZwOwogCQlJRlBfVE9fSUEoaWZwLCBpYSk7CisJCWhhdmVfaWFfcmVmID0gMTsKIAkJaXNicm9h
ZGNhc3QgPSAwOwkvKiBmb29sIGdjYyAqLwogCX0gZWxzZSB7CiAJCS8qCkBAIC01NTIsOCArNTU2
LDExIEBACiAjZW5kaWYKIAkJCWVycm9yID0gbmV0aXNyX3F1ZXVlKE5FVElTUl9JUCwgbSk7CiAJ
CQlnb3RvIGRvbmU7Ci0JCX0gZWxzZQorCQl9IGVsc2UgeworCQkJaWYgKGhhdmVfaWFfcmVmICYm
IGlhKQorCQkJCWlmYV9mcmVlKCZpYS0+aWZhKTsKIAkJCWdvdG8gYWdhaW47CS8qIFJlZG8gdGhl
IHJvdXRpbmcgdGFibGUgbG9va3VwLiAqLworCQl9CiAJfQogCiAJLyogU2VlIGlmIGxvY2FsLCBp
ZiB5ZXMsIHNlbmQgaXQgdG8gbmV0aXNyIHdpdGggSVBfRkFTVEZXRF9PVVJTLiAqLwpAQCAtNTgy
LDYgKzU4OSw4IEBACiAJCW0tPm1fZmxhZ3MgfD0gTV9TS0lQX0ZJUkVXQUxMOwogCQltLT5tX2Zs
YWdzICY9IH5NX0lQX05FWFRIT1A7CiAJCW1fdGFnX2RlbGV0ZShtLCBmd2RfdGFnKTsKKwkJaWYg
KGhhdmVfaWFfcmVmICYmIGlhKQorCQkJaWZhX2ZyZWUoJmlhLT5pZmEpOwogCQlnb3RvIGFnYWlu
OwogCX0KIApAQCAtNjk0LDYgKzcwMyw4IEBACiBkb25lOgogCWlmIChybyA9PSAmaXByb3V0ZSkK
IAkJUk9fUlRGUkVFKHJvKTsKKwlpZiAoaGF2ZV9pYV9yZWYgJiYgaWEpCisJCWlmYV9mcmVlKCZp
YS0+aWZhKTsKIAlyZXR1cm4gKGVycm9yKTsKIGJhZDoKIAltX2ZyZWVtKG0pOwo=
--bcaec53d55f5594ee104fc95fcf8--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAOtMX2gV%2BnTLs-%2BpFWWETLKW2Dtrtxb-Z30Dq65Fctwy4pBfPA>