Fix routing metrics

PATCH Superseded Steffen Klassert 2 participants 5 messages Mar 1, 2012 – Mar 6, 2012

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Steffen Klassert

author

2012-03-01 06:22 UTC #0 lore raw

v2 cover Fix routing metrics

At the moment we initialize the routing metrics with the, on the inetpeer cached values in rt_init_metrics(). So if we have the metrics cached on the inetpeer, we ignore the user configured fib_metrics. This leads to the fact that we can't configure the mtu, hoplimit etc. if we have learned metrics cached.

The first patch invalidates the inetpeer tree along routing cache, so all the cached metrics are not used any more after that. This is done by replacing the old tree with a fresh initialized inet_peer_base when rt_cache_invalidate() is invoked. The old tree is added to a garbage collector list and destroyed later with a delayed work queue. I use a delay of 60 seconds, so rt_check_expire() ran at least on time before on a default configuration.

The second patch removes the redirect_genid handling. We don't need this any more because we remove the whole inetpeer tree when the redirects are invalidated.

Steffen Klassert

author

2012-03-01 06:22 UTC #1 lore raw

PATCH v2 1/2 inetpeer: Invalidate the inetpeer tree along with the routing cache

We initialize the routing metrics with the values cached on the inetpeer in rt_init_metrics(). So if we have the metrics cached on the inetpeer, we ignore the user configured fib_metrics.

To fix this issue, we replace the old tree with a fresh initialized inet_peer_base. The old tree is removed later with a delayed work queue.

Signed-off-by Steffen Klassert <[email protected]>

---
 include/net/inetpeer.h |    3 ++
 net/ipv4/inetpeer.c    |   80 +++++++++++++++++++++++++++++++++++++++++++++++-
 net/ipv4/route.c       |    1 +
 3 files changed, 83 insertions(+), 1 deletions(-)

include/net/inetpeer.h +3 -0

index 06b795d..ff04a33 100644
@@ -41,6 +41,7 @@ struct inet_peer {
 	u32			pmtu_orig;
 	u32			pmtu_learned;
 	struct inetpeer_addr_base redirect_learned;
+	struct list_head	gc_list;
 	/*
 	 * Once inet_peer is queued for deletion (refcnt == -1), following fields
 	 * are not available: rid, ip_id_count, tcp_ts, tcp_ts_stamp

@@ -96,6 +97,8 @@ static inline struct inet_peer *inet_getpeer_v6(const struct in6_addr *v6daddr,
 extern void inet_putpeer(struct inet_peer *p);
 extern bool inet_peer_xrlim_allow(struct inet_peer *peer, int timeout);
 
+extern void inetpeer_invalidate_tree(int family);
+
 /*
  * temporary check to make sure we dont access rid, ip_id_count, tcp_ts,
  * tcp_ts_stamp if no refcount is taken on inet_peer

net/ipv4/inetpeer.c +79 -1

113 lines not on this page · raw message

net/ipv4/route.c +1 -0

index bcacf54..23ce0c1 100644
@@ -938,6 +938,7 @@ static void rt_cache_invalidate(struct net *net)
 	get_random_bytes(&shuffle, sizeof(shuffle));
 	atomic_add(shuffle + 1U, &net->ipv4.rt_genid);
 	redirect_genid++;
+	inetpeer_invalidate_tree(AF_INET);
 }
 
 /*

Signature · 1 line

1.7.0.4

Steffen Klassert

author

2012-03-01 06:23 UTC #2 lore raw

PATCH v2 2/2 route: Remove redirect_genid

In reply to Steffen Klassert

As we invalidate the inetpeer tree along with the routing cache now, we don't need a genid to reset the redirect handling when the routing cache is flushed.

Signed-off-by Steffen Klassert <[email protected]>

---
 include/net/inetpeer.h |    1 -
 net/ipv4/inetpeer.c    |    1 -
 net/ipv4/route.c       |   19 +++++--------------
 3 files changed, 5 insertions(+), 16 deletions(-)

include/net/inetpeer.h +0 -1

index ff04a33..b94765e 100644
@@ -35,7 +35,6 @@ struct inet_peer {
 
 	u32			metrics[RTAX_MAX];
 	u32			rate_tokens;	/* rate limiting for ICMP */
-	int			redirect_genid;
 	unsigned long		rate_last;
 	unsigned long		pmtu_expires;
 	u32			pmtu_orig;

net/ipv4/inetpeer.c +0 -1

index 897953d..4b7c16a 100644
@@ -498,7 +498,6 @@ relookup:
 		p->rate_last = 0;
 		p->pmtu_expires = 0;
 		p->pmtu_orig = 0;
-		p->redirect_genid = 0;
 		memset(&p->redirect_learned, 0, sizeof(p->redirect_learned));
 		INIT_LIST_HEAD(&p->gc_list);

net/ipv4/route.c +5 ~~-14~~

index 23ce0c1..a796017 100644
@@ -132,7 +132,6 @@ static int ip_rt_mtu_expires __read_mostly	= 10 * 60 * HZ;
 static int ip_rt_min_pmtu __read_mostly		= 512 + 20 + 20;
 static int ip_rt_min_advmss __read_mostly	= 256;
 static int rt_chain_length_max __read_mostly	= 20;
-static int redirect_genid;
 
 static struct delayed_work expires_work;
 static unsigned long expires_ljiffies;

@@ -937,7 +936,6 @@ static void rt_cache_invalidate(struct net *net)
 
 	get_random_bytes(&shuffle, sizeof(shuffle));
 	atomic_add(shuffle + 1U, &net->ipv4.rt_genid);
-	redirect_genid++;
 	inetpeer_invalidate_tree(AF_INET);
 }

