Re: CVS commit: src/sys/kern

[maya%netbsd.org@localhost]

hey, thanks for all the fixes!

I'm trying to import a driver (while being very new to
networking/driver stuff), and updating to -current fixed an issue I've
had, most likely from this commit.

Now my ported driver is close to working :-)

On Sat, Apr 28, 2018 at 08:16:15AM +0000, Maxime Villard wrote:
> Module Name:	src
> Committed By:	maxv
> Date:		Sat Apr 28 08:16:15 UTC 2018
> 
> Modified Files:
> 	src/sys/kern: uipc_mbuf.c
> 
> Log Message:
> Modify m_defrag, so that it never frees the first mbuf of the chain. While
> here use the given 'flags' argument, and not M_DONTWAIT.
> 
> We have a problem with several drivers: they poll an mbuf chain from their
> queues and call m_defrag on them, but m_defrag could update the mbuf
> pointer, so the mbuf in the queue is no longer valid. It is not easy to
> fix each driver, because doing pop+push will reorder the queue, and we
> don't really want that to happen.
> 
> This problem was independently spotted by me, Kengo, Masanobu, and other
> people too it seems (perhaps PR/53218).
> 
> Now m_defrag leaves the first mbuf in place, and compresses the chain
> only starting from the second mbuf in the chain.
> 
> It is important not to compress the first mbuf with hacks, because the
> storage of this first mbuf may be shared with other mbufs.
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.210 -r1.211 src/sys/kern/uipc_mbuf.c
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
> 

> Modified files:
> 
> Index: src/sys/kern/uipc_mbuf.c
> diff -u src/sys/kern/uipc_mbuf.c:1.210 src/sys/kern/uipc_mbuf.c:1.211
> --- src/sys/kern/uipc_mbuf.c:1.210	Fri Apr 27 19:06:48 2018
> +++ src/sys/kern/uipc_mbuf.c	Sat Apr 28 08:16:15 2018
> @@ -1,4 +1,4 @@
> -/*	$NetBSD: uipc_mbuf.c,v 1.210 2018/04/27 19:06:48 maxv Exp $	*/
> +/*	$NetBSD: uipc_mbuf.c,v 1.211 2018/04/28 08:16:15 maxv Exp $	*/
>  
>  /*
>   * Copyright (c) 1999, 2001 The NetBSD Foundation, Inc.
> @@ -62,7 +62,7 @@
>   */
>  
>  #include <sys/cdefs.h>
> -__KERNEL_RCSID(0, "$NetBSD: uipc_mbuf.c,v 1.210 2018/04/27 19:06:48 maxv Exp $");
> +__KERNEL_RCSID(0, "$NetBSD: uipc_mbuf.c,v 1.211 2018/04/28 08:16:15 maxv Exp $");
>  
>  #ifdef _KERNEL_OPT
>  #include "opt_mbuftrace.h"
> @@ -1475,27 +1475,32 @@ enobufs:
>  }
>  
>  /*
> - * Copy the mbuf chain to a new mbuf chain that is as short as possible.
> - * Return the new mbuf chain on success, NULL on failure.  On success,
> - * free the old mbuf chain.
> + * Compress the mbuf chain. Return the new mbuf chain on success, NULL on
> + * failure. The first mbuf is preserved, and on success the pointer returned
> + * is the same as the one passed.
>   */
>  struct mbuf *
>  m_defrag(struct mbuf *mold, int flags)
>  {
>  	struct mbuf *m0, *mn, *n;
> -	size_t sz = mold->m_pkthdr.len;
> +	int sz;
>  
>  	KASSERT((mold->m_flags & M_PKTHDR) != 0);
>  
> -	m0 = m_gethdr(flags, MT_DATA);
> +	if (mold->m_next == NULL)
> +		return mold;
> +
> +	m0 = m_get(flags, MT_DATA);
>  	if (m0 == NULL)
>  		return NULL;
> -	M_COPY_PKTHDR(m0, mold);
>  	mn = m0;
>  
> +	sz = mold->m_pkthdr.len - mold->m_len;
> +	KASSERT(sz >= 0);
> +
>  	do {
> -		if (sz > MHLEN) {
> -			MCLGET(mn, M_DONTWAIT);
> +		if (sz > MLEN) {
> +			MCLGET(mn, flags);
>  			if ((mn->m_flags & M_EXT) == 0) {
>  				m_freem(m0);
>  				return NULL;
> @@ -1511,7 +1516,7 @@ m_defrag(struct mbuf *mold, int flags)
>  
>  		if (sz > 0) {
>  			/* need more mbufs */
> -			n = m_get(M_NOWAIT, MT_DATA);
> +			n = m_get(flags, MT_DATA);
>  			if (n == NULL) {
>  				m_freem(m0);
>  				return NULL;
> @@ -1522,9 +1527,10 @@ m_defrag(struct mbuf *mold, int flags)
>  		}
>  	} while (sz > 0);
>  
> -	m_freem(mold);
> +	m_freem(mold->m_next);
> +	mold->m_next = m0;
>  
> -	return m0;
> +	return mold;
>  }
>  
>  void
>