Skip to contentSkip to conversation

tunnels: Fix tunnels change rcu protection

PATCH Applied Pavel Emelyanov 3 participants 4 messages Oct 27, 2010

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

Pavel Emelyanov

author
#0 lore raw

PATCH 1/1 tunnels: Fix tunnels change rcu protection

After making rcu protection for tunnels (ipip, gre, sit and ip6) a bug was introduced into the SIOCCHGTUNNEL code.

The tunnel is first unlinked, then addresses change, then it is linked back probably into another bucket. But while changing the parms, the hash table is unlocked to readers and they can lookup the improper tunnel.

Respective commits are b7285b79 (ipip: get rid of ipip_lock), 1507850b (gre: get rid of ipgre_lock), 3a43be3c (sit: get rid of ipip6_lock) and 94767632 (ip6tnl: get rid of ip6_tnl_lock).

The quick fix is to wait for quiescent state to pass after unlinking, but if it is inappropriate I can invent something better, just let me know.

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

---

net/ipv4/ip_gre.c +1 -0
index d0ffcbe..01087e0 100644
@@ -1072,6 +1072,7 @@ ipgre_tunnel_ioctl (struct net_device *dev, struct ifreq *ifr, int cmd)
 					break;
 				}
 				ipgre_tunnel_unlink(ign, t);
+				synchronize_net();
 				t->parms.iph.saddr = p.iph.saddr;
 				t->parms.iph.daddr = p.iph.daddr;
 				t->parms.i_key = p.i_key;
net/ipv4/ipip.c +1 -0
index e9b816e..cd300aa 100644
@@ -676,6 +676,7 @@ ipip_tunnel_ioctl (struct net_device *dev, struct ifreq *ifr, int cmd)
 				}
 				t = netdev_priv(dev);
 				ipip_tunnel_unlink(ipn, t);
+				synchronize_net();
 				t->parms.iph.saddr = p.iph.saddr;
 				t->parms.iph.daddr = p.iph.daddr;
 				memcpy(dev->dev_addr, &p.iph.saddr, 4);
net/ipv6/ip6_tunnel.c +1 -0
index 38b9a56..2a59610 100644
@@ -1284,6 +1284,7 @@ ip6_tnl_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 				t = netdev_priv(dev);
 
 			ip6_tnl_unlink(ip6n, t);
+			synchronize_net();
 			err = ip6_tnl_change(t, &p);
 			ip6_tnl_link(ip6n, t);
 			netdev_state_change(dev);
net/ipv6/sit.c +1 -0
index 367a6cc..d6bfaec 100644
@@ -963,6 +963,7 @@ ipip6_tunnel_ioctl (struct net_device *dev, struct ifreq *ifr, int cmd)
 				}
 				t = netdev_priv(dev);
 				ipip6_tunnel_unlink(sitn, t);
+				synchronize_net();
 				t->parms.iph.saddr = p.iph.saddr;
 				t->parms.iph.daddr = p.iph.daddr;
 				memcpy(dev->dev_addr, &p.iph.saddr, 4);
#1 lore raw
Quoted 11 lines · Pavel Emelyanov

After making rcu protection for tunnels (ipip, gre, sit and ip6) a bug was introduced into the SIOCCHGTUNNEL code.

The tunnel is first unlinked, then addresses change, then it is linked back probably into another bucket. But while changing the parms, the hash table is unlocked to readers and they can lookup the improper tunnel.

Respective commits are b7285b79 (ipip: get rid of ipip_lock), 1507850b (gre: get rid of ipgre_lock), 3a43be3c (sit: get rid of ipip6_lock) and 94767632 (ip6tnl: get rid of ip6_tnl_lock).

Quoted 5 lines

The quick fix is to wait for quiescent state to pass after unlinking, but if it is inappropriate I can invent something better, just let me know.

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

Good catch, I missed a change was possible at all :(

I guess some setups could scream with this fix, since this adds a synchronize_net() call while holding RTNL ...

Hmm, maybe we should allocate a "struct ip_tunnel_parm" instead of using an embedded one (in struct ip_tunnel), and stick an rcu_head in it to delay its freeing...

Acked-by
#2 lore raw
Quoted 4 lines · Eric Dumazet

Hmm, maybe we should allocate a "struct ip_tunnel_parm" instead of using an embedded one (in struct ip_tunnel), and stick an rcu_head in it to delay its freeing...

I forgot to Ack your patch, of course.

We can implement something better when net-next-2.6 re-opens.

Acked-by Eric Dumazet <[email protected]>
maintainer Applied
#3 lore raw

From: Eric Dumazet <[email protected]>
Date: Wed, 27 Oct 2010 21:06:12 +0200

Quoted 8 lines · Eric Dumazet
Quoted 6 lines

Hmm, maybe we should allocate a "struct ip_tunnel_parm" instead of using an embedded one (in struct ip_tunnel), and stick an rcu_head in it to delay its freeing...

Quoted 5 lines

I forgot to Ack your patch, of course.

We can implement something better when net-next-2.6 re-opens.

Acked-by Eric Dumazet <[email protected]>

Applied.