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

[PATCH] net-next: 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.

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



      

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

[Eric Dumazet]

Le vendredi 26 novembre 2010 à 16:26 -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.
> 
> '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;

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Thanks !

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

[Evgeniy Polyakov]

Hi.

On Fri, Nov 26, 2010 at 04:26:27PM -0800, Nagendra Tomar (tomer_iisc@yahoo.com) wrote:
> 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))

Yup, things changed from that simple patch a lot.
Thanks for fixing it up.
Ack.

-- 
	Evgeniy Polyakov

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

[David Miller]

From: Evgeniy Polyakov <zbr@ioremap.net>
Date: Mon, 29 Nov 2010 02:00:41 +0300

> Hi.
> 
> On Fri, Nov 26, 2010 at 04:26:27PM -0800, Nagendra Tomar (tomer_iisc@yahoo.com) wrote:
>> 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))
> 
> Yup, things changed from that simple patch a lot.
> Thanks for fixing it up.
> Ack.

I've decided to apply this to net-2.6, thanks everyone.

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

[Jarek Poplawski]

On 2010-11-29 00:00, Evgeniy Polyakov wrote:
> Hi.
> 
> On Fri, Nov 26, 2010 at 04:26:27PM -0800, Nagendra Tomar (tomer_iisc@yahoo.com) wrote:
>> 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))
> 
> Yup, things changed from that simple patch a lot.
> Thanks for fixing it up.
> Ack.

Probably I miss something, but since bsockets is increased by each
passive connection now, it seems it will trigger "hash table is full"
too early?

Jarek P.

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

[Eric Dumazet]

Le lundi 29 novembre 2010 à 12:39 +0000, Jarek Poplawski a écrit :

> Probably I miss something, but since bsockets is increased by each
> passive connection now, it seems it will trigger "hash table is full"
> too early?
> 

bsockets is the number of bound sockets.

It must be increased for each passive connection, since they are
bound :)

The value of bsockets is used to speedup bind() syscall, if few sockets
are bound. If not, no optimization takes place.

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

[Evgeniy Polyakov]

On Mon, Nov 29, 2010 at 12:39:09PM +0000, Jarek Poplawski (jarkao2@gmail.com) wrote:
> >> 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))
> > 
> > Yup, things changed from that simple patch a lot.
> > Thanks for fixing it up.
> > Ack.
> 
> Probably I miss something, but since bsockets is increased by each
> passive connection now, it seems it will trigger "hash table is full"
> too early?

Why would it? bsockets and num_owners are supposed to be increased for each
new socket added into the table, and are used as a hint to find a bucket with
the smallest number of sockets in it.

Hash table insertion did not change, only bucket selection algorithm got
a hint.

-- 
	Evgeniy Polyakov

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

[Jarek Poplawski]

On Mon, Nov 29, 2010 at 03:51:02PM +0300, Evgeniy Polyakov wrote:
> On Mon, Nov 29, 2010 at 12:39:09PM +0000, Jarek Poplawski (jarkao2@gmail.com) wrote:
> > >> 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))
> > > 
> > > Yup, things changed from that simple patch a lot.
> > > Thanks for fixing it up.
> > > Ack.
> > 
> > Probably I miss something, but since bsockets is increased by each
> > passive connection now, it seems it will trigger "hash table is full"
> > too early?
> 
> Why would it? bsockets and num_owners are supposed to be increased for each
> new socket added into the table, and are used as a hint to find a bucket with
> the smallest number of sockets in it.
> 
> Hash table insertion did not change, only bucket selection algorithm got
> a hint.

Evgeniy & Eric,

But it's compared to the numer of available port numbers in
inet_csk_get_port():

     "if (atomic_read(&hashinfo->bsockets) > (high - low) + 1)"

Can't you have bsockets higher than this with only one port used?

Jarek P.

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

[Eric Dumazet]

Le lundi 29 novembre 2010 à 13:12 +0000, Jarek Poplawski a écrit :

> Evgeniy & Eric,
> 
> But it's compared to the numer of available port numbers in
> inet_csk_get_port():
> 
>      "if (atomic_read(&hashinfo->bsockets) > (high - low) + 1)"
> 
> Can't you have bsockets higher than this with only one port used?
> 

Because we store tuples, not only port information.

You can have many sockets bound on a single port.

This 'optimization' was only meaningful on a machine you make only
active connections, if you ask me...

Problem is, as soon as some passive connections are done, bsockets count
becomes wrong. The patch fixes this, thats all.

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

[Jarek Poplawski]

On Mon, Nov 29, 2010 at 02:17:46PM +0100, Eric Dumazet wrote:
> Le lundi 29 novembre 2010 ?? 13:12 +0000, Jarek Poplawski a écrit :
> 
> > Evgeniy & Eric,

Eric & Evgeniy ;-)

> > But it's compared to the numer of available port numbers in
> > inet_csk_get_port():
> > 
> >      "if (atomic_read(&hashinfo->bsockets) > (high - low) + 1)"
> > 
> > Can't you have bsockets higher than this with only one port used?
> > 
> 
> Because we store tuples, not only port information.
> 
> You can have many sockets bound on a single port.
> 
> This 'optimization' was only meaningful on a machine you make only
> active connections, if you ask me...

That's what I've missed...

> Problem is, as soon as some passive connections are done, bsockets count
> becomes wrong. The patch fixes this, thats all.

Sure, the numbers are correct now. I wondered about this optimization
only.

Thanks for explanations,
Jarek P.

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