On Fri, Aug 28, 2015 at 12:47:51PM +0200, Martin Pieuchot wrote: > The rtvalid() function checks if the route entry rt is still valid and > can be used. Cached entries that are no longer valid should be released > by calling rtfree().
I like it. As it does some checks and returns a boolian value, I wonder wether rtisvalid() would be a better name. > + if (rtvalid(rt) == 0) { ... As the function returns a boolean value I perfer the logical check with !. if (rtvalid(rt)) { route is valid } if (!rtvalid(rt)) { route is not valid } > @@ -1239,8 +1233,10 @@ ip_rtaddr(struct in_addr dst, u_int rtab > ipforward_rt.ro_rt = rtalloc(&ipforward_rt.ro_dst, > RT_REPORT|RT_RESOLVE, rtableid); > } > - if (ipforward_rt.ro_rt == 0) > + if (rtvalid(ipforward_rt.ro_rt) == 0) { > + rtfree(ipforward_rt.ro_rt); > return (NULL); If you free the global variable ipforward_rt.ro_rt you have to reset the pointer with ipforward_rt.ro_rt = NULL. Otherwise you will run into a use after free. > @@ -1417,12 +1412,13 @@ ip_forward(struct mbuf *m, struct ifnet > > ipforward_rt.ro_rt = rtalloc_mpath(&ipforward_rt.ro_dst, > &ip->ip_src.s_addr, ipforward_rt.ro_tableid); > - if (ipforward_rt.ro_rt == 0) { > + if (rtvalid(ipforward_rt.ro_rt) == 0) { > + rtfree(ipforward_rt.ro_rt); > icmp_error(m, ICMP_UNREACH, ICMP_UNREACH_HOST, dest, 0); > return; Same here, ipforward_rt.ro_rt = NULL > +++ sys/netinet6/ip6_forward.c 28 Aug 2015 09:40:40 -0000 > @@ -240,24 +238,23 @@ reroute: > ip6_forward_rt.ro_tableid); > } > > - if (ip6_forward_rt.ro_rt == NULL) { > + if (rtvalid(ip6_forward_rt.ro_rt) == 0) { > ip6stat.ip6s_noroute++; > /* XXX in6_ifstat_inc(rt->rt_ifp, ifs6_in_noroute) */ > if (mcopy) { > icmp6_error(mcopy, ICMP6_DST_UNREACH, > ICMP6_DST_UNREACH_NOROUTE, 0); > } > + rtfree(rt); > m_freem(m); > return; rt is uninitialized. You want to free ip6_forward_rt.ro_rt and set it to NULL. > @@ -268,13 +265,14 @@ reroute: > &ip6->ip6_src.s6_addr32[0], > ip6_forward_rt.ro_tableid); > > - if (ip6_forward_rt.ro_rt == NULL) { > + if (rtvalid(ip6_forward_rt.ro_rt) == 0) { > ip6stat.ip6s_noroute++; > /* XXX in6_ifstat_inc(rt->rt_ifp, ifs6_in_noroute) */ > if (mcopy) { > icmp6_error(mcopy, ICMP6_DST_UNREACH, > ICMP6_DST_UNREACH_NOROUTE, 0); > } > + rtfree(rt); > m_freem(m); > return; Same here. > @@ -429,10 +429,9 @@ ip6_input(struct mbuf *m) > /* > * Unicast check > */ > - if (ip6_forward_rt.ro_rt != NULL && > - (ip6_forward_rt.ro_rt->rt_flags & RTF_UP) != 0 && > + if (rtvalid(ip6_forward_rt.ro_rt) && > IN6_ARE_ADDR_EQUAL(&ip6->ip6_dst, > - &ip6_forward_rt.ro_dst.sin6_addr) && > + &ip6_forward_rt.ro_dst.sin6_addr) && Strange indentation. All other conversions look correct to me. bluhm