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

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>

---
---



      
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 ?



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--- 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. 


      
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 ;)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--- 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


      
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html