Date: Tue, 1 Feb 2011 08:29:23 -0500 From: John Baldwin <jhb@freebsd.org> To: Lawrence Stewart <lstewart@freebsd.org> Cc: "Bjoern A. Zeeb" <bz@freebsd.org>, Andre Oppermann <andre@freebsd.org>, net@freebsd.org Subject: Re: Bogus KASSERT() in tcp_output()? Message-ID: <201102010829.24043.jhb@freebsd.org> In-Reply-To: <4D477289.8040901@freebsd.org> References: <201101311217.07073.jhb@freebsd.org> <4D477289.8040901@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Monday, January 31, 2011 9:40:09 pm Lawrence Stewart wrote:
> On 02/01/11 04:17, John Baldwin wrote:
> > Somewhat related fallout to the bug reported on security@ recently, I think
> > this KASSERT() in tcp_output() is bogus:
> >
> >
> > KASSERT(len + hdrlen + ipoptlen == m_length(m, NULL),
> > ("%s: mbuf chain shorter than expected", __func__));
> >
> > Specifically, just a few lines earlier in tcp_output() we set the packet
> > header length to just 'len + hdrlen':
> >
> > /*
> > * Put TCP length in extended header, and then
> > * checksum extended header and data.
> > */
> > m->m_pkthdr.len = hdrlen + len; /* in6_cksum() need this */
> >
> > Also, the ipoptions are stored in a separate mbuf chain in the in pcb
> > (inp_options) that is passed as a separate argument to ip_output(). Given
> > that, I would think that m_length() should not reflect ipoptlen since it
> > should not include IP options in that chain?
> >
>
> There is some relevant prior discussion on src-committers@ for r212803
> between Andre and Bjoern.
I still don't see where ipoptlen bytes are reserved in the mbuf chain. After
this block where 'm' is allocated and initialized:
/*
* Grab a header mbuf, attaching a copy of data to
* be transmitted, and initialize the header from
* the template for sends on this connection.
*/
if (len) {
...
m->m_len = hdrlen;
...
if (len <= MHLEN - hdrlen - max_linkhdr) {
...
m->m_len += len;
} else {
m->m_next = m_copy(mb, moff, (int)len);
...
}
...
} else {
...
m->m_len = hdrlen;
}
The length of the mbuf chain headed by 'm' is clearly hdrlen + len.
At no point anywhere do we do any sort of m_prepend() or other operation to
allocate space in the mbuf chain for the IP options. They are merged in in
ip_output(). I think the only reason this KASSERT() isn't firing in HEAD is
that IP options are rarely used?
Is there an easy way to test a connection with IP options enabled with this
KASSERT() enabled?
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201102010829.24043.jhb>