Date: Wed, 02 Feb 2011 02:02:04 +0100 From: Andre Oppermann <andre@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: Lawrence Stewart <lstewart@freebsd.org>, "Bjoern A. Zeeb" <bz@freebsd.org>, net@freebsd.org Subject: Re: Bogus KASSERT() in tcp_output()? Message-ID: <4D48AD0C.1020703@freebsd.org> In-Reply-To: <201102010829.24043.jhb@freebsd.org> References: <201101311217.07073.jhb@freebsd.org> <4D477289.8040901@freebsd.org> <201102010829.24043.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 01.02.2011 14:29, John Baldwin wrote:
> 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
What was the bug reported to security@?
>>> 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':
Yes. ipoptlen should not be added in this comparison as the space for
the ipoptions is not reserved in the header mbuf. The value of ipoptlen
is used for earlier checks to make sure the overall packet length does
not exceed 64K.
>>> /*
>>> * 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?
Correct.
>> There is some relevant prior discussion on src-committers@ for r212803
>> between Andre and Bjoern.
I have a pile of other patches to TCP and other parts laying around. Many
of them fixes for long standing PR's. After EuroBSDCon I got stalled and
lost bit of interest because of a severe reviewer shortage (neither Lawrence,
Qing or Björn responded with reviews to my requests due to severe time shortage
on their part). I then got sidetracked with other work.
> 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?
Essentially non-existent on protocols other than ICMP (for ping). In
most parts of the Internet IP options are filtered. They haven't been
very useful except for record route perhaps. That's of very limited
use though to being able to record only the eight most recent hops.
> Is there an easy way to test a connection with IP options enabled with this
> KASSERT() enabled?
--
Andre
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4D48AD0C.1020703>