[PATCH] net: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[PATCH] net: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Nagendra Tomar]

inet sockets corresponding to passive connections are added to the bind hash
using ___inet_inherit_port(). These sockets are later removed from the bind 
hash using __inet_put_port(). These two functions are not exactly symmetrical. 
__inet_put_port() decrements hashinfo->bsockets and tb->num_owners, whereas 
___inet_inherit_port() does not increment them. This results in both of these 
going to -ve values.

This patch fixes this by calling inet_bind_hash() from ___inet_inherit_port(),
which does the right thing.

Signed-off-by: Nagendra Singh Tomar <tomer_iisc@yahoo.com>

---
--- linux-2.6.36.1/net/ipv4/inet_hashtables.c.orig	2010-11-25 14:56:37.902456597 -0500
+++ linux-2.6.36.1/net/ipv4/inet_hashtables.c	2010-11-25 15:44:15.038403317 -0500
@@ -107,12 +107,10 @@ void __inet_inherit_port(struct sock *sk
 	const int bhash = inet_bhashfn(sock_net(sk), inet_sk(child)->inet_num,
 			table->bhash_size);
 	struct inet_bind_hashbucket *head = &table->bhash[bhash];
-	struct inet_bind_bucket *tb;
 
 	spin_lock(&head->lock);
-	tb = inet_csk(sk)->icsk_bind_hash;
-	sk_add_bind_node(child, &tb->owners);
-	inet_csk(child)->icsk_bind_hash = tb;
+	inet_bind_hash(child, inet_csk(sk)->icsk_bind_hash, 
+			inet_sk(child)->inet_num);
 	spin_unlock(&head->lock);
 }
 EXPORT_SYMBOL_GPL(__inet_inherit_port);
---



      

Re: [PATCH] net: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Eric Dumazet]

Le jeudi 25 novembre 2010 à 21:09 -0800, Nagendra Tomar a écrit :
> inet sockets corresponding to passive connections are added to the bind hash
> using ___inet_inherit_port(). These sockets are later removed from the bind 
> hash using __inet_put_port(). These two functions are not exactly symmetrical. 
> __inet_put_port() decrements hashinfo->bsockets and tb->num_owners, whereas 
> ___inet_inherit_port() does not increment them. This results in both of these 
> going to -ve values.
> 
> This patch fixes this by calling inet_bind_hash() from ___inet_inherit_port(),
> which does the right thing.
> 
> Signed-off-by: Nagendra Singh Tomar <tomer_iisc@yahoo.com>
> 
> ---
> --- linux-2.6.36.1/net/ipv4/inet_hashtables.c.orig	2010-11-25 14:56:37.902456597 -0500
> +++ linux-2.6.36.1/net/ipv4/inet_hashtables.c	2010-11-25 15:44:15.038403317 -0500
> @@ -107,12 +107,10 @@ void __inet_inherit_port(struct sock *sk
>  	const int bhash = inet_bhashfn(sock_net(sk), inet_sk(child)->inet_num,
>  			table->bhash_size);
>  	struct inet_bind_hashbucket *head = &table->bhash[bhash];
> -	struct inet_bind_bucket *tb;
>  
>  	spin_lock(&head->lock);
> -	tb = inet_csk(sk)->icsk_bind_hash;
> -	sk_add_bind_node(child, &tb->owners);
> -	inet_csk(child)->icsk_bind_hash = tb;
> +	inet_bind_hash(child, inet_csk(sk)->icsk_bind_hash, 
> +			inet_sk(child)->inet_num);
>  	spin_unlock(&head->lock);
>  }
>  EXPORT_SYMBOL_GPL(__inet_inherit_port);
> ---
> 

Interesting, how did you notice it is wrong ?

Re: [PATCH] net: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Nagendra Tomar]

--- On Fri, 26/11/10, Eric Dumazet <eric.dumazet@gmail.com> wrote:

> 
> Interesting, how did you notice it is wrong ?
> 

Bumped on it while reading related code. 


      

Re: [PATCH] net: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Eric Dumazet]

Le jeudi 25 novembre 2010 à 23:49 -0800, Nagendra Tomar a écrit :
> 
> --- On Fri, 26/11/10, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> > 
> > Interesting, how did you notice it is wrong ?
> > 
> 
> Bumped on it while reading related code. 
> 
> 
>       


OK so you'll have to make a proof, because current code seems to work ;)

Re: [PATCH] net: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Nagendra Tomar]

