Skip to contentSkip to conversation

ipv4: Restart rt_intern_hash after emergency rebuild

PATCH Applied Pavel Emelyanov 3 participants 6 messages Mar 24, 2010 – Mar 27, 2010

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Pavel Emelyanov

author
#0 lore raw

PATCH 2/2 ipv4: Restart rt_intern_hash after emergency rebuild

The the rebuild changes the genid which in turn is used at the hash calculation. Thus if we don't restart and go on with the inserting the rt will happen in wrong chain.

This doesn't seem critical to me, since the proper rt entry will be hashed at next skb.

Signed-off-by Pavel Emelyanov <[email protected]>
---
 net/ipv4/route.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)
net/ipv4/route.c +12 -6
index 28c1c2f..bbce53e 100644
@@ -1097,7 +1097,7 @@ static int slow_chain_length(const struct rtable *head)
 }
 
 static int rt_intern_hash(unsigned hash, struct rtable *rt,
-			  struct rtable **rp, struct sk_buff *skb)
+			  struct rtable **rp, struct sk_buff *skb, int in)
 {
 	struct rtable	*rth, **rthp;
 	unsigned long	now;

@@ -1217,6 +1217,12 @@ restart:
 					rt->u.dst.dev->name, num);
 			}
 			rt_emergency_hash_rebuild(net);
+			spin_unlock_bh(rt_hash_lock_addr(hash));
+
+			hash = rt_hash(rt->fl.fl4_dst, rt->fl.fl4_src,
+					in ? rt->fl.iif : rt->fl.oif,
+					rt_genid(net));
+			goto restart;
 		}
 	}
 

@@ -1477,7 +1483,7 @@ void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
 							&netevent);
 
 				rt_del(hash, rth);
-				if (!rt_intern_hash(hash, rt, &rt, NULL))
+				if (!rt_intern_hash(hash, rt, &rt, NULL, 0))
 					ip_rt_put(rt);
 				goto do_next;
 			}

