[PATCH] ipv4 skbuff locking scope

[PATCH] ipv4 skbuff locking scope

[Tom Leete]

[-- Attachment #1: Type: text/plain, Size: 174 bytes --]

Hello,

This fixes tests of a socket buffer done without holding the
lock. tcp_data_wait() and wait_for_tcp_memory() both had
unguarded refs in their sleep conditionals.

Tom

[-- Attachment #2: tcp.lock.scope.patch --]
[-- Type: text/plain, Size: 1387 bytes --]

--- linux-2.4.0-test10-pre5/net/ipv4/tcp.c~	Sun Oct 29 01:21:09 2000
+++ linux/net/ipv4/tcp.c	Mon Oct 30 16:53:19 2000
@@ -204,6 +204,9 @@
  *		Andi Kleen 	:	Make poll agree with SIGIO
  *	Salvatore Sanfilippo	:	Support SO_LINGER with linger == 1 and
  *					lingertime == 0 (RFC 793 ABORT Call)
+ *              Tom Leete       :       Fix locking scope in
+ *                                      wait_for_tcp_memory, tcp_data_wait
+ *
  *					
  *		This program is free software; you can redistribute it and/or
  *		modify it under the terms of the GNU General Public License
@@ -871,10 +874,11 @@
 			break;
 		if (sk->err)
 			break;
-		release_sock(sk);
-		if (!tcp_memory_free(sk) || vm_wait)
+		if (!tcp_memory_free(sk) || vm_wait) {
+			release_sock(sk);
 			current_timeo = schedule_timeout(current_timeo);
-		lock_sock(sk);
+			lock_sock(sk);
+		}
 		if (vm_wait) {
 			if (timeo != MAX_SCHEDULE_TIMEOUT &&
 			    (timeo -= vm_wait-current_timeo) < 0)
@@ -1273,12 +1277,12 @@
 	__set_current_state(TASK_INTERRUPTIBLE);
 
 	set_bit(SOCK_ASYNC_WAITDATA, &sk->socket->flags);
-	release_sock(sk);
 
-	if (skb_queue_empty(&sk->receive_queue))
+	if (skb_queue_empty(&sk->receive_queue)){
+		release_sock(sk);
 		timeo = schedule_timeout(timeo);
-
-	lock_sock(sk);
+		lock_sock(sk);
+	}
 	clear_bit(SOCK_ASYNC_WAITDATA, &sk->socket->flags);
 
 	remove_wait_queue(sk->sleep, &wait);

Re: [PATCH] ipv4 skbuff locking scope

[David S. Miller]

   Date: Mon, 30 Oct 2000 17:24:24 -0500
   From: Tom Leete <tleete@mountain.net>

   This fixes tests of a socket buffer done without holding the
   lock. tcp_data_wait() and wait_for_tcp_memory() both had
   unguarded refs in their sleep conditionals.

These are not buggy at all, see the discussion which took place here
over the past few days.

Look, if the sleep condition test "races" due to not holding the lock,
the schedule() just returns because if the sleep condidion test passes
then by definition this means we were also woken up, see?

BTW, while we're on the topic of people not understanding the
networking locking and proposing bogus patches, does anyone know who
sent the bogon IP tunneling locking "fixes" to Linus behind my back?

They were crap too, and I had to revert them in test10-pre7.  It's
another case of people just not understanding how the code works and
thus that it is correct without any changes.

Please send such fixes to me, and I'll set you straight with a
description as to why your change is unnecessary :-)

Later,
David S. Miller
davem@redhat.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: [PATCH] ipv4 skbuff locking scope

[Horst von Brand]

"David S. Miller" <davem@redhat.com> said:

[...]

> Please send such fixes to me, and I'll set you straight with a
> description as to why your change is unnecessary :-)

Could you please Cc: them into the kernel? ;-)
-- 
Horst von Brand                             vonbrand@sleipnir.valparaiso.cl
Casilla 9G, Vin~a del Mar, Chile                               +56 32 672616

-
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: [PATCH] ipv4 skbuff locking scope

[Tom Leete]

"David S. Miller" wrote:
> 
>    Date: Mon, 30 Oct 2000 17:24:24 -0500
>    From: Tom Leete <tleete@mountain.net>
> 
>    This fixes tests of a socket buffer done without holding the
>    lock. tcp_data_wait() and wait_for_tcp_memory() both had
>    unguarded refs in their sleep conditionals.
> 
> These are not buggy at all, see the discussion which took place here
> over the past few days.

I'll post traces privately. It was my lockups that got Rick
interested.
 
> Look, if the sleep condition test "races" due to not holding the lock,
> the schedule() just returns because if the sleep condidion test passes
> then by definition this means we were also woken up, see?

Would you explain what is accomplished by toggling the lock
every time through? What breaks by not doing so?

> BTW, while we're on the topic of people not understanding the
> networking locking and proposing bogus patches, does anyone know who
> sent the bogon IP tunneling locking "fixes" to Linus behind my back?

Not me, but see below.
 
> They were crap too, and I had to revert them in test10-pre7.  It's
> another case of people just not understanding how the code works and
> thus that it is correct without any changes.

If it's perfect why can't I use it without locking up the
machine? BTW, I'm currently running my patch. I can now
flood ping 100000 packets in either direction. With
unmodified tcp that is a red switcher (reliably hard locks
all i/o including the serial console).

> Please send such fixes to me, and I'll set you straight with a
> description as to why your change is unnecessary :-)

Perhaps that's why somebody wants to bypass you.

Tom Leete
-
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: [PATCH] ipv4 skbuff locking scope

[David S. Miller]

   Date: Mon, 30 Oct 2000 22:18:07 -0500
   From: Tom Leete <tleete@mountain.net>

   > Look, if the sleep condition test "races" due to not holding the
   > lock, the schedule() just returns because if the sleep condidion
   > test passes then by definition this means we were also woken up,
   > see?

   Would you explain what is accomplished by toggling the lock every
   time through? What breaks by not doing so?

Incoming packets are not processed while the lock is held, they
get queued to the backlog.  When the lock is released, the backlog
packets get processed.

For a TCP sender, these backlog packets are ACKs making room in the
send buffer so that the application can put more data into the send
buffer.

Later,
David S. Miller
davem@redhat.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: [PATCH] ipv4 skbuff locking scope

[Albert D. Cahalan]

David S. Miller writes:

> BTW, while we're on the topic of people not understanding the
> networking locking and proposing bogus patches, does anyone know who
> sent the bogon IP tunneling locking "fixes" to Linus behind my back?
...
> Please send such fixes to me, and I'll set you straight with a
> description as to why your change is unnecessary :-)

You can prevent future "fixes" by putting your comments in the code.

/*  like this  */
-
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/