Hi,
I found a problem to unix domain socket.
The unix_gc function removes socket buffers of the socket
which is connectted but not acceptted yet.
After one process executes "Mark phase" of unix_gc function,
another process registers socket buffer by using sendmsg with SCM_RIGHTS.
At the next moment, the socket buffer is removed.
I attached the test program.
When I execute one server and two clients on SMP machine
(kernel 2.4.16 and PentiumIII x 2), I can reporduce this problem.
Following is a patch to avoid this problem.
--- kernel-2.4.16/net/unix/garbage.c.sv Mon Jan 7 15:46:22 2002
+++ kernel-2.4.16/net/unix/garbage.c Mon Jan 7 15:51:22 2002
@@ -279,7 +279,7 @@
/*
* Do we have file descriptors ?
*/
- if(UNIXCB(skb).fp)
+ if(s->dead && UNIXCB(skb).fp)
{
__skb_unlink(skb, skb->list);
__skb_queue_tail(&hitlist,skb);
Regards,
Yasuma Takeda
On Mon, 7 Jan 2002, Yasuma Takeda wrote:
>
> Hi,
>
> I found a problem to unix domain socket.
>
> The unix_gc function removes socket buffers of the socket
> which is connectted but not acceptted yet.
>
> After one process executes "Mark phase" of unix_gc function,
> another process registers socket buffer by using sendmsg with SCM_RIGHTS.
> At the next moment, the socket buffer is removed.
>
> I attached the test program.
> When I execute one server and two clients on SMP machine
> (kernel 2.4.16 and PentiumIII x 2), I can reporduce this problem.
>
> Following is a patch to avoid this problem.
It looks bogus. Are you sure that listening socket is not garbage at that
point in your testcase?
Your patch is definitely wrong - consider the case when we send fd1 to
ourselves (fd2 would be receiving end), then send fd2 (fd3 would be
receiving end), then close fd3. We want _both_ packets (carrying fd1
and fd2 resp.) to be taken out of queues in that loop. So added check
is broken.
We scan the queues of listening sockets, all right, but only if listening
socket itself becomes a garbage. I.e. only if connection is never going
to be accepted...
> */
> - if(UNIXCB(skb).fp)
> + if(s->dead && UNIXCB(skb).fp)
> {
The bug may be real but the fix would prevent garbage collection working
at all - which I grant would fix the problem.
You don't need a socket to be dead to want to garbage collect it. If a
socket is getting disposed of while in use then there is a
maybe_unmark_and.. call missing, or a lock on the unix socket table missing
somewhere.
Hello!
> The bug may be real
Yes, it is... There is window when socket is already in hash table
(and visible by gc) but still not referenced by listening socket.
It is nice for everything, but GC fairly concludes that such socket
is orphan. Damn. It is even no related to SMP, the problem is present
in 2.2 as well if we sleep in sock_wmalloc...
Well, the following ugly patch is supposed to cure this. (Please, check)
To explain: embrion while it is in flight can be marked for GC to see
that it is not orphan. Deflated inflight counter is ugly but valid marker.
In 2.2 interning to hash table can be moved to point after enqueueing
to listening socket or to an area protected by kernel lock.
This is impossible in 2.4, and I see no solution but marking.
Alexey
diff -ur ../t/vger3-011229+local/linux/net/unix/af_unix.c linux/net/unix/af_unix.c
--- ../t/vger3-011229+local/linux/net/unix/af_unix.c Sat Jan 5 04:30:19 2002
+++ linux/net/unix/af_unix.c Wed Jan 9 03:28:44 2002
@@ -484,7 +484,7 @@
sk->protinfo.af_unix.dentry=NULL;
sk->protinfo.af_unix.mnt=NULL;
sk->protinfo.af_unix.lock = RW_LOCK_UNLOCKED;
- atomic_set(&sk->protinfo.af_unix.inflight, 0);
+ atomic_set(&sk->protinfo.af_unix.inflight, sock ? 0 : -1);
init_MUTEX(&sk->protinfo.af_unix.readsem);/* single task reading lock */
init_waitqueue_head(&sk->protinfo.af_unix.peer_wait);
sk->protinfo.af_unix.list=NULL;
@@ -991,7 +991,12 @@
unix_state_wunlock(sk);
/* take ten and and send info to listening sock */
- skb_queue_tail(&other->receive_queue,skb);
+ spin_lock(&other->receive_queue.lock);
+ __skb_queue_tail(&other->receive_queue,skb);
+ /* Undo artificially decreased inflight after embrion
+ * is installed to listening socket. */
+ atomic_inc(&newsk->protinfo.af_unix.inflight);
+ spin_unlock(&other->receive_queue.lock);
unix_state_runlock(other);
other->data_ready(other, 0);
sock_put(other);
diff -ur ../t/vger3-011229+local/linux/net/unix/garbage.c linux/net/unix/garbage.c
--- ../t/vger3-011229+local/linux/net/unix/garbage.c Fri Jul 20 22:12:11 2001
+++ linux/net/unix/garbage.c Wed Jan 9 03:27:49 2002
@@ -205,12 +205,21 @@
forall_unix_sockets(i, s)
{
+ int open_count = 0;
+
/*
* If all instances of the descriptor are not
* in flight we are in use.
+ *
+ * Special case: when socket s is embrion, it may be
+ * hashed but still not in queue of listening socket.
+ * In this case (see unix_create1()) we set artificial
+ * negative inflight counter to close race window.
+ * It is trick of course and dirty one.
*/
- if(s->socket && s->socket->file &&
- file_count(s->socket->file) > atomic_read(&s->protinfo.af_unix.inflight))
+ if(s->socket && s->socket->file)
+ open_count = file_count(s->socket->file);
+ if (open_count > atomic_read(&s->protinfo.af_unix.inflight))
maybe_unmark_and_push(s);
}
--- linux/include/net/af_unix.h.orig Tue Apr 25 05:43:04 2000
+++ linux/include/net/af_unix.h Fri Jan 11 21:49:57 2002
@@ -14,7 +14,7 @@
extern atomic_t unix_tot_inflight;
-#define forall_unix_sockets(i, s) for (i=0; i<=UNIX_HASH_SIZE; i++) \
+#define forall_unix_sockets(i, s) for (i=0; i<UNIX_HASH_SIZE; i++) \
for (s=unix_socket_table[i]; s; s=s->next)
struct unix_address
From: Go Taniguchi <[email protected]>
Date: Fri, 11 Jan 2002 22:23:56 +0900
What size of actually used hash table --unix_socket_table--?
If it is 256, probably forall_unix_sockets is dangerous.
forall_unix_sockets use 257 table size.
And If I apply this fix, test program can work.
Slot 256 is a special slot fo unbound sockets. The table is
sized to UNIX_HASH_SIZE + 1, so it is ok and your patch is
not right.
Please see the other email from Alexey Kuznetsov which includes
a real fix for your bug.
Thank you very much for your comment.
>
> Slot 256 is a special slot fo unbound sockets. The table is
> sized to UNIX_HASH_SIZE + 1, so it is ok and your patch is
> not right.
>
> Please see the other email from Alexey Kuznetsov which includes
> a real fix for your bug.
>
>
OK, however that fix can not work the test program.
The problem always occurred by process of slot 256.
I try to confirm the "real fix" once again.
Thanx.