Date: Tue, 1 Feb 2011 17:37:58 +0000 (UTC) From: "Bjoern A. Zeeb" <bz@FreeBSD.org> To: John Baldwin <jhb@freebsd.org> Cc: Lawrence Stewart <lstewart@freebsd.org>, Andre Oppermann <andre@freebsd.org>, net@freebsd.org Subject: Re: Bogus KASSERT() in tcp_output()? Message-ID: <20110201171959.W43179@maildrop.int.zabbadoz.net> 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 Tue, 1 Feb 2011, 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
>>> 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.
It is.
> 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?
Right and probably reason why I also hit it with IPSec as result of
718 #ifdef IPSEC
719 ipoptlen += ipsec_optlen;
720 #endif
which wasn't because of ipsec_optlen really, I had just stopping
looking too soon back last year.
> Is there an easy way to test a connection with IP options enabled with this
> KASSERT() enabled?
Yes, see patch at [1], and using my modified KASSERT still I get...
which btw sounds wrong to me as well btw as I wouldn't expect ipoptlen
to be 4 here given the test case.
# ./tcpconnect client 127.0.0.1 12345 1 ipopt
panic: tcp_output: mbuf chain shorter than expected: 0 + 60 + 4 - 0 != 60
cpuid = 2
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2a
kdb_backtrace() at kdb_backtrace+0x37
panic() at panic+0x187
tcp_output() at tcp_output+0x1d01
tcp_usr_connect() at tcp_usr_connect+0x15f
soconnect() at soconnect+0x14f
kern_connect() at kern_connect+0x12e
connect() at connect+0x41
syscallenter() at syscallenter+0x1cb
syscall() at syscall+0x4c
Xfast_syscall() at Xfast_syscall+0xe2
--- syscall (98, FreeBSD ELF64, connect), rip = 0x80072934c, rsp = 0x7fffffffe9d8, rbp = 0x3 ---
/bz
References:
[1] http://people.freebsd.org/~bz/20110201-01-tcpconnect-ipopt.diff
--
Bjoern A. Zeeb You have to have visions!
<ks> Going to jail sucks -- <bz> All my daemons like it!
http://www.freebsd.org/doc/en_US.ISO8859-1/books/handbook/jails.html
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110201171959.W43179>