[PATCH net] inet: add proper locking in __inet{6}_lookup()

[PATCH net] inet: add proper locking in __inet{6}_lookup()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Blocking BH in __inet{6}_lookup() is not correct, as the lookups
are done using RCU protection.

It matters only starting from Lorenzo Colitti patches to destroy
a TCP socket, since rcu_read_lock() is already held by other users
of these functions.

This can be backported to all known stable versions, since TCP got RCU
lookups back in 2.6.29 : Even if iproute2 contained no code to trigger
the bug, some user programs could have used the API.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Lorenzo Colitti <lorenzo@google.com>
---
 include/net/inet_hashtables.h |    5 ++---
 net/ipv6/inet6_hashtables.c   |    5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index 50f635c2c536..4575d4f9509a 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -284,7 +284,6 @@ static inline struct sock *inet_lookup_listener(struct net *net,
  * Sockets in TCP_CLOSE state are _always_ taken out of the hash, so we need
  * not check it for lookups anymore, thanks Alexey. -DaveM
  *
- * Local BH must be disabled here.
  */
 struct sock *__inet_lookup_established(struct net *net,
 				       struct inet_hashinfo *hashinfo,
@@ -326,10 +325,10 @@ static inline struct sock *inet_lookup(struct net *net,
 {
 	struct sock *sk;
 
-	local_bh_disable();
+	rcu_read_lock();
 	sk = __inet_lookup(net, hashinfo, skb, doff, saddr, sport, daddr,
 			   dport, dif);
-	local_bh_enable();
+	rcu_read_unlock();
 
 	return sk;
 }
diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
index 70f2628be6fa..9dcd0481fae2 100644
--- a/net/ipv6/inet6_hashtables.c
+++ b/net/ipv6/inet6_hashtables.c
@@ -200,11 +200,10 @@ struct sock *inet6_lookup(struct net *net, struct inet_hashinfo *hashinfo,
 {
 	struct sock *sk;
 
-	local_bh_disable();
+	rcu_read_lock();
 	sk = __inet6_lookup(net, hashinfo, skb, doff, saddr, sport, daddr,
 			    ntohs(dport), dif);
-	local_bh_enable();
-
+	rcu_read_unlock();
 	return sk;
 }
 EXPORT_SYMBOL_GPL(inet6_lookup);

Re: [PATCH net] inet: add proper locking in __inet{6}_lookup()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 25 Mar 2016 15:15:15 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> Blocking BH in __inet{6}_lookup() is not correct, as the lookups
> are done using RCU protection.
> 
> It matters only starting from Lorenzo Colitti patches to destroy
> a TCP socket, since rcu_read_lock() is already held by other users
> of these functions.
> 
> This can be backported to all known stable versions, since TCP got RCU
> lookups back in 2.6.29 : Even if iproute2 contained no code to trigger
> the bug, some user programs could have used the API.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Lorenzo Colitti <lorenzo@google.com>

This is quite the maze of RCU locking here.  With this change,
inet_lookup is now:

	rcu_read_lock();
	sk = x(); {
		rcu_read_lock();
		...
		rcu_read_unlock();
	}
	if (!sk) {
		sk = y(); {
			rcu_read_lock();
			...
			rcu_read_unlock();
		}
	}
	rcu_read_unlock();

It would seem to me that we should bubble up the rcu locking calls.

If, as you say, the other users do RCU locking already, then it should
be safe to do what your patch is doing and also remove the RCU locking
done by __inet_lookup_established() and __inet_lookup_listener().

Re: [PATCH net] inet: add proper locking in __inet{6}_lookup()

[Eric Dumazet]

On Sun, 2016-03-27 at 22:32 -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Fri, 25 Mar 2016 15:15:15 -0700
> 
> > From: Eric Dumazet <edumazet@google.com>
> > 
> > Blocking BH in __inet{6}_lookup() is not correct, as the lookups
> > are done using RCU protection.
> > 
> > It matters only starting from Lorenzo Colitti patches to destroy
> > a TCP socket, since rcu_read_lock() is already held by other users
> > of these functions.
> > 
> > This can be backported to all known stable versions, since TCP got RCU
> > lookups back in 2.6.29 : Even if iproute2 contained no code to trigger
> > the bug, some user programs could have used the API.
> > 
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Lorenzo Colitti <lorenzo@google.com>
> 
> This is quite the maze of RCU locking here.  With this change,
> inet_lookup is now:
> 
> 	rcu_read_lock();
> 	sk = x(); {
> 		rcu_read_lock();
> 		...
> 		rcu_read_unlock();
> 	}
> 	if (!sk) {
> 		sk = y(); {
> 			rcu_read_lock();
> 			...
> 			rcu_read_unlock();
> 		}
> 	}
> 	rcu_read_unlock();
> 
> It would seem to me that we should bubble up the rcu locking calls.
> 
> If, as you say, the other users do RCU locking already, then it should
> be safe to do what your patch is doing and also remove the RCU locking
> done by __inet_lookup_established() and __inet_lookup_listener().
> 

Sure, but the caller changed quite a lot in all stable versions.

I took the easiest path for stable maintainers, and was planing to
implement a better way in net-next.

I certainly can take the final form and let you do the backports ?

Thanks.

Re: [PATCH net] inet: add proper locking in __inet{6}_lookup()

[Eric Dumazet]

On Mon, 2016-03-28 at 06:29 -0700, Eric Dumazet wrote:

> Sure, but the caller changed quite a lot in all stable versions.
> 
> I took the easiest path for stable maintainers, and was planing to
> implement a better way in net-next.

I misread your suggestion David, I send a V2 shortly.

Re: [PATCH net] inet: add proper locking in __inet{6}_lookup()

[Eric Dumazet]

On Mon, 2016-03-28 at 07:10 -0700, Eric Dumazet wrote:
> On Mon, 2016-03-28 at 06:29 -0700, Eric Dumazet wrote:
> 
> > Sure, but the caller changed quite a lot in all stable versions.
> > 
> > I took the easiest path for stable maintainers, and was planing to
> > implement a better way in net-next.
> 
> I misread your suggestion David, I send a V2 shortly.
> 

Actually, I'll wait for net-next opening.

My brain farted while working on the 'non refcounted TCP listener'
because __inet_lookup_listener() will really need to be called from
enclosed rcu_read_lock(), and for a reason I though net tree had a bug.

The local_bh_disable()/local_bh_enable() removal can certainly wait
net-next.

Sorry for the confusion.

Re: [PATCH net] inet: add proper locking in __inet{6}_lookup()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 28 Mar 2016 07:23:55 -0700

> On Mon, 2016-03-28 at 07:10 -0700, Eric Dumazet wrote:
>> On Mon, 2016-03-28 at 06:29 -0700, Eric Dumazet wrote:
>> 
>> > Sure, but the caller changed quite a lot in all stable versions.
>> > 
>> > I took the easiest path for stable maintainers, and was planing to
>> > implement a better way in net-next.
>> 
>> I misread your suggestion David, I send a V2 shortly.
>> 
> 
> Actually, I'll wait for net-next opening.
> 
> My brain farted while working on the 'non refcounted TCP listener'
> because __inet_lookup_listener() will really need to be called from
> enclosed rcu_read_lock(), and for a reason I though net tree had a bug.
> 
> The local_bh_disable()/local_bh_enable() removal can certainly wait
> net-next.
> 
> Sorry for the confusion.

Ok, I'll mark this patch as deferred then.

Thanks Eric.