@@ -1930,7 +1936,7 @@ static int ip_route_input_mc(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 
 	in_dev_put(in_dev);
 	hash = rt_hash(daddr, saddr, dev->ifindex, rt_genid(dev_net(dev)));
-	return rt_intern_hash(hash, rth, NULL, skb);
+	return rt_intern_hash(hash, rth, NULL, skb, 1);
 
 e_nobufs:
 	in_dev_put(in_dev);

@@ -2097,7 +2103,7 @@ static int ip_mkroute_input(struct sk_buff *skb,
 	/* put it into the cache */
 	hash = rt_hash(daddr, saddr, fl->iif,
 		       rt_genid(dev_net(rth->u.dst.dev)));
-	return rt_intern_hash(hash, rth, NULL, skb);
+	return rt_intern_hash(hash, rth, NULL, skb, 1);
 }
 
 /*

@@ -2254,7 +2260,7 @@ local_input:
 	}
 	rth->rt_type	= res.type;
 	hash = rt_hash(daddr, saddr, fl.iif, rt_genid(net));
-	err = rt_intern_hash(hash, rth, NULL, skb);
+	err = rt_intern_hash(hash, rth, NULL, skb, 1);
 	goto done;
 
 no_route:

@@ -2501,7 +2507,7 @@ static int ip_mkroute_output(struct rtable **rp,
 	if (err == 0) {
 		hash = rt_hash(oldflp->fl4_dst, oldflp->fl4_src, oldflp->oif,
 			       rt_genid(dev_net(dev_out)));
-		err = rt_intern_hash(hash, rth, rp, NULL);
+		err = rt_intern_hash(hash, rth, rp, NULL, 0);
 	}
 
 	return err;
#1 lore raw
Quoted 27 lines · Pavel Emelyanov

The the rebuild changes the genid which in turn is used at the hash calculation. Thus if we don't restart and go on with the inserting the rt will happen in wrong chain.

This doesn't seem critical to me, since the proper rt entry will be hashed at next skb.

Signed-off-by Pavel Emelyanov <[email protected]>
---
 net/ipv4/route.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)
net/ipv4/route.c +1 -1
index 28c1c2f..bbce53e 100644
@@ -1097,7 +1097,7 @@ static int slow_chain_length(const struct rtable *head)
 }
 
 static int rt_intern_hash(unsigned hash, struct rtable *rt,
-			  struct rtable **rp, struct sk_buff *skb)
+			  struct rtable **rp, struct sk_buff *skb, int in)
 {
 	struct rtable	*rth, **rthp;
 	unsigned long	now;

@@ -1217,6 +1217,12 @@ restart:
 					rt->u.dst.dev->name, num);

net/ipv4/route.c restart: · line 1227

Quoted 10 lines
net/ipv4/route.c +6 -0
 			}
 			rt_emergency_hash_rebuild(net);
+			spin_unlock_bh(rt_hash_lock_addr(hash));
+
+			hash = rt_hash(rt->fl.fl4_dst, rt->fl.fl4_src,
+					in ? rt->fl.iif : rt->fl.oif,
+					rt_genid(net));
+			goto restart;
 		}
 	}

Why pass in a flag here? Perhaps it would be easier just to pass in the ifindex that was origionally used by the caller of rt_intern_hash so that the hash could be redone without needing to toggle on the value of the flag.

Regards
Neil

Quoted 1 line
author
#2 lore raw
Quoted 7 lines
Quoted 7 lines
net/ipv4/route.c +3 -0
@@ -1217,6 +1217,12 @@ restart:
 					rt->u.dst.dev->name, num);
 			}
 			rt_emergency_hash_rebuild(net);
+			spin_unlock_bh(rt_hash_lock_addr(hash));
+
+			hash = rt_hash(rt->fl.fl4_dst, rt->fl.fl4_src,
Quoted 8 lines

net/ipv4/route.c restart: · line 1227

Quoted 5 lines
net/ipv4/route.c +3 -0
+					in ? rt->fl.iif : rt->fl.oif,
+					rt_genid(net));
+			goto restart;
 		}
 	}

Why pass in a flag here? Perhaps it would be easier just to pass in the ifindex that was origionally used by the caller of rt_intern_hash so that the hash could be redone without needing to toggle on the value of the flag.

That's perfectly fine with me. I'll resend the patch shortly.

Quoted 5 lines

Regards
Neil

Quoted 1 line
author
#3 lore raw

PATCH 2/2 ipv4: Restart rt_intern_hash after emergency rebuild (v2)

In reply to Pavel Emelyanov

The the rebuild changes the genid which in turn is used at the hash calculation. Thus if we don't restart and go on with inserting the rt will happen in wrong chain.

(Fixed Neil's comment about the index passed into the rt_intern_hash)

Signed-off-by Pavel Emelyanov <[email protected]>
---
 net/ipv4/route.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)
net/ipv4/route.c +11 -6
index 28c1c2f..54ddc84 100644
@@ -1097,7 +1097,7 @@ static int slow_chain_length(const struct rtable *head)
 }
 
 static int rt_intern_hash(unsigned hash, struct rtable *rt,
-			  struct rtable **rp, struct sk_buff *skb)
+			  struct rtable **rp, struct sk_buff *skb, int ifindex)
 {
 	struct rtable	*rth, **rthp;
 	unsigned long	now;

@@ -1217,6 +1217,11 @@ restart:
 					rt->u.dst.dev->name, num);
 			}
 			rt_emergency_hash_rebuild(net);
+			spin_unlock_bh(rt_hash_lock_addr(hash));
+
+			hash = rt_hash(rt->fl.fl4_dst, rt->fl.fl4_src,
+					ifindex, rt_genid(net));
+			goto restart;
 		}
 	}
 

@@ -1477,7 +1482,7 @@ void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
 							&netevent);
 
 				rt_del(hash, rth);
-				if (!rt_intern_hash(hash, rt, &rt, NULL))
+				if (!rt_intern_hash(hash, rt, &rt, NULL, rt->fl.oif))
 					ip_rt_put(rt);
 				goto do_next;
 			}

@@ -1930,7 +1935,7 @@ static int ip_route_input_mc(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 
 	in_dev_put(in_dev);
 	hash = rt_hash(daddr, saddr, dev->ifindex, rt_genid(dev_net(dev)));
-	return rt_intern_hash(hash, rth, NULL, skb);
+	return rt_intern_hash(hash, rth, NULL, skb, dev->ifindex);
 
 e_nobufs:
 	in_dev_put(in_dev);

@@ -2097,7 +2102,7 @@ static int ip_mkroute_input(struct sk_buff *skb,
 	/* put it into the cache */
 	hash = rt_hash(daddr, saddr, fl->iif,
 		       rt_genid(dev_net(rth->u.dst.dev)));
-	return rt_intern_hash(hash, rth, NULL, skb);
+	return rt_intern_hash(hash, rth, NULL, skb, fl->iif);
 }
 
 /*

@@ -2254,7 +2259,7 @@ local_input:
 	}
 	rth->rt_type	= res.type;
 	hash = rt_hash(daddr, saddr, fl.iif, rt_genid(net));
-	err = rt_intern_hash(hash, rth, NULL, skb);
+	err = rt_intern_hash(hash, rth, NULL, skb, fl.iif);
 	goto done;
 
 no_route:

@@ -2501,7 +2506,7 @@ static int ip_mkroute_output(struct rtable **rp,
 	if (err == 0) {
 		hash = rt_hash(oldflp->fl4_dst, oldflp->fl4_src, oldflp->oif,
 			       rt_genid(dev_net(dev_out)));
-		err = rt_intern_hash(hash, rth, rp, NULL);
+		err = rt_intern_hash(hash, rth, rp, NULL, oldflp->oif);
 	}
 
 	return err;
Signature · 1 line
1.5.5.6
Reviewed-by on v1 2/2
#4 lore raw

Re: [PATCH 2/2] ipv4: Restart rt_intern_hash after emergency rebuild (v2)

Quoted 7 lines · Pavel Emelyanov

The the rebuild changes the genid which in turn is used at the hash calculation. Thus if we don't restart and go on with inserting the rt will happen in wrong chain.

(Fixed Neil's comment about the index passed into the rt_intern_hash)

Signed-off-by Pavel Emelyanov <[email protected]>

Thanks! I think this is pretty reasonable.

Reviewed-by Neil Horman <[email protected]>
maintainer Applied
#5 lore raw

Re: [PATCH 2/2] ipv4: Restart rt_intern_hash after emergency rebuild (v2)

From: Neil Horman <[email protected]>
Date: Thu, 25 Mar 2010 09:17:21 -0400

Quoted 10 lines · Pavel Emelyanov
Quoted 7 lines

The the rebuild changes the genid which in turn is used at the hash calculation. Thus if we don't restart and go on with inserting the rt will happen in wrong chain.

(Fixed Neil's comment about the index passed into the rt_intern_hash)

Signed-off-by Pavel Emelyanov <[email protected]>

Thanks! I think this is pretty reasonable.

Reviewed-by Neil Horman <[email protected]>

Applied, thanks.