@@ -1485,15 +1483,11 @@ void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
 					rt_bind_peer(rt, rt->rt_dst, 1);
 
 				peer = rt->peer;
-				if (peer) {
-					if (peer->redirect_learned.a4 != new_gw ||
-					    peer->redirect_genid != redirect_genid) {
-						peer->redirect_learned.a4 = new_gw;
-						peer->redirect_genid = redirect_genid;
-						atomic_inc(&__rt_peer_genid);
-					}
-					check_peer_redir(&rt->dst, peer);
+				if (peer && peer->redirect_learned.a4 != new_gw) {
+					peer->redirect_learned.a4 = new_gw;
+					atomic_inc(&__rt_peer_genid);
 				}
+				check_peer_redir(&rt->dst, peer);
 			}
 		}
 	}

@@ -1794,8 +1788,6 @@ static void ipv4_validate_peer(struct rtable *rt)
 		if (peer) {
 			check_peer_pmtu(&rt->dst, peer);
 
-			if (peer->redirect_genid != redirect_genid)
-				peer->redirect_learned.a4 = 0;
 			if (peer->redirect_learned.a4 &&
 			    peer->redirect_learned.a4 != rt->rt_gateway)
 				check_peer_redir(&rt->dst, peer);

@@ -1959,8 +1951,7 @@ static void rt_init_metrics(struct rtable *rt, const struct flowi4 *fl4,
 		dst_init_metrics(&rt->dst, peer->metrics, false);
 
 		check_peer_pmtu(&rt->dst, peer);
-		if (peer->redirect_genid != redirect_genid)
-			peer->redirect_learned.a4 = 0;
+
 		if (peer->redirect_learned.a4 &&
 		    peer->redirect_learned.a4 != rt->rt_gateway) {
 			rt->rt_gateway = peer->redirect_learned.a4;

Signature · 1 line

1.7.0.4

David Miller

maintainer

2012-03-05 02:55 UTC #3 lore raw

Re: [PATCH 2/2] route: Remove redirect_genid

From: Steffen Klassert <[email protected]>
Date: Thu, 1 Mar 2012 07:23:16 +0100

Quoted 5 lines

net/ipv4/route.c +0 -4

 				peer = rt->peer;
-				if (peer) {
-					if (peer->redirect_learned.a4 != new_gw ||
-					    peer->redirect_genid != redirect_genid) {
-						peer->redirect_learned.a4 = new_gw;

net/ipv4/route.c void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw, · line 1491

Quoted 10 lines

net/ipv4/route.c +4 -4

-						peer->redirect_genid = redirect_genid;
-						atomic_inc(&__rt_peer_genid);
-					}
-					check_peer_redir(&rt->dst, peer);
+				if (peer && peer->redirect_learned.a4 != new_gw) {
+					peer->redirect_learned.a4 = new_gw;
+					atomic_inc(&__rt_peer_genid);
 				}
+				check_peer_redir(&rt->dst, peer);
 			}

If peer is NULL you cannot call check_peer_redir() because it assumes that the passed in peer is non-NULL and dereferences it unconditionally.

Please keep the conditionals as they were, the check_peer_redir() call was intentionally protected by the if (peer) check, and that way you won't be introducing this bug.

Steffen Klassert

author

2012-03-06 08:06 UTC #4 lore raw

Re: [PATCH 2/2] route: Remove redirect_genid

Quoted 13 lines · David Miller

From: Steffen Klassert <[email protected]>
Date: Thu, 1 Mar 2012 07:23:16 +0100

Quoted 10 lines

net/ipv4/route.c +1 -8

 				peer = rt->peer;
-				if (peer) {
-					if (peer->redirect_learned.a4 != new_gw ||
-					    peer->redirect_genid != redirect_genid) {
-						peer->redirect_learned.a4 = new_gw;
-						peer->redirect_genid = redirect_genid;
-						atomic_inc(&__rt_peer_genid);
-					}
-					check_peer_redir(&rt->dst, peer);
+				if (peer && peer->redirect_learned.a4 != new_gw) {

Quoted 9 lines

net/ipv4/route.c void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw, · line 1491
Quoted 5 lines
net/ipv4/route.c +3 -0
+					peer->redirect_learned.a4 = new_gw;
+					atomic_inc(&__rt_peer_genid);
 				}
+				check_peer_redir(&rt->dst, peer);
 			}
If peer is NULL you cannot call check_peer_redir() because it assumes that the passed in peer is non-NULL and dereferences it unconditionally.

Ugh, indeed. I'll fix it and resend the patchset.
Thanks for the review!