From owner-freebsd-net@FreeBSD.ORG Fri Mar 20 20:03:36 2015 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 38A27135 for ; Fri, 20 Mar 2015 20:03:36 +0000 (UTC) Received: from mail-ig0-x22c.google.com (mail-ig0-x22c.google.com [IPv6:2607:f8b0:4001:c05::22c]) (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 EEBA3251 for ; Fri, 20 Mar 2015 20:03:35 +0000 (UTC) Received: by igcqo1 with SMTP id qo1so904945igc.0 for ; Fri, 20 Mar 2015 13:03:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:subject:references :in-reply-to:content-type:content-transfer-encoding; bh=n7DxMTTpVjSINKYOrw+CZJuSlL7N3ErOJ6r/kYH219E=; b=m/AKirbWNcpZktdcnzKLQ+gFyFIcs5QBKIcAusRYtiehCSiXoce6d8WzjSZiPCuhg4 /273XJTr8zcsyhwIKm1knGwo/jib5QPQfkMbOO5Q8rMYfuVzwszJrB1Cz7duimLgIYBF TMFsxPuPCsustxC0UJUTbAH5uUvN8ppdq/jZr/OLGvo/81CRk47MgILxmexgMvTxePkw d8mRM4t2bdttbILYAXqW9VCmDJ52BUPnmAB/rUptQ31vJsYikf3qJyiH5saq5/ea++B7 pBjuwTbYCZMm0wfA8EnHHc/oqR241ic7dQ/CgrSoNA+odMTAYKiFtpMy7YyWUVDpOnFh ZnZg== X-Received: by 10.107.10.157 with SMTP id 29mr126152949iok.79.1426881815446; Fri, 20 Mar 2015 13:03:35 -0700 (PDT) Received: from [10.10.1.5] ([192.252.130.194]) by mx.google.com with ESMTPSA id l64sm3811669iod.19.2015.03.20.13.03.34 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 20 Mar 2015 13:03:34 -0700 (PDT) Message-ID: <550C7D0C.3090603@gmail.com> Date: Fri, 20 Mar 2015 16:03:24 -0400 From: Karim Fodil-Lemelin User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: freebsd-net@freebsd.org Subject: Re: Another fragment question / patch References: <550C3A62.3080403@gmail.com> <550C5F6C.3080302@selasky.org> In-Reply-To: <550C5F6C.3080302@selasky.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.18-1 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 Mar 2015 20:03:36 -0000 On 2015-03-20 1:57 PM, Hans Petter Selasky wrote: > On 03/20/15 16:18, Karim Fodil-Lemelin wrote: >> Hi, >> >> While reading through a previous comment on this list about fragments >> I've noticed that mbuf tags aren't being copied from the leading >> fragment (header) to the subsequent fragment packets. In other words, >> one would expect that all fragments of a packet are carrying the same >> tags that were set on the original packet. I have built a simple test >> were I use ipfw with ALTQ and sent large packet (bigger then MTU) off >> that BSD machine. I have observed that the leading fragment (m0) packet >> is going through the right class although the next fragments are hitting >> the default class for unclassified packets. >> >> Here is a patch that makes things works as expected (all fragments carry >> the ALTQ tag): >> >> diff --git a/freebsd/sys/netinet/ip_output.c >> b/freebsd/sys/netinet/ip_output.c >> index d650949..7d8f041 100644 >> --- a/freebsd/sys/netinet/ip_output.c >> +++ b/freebsd/sys/netinet/ip_output.c >> @@ -1184,7 +1184,10 @@ smart_frag_failure: >> ipstat.ips_odropped++; >> goto done; >> } >> - m->m_flags |= (m0->m_flags & M_MCAST) | M_FRAG; >> + >> + m->m_flags |= (m0->m_flags & M_COPYFLAGS) | M_FRAG; >> + m_tag_copy_chain(m, m0, M_NOWAIT); >> + >> /* >> * In the first mbuf, leave room for the link >> header, then >> * copy the original IP header including options. The >> payload >> diff --git a/freebsd/sys/sys/mbuf.h b/freebsd/sys/sys/mbuf.h >> index 2efff38..6ad8439 100644 >> --- a/freebsd/sys/sys/mbuf.h >> > > Hi, > > I see your point about copying the tags. I'm not sure however that > M_COPYFLAGS is correct, because it also copies M_RDONLY, which is not > relevant for this case. Can you explain what flags need copying in > addition to M_MCAST ? Maybe we need to define these flags separately. > > Thank you! > > --HPS Hi, Arguably the M_RDONLY is added when m_copy() is called a bit later in that function. m_copym() does a shallow copy (through a call to mb_dupcl) and will set the RDONLY flag when doing so. So the fact it was copied over from M_COPYFLAGS shouldn't be a problem in terms of functionality. A similar logic applies to the M_VLANTAG since it should never be set in ip_output() (severe layer violation). I guess M_COPYFLAGS is kinda safe but not necessarily correct. In terms of appropriate behavior (whats the real purpose of M_COPYFLAGS?) my initial patch is debatable and to answer your question I would consider to copy the remaining flags: M_PKTHDR => already in there through the m_gethdr() call M_BCAST => no need to copy M_MCAST => already in there in ip_fragment() M_PROTOFLAGS M_SKIP_FIREWALL => for layer 2 fire-walling? So something like? - m->m_flags |= (m0->m_flags & M_MCAST); + m->m_flags |= (m0->m_flags & (M_MCAST | M_PROTOFLAGS)); + m_tag_copy_chain(m, m0, M_NOWAIT); Cheers! Karim.