On Fri, Dec 14, 2007 at 10:38:03AM -0500, Sean Boudreau wrote:
> Hi:
> 
> It's pretty easy to tickle the
> IPSEC_ASSERT(remain < MLEN, ("m_makespace: remainder too big: %u", remain));
> in m_makespace().  If not running DIAGNOSTIC an memcpy()
> past a buffer occurs.  The following is more generic and
> handles this case.  Any comments before I commit?

The patch seems ok. Maybe we can be a bit smarter in case where 
hlen > M_TRAILINGSPACE(m) + remain. As we already need to allocate at
least one mbuf for remain, we may try to preserve some space for hlen if
we can't put in m after that. It can save one mbuf allocation in some cases. Not
sure it is really important in fact. 

It would be nice if the patch can be pulled-up in NetBSD-4 (or must we
wait for 4.1 ?).


> 
> Index: ipsec_mbuf.c
> ===================================================================
> RCS file: /cvsroot/src/sys/netipsec/ipsec_mbuf.c,v
> retrieving revision 1.9
> diff -U 10 -r1.9 ipsec_mbuf.c
> --- ipsec_mbuf.c	4 Mar 2007 19:54:48 -0000	1.9
> +++ ipsec_mbuf.c	14 Dec 2007 15:12:49 -0000
> @@ -231,81 +231,82 @@
>  	/*
>  	 * At this point skip is the offset into the mbuf m
>  	 * where the new header should be placed.  Figure out
>  	 * if there's space to insert the new header.  If so,
>  	 * and copying the remainder makese sense then do so.
>  	 * Otherwise insert a new mbuf in the chain, splitting
>  	 * the contents of m as needed.
>  	 */
>  	remain = m->m_len - skip;		/* data to move */
>  	if (hlen > M_TRAILINGSPACE(m)) {
> -		struct mbuf *n;
> +		struct mbuf *n0, *n, **np;
> +		int todo, len, done, alloc;
> +
> +		n0 = NULL;
> +		np = &n0;
> +		alloc = 0;
> +		done = 0;
> +		todo = remain;
> +		while (todo > 0) {
> +			if (todo > MHLEN) {
> +				n = m_getcl(M_DONTWAIT, m->m_type, 0);
> +				len = MCLBYTES;
> +			}
> +			else {
> +				n = m_get(M_DONTWAIT, m->m_type);
> +				len = MHLEN;
> +			}
> +			if (n == NULL) {
> +				m_freem(n0);
> +				return NULL;
> +			}
> +			*np = n;
> +			np = &n->m_next;
> +			alloc++;
> +			len = min(todo, len);
> +			memcpy(n->m_data, mtod(m, char *) + skip + done, len);
> +			n->m_len = len;
> +			done += len;
> +			todo -= len;
> +		}
>  
> -		/* XXX code doesn't handle clusters XXX */
> -		IPSEC_ASSERT(remain < MLEN,
> -			("m_makespace: remainder too big: %u", remain));
> -		/*
> -		 * Not enough space in m, split the contents
> -		 * of m, inserting new mbufs as required.
> -		 *
> -		 * NB: this ignores mbuf types.
> -		 */
> -		MGET(n, M_DONTWAIT, MT_DATA);
> -		if (n == NULL)
> -			return (NULL);
> -		n->m_next = m->m_next;		/* splice new mbuf */
> -		m->m_next = n;
> -		newipsecstat.ips_mbinserted++;
>  		if (hlen <= M_TRAILINGSPACE(m) + remain) {
> -			/*
> -			 * New header fits in the old mbuf if we copy
> -			 * the remainder; just do the copy to the new
> -			 * mbuf and we're good to go.
> -			 */
> -			memcpy(mtod(n, char *),
> -			       mtod(m, char *) + skip, remain);
> -			n->m_len = remain;
>  			m->m_len = skip + hlen;
>  			*off = skip;
> -		} else {
> -			/*
> -			 * No space in the old mbuf for the new header.
> -			 * Make space in the new mbuf and check the
> -			 * remainder'd data fits too.  If not then we
> -			 * must allocate an additional mbuf (yech).
> -			 */
> -			n->m_len = 0;
> -			if (remain + hlen > M_TRAILINGSPACE(n)) {
> -				struct mbuf *n2;
> -
> -				MGET(n2, M_DONTWAIT, MT_DATA);
> -				/* NB: new mbuf is on chain, let caller free */
> -				if (n2 == NULL)
> -					return (NULL);
> -				n2->m_len = 0;
> -				memcpy(mtod(n2, char *),
> -				       mtod(m, char *) + skip, remain);
> -				n2->m_len = remain;
> -				/* splice in second mbuf */
> -				n2->m_next = n->m_next;
> -				n->m_next = n2;
> -				newipsecstat.ips_mbinserted++;
> -			} else {
> -				memcpy(mtod(n, char *) + hlen,
> -				       mtod(m, char *) + skip, remain);
> -				n->m_len += remain;
> +			if (n0 != NULL) {
> +				*np = m->m_next;
> +				m->m_next = n0;
>  			}
> -			m->m_len -= remain;
> -			n->m_len += hlen;
> +		}
> +		else {
> +			n = m_get(M_DONTWAIT, m->m_type);
> +			if (n == NULL) {
> +				m_freem(n0);
> +				return NULL;
> +			}
> +			alloc++;
> +
> +			if ((n->m_next = n0) == NULL)
> +				np = &n->m_next;
> +			n0 = n;
> +
> +			*np = m->m_next;
> +			m->m_next = n0;
> +
> +			n->m_len = hlen;
> +			m->m_len = skip;
> +
>  			m = n;			/* header is at front ... */
>  			*off = 0;		/* ... of new mbuf */
>  		}
> +
> +		newipsecstat.ips_mbinserted += alloc;
>  	} else {
>  		/*
>  		 * Copy the remainder to the back of the mbuf
>  		 * so there's space to write the new header.
>  		 */
>  		/* XXX can this be memcpy? does it handle overlap? */
>  		ovbcopy(mtod(m, char *) + skip,
>  			mtod(m, char *) + skip + hlen, remain);
>  		m->m_len += hlen;
>  		*off = skip;
> 

-- 
Arnaud Degroote
degroote@netbsd.org