Re: virt-manager broken by bind(0) in net-next.

[Eric Dumazet <dada1@cosmosbay.com>]

Evgeniy Polyakov a écrit :
> On Fri, Jan 30, 2009 at 07:41:59PM +0100, Eric Dumazet (dada1@cosmosbay.com) wrote:
>> Reviewing commit a9d8f9110d7e953c2f2b521087a4179677843c2a
>>
>> I see use of a hashinfo->bsockets field that :
>>
>> - lacks proper lock/synchronization
> 
> It should contain rough number of sockets, there is no need to be very
> precise because of this hueristic.

Denying there is a bug is... well... I dont know what to say.

I wonder why we still use atomic_t all over the kernel.

> 
>> - suffers from cache line ping pongs on SMP
> 
> I used free alignment slot so that socket structure would not be
> icreased.

Are you kidding ?

bsockets is not part of socket structure, but part of "struct inet_hashinfo",
shared by all cpus and accessed several thousand times per second on many
machines.

Please read the comment three lines after 'the free alignemnt slot'
you chose.... You just introduced one write on a cache line
that is supposed to *not* be written.

        unsigned int                    bhash_size;
        int                             bsockets;

        struct kmem_cache               *bind_bucket_cachep;

        /* All the above members are written once at bootup and
         * never written again _or_ are predominantly read-access.
         *
         * Now align to a new cache line as all the following members
         * might be often dirty.
         */



> 
>> Also there might be a problem at line 175
>>
>> if (sk->sk_reuse && sk->sk_state != TCP_LISTEN && --attempts >= 0) { 
>> 	spin_unlock(&head->lock);
>> 	goto again;
>>
>> If we entered inet_csk_get_port() with a non null snum, we can "goto again"
>> while it was not expected.
>>
>> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
>> index df8e72f..752c6b2 100644
>> --- a/net/ipv4/inet_connection_sock.c
>> +++ b/net/ipv4/inet_connection_sock.c
>> @@ -172,7 +172,8 @@ tb_found:
>>  		} else {
>>  			ret = 1;
>>  			if (inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb)) {
>> -				if (sk->sk_reuse && sk->sk_state != TCP_LISTEN && --attempts >= 0) {
>> +				if (sk->sk_reuse && sk->sk_state != TCP_LISTEN &&
>> +					smallest_size == -1 &&  --attempts >= 0) {
> 
> I think it should be smallest_size != -1, since we really want to goto
> to the again label when hueristic is used, which in turn changes
> smallest_size.
> 

Yep