race in net/ipv4/ipip.c ?

race in net/ipv4/ipip.c ?

[Lennert Buytenhek]

Hi!

	static void ipip_tunnel_link(struct ipip_tunnel *t)
	{
		struct ipip_tunnel **tp = ipip_bucket(t);

		t->next = *tp;
		write_lock_bh(&ipip_lock);
		*tp = t;
		write_unlock_bh(&ipip_lock);
	}

Shouldn't the "t->next = *tp" be done inside the write lock?

A similar race exists in ipip_tunnel_unlink, and ip_gre seems to have
the same issues.


cheers,
Lennert

Re: race in net/ipv4/ipip.c ?

[Thomas Graf]

* Lennert Buytenhek <20050112122300.GA12155@xi.wantstofly.org> 2005-01-12 13:23
> Hi!
> 
> 	static void ipip_tunnel_link(struct ipip_tunnel *t)
> 	{
> 		struct ipip_tunnel **tp = ipip_bucket(t);
> 
> 		t->next = *tp;
> 		write_lock_bh(&ipip_lock);
> 		*tp = t;
> 		write_unlock_bh(&ipip_lock);
> 	}
> 
> Shouldn't the "t->next = *tp" be done inside the write lock?

Why do you think so? linking may only happen on new tunnels
so they can't be found before they're assigned to the bucket.
dev_hold is called correctly so dev->priv usage is safe as
well.

Re: race in net/ipv4/ipip.c ?

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20050112125336.GH26856@postel.suug.ch> (at Wed, 12 Jan 2005 13:53:36 +0100), Thomas Graf <tgraf@suug.ch> says:

> * Lennert Buytenhek <20050112122300.GA12155@xi.wantstofly.org> 2005-01-12 13:23
:
> > 	static void ipip_tunnel_link(struct ipip_tunnel *t)
> > 	{
> > 		struct ipip_tunnel **tp = ipip_bucket(t);
> > 
> > 		t->next = *tp;
> > 		write_lock_bh(&ipip_lock);
> > 		*tp = t;
> > 		write_unlock_bh(&ipip_lock);
> > 	}
> > 
> > Shouldn't the "t->next = *tp" be done inside the write lock?
> 
> Why do you think so? linking may only happen on new tunnels
> so they can't be found before they're assigned to the bucket.
> dev_hold is called correctly so dev->priv usage is safe as
> well.

How about adding multiple tunnels (with same ipip_bucket(t))
concurrently? :-)

--yoshfuji

Re: race in net/ipv4/ipip.c ?

[Thomas Graf]

* YOSHIFUJI Hideaki / ?$B5HF#1QL@ <20050112.220816.56650893.yoshfuji@linux-ipv6.org> 2005-01-12 22:08
> How about adding multiple tunnels (with same ipip_bucket(t))
> concurrently? :-)
> 

* Lennert Buytenhek <20050112130940.GA12547@xi.wantstofly.org> 2005-01-12 14:09
> What if you add two tunnels at the same time?  (Or is that perhaps
> synchronised on a higher level?)

Not possible, protected via rtnl semaphore acquired in default
handler of dev_ioctl.

Re: race in net/ipv4/ipip.c ?

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20050112132126.GI26856@postel.suug.ch> (at Wed, 12 Jan 2005 14:21:26 +0100), Thomas Graf <tgraf@suug.ch> says:

> * YOSHIFUJI Hideaki / ?$B5HF#1QL@ <20050112.220816.56650893.yoshfuji@linux-ipv6.org> 2005-01-12 22:08
> > How about adding multiple tunnels (with same ipip_bucket(t))
> > concurrently? :-)
> > 
> 
> * Lennert Buytenhek <20050112130940.GA12547@xi.wantstofly.org> 2005-01-12 14:09
> > What if you add two tunnels at the same time?  (Or is that perhaps
> > synchronised on a higher level?)
> 
> Not possible, protected via rtnl semaphore acquired in default
> handler of dev_ioctl.

Good.


David, I think we need to fix sit.c, anyway.

Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>

===== net/ipv6/sit.c 1.43 vs edited =====
--- 1.43/net/ipv6/sit.c	2004-10-04 07:03:07 +09:00
+++ edited/net/ipv6/sit.c	2005-01-12 22:29:32 +09:00
@@ -135,10 +135,10 @@
 {
 	struct ip_tunnel **tp = ipip6_bucket(t);
 
-	write_lock_bh(&ipip6_lock);
 	t->next = *tp;
-	write_unlock_bh(&ipip6_lock);
+	write_lock_bh(&ipip6_lock);
 	*tp = t;
+	write_unlock_bh(&ipip6_lock);
 }
 
 static struct ip_tunnel * ipip6_tunnel_locate(struct ip_tunnel_parm *parms, int create)

--yoshfuji

Re: race in net/ipv4/ipip.c ?

[David S. Miller]

On Wed, 12 Jan 2005 22:36:00 +0900 (JST)
YOSHIFUJI Hideaki / ^[$B5HF#1QL@^[(B <yoshfuji@linux-ipv6.org> wrote:

> David, I think we need to fix sit.c, anyway.
> 
> Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>

Good catch, I will apply this patch.

Re: race in net/ipv4/ipip.c ?

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20050113205407.13154ccd.davem@davemloft.net> (at Thu, 13 Jan 2005 20:54:07 -0800), "David S. Miller" <davem@davemloft.net> says:

> On Wed, 12 Jan 2005 22:36:00 +0900 (JST)
> YOSHIFUJI Hideaki / ^[$B5HF#1QL@^[(B <yoshfuji@linux-ipv6.org> wrote:
> 
> > David, I think we need to fix sit.c, anyway.
> > 
> > Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
> 
> Good catch, I will apply this patch.

Please apply against 2.4.x as well.
Thanks.

--yoshfuji

Re: race in net/ipv4/ipip.c ?

[David S. Miller]

On Wed, 12 Jan 2005 14:21:26 +0100
Thomas Graf <tgraf@suug.ch> wrote:

> * YOSHIFUJI Hideaki / ?$B5HF#1QL@ <20050112.220816.56650893.yoshfuji@linux-ipv6.org> 2005-01-12 22:08
> > How about adding multiple tunnels (with same ipip_bucket(t))
> > concurrently? :-)
> > 
> 
> * Lennert Buytenhek <20050112130940.GA12547@xi.wantstofly.org> 2005-01-12 14:09
> > What if you add two tunnels at the same time?  (Or is that perhaps
> > synchronised on a higher level?)
> 
> Not possible, protected via rtnl semaphore acquired in default
> handler of dev_ioctl.

Yeah.  Alexey does this trick everywhere, when we have the RTNL semaphore
held we only need to lock for the actual linkage operation that adds the
new object to the tree.

Re: race in net/ipv4/ipip.c ?

[Lennert Buytenhek]

On Wed, Jan 12, 2005 at 01:53:36PM +0100, Thomas Graf wrote:

> > 	static void ipip_tunnel_link(struct ipip_tunnel *t)
> > 	{
> > 		struct ipip_tunnel **tp = ipip_bucket(t);
> > 
> > 		t->next = *tp;
> > 		write_lock_bh(&ipip_lock);
> > 		*tp = t;
> > 		write_unlock_bh(&ipip_lock);
> > 	}
> > 
> > Shouldn't the "t->next = *tp" be done inside the write lock?
> 
> Why do you think so? linking may only happen on new tunnels
> so they can't be found before they're assigned to the bucket.
> dev_hold is called correctly so dev->priv usage is safe as
> well.

What if you add two tunnels at the same time?  (Or is that perhaps
synchronised on a higher level?)


--L