removed socket buffer in unix domain socket

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.

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

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

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

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

Hello!

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

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.

-- GO!

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

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.