[PATCH] removed socket buffer in unix domain socket

[PATCH] removed socket buffer in unix domain socket

[Yasuma Takeda]

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


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



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: client.c --]
[-- Type: text/x-csrc; name="client.c", Size: 2449 bytes --]

#include <stdio.h>
#include <sys/socket.h>
#include <sys/time.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/fcntl.h>
#include <netinet/in.h>
#include <sys/uio.h>
#include <sys/un.h>
#include <unistd.h>
#include <errno.h>

main(int argc,char ** argv)
{

	struct sockaddr_un sockaddr;
	char line[128];
	char reply[128];
	int socket_errno;
	int i, err, n;
	int s;
	int ret;
	int	fd[2];
	struct 	cmsghdr *cmsg;
	int 	*fdptr;
	char 	cmsgbuf[CMSG_SPACE(sizeof (int))];
	struct iovec vec;
	struct msghdr msghdr;

	if ( socketpair( AF_UNIX, SOCK_STREAM, 0, fd ) == -1 ) {
		fprintf(stderr, "socketpait error\n");
	}
	close(fd[1]);

	n = 0;
	while (1) {
		if ((s=socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
			socket_errno = errno;
			fprintf(stderr,"can't create socket [%d]\n",socket_errno);
			exit(-1);
		}

		memset(&sockaddr, '\0', sizeof(sockaddr));
		sockaddr.sun_family      = AF_UNIX;
		strcpy(sockaddr.sun_path, "/tmp/unix_d_test");
		
		if (connect(s,(struct sockaddr *)&sockaddr,sizeof(sockaddr)) == -1) {
			socket_errno = errno;
			fprintf(stderr,"connect error [%d]\n",errno);
			exit(-1);
		}
		
		for(i = 0 ; i < 128 ; i++) {
			line[i] = 0;
		}
		sprintf(line, "[%d]:client pid is %d", n, getpid());
		
		vec.iov_base = line;
		vec.iov_len  = 128;
		
		msghdr.msg_name       = NULL;
		msghdr.msg_namelen    = 0;
		msghdr.msg_iov        = &vec;
		msghdr.msg_iovlen     = 1;
		msghdr.msg_flags = 0;
		
		msghdr.msg_control = cmsgbuf;
		msghdr.msg_controllen = sizeof cmsgbuf;
		cmsg = CMSG_FIRSTHDR(&msghdr);
		cmsg->cmsg_len   = CMSG_LEN(sizeof(int) * 1);
		cmsg->cmsg_level = SOL_SOCKET;
		cmsg->cmsg_type  = SCM_RIGHTS;
		fdptr = (int *)CMSG_DATA(cmsg);
		memcpy(fdptr, fd, sizeof(int) * 1);
		msghdr.msg_controllen = cmsg->cmsg_len;
		
		if ((ret = sendmsg(s, &msghdr, 0 )) < 128) {
			fprintf(stderr, "sendmsg error %d\n", errno);
			exit(-1);
		}
		fprintf(stderr, "[%d]:sendmsg() %d byte OK. fd:%d\n", n, ret, fd[0]);
		
		vec.iov_base = reply;
		vec.iov_len  = 128;
		
		msghdr.msg_name       = NULL;
		msghdr.msg_namelen    = 0;
		msghdr.msg_iov        = &vec;
		msghdr.msg_iovlen     = 1;
		msghdr.msg_flags = 0;
		msghdr.msg_controllen = 0;
		msghdr.msg_control    = NULL;
		
		if ((ret = recvmsg(s, &msghdr, 0)) < 128) {
			fprintf(stderr, "recvmsg() error %d\n", errno);
			exit(-1);
		}
		fprintf(stderr, "recvmgs() %d byte OK.\n", ret);

		close(s);
		n++;
	}
	
	fprintf(stderr,"GOOD BY \n");
	exit (0);
}





[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: server.c --]
[-- Type: text/x-csrc; name="server.c", Size: 3215 bytes --]

#include <stdio.h>
#include <sys/socket.h>
#include <sys/time.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <sys/uio.h>
#include <sys/un.h>
#include <fcntl.h>
#include <unistd.h>
#include <errno.h>
#include <time.h>

main(int argc,char ** argv)
{
	int s;
	struct sockaddr_un sockaddr;
	char line[128];
	char reply[128];
	int socket_errno;
	int i, err;
	int flags;
	fd_set read_set, read_cur;
	int max_fd;
	int ret;
	struct  iovec vec;
	struct  msghdr msghdr;
	int	client_fd;
	struct  cmsghdr *cmsg;
	int	*pass_fd_ptr;
	char	cmsgbuf[CMSG_SPACE(sizeof (int))];
	
	if ((s=socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
		socket_errno = errno;
		fprintf(stdout,"can't create socket [%d]\n",socket_errno);
		exit(-1);
	}
	
	unlink("/tmp/unix_d_test");
	memset(&sockaddr, '\0', sizeof(sockaddr));
	sockaddr.sun_family      = AF_UNIX;
	strcpy(sockaddr.sun_path, "/tmp/unix_d_test");
	
	if (bind(s,(struct sockaddr *)&sockaddr, sizeof(sockaddr)) < 0) {
		socket_errno = errno;
		fprintf(stdout,"can't bind socket [%d]\n",errno);
		exit(-1);
	}
	
	if (listen(s, 10) < 0) {
		fprintf(stdout, "listen() error %d\n", socket_errno);
		exit(-1);
	}

	FD_ZERO(&read_set);
	FD_SET(s, &read_set);
	max_fd = s;
	client_fd = -1;

	for(;;) {
		for(i = 0 ; i < 128 ; i++) {
			line[i] = 0;
		}
		
		read_cur = read_set;
		ret = select(max_fd+1, &read_cur, NULL, NULL, NULL);
		if (ret == -1) {
			fprintf(stdout, "select() error %d\n", errno);
			continue;
		}
		for (i = 0; i <= max_fd; i++) {
			if (FD_ISSET(i, &read_cur)) {
				if (i == s) {
					struct sockaddr_un    address;
					int	len;

					if (client_fd != -1) {
						close(client_fd);
					}

					len = sizeof(struct sockaddr_un);
					client_fd = accept(s, (struct sockaddr *)&address, &len);
					if (client_fd == -1) {
						fprintf(stdout, "accept() error %d\n", errno);
						exit(-1);
					}
					if ( client_fd > max_fd ) max_fd = client_fd;
				} else if (i == client_fd) {
					fprintf(stdout, "read from client_fd\n");
				}
				
				msghdr.msg_name    = NULL;
				msghdr.msg_namelen = 0;
				msghdr.msg_iov     = &vec;
				msghdr.msg_iovlen  = 1;
				msghdr.msg_control    = cmsgbuf;
				msghdr.msg_controllen = sizeof(cmsgbuf);
				vec.iov_base  = line;
				vec.iov_len   = 128;
				if ((ret = recvmsg(client_fd, &msghdr, 0)) < 128) {
					fprintf(stdout, "recvmsg() error %d\n", errno);
					exit(-1);
				}
				for (cmsg = CMSG_FIRSTHDR(&msghdr); cmsg != NULL; cmsg = CMSG_NXTHDR(&msghdr, cmsg)) {
					if (cmsg->cmsg_level == SOL_SOCKET) {
						pass_fd_ptr = (int *)CMSG_DATA(cmsg);
						break;
					}
				}
				fprintf(stdout, "recvmsg() is done.\n");
				close(*pass_fd_ptr);

				msghdr.msg_name    = NULL;
				msghdr.msg_namelen = 0;
				msghdr.msg_iov     = &vec;
				msghdr.msg_iovlen  = 1;
				msghdr.msg_controllen = 0;
				msghdr.msg_control    = NULL;
				vec.iov_base  = reply;
				vec.iov_len   = 128;
				if ((ret = sendmsg(client_fd, &msghdr, 0 )) < 128) {
					fprintf(stdout, "sendmsg error %d\n", errno);
					exit(-1);
				}
				fprintf(stdout, "sendmsg() %d byte OK. \n", ret);
				
				FD_ZERO(&read_set);
				FD_SET(s, &read_set);
				break;
			}
		}
		
	}
	fprintf(stdout,"GOOD BY \n");
	exit (0);
}

Re: [PATCH] removed socket buffer in unix domain socket

[Alexander Viro]

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

Re: [PATCH] removed socket buffer in unix domain socket

[Alan Cox]

>                                  */
> -                               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.

Re: [PATCH] removed socket buffer in unix domain socket

[kuznet]

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

Re: [PATCH] removed socket buffer in unix domain socket

[Go Taniguchi]

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

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.

Alan Cox wrote:

 >>                                 */
 >>-                               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.

-- GO!

[-- Attachment #2: af_net.h.patch --]
[-- Type: text/plain, Size: 422 bytes --]

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

Re: [PATCH] removed socket buffer in unix domain socket

[David S. Miller]

   From: Go Taniguchi <go@turbolinux.co.jp>
   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.

Re: [sd:04032] Re: [PATCH] removed socket buffer in unix domain socket

[Go Taniguchi]

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.