tcp.c::wait_for_tcp_memory() buggy ?

tcp.c::wait_for_tcp_memory() buggy ?

[Rik van Riel]

Hi Davem,

I can't quite put my finger on what wait_for_tcp_memory() is
supposed to do, but the code looks EXTREMELY suspect and I've
had a report of somebody looping in the for(;;) loop in that
function without ever exiting and getting his TCP connection
stuck there...

Also, the locking inside the loop seems fragile, to say the
least.

from tcp.c:

  865         if (tcp_memory_free(sk) && !vm_wait)
  866                 break;

  880         release_sock(sk);
  881         if (!tcp_memory_free(sk) || vm_wait)
  882                 current_timeo = schedule_timeout(current_timeo);
  883         lock_sock(sk);

Here we hold the lock for the entire loop (meaning that
other people cannot make the exit condition on line 865
come true.

Except for doing a test on tcp_memory_free(sk), where we
do NOT hold the lock we're so dutifully clinging to during
the rest of the loop...

As I said, I can't put my finger down on what exactly is
wrong, but this code looks subtle enough that, together
with the bugreport I got (on IRC), I have the feeling that
it just _can't_ be right ...

regards, 

Rik
--
"What you're running that piece of shit Gnome?!?!"
       -- Miguel de Icaza, UKUUG 2000

http://www.conectiva.com/		http://www.surriel.com/

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: tcp.c::wait_for_tcp_memory() buggy ?

[Andi Kleen]

On Sun, Oct 29, 2000 at 01:28:38AM -0200, Rik van Riel wrote:
> Except for doing a test on tcp_memory_free(sk), where we
> do NOT hold the lock we're so dutifully clinging to during
> the rest of the loop...

And rechecking it later while holding the loop on the next iteration.

Also usually the caller also does a check again and iterates as needed.

> 
> As I said, I can't put my finger down on what exactly is
> wrong, but this code looks subtle enough that, together
> with the bugreport I got (on IRC), I have the feeling that
> it just _can't_ be right ...

With the right setup of multiple threads writing all the time
on a single socket you could probably starve one, making it loop forever here.
(it does not try to be fair) 



-Andi

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: tcp.c::wait_for_tcp_memory() buggy ?

[Ingo Molnar]

On Sun, 29 Oct 2000, Rik van Riel wrote:

> I can't quite put my finger on what wait_for_tcp_memory() is supposed
> to do, [...]

it's waiting for TCP output packets to be processed. This is a TCP
protocol detail and is not connected to VM issues. The function name might
be a bit misleading, it could be 'tcp_write_possible()', or
'tcp_wmem_free()'.

> [...] but the code looks EXTREMELY suspect and I've had a report of
> somebody looping in the for(;;) loop in that function without ever
> exiting and getting his TCP connection stuck there...

as far as i understand, this can happen if another host (for whatever
reason) does not process the TCP output packets this host has sent.

> Also, the locking inside the loop seems fragile, to say the
> least.
> 
> from tcp.c:
> 
>   865         if (tcp_memory_free(sk) && !vm_wait)
>   866                 break;
> 
>   880         release_sock(sk);
>   881         if (!tcp_memory_free(sk) || vm_wait)
>   882                 current_timeo = schedule_timeout(current_timeo);
>   883         lock_sock(sk);
> 
> Here we hold the lock for the entire loop (meaning that
> other people cannot make the exit condition on line 865
> come true.

we do not keep the lock for the entire loop, we schedule away on line 882
with the socket lock dropped. This is a pretty standard (and safe) locking
technique.

> Except for doing a test on tcp_memory_free(sk), where we
> do NOT hold the lock we're so dutifully clinging to during
> the rest of the loop...

well, thats the point of the socket lock - we can access socket data
structures almost only via the socket lock.

	Ingo

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/