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  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

Powered by blists - more mailing lists