From owner-freebsd-net@FreeBSD.ORG Thu Feb 20 18:27:29 2014 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id B3E3E87A for ; Thu, 20 Feb 2014 18:27:29 +0000 (UTC) Received: from mail-wi0-x230.google.com (mail-wi0-x230.google.com [IPv6:2a00:1450:400c:c05::230]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 35107100B for ; Thu, 20 Feb 2014 18:27:29 +0000 (UTC) Received: by mail-wi0-f176.google.com with SMTP id hi5so55wib.3 for ; Thu, 20 Feb 2014 10:27:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:cc:content-type; bh=NXylJLnHkVsW994gMqfXJF8eMy6uXJ48WhxIcnbkyio=; b=QZ7xKu867J9YiuBlbt6l86RHy2alt1qtlm6kPSzPFNoP898zmHZtH0iD+avvQGkC/T ZYm/K4SbKGjku1S3enyTmJL9EpCkcYD9marqU/2njWR37Loc4fZYCIqWfoq1tAJGJhzQ IY4YcX8J5DhN4360v6k4AH/qS9kldIJC6r0niSgIrp/UP4176uOvPQ9PYZ2RQpB7LR1E caIpewmbjQ1yV4krmeaACksx3rypibHRAlh5N9fmkBfXIVKcMZsBWctdgIO64w7Za7jp qOvPdZJLZC17rXFgJyLvldkvHI8bKnEtIWeEQNTNq6fafQWpvQPXEQMFeTtCAIM9yiSl batA== MIME-Version: 1.0 X-Received: by 10.194.63.228 with SMTP id j4mt4152355wjs.34.1392920846680; Thu, 20 Feb 2014 10:27:26 -0800 (PST) Sender: asomers@gmail.com Received: by 10.194.168.197 with HTTP; Thu, 20 Feb 2014 10:27:26 -0800 (PST) In-Reply-To: References: Date: Thu, 20 Feb 2014 11:27:26 -0700 X-Google-Sender-Auth: rk_pNSUgco2_njXYDGEXZSEDygc Message-ID: Subject: Re: kern/185813: SOCK_SEQPACKET AF_UNIX sockets with asymmetrical buffers drop packets From: Alan Somers Content-Type: text/plain; charset=ISO-8859-1 Cc: FreeBSD Net , Adrian Chadd X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 Feb 2014 18:27:29 -0000 On Thu, Jan 23, 2014 at 5:31 PM, Alan Somers wrote: > On Thu, Jan 23, 2014 at 11:26 AM, Adrian Chadd 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); }