kern/185813: SOCK_SEQPACKET AF_UNIX sockets with asymmetrical buffers drop packets

[Alan Somers]

On Thu, Jan 23, 2014 at 5:31 PM, Alan Somers <asomers at freebsd.org> wrote:
> On Thu, Jan 23, 2014 at 11:26 AM, Adrian Chadd <adrian at freebsd.org> wrote:
>> Well, shouldn't we fix the API/code so it doesn't drop packets,
>> regardless of the sensibility or non-sensibility of different
>> transmit/receive buffer sizes?
>
> That would be nice, but it may be beyond my ability to do so.  The
> relevant code is very complicated, and most of it is in
> domain-agnostic code where we can't introduce AF_UNIX specific special
> cases.
>
> It may be possible to change the single-buffer optimization to use the
> receiving sockbuf's size for space calculations in uipc_send() instead
> of the transmitting sockbuf's size.  I could try to do that, though it
> may cause existing programs to fail if they're depending on
> setsockopt(s, SOL_SOCKET, SO_SNDBUF, ...) to have an effect.
>

I decided that the space check within uipc_send() wasn't necessary.
Here is a new patch.  It simply eliminates the space check within
uipc_send(), relying on the space check in sosend_generic to enforce
the sockbuf limits.  I audited all the callers of sbappendaddr() and
sbappendaddr_locked() and decided that the space check is appropriate
for all of them except uipc_send().

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.

-Alan

Index: sys/kern/uipc_sockbuf.c
===================================================================
--- sys/kern/uipc_sockbuf.c    (revision 262245)
+++ sys/kern/uipc_sockbuf.c    (working copy)
@@ -620,29 +620,12 @@
     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 @@
         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 @@
  * 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)
 {
Index: sys/kern/uipc_usrreq.c
===================================================================
--- sys/kern/uipc_usrreq.c    (revision 262245)
+++ sys/kern/uipc_usrreq.c    (working copy)
@@ -892,7 +892,8 @@
             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 @@
             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 in
+             * sosend_generic.
+             */
+            if (sbappendaddr_nospacecheck_locked(&so2->so_rcv,
+                from, m, control))
                 control = NULL;
             break;
             }
Index: sys/sys/sockbuf.h
===================================================================
--- sys/sys/sockbuf.h    (revision 262245)
+++ sys/sys/sockbuf.h    (working copy)
@@ -127,6 +127,8 @@
         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,
Index: tests/sys/kern/unix_seqpacket_test.c
===================================================================
--- tests/sys/kern/unix_seqpacket_test.c    (revision 262245)
+++ tests/sys/kern/unix_seqpacket_test.c    (working copy)
@@ -951,7 +951,6 @@
 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);
 }