From owner-freebsd-net@FreeBSD.ORG Mon Mar 12 21:08:37 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 D9DDF106568E for ; Mon, 12 Mar 2012 21:08:37 +0000 (UTC) (envelope-from andre@freebsd.org) Received: from c00l3r.networx.ch (c00l3r.networx.ch [62.48.2.2]) by mx1.freebsd.org (Postfix) with ESMTP id 3A8B78FC18 for ; Mon, 12 Mar 2012 21:08:37 +0000 (UTC) Received: (qmail 34430 invoked from network); 12 Mar 2012 19:14:37 -0000 Received: from c00l3r.networx.ch (HELO [127.0.0.1]) ([62.48.2.2]) (envelope-sender ) by c00l3r.networx.ch (qmail-ldap-1.03) with SMTP for ; 12 Mar 2012 19:14:37 -0000 Message-ID: <4F5E643D.3010006@freebsd.org> Date: Mon, 12 Mar 2012 22:01:49 +0100 From: Andre Oppermann User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0.2) Gecko/20120216 Thunderbird/10.0.2 MIME-Version: 1.0 To: Mikolaj Golub References: <86ehzwwt6a.fsf@kopusha.home.net> <86r53uhibq.fsf@kopusha.home.net> <4F566A8A.3080607@freebsd.org> <86boo78itm.fsf@kopusha.home.net> In-Reply-To: <86boo78itm.fsf@kopusha.home.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Mon, 12 Mar 2012 21:08:37 -0000 On 08.03.2012 11:53, Mikolaj Golub wrote: > 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? Yes, doesn't compute this way. I've put in your fix in this revision: http://svn.freebsd.org/changeset/base/232867 -- Andre > 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> >