From 8de34a88de1d5795c302c057b64cc27c26db460c Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Thu, 6 Mar 2014 20:24:15 +0000 Subject: [PATCH] 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 --- sys/kern/uipc_sockbuf.c | 73 +++++++++++++++++++--------- sys/kern/uipc_usrreq.c | 13 +++-- sys/sys/sockbuf.h | 2 + tests/sys/kern/unix_seqpacket_test.c | 7 ++- 4 files changed, 68 insertions(+), 27 deletions(-) diff --git a/sys/kern/uipc_sockbuf.c b/sys/kern/uipc_sockbuf.c index 8e99f3e3899d..d804d9ae3f39 100644 --- a/sys/kern/uipc_sockbuf.c +++ b/sys/kern/uipc_sockbuf.c @@ -620,29 +620,12 @@ sbappendrecord(struct sockbuf *sb, struct mbuf *m0) 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, const struct sockaddr *asa, 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; @@ -670,6 +653,50 @@ sbappendaddr_locked(struct sockbuf *sb, const struct sockaddr *asa, return (1); } +/* + * 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) +{ + 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 diff --git a/sys/kern/uipc_usrreq.c b/sys/kern/uipc_usrreq.c index e1eee36e45ed..676029a689d5 100644 --- a/sys/kern/uipc_usrreq.c +++ b/sys/kern/uipc_usrreq.c @@ -892,7 +892,8 @@ uipc_send(struct socket *so, int flags, struct mbuf *m, struct sockaddr *nam, 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, struct mbuf *m, struct sockaddr *nam, 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; } diff --git a/sys/sys/sockbuf.h b/sys/sys/sockbuf.h index e7162418e67b..6994fd3fd60f 100644 --- a/sys/sys/sockbuf.h +++ b/sys/sys/sockbuf.h @@ -127,6 +127,8 @@ int sbappendaddr(struct sockbuf *sb, const struct sockaddr *asa, 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, diff --git a/tests/sys/kern/unix_seqpacket_test.c b/tests/sys/kern/unix_seqpacket_test.c index f2904aec060b..51769c4abd42 100644 --- a/tests/sys/kern/unix_seqpacket_test.c +++ b/tests/sys/kern/unix_seqpacket_test.c @@ -361,6 +361,12 @@ test_pipe(size_t sndbufsize, size_t rcvbufsize) 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); }