Date: Tue, 30 Sep 2014 11:24:57 -0400 From: Vedant Mathur <vmathur3@ncsu.edu> To: freebsd-net@freebsd.org Subject: Addressing refcount issues in ip6_setdstifaddr and ip6_getdstifaddr routines. Message-ID: <CAKeOEPB=e-5kdFP7paYgWSwNGtBONM9xCpkis7HhkvU1b9Oavg@mail.gmail.com>
next in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAKeOEPB=e-5kdFP7paYgWSwNGtBONM9xCpkis7HhkvU1b9Oavg>