From owner-freebsd-net@freebsd.org Mon Oct 16 14:37:13 2017 Return-Path: Delivered-To: freebsd-net@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 25A13E3C083 for ; Mon, 16 Oct 2017 14:37:13 +0000 (UTC) (envelope-from kfodil-lemelin@xiplink.com) Received: from smtp97.iad3a.emailsrvr.com (smtp97.iad3a.emailsrvr.com [173.203.187.97]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id E6BA482D6D for ; Mon, 16 Oct 2017 14:37:12 +0000 (UTC) (envelope-from kfodil-lemelin@xiplink.com) Received: from smtp13.relay.iad3a.emailsrvr.com (localhost [127.0.0.1]) by smtp13.relay.iad3a.emailsrvr.com (SMTP Server) with ESMTP id 7CF4D5676; Mon, 16 Oct 2017 10:37:03 -0400 (EDT) X-Auth-ID: kfodil-lemelin@xiplink.com Received: by smtp13.relay.iad3a.emailsrvr.com (Authenticated sender: kfodil-lemelin-AT-xiplink.com) with ESMTPSA id 27C8657D2; Mon, 16 Oct 2017 10:37:03 -0400 (EDT) X-Sender-Id: kfodil-lemelin@xiplink.com Received: from [10.10.1.188] ([UNAVAILABLE]. [192.252.130.194]) (using TLSv1.2 with cipher DHE-RSA-AES128-SHA) by 0.0.0.0:465 (trex/5.7.12); Mon, 16 Oct 2017 10:37:03 -0400 Subject: Re: m_move_pkthdr leaves m_nextpkt 'dangling' To: Gleb Smirnoff References: <59567148.1020902@xiplink.com> <31535133-f95a-5db6-a04c-acc0175fa287@yandex.ru> <59DFD3CC.2000401@xiplink.com> <20171013211026.GB1055@FreeBSD.org> Cc: Adrian Chadd , FreeBSD Net , "Andrey V. Elsukov" From: Karim Fodil-Lemelin Message-ID: <59E4C40E.9060103@xiplink.com> Date: Mon, 16 Oct 2017 10:37:02 -0400 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: <20171013211026.GB1055@FreeBSD.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Oct 2017 14:37:13 -0000 On 2017-10-13 5:10 PM, Gleb Smirnoff wrote: > On Fri, Oct 13, 2017 at 12:59:47AM -0700, Adrian Chadd wrote: > A> >>>> When doing so m_move_pkthdr is called to copy the current PKTHDR fields > A> >>>> (tags and flags) to the mbuf that was prepended. The function also does: > A> >>>> > A> >>>> to->m_pkthdr = from->m_pkthdr; > A> >>>> > A> >>>> This, for the case I am interested in, essentially leaves the 'from' > A> >>>> mbuf > A> >>>> with a dangling pointer m_nextpkt pointing to the next fragment. While > A> >>>> this > A> >>>> is mostly harmless because only mbufs of pkthdr types are supposed to > A> >>>> have > A> >>>> m_nextpkt it triggers some panics when running with INVARIANTS in > A> >>>> NetGraph > A> >>>> (see ng_base.c :: CHECK_DATA_MBUF(m)): > A> >>>> > A> >>>> ... > A> >>>> if (n->m_nextpkt != NULL) > A> >>>> \ > A> >>>> panic("%s: m_nextpkt", __func__); > A> >>>> \ > A> >>>> } > A> >>>> ... > A> >>>> > A> >>>> So I would like to propose the following patch: > A> >>>> > A> >>>> @@ -442,10 +442,11 @@ m_move_pkthdr(struct mbuf *to, struct mbuf *from) > A> >>>> if ((to->m_flags & M_EXT) == 0) > A> >>>> to->m_data = to->m_pktdat; > A> >>>> to->m_pkthdr = from->m_pkthdr; /* especially tags */ > A> >>>> SLIST_INIT(&from->m_pkthdr.tags); /* purge tags from src > A> >>>> */ > A> >>>> from->m_flags &= ~M_PKTHDR; > A> >>>> + from->m_nextpkt = NULL; > A> >>>> } > > Not only mbufs of M_PKTHDR may have m_nextpkt set. However, I tend to agree > with the patch. But shouldn't we first copy the m_nextpkt to the new mbuf: > > + to->m_nextpkt = from->m_nextpkt; > + from->m_nextpkt = NULL; > > Same way as we deal with tags. > > Hi Gleb, I think you are correct. If we look at the 'spirit' of m_move_pkthdr(); In my mind, it is to deep copy all fields related to a packet header and since m_nextpkt should only be carried by packet headers, it makes sense to copy it within m_move_pkthdr(). This also raises the question (my apologies in advance from bringing this up...) of weather or not m_nextpkt belongs in struct m_hdr and not in struct pkthdr. In our case we are copying it explicitly outside the function as most of users of m_move_pkthdr() do. Thanks for looking in to this. Karim.