Subject: Re: kern/36851: large packets will crash FAST_IPSEC-kernel
To: None <kern-bug-people@netbsd.org, gnats-admin@netbsd.org,>
From: Wolfgang Stukenbrock <Wolfgang.Stukenbrock@nagler-company.com>
List: netbsd-bugs
Date: 08/27/2007 15:45:02
The following reply was made to PR kern/36851; it has been noted by GNATS.
From: Wolfgang Stukenbrock <Wolfgang.Stukenbrock@nagler-company.com>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/36851: large packets will crash FAST_IPSEC-kernel
Date: Mon, 27 Aug 2007 17:41:47 +0200
This is a multi-part message in MIME format.
--------------040809010207080805080801
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit
Hi, again
some seconds after I've send the patch I've detected a problem in it.
The case remain == 0 is not handled correctly - sorry, bad testing from
me ....
Her is the new version of the patch - and this time I've tested it with
small packets too ...
W. Stukenbrock
*** ipsec_mbuf.c 2007/08/16 16:38:49 1.2
--- ipsec_mbuf.c 2007/08/27 15:14:38
***************
*** 240,248 ****
if (hlen > M_TRAILINGSPACE(m)) {
struct mbuf *n;
- /* 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.
--- 240,245 ----
***************
*** 255,315 ****
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, caddr_t),
! mtod(m, caddr_t) + 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, caddr_t),
! mtod(m, caddr_t) + 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, caddr_t) + hlen,
! mtod(m, caddr_t) + skip, remain);
! n->m_len += remain;
! }
! m->m_len -= remain;
! n->m_len += hlen;
! m = n; /* header is at front ... */
! *off = 0; /* ... of new mbuf */
! }
! } 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, caddr_t) + skip,
! mtod(m, caddr_t) + skip + hlen, remain);
! m->m_len += hlen;
! *off = skip;
! }
m0->m_pkthdr.len += hlen; /* adjust packet length */
return m;
}
--- 252,305 ----
n->m_next = m->m_next; /* splice new mbuf */
m->m_next = n;
newipsecstat.ips_mbinserted++;
! /* copy somespace of mbuf into new one ... */
! if (remain < MLEN)
! {
! if (hlen > M_TRAILINGSPACE(m) + remain)
! { /* will not fit into first buffer - even after
copy ... */
! if (remain + hlen < MLEN)
! { /* both will fit into new buffer */
! if (remain > 0)
! { /* something to copy present ... */
! memcpy(mtod(n, caddr_t) + hlen, mtod(m,
caddr_t) + m->m_len - remain, remain);
! m->m_len -= remain;
! }
! n->m_len = hlen + remain;
! }
! else
! { /* need a third buffer ... - move remain
into this one and use additonal buffer for header */
! memcpy(mtod(n, caddr_t), mtod(m, caddr_t) +
m->m_len - remain, remain);
! m->m_len -= remain;
! n->m_len = remain;
! MGET(n, M_DONTWAIT, MT_DATA);
! if (n == NULL)
! return (NULL);
! newipsecstat.ips_mbinserted++;
! n->m_next = m->m_next; /* splice
new mbuf */
! m->m_next = n;
! n->m_len = hlen;
! }
! *off = 0;
! m0->m_pkthdr.len += hlen;
! return n;
! }
! n->m_len = remain;
! }
! else
! n->m_len = MLEN;
! memcpy(mtod(n, caddr_t), mtod(m, caddr_t) + m->m_len -
n->m_len, n->m_len);
! remain -= n->m_len;
! m->m_len -= n->m_len;
! }
! /*
! * 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? */
! if (remain > 0)
! ovbcopy(mtod(m, caddr_t) + skip, mtod(m, caddr_t) + skip +
hlen, remain);
! m->m_len += hlen;
! *off = skip;
m0->m_pkthdr.len += hlen; /* adjust packet length */
return m;
}
gnats-admin@NetBSD.org wrote:
> Thank you very much for your problem report.
> It has the internal identification `kern/36851'.
> The individual assigned to look at your
> report is: kern-bug-people.
>
>
>>Category: kern
>>Responsible: kern-bug-people
>>Synopsis: large packets will crash FAST_IPSEC-kernel
>>Arrival-Date: Mon Aug 27 14:25:01 +0000 2007
>>
>
--------------040809010207080805080801
Content-Type: text/plain;
name="ddddd"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="ddddd"
243,245d242
< /* XXX code doesn't handle clusters XXX */
< IPSEC_ASSERT(remain < MLEN,
< ("m_makespace: remainder too big: %u", remain));
258,312c255,302
< 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, caddr_t),
< mtod(m, caddr_t) + 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, caddr_t),
< mtod(m, caddr_t) + 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, caddr_t) + hlen,
< mtod(m, caddr_t) + skip, remain);
< n->m_len += remain;
< }
< m->m_len -= remain;
< n->m_len += hlen;
< m = n; /* header is at front ... */
< *off = 0; /* ... of new mbuf */
< }
< } 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, caddr_t) + skip,
< mtod(m, caddr_t) + skip + hlen, remain);
< m->m_len += hlen;
< *off = skip;
< }
---
> /* copy somespace of mbuf into new one ... */
> if (remain < MLEN)
> {
> if (hlen > M_TRAILINGSPACE(m) + remain)
> { /* will not fit into first buffer - even after copy ... */
> if (remain + hlen < MLEN)
> { /* both will fit into new buffer */
> if (remain > 0)
> { /* something to copy present ... */
> memcpy(mtod(n, caddr_t) + hlen, mtod(m, caddr_t) + m->m_len - remain, remain);
> m->m_len -= remain;
> }
> n->m_len = hlen + remain;
> }
> else
> { /* need a third buffer ... - move remain into this one and use additonal buffer for header */
> memcpy(mtod(n, caddr_t), mtod(m, caddr_t) + m->m_len - remain, remain);
> m->m_len -= remain;
> n->m_len = remain;
> MGET(n, M_DONTWAIT, MT_DATA);
> if (n == NULL)
> return (NULL);
> newipsecstat.ips_mbinserted++;
> n->m_next = m->m_next; /* splice new mbuf */
> m->m_next = n;
> n->m_len = hlen;
> }
> *off = 0;
> m0->m_pkthdr.len += hlen;
> return n;
> }
> n->m_len = remain;
> }
> else
> n->m_len = MLEN;
> memcpy(mtod(n, caddr_t), mtod(m, caddr_t) + m->m_len - n->m_len, n->m_len);
> remain -= n->m_len;
> m->m_len -= n->m_len;
> }
> /*
> * 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? */
> if (remain > 0)
> ovbcopy(mtod(m, caddr_t) + skip, mtod(m, caddr_t) + skip + hlen, remain);
> m->m_len += hlen;
> *off = skip;
--------------040809010207080805080801--