Bug 185813 - SOCK_SEQPACKET AF_UNIX sockets with asymmetrical buffers drop packets
Summary: SOCK_SEQPACKET AF_UNIX sockets with asymmetrical buffers drop packets
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: Unspecified
Hardware: Any Any
: Normal Affects Only Me
Assignee: Alan Somers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-15 21:10 UTC by Alan Somers
Modified: 2018-07-31 02:32 UTC (History)
0 users

See Also:


Attachments
file.diff (18.49 KB, patch)
2014-01-15 21:10 UTC, Alan Somers
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alan Somers freebsd_committer 2014-01-15 21:10:01 UTC
Unix domain sequential packet sockets can silently drop packets.

In fact, the problem is even worse than it appears.  Every time a packet is lost, the kernel leaks mbufs.  Eventually, the system will run out of mbufs, causing some processes to lock up.

The cause is in sbappendaddr_locked() in sys/kern/uipc_sockbuf.c:644.  That line checks whether the receiving sockbuf would overflow before appending a new mbuf.  If it would overflow, the function fails.  However, uipc_send() in sys/kern/uipc_usrreq.c:980 does not free the mbuf, nor does it return an error to the caller.  That causes the packet to disappear and the mbufs to leak.

The space check shouldn't fail because we've enlarged the send buffer.  However, there is an optimization for unix domain sockets that does not apply to AF_INET sockets: send() writes packets directly to the receiving buffer, skipping the sending buffer.  Despite that optimization, sosend_generic() in kern/uipc_socket.c:1230 uses the _sending_ sockbuf for its space calculation.  The mismatched calculations in sosend_generic() and sbappendaddr_locked() cause the bug.

The exact same optimization applies to SOCK_STREAM sockets, but they don't exhibit this bug.  Why not?  Because they don't use sbappendaddr_locked().  They go through a parallel code path: sbappend_locked().  That function does not do a space check at all.

Fix: The simple solution would be to simply delete the space check from sbappendaddr_locked().  I've tested it and it works.  However, sbappendaddr_locked() is called by many other functions than just uipc_send().

A better fix might be that setsockopt(s, SOL_SOCKET, SO_SNDBUF, &x, sizeof(x)) should set both the sending buffer size, _and_ the receiving buffer size of the companion socket.  Conversely, setsockopt(s, SOL_SOCKET, SO_RCVBUF, &x, sizeof(x)) would have to set both the receiving buffer size, _and_ the sending buffer size of the companion socket.  I haven't tested that solution yet.

Patch attached with submission follows:
How-To-Repeat: 1) Create a pair of sequential packet unix domain sockets, either using socketpair(2), or by using socket(2), bind(2), listen(2) and accept(2).

2) Increase the sending buffer with setsockopt(2), but do not change the receiving buffer

3) Send one or more packets with size totalling 8kB or greater.

4) Read packets using receive.  All packets whose cumulative size was > 8kB will be lost.

The attached patch contains an ATF test program for SEQPACKET unix domain sockets.  This problem is most clearly reproduced with the testcase "unix_seqpacket:pipe_simulator_128k_8k", but most concisely reproduced with "unix_seqpacket:eagain_128k_8k"
Comment 1 dfilter service freebsd_committer 2014-01-23 17:26:41 UTC
Author: asomers
Date: Thu Jan 23 17:26:28 2014
New Revision: 261081
URL: http://svnweb.freebsd.org/changeset/base/261081

Log:
  Replace the old unix_seqpacket and unix_seqpacket_exercise tests, which
  were a little broken and not automatable, with unix_seqpacket_test.
  It's coverage is a superset of the old tests and it uses ATF.  It
  includes test cases for bugs kern/185813 and kern/185812.
  
  PR:		kern/185812
  PR:		kern/185813
  Sponsored by:	Spectra Logic
  MFC after:	2 weeks

Added:
  head/tests/sys/
  head/tests/sys/Makefile   (contents, props changed)
  head/tests/sys/kern/
  head/tests/sys/kern/Makefile   (contents, props changed)
  head/tests/sys/kern/unix_seqpacket_test.c   (contents, props changed)
Deleted:
  head/tools/regression/sockets/unix_seqpacket/
  head/tools/regression/sockets/unix_seqpacket_exercise/
Modified:
  head/Makefile.inc1
  head/etc/mtree/BSD.tests.dist

Modified: head/Makefile.inc1
==============================================================================
--- head/Makefile.inc1	Thu Jan 23 17:24:26 2014	(r261080)
+++ head/Makefile.inc1	Thu Jan 23 17:26:28 2014	(r261081)
@@ -417,7 +417,7 @@ LIB32WMAKEFLAGS+=	\
 		-DNO_LINT
 
 LIB32WMAKE=	${LIB32WMAKEENV} ${MAKE} ${LIB32WMAKEFLAGS} \
-		-DWITHOUT_MAN -DWITHOUT_INFO -DWITHOUT_HTML
+		-DWITHOUT_MAN -DWITHOUT_INFO -DWITHOUT_HTML -DNO_TESTS
 LIB32IMAKE=	${LIB32WMAKE:NINSTALL=*:NDESTDIR=*:N_LDSCRIPTROOT=*} -DNO_INCS \
 		${IMAKE_INSTALL}
 .endif

Modified: head/etc/mtree/BSD.tests.dist
==============================================================================
--- head/etc/mtree/BSD.tests.dist	Thu Jan 23 17:24:26 2014	(r261080)
+++ head/etc/mtree/BSD.tests.dist	Thu Jan 23 17:26:28 2014	(r261081)
@@ -78,6 +78,10 @@
                 ..
             ..
         ..
+        sys
+            kern
+            ..
+        ..
         usr.bin
             atf
                 atf-sh

Added: head/tests/sys/Makefile
==============================================================================
--- /dev/null	00:00:00 1970	(empty, because file is newly added)
+++ head/tests/sys/Makefile	Thu Jan 23 17:26:28 2014	(r261081)
@@ -0,0 +1,13 @@
+# $FreeBSD$
+
+.include <bsd.own.mk>
+
+TESTSDIR= ${TESTSBASE}/sys
+
+KYUAFILE= yes
+
+CLEANFILES+= Kyuafile
+Kyuafile: ${.CURDIR}/../Kyuafile
+	cp -f ${.CURDIR}/../Kyuafile .
+
+.include <bsd.test.mk>

Added: head/tests/sys/kern/Makefile
==============================================================================
--- /dev/null	00:00:00 1970	(empty, because file is newly added)
+++ head/tests/sys/kern/Makefile	Thu Jan 23 17:26:28 2014	(r261081)
@@ -0,0 +1,10 @@
+# $FreeBSD$
+
+TESTSDIR=	${TESTSBASE}/sys/kern
+
+ATF_TESTS_C=	unix_seqpacket_test
+TEST_METADATA.unix_seqpacket_test+=	timeout="15"
+
+LDADD+=		-lpthread
+
+.include <atf.test.mk>

