soreceive_stream: mbuf leak if called with mp0 and MSG_WAITALL

[Andre Oppermann]

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>
>