--- On Fri, 26/11/10, Eric Dumazet <eric.dumazet@gmail.com> wrote:

> 
> OK so you'll have to make a proof, because current code
> seems to work ;)
> 
> 

ok, so I printed hashinfo->bsockets and tb->num_owners inside __inet_put_port() and I could see both of them to be -ve. All we need to do is establish and terminate a connection. I used netcat for that.

The only place 'bsockets' and 'num_owners' are used is inet_csk_get_port() and the only effect that they might have is on the choice of the port to be used for binding. 
'bsockets' is used as a hint to stop searching for a free port (and instead share an already used port) when we know that all the ports could be used up.
'num_owners' is used to find the port which is least shared (to balance the 'owners' list) in case we need to share a port.

Since both of these are used as optimizations (in the bind path), they do not affect correctness and hence the code works even with these values not being updated correctly.

Thanks,
Tomar


      

Re: [PATCH] net: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Eric Dumazet]

From: Nagendra Tomar <tomer_iisc@yahoo.com>

Le vendredi 26 novembre 2010 à 01:40 -0800, Nagendra Tomar a écrit :
> 
> --- On Fri, 26/11/10, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> > 
> > OK so you'll have to make a proof, because current code
> > seems to work ;)
> > 
> > 
> 
> ok, so I printed hashinfo->bsockets and tb->num_owners inside
> __inet_put_port() and I could see both of them to be -ve. All we need
> to do is establish and terminate a connection. I used netcat for that.
> 
> The only place 'bsockets' and 'num_owners' are used is
> inet_csk_get_port() and the only effect that they might have is on the
> choice of the port to be used for binding. 
> 'bsockets' is used as a hint to stop searching for a free port (and
> instead share an already used port) when we know that all the ports
> could be used up.
> 'num_owners' is used to find the port which is least shared (to
> balance the 'owners' list) in case we need to share a port.
> 
> Since both of these are used as optimizations (in the bind path), they
> do not affect correctness and hence the code works even with these
> values not being updated correctly.

Hmm, thanks for clarification.

bsockets / mnum_owners iscount is indeed an 'optimization' problem.

Problem is your patch is not applicable to current tree.

In order to submit it to stable team, you should first post a patch for
next/current kernel (net-next-2.6 tree).

David will decide if its net-2.6 material or not.

You could add in your changelog the problem comes from commit 
a9d8f9110d7e953c (inet: Allowing more than 64k connections and heavily
optimize bind(0)), included in 2.6.30, to ease stable team work.

On current tree your patch would be :

diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 1b344f3..3c0369a 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -133,8 +133,7 @@ int __inet_inherit_port(struct sock *sk, struct sock *child)
 			}
 		}
 	}
-	sk_add_bind_node(child, &tb->owners);
-	inet_csk(child)->icsk_bind_hash = tb;
+	inet_bind_hash(child, tb, port);
 	spin_unlock(&head->lock);
 
 	return 0;

Re: [PATCH] net: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Nagendra Tomar]

--- On Fri, 26/11/10, Eric Dumazet <eric.dumazet@gmail.com> wrote:

> 
> Problem is your patch is not applicable to current tree.
> 
> In order to submit it to stable team, you should first post
> a patch for
> next/current kernel (net-next-2.6 tree).
> 

Thanks, Erik.
I'd made the patch against 2.6.36.1 which is the latest stable kernel per kernel.org. I thought that was the right kernel version to make a patch against.
I do not use git. Shall I make a patch against linux-next as it appears in kernel.org.


> David will decide if its net-2.6 material or not.
> 
> You could add in your changelog the problem comes from
> commit 
> a9d8f9110d7e953c (inet: Allowing more than 64k connections
> and heavily
> optimize bind(0)), included in 2.6.30, to ease stable team
> work.
> 
> On current tree your patch would be :
> 
> diff --git a/net/ipv4/inet_hashtables.c
> b/net/ipv4/inet_hashtables.c
> index 1b344f3..3c0369a 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -133,8 +133,7 @@ int __inet_inherit_port(struct sock
> *sk, struct sock *child)
>             
> }
>          }
>      }
> -    sk_add_bind_node(child,
> &tb->owners);
> -    inet_csk(child)->icsk_bind_hash =
> tb;
> +    inet_bind_hash(child, tb, port);
>      spin_unlock(&head->lock);
>  
>      return 0;
> 
> 
> 

Thanks,
Tomar


      

Re: [PATCH] net: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Evgeniy Polyakov]

Hi.