Added: head/tests/sys/kern/unix_seqpacket_test.c
==============================================================================
--- /dev/null	00:00:00 1970	(empty, because file is newly added)
+++ head/tests/sys/kern/unix_seqpacket_test.c	Thu Jan 23 17:26:28 2014	(r261081)
@@ -0,0 +1,1117 @@
+/*-
+ * Copyright (c) 2014 Spectra Logic Corporation. All rights reserved.
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY AUTHOR AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL AUTHOR OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <sys/cdefs.h>
+__FBSDID("$FreeBSD$");
+
+#include <errno.h>
+#include <fcntl.h>
+#include <pthread.h>
+#include <signal.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+
+#include <stdio.h>
+
+#include <atf-c.h>
+
+/*
+ * Helper functions
+ */
+
+#define MIN(x, y)	((x) < (y) ? (x) : (y))
+#define MAX(x, y)	((x) > (y) ? (x) : (y))
+
+void
+do_socketpair(int *sv)
+{
+	int s;
+	
+	s = socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sv);
+	ATF_REQUIRE_EQ(0, s);
+	ATF_REQUIRE(sv[0] >= 0);
+	ATF_REQUIRE(sv[1] >= 0);
+	ATF_REQUIRE(sv[0] != sv[1]);
+}
+
+void
+do_socketpair_nonblocking(int *sv)
+{
+	int s;
+	
+	s = socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sv);
+	ATF_REQUIRE_EQ(0, s);
+	ATF_REQUIRE(sv[0] >= 0);
+	ATF_REQUIRE(sv[1] >= 0);
+	ATF_REQUIRE(sv[0] != sv[1]);
+	ATF_REQUIRE(-1 != fcntl(sv[0], F_SETFL, O_NONBLOCK));
+	ATF_REQUIRE(-1 != fcntl(sv[1], F_SETFL, O_NONBLOCK));
+}
+
+/* 
+ * Returns a pair of sockets made the hard way: bind, listen, connect & accept
+ * @return	const char* The path to the socket
+ */
+const char*
+mk_pair_of_sockets(int *sv)
+{
+	struct sockaddr_un sun;
+	/* ATF's isolation mechanisms will guarantee uniqueness of this file */
+	const char *path = "sock";
+	int s, err, s2, s1;
+
+	s = socket(PF_LOCAL, SOCK_SEQPACKET, 0);
+	ATF_REQUIRE(s >= 0);
+
+	bzero(&sun, sizeof(sun));
+	sun.sun_family = AF_LOCAL;
+	sun.sun_len = sizeof(sun);
+	strlcpy(sun.sun_path, path, sizeof(sun.sun_path));
+	err = bind(s, (struct sockaddr *)&sun, sizeof(sun));
+	err = listen(s, -1);
+	ATF_CHECK_EQ(0, err);
+	ATF_CHECK_EQ(0, err);
+
+	/* Create the other socket */
+	s2 = socket(PF_LOCAL, SOCK_SEQPACKET, 0);
+	ATF_REQUIRE(s2 >= 0);
+	err = connect(s2, (struct sockaddr*)&sun, sizeof(sun));
+	if (err != 0) {
+		perror("connect");
+		atf_tc_fail("connect(2) failed");
+	}
+	
+	/* Accept it */
+	s1 = accept(s, NULL, NULL);
+	if (s1 == -1) {
+		perror("accept");
+		atf_tc_fail("accept(2) failed");
+	}
+
+	sv[0] = s1;
+	sv[1] = s2;
+	return (path);
+}
+
+static volatile sig_atomic_t got_sigpipe = 0;
+static void
+shutdown_send_sigpipe_handler(int x)
+{
+	got_sigpipe = 1;
+}
+
+/*
+ * Parameterized test function bodies
+ */
+void
+test_eagain(size_t sndbufsize, size_t rcvbufsize)
+{
+	int i;
+	int sv[2];
+	const size_t totalsize = (sndbufsize + rcvbufsize) * 2;
+	const size_t pktsize = MIN(sndbufsize, rcvbufsize) / 4;
+	char sndbuf[pktsize];
+	char recv_buf[pktsize];
+	ssize_t ssize, rsize;
+
+	/* setup the socket pair */
+	do_socketpair(sv);
+	/* Setup the buffers */
+	ATF_REQUIRE_EQ(0, setsockopt(sv[0], SOL_SOCKET, SO_SNDBUF, &sndbufsize,
+	    sizeof(sndbufsize)));
+	ATF_REQUIRE_EQ(0, setsockopt(sv[1], SOL_SOCKET, SO_RCVBUF, &rcvbufsize,
+	    sizeof(rcvbufsize)));
+
+	bzero(sndbuf, pktsize);
+	/* Send data until we get EAGAIN */
+	for(i=0; i < totalsize / pktsize; i++) {
+		ssize = send(sv[0], sndbuf, pktsize, MSG_EOR);
+		if (ssize == -1) {
+			if (errno == EAGAIN)
+				atf_tc_pass();
+			else {
+				perror("send");
+				atf_tc_fail("send returned < 0 but not EAGAIN");
+			}
+		}
+	}
+	atf_tc_fail("Never got EAGAIN");
+}
+
+void
+test_sendrecv_symmetric_buffers(size_t bufsize, int blocking) {
+	int s;
+	int sv[2];
+	const size_t pktsize = bufsize / 2;
+	char sndbuf[pktsize];
+	char recv_buf[pktsize];
+	ssize_t ssize, rsize;
+
+	/* setup the socket pair */
+	if (blocking)
+		do_socketpair(sv);
+	else
+		do_socketpair_nonblocking(sv);
+
+	/* Setup the buffers */
+	s = setsockopt(sv[0], SOL_SOCKET, SO_SNDBUF, &bufsize, sizeof(bufsize));
+	ATF_REQUIRE_EQ(0, s);
+	s = setsockopt(sv[1], SOL_SOCKET, SO_RCVBUF, &bufsize, sizeof(bufsize));
+	ATF_REQUIRE_EQ(0, s);
+
+	/* Fill the send buffer */
+	bzero(sndbuf, pktsize);
+
+	/* send and receive the packet */
+	ssize = send(sv[0], sndbuf, pktsize, MSG_EOR);
+	if (ssize < 0) {
+		perror("send");
+		atf_tc_fail("send returned < 0");
+	}
+	ATF_CHECK_EQ_MSG(pktsize, ssize, "expected %zd=send(...) but got %zd",
+	    pktsize, ssize);
+
+	rsize = recv(sv[1], recv_buf, pktsize, MSG_WAITALL);
+	if (rsize < 0) {
+		perror("recv");
+		atf_tc_fail("recv returned < 0");
+	}
+	ATF_CHECK_EQ_MSG(pktsize, rsize, "expected %zd=send(...) but got %zd",
+	    pktsize, rsize);
+}
+
+void
+test_pipe_simulator(size_t sndbufsize, size_t rcvbufsize)
+{
+	int s, num_sent, num_received;
+	int sv[2];
+	const size_t pktsize = MIN(sndbufsize, rcvbufsize) / 4;
+	int numpkts;
+	char sndbuf[pktsize];
+	char rcvbuf[pktsize];
+	char comparebuf[pktsize];
+	ssize_t ssize, rsize;
+	bool currently_sending = true;
+
+	/* setup the socket pair */
+	do_socketpair_nonblocking(sv);
+	/* Setup the buffers */
+	ATF_REQUIRE_EQ(0, setsockopt(sv[0], SOL_SOCKET, SO_SNDBUF, &sndbufsize,
+	    sizeof(sndbufsize)));
+	ATF_REQUIRE_EQ(0, setsockopt(sv[1], SOL_SOCKET, SO_RCVBUF, &rcvbufsize,
+	    sizeof(rcvbufsize)));
+
+	/* Send a total amount of data comfortably greater than the buffers */
+	numpkts = MAX(sndbufsize, rcvbufsize) * 8 / pktsize;
+	for (num_sent=0, num_received=0;
+	     num_sent < numpkts || num_received < numpkts; ) {
+		if (currently_sending && num_sent < numpkts) {
+			/* The simulated sending process */
+			/* fill the buffer */
+			memset(sndbuf, num_sent, pktsize);
+			ssize = send(sv[0], sndbuf, pktsize, MSG_EOR);
+			if (ssize < 0) {
+				/* 
+				 * XXX: This is bug-compatible with the kernel.
+				 * The kernel returns EMSGSIZE when it should
+				 * return EAGAIN
+				 */
+				if (errno == EAGAIN || errno == EMSGSIZE)
+					currently_sending = false;
+				else {
+					perror("send");
+					atf_tc_fail("send failed");
+				}
+			} else  {
+				ATF_CHECK_EQ_MSG(pktsize, ssize,
+				    "expected %zd=send(...) but got %zd",
+				    pktsize, ssize);
+				num_sent++;
+			}
+		} else {
+			/* The simulated receiving process */
+			rsize = recv(sv[1], rcvbuf, pktsize, MSG_WAITALL);
+			if (rsize < 0) {
+				if (errno == EAGAIN) {
+					currently_sending = true;
+					ATF_REQUIRE_MSG(num_sent < numpkts,
+					    "Packets were lost!");
+				}
+				else {
+					perror("recv");
+					atf_tc_fail("recv failed");
+				}
+			} else  {
+				ATF_CHECK_EQ_MSG(pktsize, rsize,
+				    "expected %zd=recv(...) but got %zd",
+				    pktsize, rsize);
+				memset(comparebuf, num_received, pktsize);
+				ATF_CHECK_EQ_MSG(0, memcmp(comparebuf, rcvbuf,
+				    			   pktsize), 
+				    "Received data miscompare");
+				num_received++;
+			}
+		}
+	}
+}
+
+typedef struct {
+	ssize_t	pktsize;
+	int	numpkts;
+	int	so;
+} test_pipe_thread_data_t;
+
+static void*
+test_pipe_writer(void* args)
+{
+	test_pipe_thread_data_t* td = args;
+	char sndbuf[td->pktsize];
+	ssize_t ssize;
+	int i;
+
+	for(i=0; i < td->numpkts; i++) {
+			memset(sndbuf, i, td->pktsize);
+			ssize = send(td->so, sndbuf, td->pktsize, MSG_EOR);
+			if (ssize < 0) {
+				perror("send");
+				atf_tc_fail("send returned < 0");
+			}
+			ATF_CHECK_EQ_MSG(td->pktsize, ssize,
+			    		 "expected %zd=send(...) but got %zd",
+			    		  td->pktsize, ssize);
+	}
+	return (0);
+}
+
+static void*
+test_pipe_reader(void* args)
+{
+	test_pipe_thread_data_t* td = args;
+	char rcvbuf[td->pktsize];
+	char comparebuf[td->pktsize];
+	ssize_t rsize;
+	int i, d;
+
+	for(i=0; i < td->numpkts; i++) {
+		memset(comparebuf, i, td->pktsize);
+		rsize = recv(td->so, rcvbuf, td->pktsize, MSG_WAITALL);
+		if (rsize < 0) {
+			perror("recv");
+			atf_tc_fail("recv returned < 0");
+		}
+		ATF_CHECK_EQ_MSG(td->pktsize, rsize,
+		    		 "expected %zd=send(...) but got %zd",
+				 td->pktsize, rsize);
+		d = memcmp(comparebuf, rcvbuf, td->pktsize);
+		ATF_CHECK_EQ_MSG(0, d, 
+		    		 "Received data miscompare on packet %d", i);
+	}
+	return (0);
+}
+
+
+void
+test_pipe(size_t sndbufsize, size_t rcvbufsize)
+{
+	test_pipe_thread_data_t writer_data, reader_data;
+	pthread_t writer, reader;
+	int num_sent, num_received;
+	int sv[2];
+	const size_t pktsize = MIN(sndbufsize, rcvbufsize) / 4;
+	int numpkts;
+
+	/* setup the socket pair */
+	do_socketpair(sv);
+	/* Setup the buffers */
+	ATF_REQUIRE_EQ(0, setsockopt(sv[0], SOL_SOCKET, SO_SNDBUF, &sndbufsize,
+	    sizeof(sndbufsize)));
+	ATF_REQUIRE_EQ(0, setsockopt(sv[1], SOL_SOCKET, SO_RCVBUF, &rcvbufsize,
+	    sizeof(rcvbufsize)));
+
+	/* Send a total amount of data comfortably greater than the buffers */
+	numpkts = MAX(sndbufsize, rcvbufsize) * 8 / pktsize;
+
+	/* Start the child threads */
+	writer_data.pktsize = pktsize;
+	writer_data.numpkts = numpkts;
+	writer_data.so = sv[0];
+	reader_data.pktsize = pktsize;
+	reader_data.numpkts = numpkts;
+	reader_data.so = sv[1];
+	ATF_REQUIRE_EQ(0, pthread_create(&writer, NULL, test_pipe_writer,
+	    				 (void*)&writer_data));
+	ATF_REQUIRE_EQ(0, pthread_create(&reader, NULL, test_pipe_reader,
+	    				 (void*)&reader_data));
+
+	/* Join the children */
+	ATF_REQUIRE_EQ(0, pthread_join(writer, NULL));
+	ATF_REQUIRE_EQ(0, pthread_join(reader, NULL));
+}
+
+
+/*
+ * Test Cases
+ */
+
+/* Create a SEQPACKET socket */
+ATF_TC_WITHOUT_HEAD(create_socket);
+ATF_TC_BODY(create_socket, tc)
+{
+	int s;
+
+	s = socket(PF_LOCAL, SOCK_SEQPACKET, 0);
+	ATF_CHECK(s >= 0);
+}
+
+/* Create SEQPACKET sockets using socketpair(2) */
+ATF_TC_WITHOUT_HEAD(create_socketpair);
+ATF_TC_BODY(create_socketpair, tc)
+{
+	int sv[2];
+	int s;
+
+	s = socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sv);
+	ATF_CHECK_EQ(0, s);
+	ATF_CHECK(sv[0] >= 0);
+	ATF_CHECK(sv[1] >= 0);
+	ATF_CHECK(sv[0] != sv[1]);
+}
+
+/* Call listen(2) without first calling bind(2).  It should fail */
+ATF_TC_WITHOUT_HEAD(listen_unbound);
+ATF_TC_BODY(listen_unbound, tc)
+{
+	int s, r;
+
+	s = socket(PF_LOCAL, SOCK_SEQPACKET, 0);
+	ATF_REQUIRE(s > 0);
+	r = listen(s, -1);
+	/* expect listen to fail since we haven't called bind(2) */
+	ATF_CHECK(r != 0);
+}
+
+/* Bind the socket to a file */
+ATF_TC_WITHOUT_HEAD(bind);
+ATF_TC_BODY(bind, tc)
+{
+	struct sockaddr_un sun;
+	/* ATF's isolation mechanisms will guarantee uniqueness of this file */
+	const char *path = "sock";
+	int s, r;
+
+	s = socket(PF_LOCAL, SOCK_SEQPACKET, 0);
+	ATF_REQUIRE(s >= 0);
+
+	bzero(&sun, sizeof(sun));
+	sun.sun_family = AF_LOCAL;
+	sun.sun_len = sizeof(sun);
+	strlcpy(sun.sun_path, path, sizeof(sun.sun_path));
+	r = bind(s, (struct sockaddr *)&sun, sizeof(sun));
+	ATF_CHECK_EQ(0, r);
+}
+
+/* listen(2) a socket that is already bound(2) should succeed */
+ATF_TC_WITHOUT_HEAD(listen_bound);
+ATF_TC_BODY(listen_bound, tc)
+{
+	struct sockaddr_un sun;
+	/* ATF's isolation mechanisms will guarantee uniqueness of this file */
+	const char *path = "sock";
+	int s, r, l;
+
+	s = socket(PF_LOCAL, SOCK_SEQPACKET, 0);
+	ATF_REQUIRE(s >= 0);
+
+	bzero(&sun, sizeof(sun));
+	sun.sun_family = AF_LOCAL;
+	sun.sun_len = sizeof(sun);
+	strlcpy(sun.sun_path, path, sizeof(sun.sun_path));
+	r = bind(s, (struct sockaddr *)&sun, sizeof(sun));
+	l = listen(s, -1);
+	ATF_CHECK_EQ(0, r);
+	ATF_CHECK_EQ(0, l);
+}
+
+/* connect(2) can make a connection */
+ATF_TC_WITHOUT_HEAD(connect);
+ATF_TC_BODY(connect, tc)
+{
+	struct sockaddr_un sun;
+	/* ATF's isolation mechanisms will guarantee uniqueness of this file */
+	const char *path = "sock";
+	int s, r, err, l, s2;
+
+	s = socket(PF_LOCAL, SOCK_SEQPACKET, 0);
+	ATF_REQUIRE(s >= 0);
+
+	bzero(&sun, sizeof(sun));
+	sun.sun_family = AF_LOCAL;
+	sun.sun_len = sizeof(sun);
+	strlcpy(sun.sun_path, path, sizeof(sun.sun_path));
+	r = bind(s, (struct sockaddr *)&sun, sizeof(sun));
+	l = listen(s, -1);
+	ATF_CHECK_EQ(0, r);
+	ATF_CHECK_EQ(0, l);
+
+	/* Create the other socket */
+	s2 = socket(PF_LOCAL, SOCK_SEQPACKET, 0);
+	ATF_REQUIRE(s2 >= 0);
+	err = connect(s2, (struct sockaddr*)&sun, sizeof(sun));
+	if (err != 0) {
+		perror("connect");
+		atf_tc_fail("connect(2) failed");
+	}
+}
+
+/* accept(2) can receive a connection */
+ATF_TC_WITHOUT_HEAD(accept);
+ATF_TC_BODY(accept, tc)
+{
+	int sv[2];
+
+	mk_pair_of_sockets(sv);
+}
+
+
+/* Set O_NONBLOCK on the socket */
+ATF_TC_WITHOUT_HEAD(fcntl_nonblock);
+ATF_TC_BODY(fcntl_nonblock, tc)
+{
+	int s;
+
+	s = socket(PF_LOCAL, SOCK_SEQPACKET, 0);
+	ATF_REQUIRE(s >= 0);
+	if (fcntl(s, F_SETFL, O_NONBLOCK) == -1) {
+		perror("fcntl");
+		atf_tc_fail("fcntl failed");
+	}
+}
+
+/* Resize the send and receive buffers */
+ATF_TC_WITHOUT_HEAD(resize_buffers);
+ATF_TC_BODY(resize_buffers, tc)
+{
+	int s;
+	int sndbuf = 12345;
+	int rcvbuf = 23456;
+	int xs, xr;
+	socklen_t sl = sizeof(xs);
+
+	s = socket(PF_LOCAL, SOCK_SEQPACKET, 0);
+	ATF_REQUIRE(s >= 0);
+
+	printf("                       Socket Buffer Sizes\n");
+	printf("                              | SNDBUF  | RCVBUF  |\n");
+	ATF_CHECK_EQ(0, getsockopt(s, SOL_SOCKET, SO_SNDBUF, &xs, &sl));
+	ATF_CHECK_EQ(0, getsockopt(s, SOL_SOCKET, SO_RCVBUF, &xr, &sl));
+	printf("Default                       | %7d | %7d |\n", xs, xr);
+
+	if (setsockopt(s, SOL_SOCKET, SO_SNDBUF, &sndbuf, sizeof(sndbuf)) != 0){
+		perror("setsockopt");
+		atf_tc_fail("setsockopt(SO_SNDBUF) failed");
+	}
+	ATF_CHECK_EQ(0, getsockopt(s, SOL_SOCKET, SO_SNDBUF, &xs, &sl));
+	ATF_CHECK_EQ(0, getsockopt(s, SOL_SOCKET, SO_RCVBUF, &xr, &sl));
+	printf("After changing SNDBUF         | %7d | %7d |\n", xs, xr);
+	
+	if (setsockopt(s, SOL_SOCKET, SO_RCVBUF, &rcvbuf, sizeof(rcvbuf)) != 0){
+		perror("setsockopt");
+		atf_tc_fail("setsockopt(SO_RCVBUF) failed");
+	}
+	ATF_CHECK_EQ(0, getsockopt(s, SOL_SOCKET, SO_SNDBUF, &xs, &sl));
+	ATF_CHECK_EQ(0, getsockopt(s, SOL_SOCKET, SO_RCVBUF, &xr, &sl));
+	printf("After changing RCVBUF         | %7d | %7d |\n", xs, xr);
+}
+
+/*
+ * Resize the send and receive buffers of a connected socketpair
+ * Print some useful debugging info too
+ */
+ATF_TC_WITHOUT_HEAD(resize_connected_buffers);
+ATF_TC_BODY(resize_connected_buffers, tc)
+{
+	int sv[2];
+	int sndbuf = 12345;
+	int rcvbuf = 23456;
+	int err;
+	int ls, lr, rs, rr;
+	socklen_t sl = sizeof(ls);
+
+	/* setup the socket pair */
+	do_socketpair(sv);
+
+	printf("                       Socket Buffer Sizes\n");
+	printf("                              | Left Socket       | Right Socket      |\n");
+	printf("                              | SNDBUF  | RCVBUF  | SNDBUF  | RCVBUF  |\n");
+	ATF_CHECK_EQ(0, getsockopt(sv[0], SOL_SOCKET, SO_SNDBUF, &ls, &sl));
+	ATF_CHECK_EQ(0, getsockopt(sv[0], SOL_SOCKET, SO_RCVBUF, &lr, &sl));
+	ATF_CHECK_EQ(0, getsockopt(sv[1], SOL_SOCKET, SO_SNDBUF, &rs, &sl));
+	ATF_CHECK_EQ(0, getsockopt(sv[1], SOL_SOCKET, SO_RCVBUF, &rr, &sl));
+	printf("Default                       | %7d | %7d | %7d | %7d |\n",
+	    ls, lr, rs, rr);
+
+	/* Update one side's send buffer */
+	err = setsockopt(sv[0], SOL_SOCKET, SO_SNDBUF, &sndbuf, sizeof(sndbuf));
+	if (err != 0){
+		perror("setsockopt");
+		atf_tc_fail("setsockopt(SO_SNDBUF) failed");
+	}
+
+	ATF_CHECK_EQ(0, getsockopt(sv[0], SOL_SOCKET, SO_SNDBUF, &ls, &sl));
+	ATF_CHECK_EQ(0, getsockopt(sv[0], SOL_SOCKET, SO_RCVBUF, &lr, &sl));
+	ATF_CHECK_EQ(0, getsockopt(sv[1], SOL_SOCKET, SO_SNDBUF, &rs, &sl));
+	ATF_CHECK_EQ(0, getsockopt(sv[1], SOL_SOCKET, SO_RCVBUF, &rr, &sl));
+	printf("After changing Left's SNDBUF  | %7d | %7d | %7d | %7d |\n",
+	    ls, lr, rs, rr);
+
+	/* Update the same side's receive buffer */
+	err = setsockopt(sv[0], SOL_SOCKET, SO_RCVBUF, &rcvbuf, sizeof(rcvbuf));
+	if (err != 0){
+		perror("setsockopt");
+		atf_tc_fail("setsockopt(SO_RCVBUF) failed");
+	}
+
+	ATF_CHECK_EQ(0, getsockopt(sv[0], SOL_SOCKET, SO_SNDBUF, &ls, &sl));
+	ATF_CHECK_EQ(0, getsockopt(sv[0], SOL_SOCKET, SO_RCVBUF, &lr, &sl));
+	ATF_CHECK_EQ(0, getsockopt(sv[1], SOL_SOCKET, SO_SNDBUF, &rs, &sl));
+	ATF_CHECK_EQ(0, getsockopt(sv[1], SOL_SOCKET, SO_RCVBUF, &rr, &sl));
+	printf("After changing Left's RCVBUF  | %7d | %7d | %7d | %7d |\n",
+	    ls, lr, rs, rr);
+}
+
+
+/* send(2) and recv(2) a single short record */
+ATF_TC_WITHOUT_HEAD(send_recv);
+ATF_TC_BODY(send_recv, tc)
+{
+	int s;
+	int sv[2];
+	const int bufsize = 64;
+	const char *data = "data";
+	char recv_buf[bufsize];
+	size_t datalen;
+	ssize_t ssize, rsize;
+
+	/* setup the socket pair */
+	do_socketpair(sv);
+
+	/* send and receive a small packet */
+	datalen = strlen(data) + 1;	/* +1 for the null */
+	ssize = send(sv[0], data, datalen, MSG_EOR);
+	if (ssize < 0) {
+		perror("send");
+		atf_tc_fail("send returned < 0");
+	}
+	ATF_CHECK_EQ_MSG(datalen, ssize, "expected %zd=send(...) but got %zd",
+	    datalen, ssize);
+
+	rsize = recv(sv[1], recv_buf, bufsize, MSG_WAITALL);
+	ATF_CHECK_EQ(datalen, rsize);
+}
+
+/* sendto(2) and recvfrom(2) a single short record
+ * According to The Open Group Base Specifications Issue 6 IEEE Std 1003.1, 2004
+ * Edition, sendto(2) is exactly the same as send(2) on a connection-mode socket
+ *
+ * According to the same spec, not all protocols are required to provide the
+ * source addres in recvfrom(2).
+ */
+ATF_TC_WITHOUT_HEAD(sendto_recvfrom);
+ATF_TC_BODY(sendto_recvfrom, tc)
+{
+	const char* path;
+	struct sockaddr_storage from;
+	int s;
+	int sv[2];
+	const int bufsize = 64;
+	const char *data = "data";
+	char recv_buf[bufsize];
+	size_t datalen;
+	ssize_t ssize, rsize;
+	socklen_t fromlen;
+
+	/* setup the socket pair */
+	path = mk_pair_of_sockets(sv);
+
+	/* send and receive a small packet */
+	datalen = strlen(data) + 1;	/* +1 for the null */
+	ssize = sendto(sv[0], data, datalen, MSG_EOR, NULL, 0);
+	if (ssize < 0) {
+		perror("send");
+		atf_tc_fail("send returned < 0");
+	}
+	ATF_CHECK_EQ_MSG(datalen, ssize, "expected %zd=send(...) but got %zd",
+	    datalen, ssize);
+
+	fromlen = sizeof(from);
+	rsize = recvfrom(sv[1], recv_buf, bufsize, MSG_WAITALL,
+	    (struct sockaddr*)&from, &fromlen);
+	if (ssize < 0) {
+		perror("recvfrom");
+		atf_tc_fail("recvfrom returned < 0");
+	}
+	ATF_CHECK_EQ(datalen, rsize);
+
+	/* 
+	 * FreeBSD does not currently provide the source address for SEQ_PACKET
+	 * AF_UNIX sockets, and POSIX does not require it, so these two checks
+	 * are disabled.  If FreeBSD gains that feature in the future, then
+	 * these checks may be reenabled
+	 */
+	/* ATF_CHECK_EQ(PF_LOCAL, from.ss_family); */
+	/* ATF_CHECK_STREQ(path, ((struct sockaddr_un*)&from)->sun_path); */
+}
+
+/* 
+ * send(2) and recv(2) a single short record with sockets created the
+ * traditional way, involving bind, listen, connect, and accept
+ */
+ATF_TC_WITHOUT_HEAD(send_recv_with_connect);
+ATF_TC_BODY(send_recv_with_connect, tc)
+{
+	const char* path;
+	int sv[2];
+	const int bufsize = 64;
+	const char *data = "data";
+	char recv_buf[bufsize];
+	size_t datalen;
+	ssize_t ssize, rsize;
+
+	mk_pair_of_sockets(sv);
+
+	/* send and receive a small packet */
+	datalen = strlen(data) + 1;	/* +1 for the null */
+	ssize = send(sv[0], data, datalen, MSG_EOR);
+	if (ssize < 0) {
+		perror("send");
+		atf_tc_fail("send returned < 0");
+	}
+	ATF_CHECK_EQ_MSG(datalen, ssize, "expected %zd=send(...) but got %zd",
+	    datalen, ssize);
+
+	rsize = recv(sv[1], recv_buf, bufsize, MSG_WAITALL);
+	ATF_CHECK_EQ(datalen, rsize);
+}
+
+/* send(2) should fail on a shutdown socket */
+ATF_TC_WITHOUT_HEAD(shutdown_send);
+ATF_TC_BODY(shutdown_send, tc)
+{
+	int s;
+	const char *data = "data";
+	ssize_t ssize;
+
+	s = socket(PF_LOCAL, SOCK_SEQPACKET, 0);
+	ATF_CHECK(s >= 0);
+	ATF_CHECK_EQ(0, shutdown(s, SHUT_RDWR));
+	/* USE MSG_NOSIGNAL so we don't get SIGPIPE */
+	ssize = send(s, data, sizeof(data), MSG_EOR | MSG_NOSIGNAL);
+	ATF_CHECK_EQ(EPIPE, errno);
+	ATF_CHECK_EQ(-1, ssize);
+}
+
+/* send(2) should cause SIGPIPE on a shutdown socket */
+ATF_TC_WITHOUT_HEAD(shutdown_send_sigpipe);
+ATF_TC_BODY(shutdown_send_sigpipe, tc)
+{
+	int s;
+	const char *data = "data";
+	ssize_t ssize;
+
+	s = socket(PF_LOCAL, SOCK_SEQPACKET, 0);
+	ATF_CHECK(s >= 0);
+	ATF_CHECK_EQ(0, shutdown(s, SHUT_RDWR));
+	ATF_REQUIRE(SIG_ERR != signal(SIGPIPE, shutdown_send_sigpipe_handler));
+	ssize = send(s, data, sizeof(data), MSG_EOR);
+	ATF_CHECK_EQ(1, got_sigpipe);
+}
+
+/* nonblocking send(2) and recv(2) a single short record */
+ATF_TC_WITHOUT_HEAD(send_recv_nonblocking);
+ATF_TC_BODY(send_recv_nonblocking, tc)
+{
+	int s;
+	int sv[2];
+	const int bufsize = 64;
+	const char *data = "data";
+	char recv_buf[bufsize];
+	size_t datalen;
+	ssize_t ssize, rsize;
+
+	/* setup the socket pair */
+	do_socketpair_nonblocking(sv);
+
+	/* Verify that there is nothing to receive */
+	rsize = recv(sv[1], recv_buf, bufsize, MSG_WAITALL);
+	ATF_CHECK_EQ(EAGAIN, errno);
+	ATF_CHECK_EQ(-1, rsize);
+
+	/* send and receive a small packet */
+	datalen = strlen(data) + 1;	/* +1 for the null */
+	ssize = send(sv[0], data, datalen, MSG_EOR);
+	if (ssize < 0) {
+		perror("send");
+		atf_tc_fail("send returned < 0");
+	}
+	ATF_CHECK_EQ_MSG(datalen, ssize, "expected %zd=send(...) but got %zd",
+	    datalen, ssize);
+
+	rsize = recv(sv[1], recv_buf, bufsize, MSG_WAITALL);
+	ATF_CHECK_EQ(datalen, rsize);
+}
+
+/* 
+ * We should get EMSGSIZE if we try to send a message larger than the socket
+ * buffer, with blocking sockets
+ */
+ATF_TC_WITHOUT_HEAD(emsgsize);
+ATF_TC_BODY(emsgsize, tc)
+{
+	int s;
+	int sv[2];
+	const size_t sndbufsize = 8192;
+	const size_t rcvbufsize = 8192;
+	const size_t pktsize = (sndbufsize + rcvbufsize) * 2;
+	char sndbuf[pktsize];
+	char recv_buf[pktsize];
+	ssize_t ssize, rsize;
+
+	/* setup the socket pair */
+	do_socketpair(sv);
+	/* Setup the buffers */
+	ATF_REQUIRE_EQ(0, setsockopt(sv[0], SOL_SOCKET, SO_SNDBUF, &sndbufsize,
+	    sizeof(sndbufsize)));
+	ATF_REQUIRE_EQ(0, setsockopt(sv[1], SOL_SOCKET, SO_RCVBUF, &rcvbufsize,
+	    sizeof(rcvbufsize)));
+
+	ssize = send(sv[0], sndbuf, pktsize, MSG_EOR);
+	ATF_CHECK_EQ(EMSGSIZE, errno);
+	ATF_CHECK_EQ(-1, ssize);
+}
+
+/* 
+ * We should get EMSGSIZE if we try to send a message larger than the socket
+ * buffer, with nonblocking sockets
+ */
+ATF_TC_WITHOUT_HEAD(emsgsize_nonblocking);
+ATF_TC_BODY(emsgsize_nonblocking, tc)
+{
+	int s;
+	int sv[2];
+	const size_t sndbufsize = 8192;
+	const size_t rcvbufsize = 8192;
+	const size_t pktsize = (sndbufsize + rcvbufsize) * 2;
+	char sndbuf[pktsize];
+	char recv_buf[pktsize];
+	ssize_t ssize, rsize;
+
+	/* setup the socket pair */
+	do_socketpair_nonblocking(sv);
+	/* Setup the buffers */
+	ATF_REQUIRE_EQ(0, setsockopt(sv[0], SOL_SOCKET, SO_SNDBUF, &sndbufsize,
+	    sizeof(sndbufsize)));
+	ATF_REQUIRE_EQ(0, setsockopt(sv[1], SOL_SOCKET, SO_RCVBUF, &rcvbufsize,
+	    sizeof(rcvbufsize)));
+
+	ssize = send(sv[0], sndbuf, pktsize, MSG_EOR);
+	ATF_CHECK_EQ(EMSGSIZE, errno);
+	ATF_CHECK_EQ(-1, ssize);
+}
+
+
+/* 
+ * We should get EAGAIN if we try to send a message larger than the socket
+ * buffer, with nonblocking sockets.  Test with several different sockbuf sizes
+ */
+ATF_TC_WITHOUT_HEAD(eagain_8k_8k);
+ATF_TC_BODY(eagain_8k_8k, tc)
+{
+	atf_tc_expect_fail("PR kern/185812 send(2) on a UNIX domain SEQPACKET socket returns EMSGSIZE instead of EAGAIN");
+	test_eagain(8192, 8192);
+}
+ATF_TC_WITHOUT_HEAD(eagain_8k_128k);
+ATF_TC_BODY(eagain_8k_128k, tc)
+{
+	atf_tc_expect_fail("PR kern/185812 send(2) on a UNIX domain SEQPACKET socket returns EMSGSIZE instead of EAGAIN");
+	test_eagain(8192, 131072);
+}
+ATF_TC_WITHOUT_HEAD(eagain_128k_8k);
+ATF_TC_BODY(eagain_128k_8k, tc)
+{
+	atf_tc_expect_fail("PR kern/185812 send(2) on a UNIX domain SEQPACKET socket returns EMSGSIZE instead of EAGAIN");
+	test_eagain(131072, 8192);
+}
+ATF_TC_WITHOUT_HEAD(eagain_128k_128k);
+ATF_TC_BODY(eagain_128k_128k, tc)
+{
+	atf_tc_expect_fail("PR kern/185812 send(2) on a UNIX domain SEQPACKET socket returns EMSGSIZE instead of EAGAIN");
+	test_eagain(131072, 131072);
+}
+
+
+/* 
+ * nonblocking send(2) and recv(2) of several records, which should collectively
+ * fill up the send buffer but not the receive buffer
+ */
+ATF_TC_WITHOUT_HEAD(rcvbuf_oversized);
+ATF_TC_BODY(rcvbuf_oversized, tc)
+{
+	int s, i, j;
+	int sv[2];
+	const size_t sndbufsize = 8192;
+	const size_t rcvbufsize = 131072;
+	const size_t geom_mean_bufsize = 32768;
+	const int pktsize = 1024;
+	char sndbuf[pktsize];
+	char recv_buf[pktsize];
+	size_t datalen;
+	ssize_t ssize, rsize;
+
+	/* setup the socket pair */
+	do_socketpair_nonblocking(sv);
+
+	/* 
+	 * Send and receive packets that are collectively greater than the send
+	 * buffer, but less than the receive buffer
+	 */
+	for (i=0; i < geom_mean_bufsize / pktsize; i++) {
+		/* Fill the buffer */
+		memset(sndbuf, i, pktsize);
+
+		/* send the packet */
+		ssize = send(sv[0], sndbuf, pktsize, MSG_EOR);
+		if (ssize < 0) {
+			perror("send");
+			atf_tc_fail("send returned < 0");
+		}
+		ATF_CHECK_EQ_MSG(pktsize, ssize,
+		    "expected %zd=send(...) but got %zd", pktsize, ssize);
+
+		/* Receive it */
+
+		rsize = recv(sv[1], recv_buf, pktsize, MSG_WAITALL);
+		if (rsize < 0) {
+			perror("recv");
+			atf_tc_fail("recv returned < 0");
+		}
+		ATF_CHECK_EQ_MSG(pktsize, rsize,
+		    "expected %zd=send(...) but got %zd", pktsize, rsize);
+
+		/* Verify the contents */
+		ATF_CHECK_EQ_MSG(0, memcmp(sndbuf, recv_buf, pktsize), 
+		    "Received data miscompare");
+	}
+
+	/* Trying to receive again should return EAGAIN */
+	rsize = recv(sv[1], recv_buf, pktsize, MSG_WAITALL);
+	ATF_CHECK_EQ(EAGAIN, errno);
+	ATF_CHECK_EQ(-1, rsize);
+}
+
+/* 
+ * Simulate the behavior of a blocking pipe.  The sender will send until his
+ * buffer fills up, then we'll simulate a scheduler switch that will allow the
+ * receiver to read until his buffer empties.  Repeat the process until the
+ * transfer is complete.
+ * Repeat the test with multiple send and receive buffer sizes
+ */
+ATF_TC_WITHOUT_HEAD(pipe_simulator_8k_8k);
+ATF_TC_BODY(pipe_simulator_8k_8k, tc)
+{
+	test_pipe_simulator(8192, 8192);
+}
+

