Bogus KASSERT() in tcp_output()?

[Andre Oppermann]

On 01.02.2011 18:37, Bjoern A. Zeeb wrote:
> 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.

IPSEC and TCP is very sub-optimal at the moment. The size of the IPSEC header/
overhead is calculated per packet including a full lookup into the SADB. After
the discussions at EuroBSDCon I attempted to solve that in a better way but
didn't finish. Have to dig it up again.

>> 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.

Byte swap issue?

> # ./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
>