From owner-freebsd-net@FreeBSD.ORG Tue Jun 24 14:43:47 2014 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id A27A9FC9; Tue, 24 Jun 2014 14:43:47 +0000 (UTC) Received: from mail-wi0-x235.google.com (mail-wi0-x235.google.com [IPv6:2a00:1450:400c:c05::235]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 094AA2193; Tue, 24 Jun 2014 14:43:46 +0000 (UTC) Received: by mail-wi0-f181.google.com with SMTP id n3so735962wiv.8 for ; Tue, 24 Jun 2014 07:43:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=1/lL62HjsZ3uJFQ6Hh+7zWVJ79PcYeC+I/Rko4Bywjo=; b=l4KCZ5MOOrnxykmgqzJ+TF1lBQ4jshFItoQD2jj4aeqhZacMXLriE2bb1bu9B5zPko k+Z1uyHd5JoI0qYMg2FZY60CLRCb75WwZn1yMC0RHW0833ZhQTm3GVB0OsTwiZqC70hX fRlJQvOyd1+vNq52r9e9cJuj33rGMvO83al2HLW9t61wdIqRfw8xP9OBJFPf6r5L4XrJ /zHfd5sQyVGLRNeq7IzuBUONqjAHV5TARcYVkpVMQt96gUxavgtRjnO3lbiD/HMfe4WO z786Pj+9wfA2qhrNLWsBXel2UjZDcn+rUU1bQ+yvONhysIrQ9UutShXdHcvwf1W/61Cd M6Ig== MIME-Version: 1.0 X-Received: by 10.180.20.206 with SMTP id p14mr3142619wie.26.1403621020873; Tue, 24 Jun 2014 07:43:40 -0700 (PDT) Sender: asomers@gmail.com Received: by 10.194.168.202 with HTTP; Tue, 24 Jun 2014 07:43:40 -0700 (PDT) In-Reply-To: <20140624090847.GB28199@glebius.int.ru> References: <53A48849.8080504@chelsio.com> <20140623085229.GQ28199@FreeBSD.org> <20140624090847.GB28199@glebius.int.ru> Date: Tue, 24 Jun 2014 08:43:40 -0600 X-Google-Sender-Auth: 4MuXNWXLCIJXMq1Nl6jq9gZCA7s Message-ID: Subject: Re: ifaddr refcount problem From: Alan Somers To: Gleb Smirnoff Content-Type: multipart/mixed; boundary=bcaec53d55f5594ee104fc95fcf8 Cc: "freebsd-net@freebsd.org" , Navdeep Parhar X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 24 Jun 2014 14:43:47 -0000 --bcaec53d55f5594ee104fc95fcf8 Content-Type: text/plain; charset=UTF-8 On Tue, Jun 24, 2014 at 3:08 AM, Gleb Smirnoff 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--