*** DIFF OUTPUT TRUNCATED AT 1000 LINES ***
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 2 Alan Somers freebsd_committer 2014-02-20 16:11:45 UTC
Responsible Changed
From-To: freebsd-bugs->asomers

Testing patch now
Comment 3 dfilter service freebsd_committer 2014-03-06 21:44:48 UTC
Author: asomers
Date: Thu Mar  6 20:24:15 2014
New Revision: 262867
URL: http://svnweb.freebsd.org/changeset/base/262867

Log:
  Fix PR kern/185813 "SOCK_SEQPACKET AF_UNIX sockets with asymmetrical
  buffers drop packets".  It was caused by a check for the space available
  in a sockbuf, but it was checking the wrong sockbuf.
  
  sys/sys/sockbuf.h
  sys/kern/uipc_sockbuf.c
      Add sbappendaddr_nospacecheck_locked(), which is just like
      sbappendaddr_locked but doesn't validate the receiving socket's
      space.  Factor out common code into sbappendaddr_locked_internal().
      We shouldn't simply make sbappendaddr_locked check the space and
      then call sbappendaddr_nospacecheck_locked, because that would cause
      the O(n) function m_length to be called twice.
  
  sys/kern/uipc_usrreq.c
      Use sbappendaddr_nospacecheck_locked for SOCK_SEQPACKET sockets,
      because the receiving sockbuf's size limit is irrelevant.
  
  tests/sys/kern/unix_seqpacket_test.c
      Now that 185813 is fixed, pipe_128k_8k fails intermittently due to
      185812.  Make it fail every time by adding a usleep after starting
      the writer thread and before starting the reader thread in
      test_pipe.  That gives the writer time to fill up its send buffer.
      Also, clear the expected failure message due to 185813.  It actually
      said "185812", but that was a typo.
  
  PR:		kern/185813
  Reviewed by:	silence from freebsd-net@ and rwatson@
  MFC after:	3 weeks
  Sponsored by:	Spectra Logic Corporation

