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
