From owner-freebsd-net@freebsd.org Wed Jul 5 16:23:12 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 52692D87D70 for ; Wed, 5 Jul 2017 16:23:12 +0000 (UTC) (envelope-from adrian.chadd@gmail.com) Received: from mail-wm0-x22f.google.com (mail-wm0-x22f.google.com [IPv6:2a00:1450:400c:c09::22f]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id DD991269D for ; Wed, 5 Jul 2017 16:23:11 +0000 (UTC) (envelope-from adrian.chadd@gmail.com) Received: by mail-wm0-x22f.google.com with SMTP id f67so118440949wmh.1 for ; Wed, 05 Jul 2017 09:23:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=mFJVgkVrt5JJfTq3MtdxfkDfw0FvK8R/hJbdpOiYcjo=; b=n3rxHUAF339Jdtrl4+Ulos1vodRbNqE8gi/8DCfozcgFcZ8+U3pNTidfJVgG6mBeN+ /VxrWhGkLJ9FIiGXH5Bwqbu0/8d0xjVMNDXloF/MgelU2Ra3GW2OTD5VcZhm9r783N6C GkrZBTLyuWgZTR260DQzprZiwc/E1ka8un6+iEheuIXZ1UPtcV1DEPtZjqfSfBEhDA7i tyfeH+fh4vp2B9SwxhrtCBC9VPROwDDjd3Zifect2WcdJClUsJH+f9GCUyEYh5ZTeVna xPVQmKiVpy7IBOK2t+BSgLwNVQ/ie4Zf8AV+pLxYnneJZ/wn0578m9scxG3+tedFA4to VV/A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=mFJVgkVrt5JJfTq3MtdxfkDfw0FvK8R/hJbdpOiYcjo=; b=C5NoHFNCLAmMSjd6rlwlzbK9EBowo5ezh3tIBQqYvnq6Q9cy+VPS42kphC5cLNejTe JeuWRjZwvogD3b8leyiNZX+0NJwF50cAtJH7m0rKso4CDWw8FCEuz4I5lddED+2Z3kmq 6M25yLGGMz6zSCo/JSFtWcBeOaoE4wpRjK6bJz06pD/xSv8yfytC34l+VIfwVPMllSMU bNy+e0TXyQz0CisRgyT/wG0LoGckOwlWJEW6pcJxNYil8L2b/K1vwUP2Z+NrZHqtuGG4 ajgazbQmsoOThgoJAKZtHnQa9c/vbHJ1egOEfUHts6HSr3cfVz/BENoyQlh90hl7aHlN igUw== X-Gm-Message-State: AIVw1117Pw+6wmeaPtrWBmqKyvHwtPRaFWsYwRJLMTlsapostVmAtd0X Y0WOobu5wUUDQh6EYarfmkE9K3LSwy+s X-Received: by 10.28.198.1 with SMTP id w1mr14357204wmf.78.1499271790397; Wed, 05 Jul 2017 09:23:10 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.141.82 with HTTP; Wed, 5 Jul 2017 09:23:09 -0700 (PDT) In-Reply-To: <59567148.1020902@xiplink.com> References: <59567148.1020902@xiplink.com> From: Adrian Chadd Date: Wed, 5 Jul 2017 09:23:09 -0700 Message-ID: Subject: Re: m_move_pkthdr leaves m_nextpkt 'dangling' To: Karim Fodil-Lemelin Cc: FreeBSD Net Content-Type: text/plain; charset="UTF-8" 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: Wed, 05 Jul 2017 16:23:12 -0000 On 30 June 2017 at 08:42, Karim Fodil-Lemelin wrote: > Hi, > > As many of you know, when dealing with IP fragments the kernel will build a > list of packets (fragments) chained together through the m_nextpkt pointer. > This is all good until someone tries to do a M_PREPEND on one of the packet > in the chain and the M_PREPEND has to create an extra mbuf to prepend at the > beginning of the chain. > > When doing so m_move_pkthdr is called to copy the current PKTHDR fields > (tags and flags) to the mbuf that was prepended. The function also does: > > to->m_pkthdr = from->m_pkthdr; > > This, for the case I am interested in, essentially leaves the 'from' mbuf > with a dangling pointer m_nextpkt pointing to the next fragment. While this > is mostly harmless because only mbufs of pkthdr types are supposed to have > m_nextpkt it triggers some panics when running with INVARIANTS in NetGraph > (see ng_base.c :: CHECK_DATA_MBUF(m)): > > ... > if (n->m_nextpkt != NULL) \ > panic("%s: m_nextpkt", __func__); \ > } > ... > > So I would like to propose the following patch: > > @@ -442,10 +442,11 @@ m_move_pkthdr(struct mbuf *to, struct mbuf *from) > if ((to->m_flags & M_EXT) == 0) > to->m_data = to->m_pktdat; > to->m_pkthdr = from->m_pkthdr; /* especially tags */ > SLIST_INIT(&from->m_pkthdr.tags); /* purge tags from src */ > from->m_flags &= ~M_PKTHDR; > + from->m_nextpkt = NULL; > } > > It will reset the m_nextpkt so we don't have two mbufs pointing to the same > next packet. This is fairly harmless and solves a problem for us here at > XipLink. This seems like a no-brainer. :-) Any objections? -adrian