Modified:
  head/sys/kern/uipc_sockbuf.c
  head/sys/kern/uipc_usrreq.c
  head/sys/sys/sockbuf.h
  head/tests/sys/kern/unix_seqpacket_test.c

Modified: head/sys/kern/uipc_sockbuf.c
==============================================================================
--- head/sys/kern/uipc_sockbuf.c	Thu Mar  6 20:05:13 2014	(r262866)
+++ head/sys/kern/uipc_sockbuf.c	Thu Mar  6 20:24:15 2014	(r262867)
@@ -620,29 +620,12 @@ sbappendrecord(struct sockbuf *sb, struc
 	SOCKBUF_UNLOCK(sb);
 }
 
-/*
- * Append address and data, and optionally, control (ancillary) data to the
- * receive queue of a socket.  If present, m0 must include a packet header
- * with total length.  Returns 0 if no space in sockbuf or insufficient
- * mbufs.
- */
-int
-sbappendaddr_locked(struct sockbuf *sb, const struct sockaddr *asa,
-    struct mbuf *m0, struct mbuf *control)
+/* Helper routine that appends data, control, and address to a sockbuf. */
+static int
+sbappendaddr_locked_internal(struct sockbuf *sb, const struct sockaddr *asa,
+    struct mbuf *m0, struct mbuf *control, struct mbuf *ctrl_last)
 {
 	struct mbuf *m, *n, *nlast;
-	int space = asa->sa_len;
-
-	SOCKBUF_LOCK_ASSERT(sb);
-
-	if (m0 && (m0->m_flags & M_PKTHDR) == 0)
-		panic("sbappendaddr_locked");
-	if (m0)
-		space += m0->m_pkthdr.len;
-	space += m_length(control, &n);
-
-	if (space > sbspace(sb))
-		return (0);
 #if MSIZE <= 256
 	if (asa->sa_len > MLEN)
 		return (0);
@@ -652,8 +635,8 @@ sbappendaddr_locked(struct sockbuf *sb, 
 		return (0);
 	m->m_len = asa->sa_len;
 	bcopy(asa, mtod(m, caddr_t), asa->sa_len);
-	if (n)
-		n->m_next = m0;		/* concatenate data to control */
+	if (ctrl_last)
+		ctrl_last->m_next = m0;	/* concatenate data to control */
 	else
 		control = m0;
 	m->m_next = control;
@@ -677,6 +660,50 @@ sbappendaddr_locked(struct sockbuf *sb, 
  * mbufs.
  */
 int
+sbappendaddr_locked(struct sockbuf *sb, const struct sockaddr *asa,
+    struct mbuf *m0, struct mbuf *control)
+{
+	struct mbuf *ctrl_last;
+	int space = asa->sa_len;
+
+	SOCKBUF_LOCK_ASSERT(sb);
+
+	if (m0 && (m0->m_flags & M_PKTHDR) == 0)
+		panic("sbappendaddr_locked");
+	if (m0)
+		space += m0->m_pkthdr.len;
+	space += m_length(control, &ctrl_last);
+
+	if (space > sbspace(sb))
+		return (0);
+	return (sbappendaddr_locked_internal(sb, asa, m0, control, ctrl_last));
+}
+
+/*
+ * Append address and data, and optionally, control (ancillary) data to the
+ * receive queue of a socket.  If present, m0 must include a packet header
+ * with total length.  Returns 0 if insufficient mbufs.  Does not validate space
+ * on the receiving sockbuf.
+ */
+int
+sbappendaddr_nospacecheck_locked(struct sockbuf *sb, const struct sockaddr *asa,
+    struct mbuf *m0, struct mbuf *control)
+{
+	struct mbuf *ctrl_last;
+
+	SOCKBUF_LOCK_ASSERT(sb);
+
+	ctrl_last = (control == NULL) ? NULL : m_last(control);
+	return (sbappendaddr_locked_internal(sb, asa, m0, control, ctrl_last));
+}
+
+/*
+ * Append address and data, and optionally, control (ancillary) data to the
+ * receive queue of a socket.  If present, m0 must include a packet header
+ * with total length.  Returns 0 if no space in sockbuf or insufficient
+ * mbufs.
+ */
+int
 sbappendaddr(struct sockbuf *sb, const struct sockaddr *asa,
     struct mbuf *m0, struct mbuf *control)
 {

Modified: head/sys/kern/uipc_usrreq.c
==============================================================================
--- head/sys/kern/uipc_usrreq.c	Thu Mar  6 20:05:13 2014	(r262866)
+++ head/sys/kern/uipc_usrreq.c	Thu Mar  6 20:24:15 2014	(r262867)
@@ -892,7 +892,8 @@ uipc_send(struct socket *so, int flags, 
 			from = &sun_noname;
 		so2 = unp2->unp_socket;
 		SOCKBUF_LOCK(&so2->so_rcv);
-		if (sbappendaddr_locked(&so2->so_rcv, from, m, control)) {
+		if (sbappendaddr_nospacecheck_locked(&so2->so_rcv, from, m,
+		    control)) {
 			sorwakeup_locked(so2);
 			m = NULL;
 			control = NULL;
@@ -977,8 +978,14 @@ uipc_send(struct socket *so, int flags, 
 			const struct sockaddr *from;
 
 			from = &sun_noname;
-			if (sbappendaddr_locked(&so2->so_rcv, from, m,
-			    control))
+			/*
+			 * Don't check for space available in so2->so_rcv.
+			 * Unix domain sockets only check for space in the
+			 * sending sockbuf, and that check is performed one
+			 * level up the stack.
+			 */
+			if (sbappendaddr_nospacecheck_locked(&so2->so_rcv,
+				from, m, control))
 				control = NULL;
 			break;
 			}

Modified: head/sys/sys/sockbuf.h
==============================================================================
--- head/sys/sys/sockbuf.h	Thu Mar  6 20:05:13 2014	(r262866)
+++ head/sys/sys/sockbuf.h	Thu Mar  6 20:24:15 2014	(r262867)
@@ -127,6 +127,8 @@ int	sbappendaddr(struct sockbuf *sb, con
 	    struct mbuf *m0, struct mbuf *control);
 int	sbappendaddr_locked(struct sockbuf *sb, const struct sockaddr *asa,
 	    struct mbuf *m0, struct mbuf *control);
+int	sbappendaddr_nospacecheck_locked(struct sockbuf *sb,
+	    const struct sockaddr *asa, struct mbuf *m0, struct mbuf *control);
 int	sbappendcontrol(struct sockbuf *sb, struct mbuf *m0,
 	    struct mbuf *control);
 int	sbappendcontrol_locked(struct sockbuf *sb, struct mbuf *m0,

Modified: head/tests/sys/kern/unix_seqpacket_test.c
==============================================================================
--- head/tests/sys/kern/unix_seqpacket_test.c	Thu Mar  6 20:05:13 2014	(r262866)
+++ head/tests/sys/kern/unix_seqpacket_test.c	Thu Mar  6 20:24:15 2014	(r262867)
@@ -361,6 +361,12 @@ test_pipe(size_t sndbufsize, size_t rcvb
 	reader_data.so = sv[1];
 	ATF_REQUIRE_EQ(0, pthread_create(&writer, NULL, test_pipe_writer,
 	    				 (void*)&writer_data));
+	/* 
+	 * Give the writer time to start writing, and hopefully block, before
+	 * starting the reader.  This increases the likelihood of the test case
+	 * failing due to PR kern/185812
+	 */
+	usleep(1000);
 	ATF_REQUIRE_EQ(0, pthread_create(&reader, NULL, test_pipe_reader,
 	    				 (void*)&reader_data));
 
@@ -951,7 +957,6 @@ ATF_TC_BODY(pipe_simulator_8k_128k, tc)
 ATF_TC_WITHOUT_HEAD(pipe_simulator_128k_8k);
 ATF_TC_BODY(pipe_simulator_128k_8k, tc)
 {
-	atf_tc_expect_fail("PR kern/185812 SOCK_SEQPACKET AF_UNIX sockets with asymmetrical buffers drop packets");
 	test_pipe_simulator(131072, 8192);
 }
 
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 4 Alan Somers freebsd_committer 2014-03-06 21:58:24 UTC
State Changed
From-To: open->closed

patched by r262867
Comment 5 dfilter service freebsd_committer 2014-03-27 16:47:39 UTC
Author: asomers
Date: Thu Mar 27 16:47:35 2014
New Revision: 263820
URL: http://svnweb.freebsd.org/changeset/base/263820

Log:
  MFC r262867
  
  Fix PR kern/185813 "SOCK_SEQPACKET AF_UNIX sockets with asymmetrical buffers
  drop packets".  It was caused by a check for the space available in a
  sockbuf, but it was checking the wrong sockbuf.
  
  sys/sys/sockbuf.h
  sys/kern/uipc_sockbuf.c
  	Add sbappendaddr_nospacecheck_locked(), which is just like
  	sbappendaddr_locked but doesn't validate the receiving socket's space.
  	Factor out common code into sbappendaddr_locked_internal().  We
  	shouldn't simply make sbappendaddr_locked check the space and then call
  	sbappendaddr_nospacecheck_locked, because that would cause the O(n)
  	function m_length to be called twice.
  
  sys/kern/uipc_usrreq.c
  	Use sbappendaddr_nospacecheck_locked for SOCK_SEQPACKET sockets,
  	because the receiving sockbuf's size limit is irrelevant.
  
  tests/sys/kern/unix_seqpacket_test.c
  	Now that 185813 is fixed, pipe_128k_8k fails intermittently due to
  	185812.  Make it fail every time by adding a usleep after starting the
  	writer thread and before starting the reader thread in test_pipe.  That
  	gives the writer time to fill up its send buffer.  Also, clear the
  	expected failure message due to 185813.  It actually said "185812", but
  	that was a typo.
  
  PR:		kern/185813

Modified:
  stable/10/sys/kern/uipc_sockbuf.c
  stable/10/sys/kern/uipc_usrreq.c
  stable/10/sys/sys/sockbuf.h
  stable/10/tests/sys/kern/unix_seqpacket_test.c
Directory Properties:
  stable/10/   (props changed)

Modified: stable/10/sys/kern/uipc_sockbuf.c
==============================================================================
--- stable/10/sys/kern/uipc_sockbuf.c	Thu Mar 27 16:28:46 2014	(r263819)
+++ stable/10/sys/kern/uipc_sockbuf.c	Thu Mar 27 16:47:35 2014	(r263820)
@@ -620,29 +620,12 @@ sbappendrecord(struct sockbuf *sb, struc
 	SOCKBUF_UNLOCK(sb);
 }
 
-/*
- * Append address and data, and optionally, control (ancillary) data to the
- * receive queue of a socket.  If present, m0 must include a packet header
- * with total length.  Returns 0 if no space in sockbuf or insufficient
- * mbufs.
- */
-int
-sbappendaddr_locked(struct sockbuf *sb, const struct sockaddr *asa,
-    struct mbuf *m0, struct mbuf *control)
+/* Helper routine that appends data, control, and address to a sockbuf. */
+static int
+sbappendaddr_locked_internal(struct sockbuf *sb, const struct sockaddr *asa,
+    struct mbuf *m0, struct mbuf *control, struct mbuf *ctrl_last)
 {
 	struct mbuf *m, *n, *nlast;
-	int space = asa->sa_len;
-
-	SOCKBUF_LOCK_ASSERT(sb);
-
-	if (m0 && (m0->m_flags & M_PKTHDR) == 0)
-		panic("sbappendaddr_locked");
-	if (m0)
-		space += m0->m_pkthdr.len;
-	space += m_length(control, &n);
-
-	if (space > sbspace(sb))
-		return (0);
 #if MSIZE <= 256
 	if (asa->sa_len > MLEN)
 		return (0);
@@ -652,8 +635,8 @@ sbappendaddr_locked(struct sockbuf *sb, 
 		return (0);
 	m->m_len = asa->sa_len;
 	bcopy(asa, mtod(m, caddr_t), asa->sa_len);
-	if (n)
-		n->m_next = m0;		/* concatenate data to control */
+	if (ctrl_last)
+		ctrl_last->m_next = m0;	/* concatenate data to control */
 	else
 		control = m0;
 	m->m_next = control;
@@ -677,6 +660,50 @@ sbappendaddr_locked(struct sockbuf *sb, 
  * mbufs.
  */
 int
+sbappendaddr_locked(struct sockbuf *sb, const struct sockaddr *asa,
+    struct mbuf *m0, struct mbuf *control)
+{
+	struct mbuf *ctrl_last;
+	int space = asa->sa_len;
+
+	SOCKBUF_LOCK_ASSERT(sb);
+
+	if (m0 && (m0->m_flags & M_PKTHDR) == 0)
+		panic("sbappendaddr_locked");
+	if (m0)
+		space += m0->m_pkthdr.len;
+	space += m_length(control, &ctrl_last);
+
+	if (space > sbspace(sb))
+		return (0);
+	return (sbappendaddr_locked_internal(sb, asa, m0, control, ctrl_last));
+}
+
+/*
+ * Append address and data, and optionally, control (ancillary) data to the
+ * receive queue of a socket.  If present, m0 must include a packet header
+ * with total length.  Returns 0 if insufficient mbufs.  Does not validate space
+ * on the receiving sockbuf.
+ */
+int
+sbappendaddr_nospacecheck_locked(struct sockbuf *sb, const struct sockaddr *asa,
+    struct mbuf *m0, struct mbuf *control)
+{
+	struct mbuf *ctrl_last;
+
+	SOCKBUF_LOCK_ASSERT(sb);
+
+	ctrl_last = (control == NULL) ? NULL : m_last(control);
+	return (sbappendaddr_locked_internal(sb, asa, m0, control, ctrl_last));
+}
+
+/*
+ * Append address and data, and optionally, control (ancillary) data to the
+ * receive queue of a socket.  If present, m0 must include a packet header
+ * with total length.  Returns 0 if no space in sockbuf or insufficient
+ * mbufs.
+ */
+int
 sbappendaddr(struct sockbuf *sb, const struct sockaddr *asa,
     struct mbuf *m0, struct mbuf *control)
 {

Modified: stable/10/sys/kern/uipc_usrreq.c
==============================================================================
--- stable/10/sys/kern/uipc_usrreq.c	Thu Mar 27 16:28:46 2014	(r263819)
+++ stable/10/sys/kern/uipc_usrreq.c	Thu Mar 27 16:47:35 2014	(r263820)
@@ -892,7 +892,8 @@ uipc_send(struct socket *so, int flags, 
 			from = &sun_noname;
 		so2 = unp2->unp_socket;
 		SOCKBUF_LOCK(&so2->so_rcv);
-		if (sbappendaddr_locked(&so2->so_rcv, from, m, control)) {
+		if (sbappendaddr_nospacecheck_locked(&so2->so_rcv, from, m,
+		    control)) {
 			sorwakeup_locked(so2);
 			m = NULL;
 			control = NULL;
@@ -977,8 +978,14 @@ uipc_send(struct socket *so, int flags, 
 			const struct sockaddr *from;
 
 			from = &sun_noname;
-			if (sbappendaddr_locked(&so2->so_rcv, from, m,
-			    control))
+			/*
+			 * Don't check for space available in so2->so_rcv.
+			 * Unix domain sockets only check for space in the
+			 * sending sockbuf, and that check is performed one
+			 * level up the stack.
+			 */
+			if (sbappendaddr_nospacecheck_locked(&so2->so_rcv,
+				from, m, control))
 				control = NULL;
 			break;
 			}

Modified: stable/10/sys/sys/sockbuf.h
==============================================================================
--- stable/10/sys/sys/sockbuf.h	Thu Mar 27 16:28:46 2014	(r263819)
+++ stable/10/sys/sys/sockbuf.h	Thu Mar 27 16:47:35 2014	(r263820)
@@ -127,6 +127,8 @@ int	sbappendaddr(struct sockbuf *sb, con
 	    struct mbuf *m0, struct mbuf *control);
 int	sbappendaddr_locked(struct sockbuf *sb, const struct sockaddr *asa,
 	    struct mbuf *m0, struct mbuf *control);
+int	sbappendaddr_nospacecheck_locked(struct sockbuf *sb,
+	    const struct sockaddr *asa, struct mbuf *m0, struct mbuf *control);
 int	sbappendcontrol(struct sockbuf *sb, struct mbuf *m0,
 	    struct mbuf *control);
 int	sbappendcontrol_locked(struct sockbuf *sb, struct mbuf *m0,

Modified: stable/10/tests/sys/kern/unix_seqpacket_test.c
==============================================================================
--- stable/10/tests/sys/kern/unix_seqpacket_test.c	Thu Mar 27 16:28:46 2014	(r263819)
+++ stable/10/tests/sys/kern/unix_seqpacket_test.c	Thu Mar 27 16:47:35 2014	(r263820)
@@ -360,6 +360,12 @@ test_pipe(size_t sndbufsize, size_t rcvb
 	reader_data.so = sv[1];
 	ATF_REQUIRE_EQ(0, pthread_create(&writer, NULL, test_pipe_writer,
 	    				 (void*)&writer_data));
+	/* 
+	 * Give the writer time to start writing, and hopefully block, before
+	 * starting the reader.  This increases the likelihood of the test case
+	 * failing due to PR kern/185812
+	 */
+	usleep(1000);
 	ATF_REQUIRE_EQ(0, pthread_create(&reader, NULL, test_pipe_reader,
 	    				 (void*)&reader_data));
 
@@ -946,7 +952,6 @@ ATF_TC_BODY(pipe_simulator_8k_128k, tc)
 ATF_TC_WITHOUT_HEAD(pipe_simulator_128k_8k);
 ATF_TC_BODY(pipe_simulator_128k_8k, tc)
 {
-	atf_tc_expect_fail("PR kern/185812 SOCK_SEQPACKET AF_UNIX sockets with asymmetrical buffers drop packets");
 	test_pipe_simulator(131072, 8192);
 }
 
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"