On Fri, Nov 26, 2010 at 11:47:57AM +0100, Eric Dumazet (eric.dumazet@gmail.com) wrote:
> > ok, so I printed hashinfo->bsockets and tb->num_owners inside
> > __inet_put_port() and I could see both of them to be -ve. All we need
> > to do is establish and terminate a connection. I used netcat for that.
> > 
> > The only place 'bsockets' and 'num_owners' are used is
> > inet_csk_get_port() and the only effect that they might have is on the
> > choice of the port to be used for binding. 
> > 'bsockets' is used as a hint to stop searching for a free port (and
> > instead share an already used port) when we know that all the ports
> > could be used up.
> > 'num_owners' is used to find the port which is least shared (to
> > balance the 'owners' list) in case we need to share a port.
> > 
> > Since both of these are used as optimizations (in the bind path), they
> > do not affect correctness and hence the code works even with these
> > values not being updated correctly.
> 
> Hmm, thanks for clarification.
> 
> bsockets / mnum_owners iscount is indeed an 'optimization' problem.
> 
> Problem is your patch is not applicable to current tree.
> 
> In order to submit it to stable team, you should first post a patch for
> next/current kernel (net-next-2.6 tree).
> 
> David will decide if its net-2.6 material or not.
> 
> You could add in your changelog the problem comes from commit 
> a9d8f9110d7e953c (inet: Allowing more than 64k connections and heavily
> optimize bind(0)), included in 2.6.30, to ease stable team work.

Frankly I did not find how those optimizations made a bug as well as
what is this bug about from given description, but I'm glad it is resolved now :)

-- 
	Evgeniy Polyakov

Re: [PATCH] net: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Nagendra Tomar]

--- On Fri, 26/11/10, Evgeniy Polyakov <zbr@ioremap.net> wrote:

> Frankly I did not find how those optimizations made a bug
> as well as
> what is this bug about from given description, but I'm glad
> it is resolved now :)
> 

I'm not sure of what all went into the "optimization" patch, but the bug is not due to the optimization per-se. As my original post says, it is due to the 'bsockets' and 'num_owners' not being incremented inside __inet_inherit_port(), where it should have been, as __inet_put_port() decrements them on port release, which causes the imbalance.

Thanks,
Tomar


      

Re: [PATCH] net: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Evgeniy Polyakov]

On Fri, Nov 26, 2010 at 03:20:31AM -0800, Nagendra Tomar (tomer_iisc@yahoo.com) wrote:
> > Frankly I did not find how those optimizations made a bug
> > as well as
> > what is this bug about from given description, but I'm glad
> > it is resolved now :)
> > 
> 
> I'm not sure of what all went into the "optimization" patch, but the bug is not due to the optimization per-se. As my original post says, it is due to the 'bsockets' and 'num_owners' not being incremented inside __inet_inherit_port(), where it should have been, as __inet_put_port() decrements them on port release, which causes the imbalance.

Argh, I see, thanks a lot for explanation!

-- 
	Evgeniy Polyakov

Re: [PATCH] net-next: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Nagendra Tomar]

Ok, here is the patch against net-next.

Thanks,
Tomar


---
inet sockets corresponding to passive connections are added to the bind hash
using ___inet_inherit_port(). These sockets are later removed from the bind 
hash using __inet_put_port(). These two functions are not exactly symmetrical. 
__inet_put_port() decrements hashinfo->bsockets and tb->num_owners, whereas 
___inet_inherit_port() does not increment them. This results in both of these 
going to -ve values.

This patch fixes this by calling inet_bind_hash() from ___inet_inherit_port(),
which does the right thing.

'bsockets' and 'num_owners' were introduced by commit a9d8f9110d7e953c 
(inet: Allowing more than 64k connections and heavily optimize bind(0))

Signed-off-by: Nagendra Singh Tomar <tomer_iisc@yahoo.com>

---
--- linux-2.6.37-rc3/net/ipv4/inet_hashtables.c.orig	2010-11-26 13:28:51.034811940 -0500
+++ linux-2.6.37-rc3/net/ipv4/inet_hashtables.c	2010-11-26 14:41:41.006268035 -0500
@@ -133,8 +133,7 @@ int __inet_inherit_port(struct sock *sk,
 			}
 		}
 	}
-	sk_add_bind_node(child, &tb->owners);
-	inet_csk(child)->icsk_bind_hash = tb;
+	inet_bind_hash(child, tb, port);
 	spin_unlock(&head->lock);
 
 	return 0;
---