netdev - Re: [PATCH RFC v3 net 2/2] ipv6: Avoid restarting fib6_lookup() for RTF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 08 Oct 2014 15:32:54 -0400 (EDT)
From:	David Miller <davem@...emloft.net>
To:	kafai@...com
Cc:	netdev@...r.kernel.org, hannes@...essinduktion.org
Subject: Re: [PATCH RFC v3 net 2/2] ipv6: Avoid restarting fib6_lookup()
 for RTF_CACHE hit case

From: Martin KaFai Lau <kafai@...com>
Date: Mon, 6 Oct 2014 17:05:15 -0700

> When there is a RTF_CACHE hit, no need to redo fib6_lookup()
> with reachable=0.
> 
> Cc: Hannes Frederic Sowa <hannes@...essinduktion.org>
> Signed-off-by: Martin KaFai Lau <kafai@...com>

Ok this looks fine, and I can see how there is a dependency with patch
#1.

But I also want to point out how this change show my point about
BACKTRACK() even more.  Read this function after this patch is
applied and someone auditing might say "oh, 'out' label is now
unused, we can remove it"

Again, hidden control flow is really bad, and we've had very serious
bugs in the past because of it (which we've fixed by ditching the
side effect causing macros in favor of properly designed inline
functions).

Trying to be constructive, why don't we go in a direction like
the following?

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index a318dd89..99612c5 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -772,6 +772,26 @@ int rt6_route_rcv(struct net_device *dev, u8 *opt, int len,
 }
 #endif
 
+static struct fib6_node *rt6_backtrack(struct net *net, struct rt6_info *rt, struct fib6_node *fn, const struct in6_addr *saddr)
+{
+	if (rt == net->ipv6.ip6_null_entry) {
+		struct fib6_node *pn;
+
+		while (1) {
+			if (fn->fn_flags & RTN_TL_ROOT)
+				break;
+			pn = fn->parent;
+			if (FIB6_SUBTREE(pn) && FIB6_SUBTREE(pn) != fn)
+				fn = fib6_lookup(FIB6_SUBTREE(pn), NULL, saddr);
+			else
+				fn = pn;
+			if (fn->fn_flags & RTN_RTINFO)
+				break;
+		}
+	}
+	return fn;
+}
+
 #define BACKTRACK(__net, saddr)			\
 do { \
 	if (rt == __net->ipv6.ip6_null_entry) {	\
@@ -934,10 +954,15 @@ restart:
 	rt = rt6_select(fn, oif, strict | reachable);
 	if (rt->rt6i_nsiblings)
 		rt = rt6_multipath_select(rt, fl6, oif, strict | reachable);
-	BACKTRACK(net, &fl6->saddr);
-	if (rt == net->ipv6.ip6_null_entry ||
-	    rt->rt6i_flags & RTF_CACHE)
-		goto out;
+	fn = rt6_backtrack(net, rt, fn, &fl6->saddr);
+	if (rt == net->ipv6.ip6_null_entry) {
+		if (fn->fn_flags & RTN_TL_ROOT)
+			goto out;
+		if (fn->fn_flags & RTN_RTINFO)
+			goto restart;
+	}
+	if (rt->rt6i_flags & RTF_CACHE)
+		goto out1;
 
 	dst_hold(&rt->dst);
 	read_unlock_bh(&table->tb6_lock);
@@ -974,6 +999,7 @@ out:
 		reachable = 0;
 		goto restart_2;
 	}
+out1:
 	dst_hold(&rt->dst);
 	read_unlock_bh(&table->tb6_lock);
 out2:
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html