From owner-freebsd-net@FreeBSD.ORG Thu Mar 8 11:15:39 2012 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 1097D1065674; Thu, 8 Mar 2012 11:15:39 +0000 (UTC) (envelope-from to.my.trociny@gmail.com) Received: from mail-bk0-f54.google.com (mail-bk0-f54.google.com [209.85.214.54]) by mx1.freebsd.org (Postfix) with ESMTP id 5419E8FC12; Thu, 8 Mar 2012 11:15:37 +0000 (UTC) Received: by bkcjc3 with SMTP id jc3so324546bkc.13 for ; Thu, 08 Mar 2012 03:15:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:references:x-comment-to:sender:date:in-reply-to :message-id:user-agent:mime-version:content-type; bh=bow8ZcdnYvoRJP0gR3ULI+bWSTEGLREIYgBC6bfeleE=; b=bBEosz/WFD4MObk1I28wO/1UhgoZ8ZqIjfogKNigzXcYpXKRKuilqRZ2CDCX6GeA5z HwJJBRk8spVPOYgTLJnj7b/9dw/E67xWb1ttT/gh5TEgN4pN/NbHxRH2CgwBH4Nb3hgX eL16gMlurEhHlNXZS1D7gIAmzFtuwbUhhCc435f88vWddt1cERFhRYjfg1kRRSiwvTuM 51KQuykHrANbNcyAIKKdj+VmxGWrPHkq3+MC6fn4mNRZ/IQe2qiF5AMYkF8PXk491u2b zhNgeewxV77aMIxRIsUqxPiQiD/eIZxViI/x7xUBXCz2HVtxPNc5LRcp93H7V+7tO/u+ 4CZQ== Received: by 10.204.143.151 with SMTP id v23mr2637219bku.63.1331204026145; Thu, 08 Mar 2012 02:53:46 -0800 (PST) Received: from localhost ([95.69.173.122]) by mx.google.com with ESMTPS id m3sm2489006bkz.0.2012.03.08.02.53.43 (version=TLSv1/SSLv3 cipher=OTHER); Thu, 08 Mar 2012 02:53:44 -0800 (PST) From: Mikolaj Golub To: Andre Oppermann References: <86ehzwwt6a.fsf@kopusha.home.net> <86r53uhibq.fsf@kopusha.home.net> <4F566A8A.3080607@freebsd.org> X-Comment-To: Andre Oppermann Sender: Mikolaj Golub Date: Thu, 08 Mar 2012 12:53:41 +0200 In-Reply-To: <4F566A8A.3080607@freebsd.org> (Andre Oppermann's message of "Tue, 06 Mar 2012 20:50:34 +0100") Message-ID: <86boo78itm.fsf@kopusha.home.net> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (berkeley-unix) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Kostik Belousov , freebsd-net@freebsd.org, Pawel Jakub Dawidek Subject: Re: soreceive_stream: mbuf leak if called with mp0 and MSG_WAITALL X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 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, 08 Mar 2012 11:15:39 -0000 Hi, On Tue, 06 Mar 2012 20:50:34 +0100 Andre Oppermann wrote: AO> On 05.09.2011 21:58, Mikolaj Golub wrote: >> >> On Sun, 04 Sep 2011 12:30:53 +0300 Mikolaj Golub wrote: >> >> MG> Apparently soreceive_stream() has an issue if it is called to receive data as a >> MG> mbuf chain (by supplying an non zero mbuf **mp0) and with MSG_WAITALL set. >> >> MG> I ran into this issue with smbfs, which uses soreceive() exactly in this way >> MG> (see netsmb/smb_trantcp.c:nbssn_recv()). >> >> Stressing smbfs a little I also observed the following soreceive_stream() >> related panic: AO> Hi Mikolaj, AO> thank you very much for testing, reporting and fixing bugs in soreceive_stream(). AO> I've altered your proposed patches a bit and committed them into my workqueue AO> with the following revisions: AO> http://svn.freebsd.org/changeset/base/232617 AO> http://svn.freebsd.org/changeset/base/232618 AO> Would you mind testing them again before they go into HEAD? With this patch smb mount fails with the error: smb_iod_recvall: tran return NULL without error AO> Index: sys/kern/uipc_socket.c AO> =================================================================== AO> --- sys/kern/uipc_socket.c (revision 232616) AO> +++ sys/kern/uipc_socket.c (revision 232617) AO> @@ -2044,7 +2044,7 @@ deliver: AO> if (mp0 != NULL) { AO> /* Dequeue as many mbufs as possible. */ AO> if (!(flags & MSG_PEEK) && len >= sb->sb_mb->m_len) { AO> - for (*mp0 = m = sb->sb_mb; AO> + for (m = sb->sb_mb; AO> m != NULL && m->m_len <= len; AO> m = m->m_next) { AO> len -= m->m_len; AO> @@ -2052,10 +2052,15 @@ deliver: AO> sbfree(sb, m); AO> n = m; AO> } AO> + n->m_next = NULL; AO> sb->sb_mb = m; AO> + sb->sb_lastrecord = sb->sb_mb; AO> if (sb->sb_mb == NULL) AO> SB_EMPTY_FIXUP(sb); AO> - n->m_next = NULL; AO> + if (*mp0 != NULL) AO> + m_cat(*mp0, m); AO> + else AO> + *mp0 = m; AO> } At that moment m points to the end of the chain. Shouldn't *mp0 be set to sb->sb_mb before the "for" loop? AO> /* Copy the remainder. */ AO> if (len > 0) { AO> @@ -2066,9 +2071,9 @@ deliver: AO> if (m == NULL) AO> len = 0; /* Don't flush data from sockbuf. */ AO> else AO> - uio->uio_resid -= m->m_len; AO> + uio->uio_resid -= len; AO> if (*mp0 != NULL) AO> - n->m_next = m; AO> + m_cat(*mp0, m); AO> else AO> *mp0 = m; AO> if (*mp0 == NULL) { AO> -- Mikolaj Golub