From owner-freebsd-net@FreeBSD.ORG Wed Feb 2 01:28:49 2011 Return-Path: Delivered-To: net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 386E5106573C for ; Wed, 2 Feb 2011 01:28:49 +0000 (UTC) (envelope-from andre@freebsd.org) Received: from c00l3r.networx.ch (c00l3r.networx.ch [62.48.2.2]) by mx1.freebsd.org (Postfix) with ESMTP id 3C4A38FC1A for ; Wed, 2 Feb 2011 01:28:47 +0000 (UTC) Received: (qmail 12802 invoked from network); 2 Feb 2011 00:30:45 -0000 Received: from localhost (HELO [127.0.0.1]) ([127.0.0.1]) (envelope-sender ) by c00l3r.networx.ch (qmail-ldap-1.03) with SMTP for ; 2 Feb 2011 00:30:45 -0000 Message-ID: <4D48AD0C.1020703@freebsd.org> Date: Wed, 02 Feb 2011 02:02:04 +0100 From: Andre Oppermann User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.13) Gecko/20101207 Thunderbird/3.1.7 MIME-Version: 1.0 To: John Baldwin References: <201101311217.07073.jhb@freebsd.org> <4D477289.8040901@freebsd.org> <201102010829.24043.jhb@freebsd.org> In-Reply-To: <201102010829.24043.jhb@freebsd.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit Cc: Lawrence Stewart , "Bjoern A. Zeeb" , net@freebsd.org Subject: Re: Bogus KASSERT() in tcp_output()? X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 Feb 2011 01:28:49 -0000 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