From owner-freebsd-net@FreeBSD.ORG Tue Sep 30 15:31:08 2014 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id E7810BEF for ; Tue, 30 Sep 2014 15:31:08 +0000 (UTC) Received: from na3sys009aog116.obsmtp.com (na3sys009aog116.obsmtp.com [74.125.149.240]) by mx1.freebsd.org (Postfix) with SMTP id 3A511CDD for ; Tue, 30 Sep 2014 15:31:06 +0000 (UTC) Received: from mail-ie0-f170.google.com ([209.85.223.170]) (using TLSv1) by na3sys009aob116.postini.com ([74.125.148.12]) with SMTP ID DSNKVCrMtIuuDIDznk4WrHUDb+J5DZk6MXJ9@postini.com; Tue, 30 Sep 2014 08:31:07 PDT Received: by mail-ie0-f170.google.com with SMTP id rd18so2357949iec.29 for ; Tue, 30 Sep 2014 08:30:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:date:message-id:subject:from:to :content-type; bh=9YLcPLJjvliPXnCxthFe5Z5x0v53pptIXJBq7UBSBkI=; b=QivWmtgntfW/3xdTyJWi01VbOMLRp18eViRywn2t2X6ddrSSnRxBJunmUcDELYtveA IQa2Kh82qT4kdosoA9UMkCJZXIeGgRjUm3kp9SzFO3+AFLM74cppYj1CfWZfwQSlgadI QSBXKcqZCdwM4tXZaoYa4z+RDd94ODXHjQx4sKzrt8swd20iuXWNfKj7NGUMCzl3kzAN lWhLCCyPMXCJCjFzyjWKw4ooQHA8Z1vis2GhXjSekPjXoJZekaw0BPibsg8JUJeo66Yv P/JXa9KRzfUCtXYaJyi2Wd6Jvu77vDyUf4k0Q3Lbx3nlChUOivUt2wrenTpbn5qg/9/T 5Usg== X-Received: by 10.50.170.196 with SMTP id ao4mr9137903igc.46.1412090697974; Tue, 30 Sep 2014 08:24:57 -0700 (PDT) X-Gm-Message-State: ALoCoQlUvLMlrh+3tAuAo/AnULq8KIsrS0vytHsWb8FFs5q10DryJZ9C7cKdE6Bi4JAOK+E31nFLmHC1y4XPqjZDvgqqtncWBf+b+8Q/jBhCBFJVWTeP89E8Inl0Qdc4i+J3zrm3S678tZsH7B+/s/HiupShDjn/dA== MIME-Version: 1.0 X-Received: by 10.50.170.196 with SMTP id ao4mr9137886igc.46.1412090697865; Tue, 30 Sep 2014 08:24:57 -0700 (PDT) Received: by 10.43.61.201 with HTTP; Tue, 30 Sep 2014 08:24:57 -0700 (PDT) Date: Tue, 30 Sep 2014 11:24:57 -0400 Message-ID: Subject: Addressing refcount issues in ip6_setdstifaddr and ip6_getdstifaddr routines. From: Vedant Mathur To: freebsd-net@freebsd.org Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.18-1 X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.18-1 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, 30 Sep 2014 15:31:09 -0000 Hi all, I am trying to address the refcount issues in the routine ip6_setdstifaddr defined in the file ip6_input.c. The issue is described below: The ip6_setdstifaddr routine is responsible for adding m_tags to an mbuf and stashing the struct in6_ifaddr reference in the m_tag. Since the in6_ifaddr struct stores a reference to the struct ifaddr as well, we have the ability to access the memory location of the struct ifaddr using the ip6_getdstifaddr routine. We can access the memory location of the struct ifaddr by accessing the reference to it stored in the in6_ifaddr in the m_tag prepended to the mbuf. The struct ifaddr is refcounted using the routines defined in the file refcount.h. The problem here is that we may be accessing freed memory when we call the ip6_getdstifaddr routine since we are not incrementing the refcount for the struct ifaddr, whose reference is stored in the in6_ifaddr, whose reference is stashed in the m_tag. We should be incrementing the refcount of the struct ifaddr when we stash the in6_ifaddr reference in the m_tag. This is the first part of the issue. The second part of the issue is addressing concerns regarding copying and freeing of the m_tag/mbuf. We need to modify the routines involved in copying and freeing of m_tags/mbufs such that they manage the refcount properly. The comments above the two routines in question mention the second part of the issue described above to some extent. It is attributed to the lack of deep copy support for the m_tags. > 1028 * XXXRW: We should bump the refcount on ia6 before sticking it in > the m_tag, > 1029 * and then bump it when the tag is copied, and release it when the > tag is > 1030 * freed. Unfortunately, m_tags don't support deep copies (yet), so > instead > 1031 * we just bump the ia refcount when we receive it. This should be > fixed. > 1032 */ > 1033 static struct ip6aux * > 1034 ip6_setdstifaddr(struct mbuf *m, struct in6_ifaddr *ia6) > 1035 { > 1036 struct ip6aux *ip6a; > 1037 > 1038 ip6a = ip6_addaux(m); > 1039 if (ip6a) > 1040 ip6a->ip6a_dstia6 = ia6; <<<<<<<<<< > We should be incrementing the refcount to struct ifaddr in &ia6->ia_ifa > here. > 1041 return ip6a; /* NULL if failed to set */ > 1042 } > 1043 > 1044 struct in6_ifaddr * > 1045 ip6_getdstifaddr(struct mbuf *m) > 1046 { > 1047 struct ip6aux *ip6a; > 1048 struct in6_ifaddr *ia; > 1049 > 1050 ip6a = ip6_findaux(m); > 1051 if (ip6a) { > 1052 ia = ip6a->ip6a_dstia6; > 1053 ifa_ref(&ia->ia_ifa); <<<<<<< This reference > to the struct ifaddr (&ia->ia_ifa) can be stale and can potentially point > to freed memory. > 1054 return ia; > 1055 } else > 1056 return NULL; > 1057 } > 1058 The following change also talks about the issue somewhat: https://svnweb.freebsd.org/base?view=revision&revision=194760 Below I have a couple solutions and would like your feedback on these solutions. Which one of these two solutions would you think is better and why. I would prefer to incorporate a solution in my code which aligns more with the FreeBSD community so that future refreshes are easier. *Solution 1:* 1) Increment the refcount when we stash the struct in6_ifaddr reference in the m_tag in ip6_setdstifaddr routine. 2) Modify the m_tag_copy routine to address the special case of the IPv6 tag where it calls ifa_ref(). This is to make sure that IF AT ANY POINT we copy the mbuf with the IPv6 tag we are incrementing the refcnt. Also any other routines capable of copying the m_tag/mbuf should be modified similarly. 3) Modify the m_tag_delete() routine to address the special case of the IPv6 tag where it calls ifa_free(). This is to make sure that when we delete the mbuf/m_tag with the IPv6 tag we decrement the refcnt. Also any other routines capable of freeing the mbuf/m_tag should be modified. *Solution 2:* In ip6_setdstifaddr() routine we can access the struct ifa using ia6->ia_ifa and retrieve the IP address from the ifa and then push it into the m_tag instead of the struct in6_ifaddr pointer. Then we will not require a refcnt increment and in the ip6_getdstifaddr() we can use the ifa_withifaddr() routines to retrieve the ia by basically looping through the list of ifaddrs. Regards, Vedant Mathur