From owner-freebsd-pf@freebsd.org Sun Nov 15 17:33:54 2015 Return-Path: Delivered-To: freebsd-pf@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 894EEA30D85 for ; Sun, 15 Nov 2015 17:33:54 +0000 (UTC) (envelope-from kp@vega.codepro.be) Received: from venus.codepro.be (venus.codepro.be [IPv6:2a01:4f8:162:1127::2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.codepro.be", Issuer "Gandi Standard SSL CA 2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 1AB881220 for ; Sun, 15 Nov 2015 17:33:53 +0000 (UTC) (envelope-from kp@vega.codepro.be) Received: from vega.codepro.be (unknown [172.16.1.3]) by venus.codepro.be (Postfix) with ESMTP id 162FE20793; Sun, 15 Nov 2015 18:33:50 +0100 (CET) Received: by vega.codepro.be (Postfix, from userid 1001) id 116B24E759; Sun, 15 Nov 2015 18:33:50 +0100 (CET) Date: Sun, 15 Nov 2015 18:33:50 +0100 From: Kristof Provost To: =?utf-8?Q?Mi=C5=82osz?= Kaniewski Cc: freebsd-pf@freebsd.org Subject: Re: Creating span interface using 'dup-to' option Message-ID: <20151115173349.GE13268@vega.codepro.be> References: <20151108000315.GC2336@vega.codepro.be> <20151108192951.GD2336@vega.codepro.be> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Checked-By-NSA: Probably User-Agent: Mutt/1.5.24 (2015-08-30) X-BeenThere: freebsd-pf@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "Technical discussion and general questions about packet filter \(pf\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 15 Nov 2015 17:33:54 -0000 On 2015-11-15 13:04:27 (+0100), Miłosz Kaniewski wrote: > > I suppose we could mark packets in pf_route() as M_SKIP_FIREWALL, but > > that might have other consequences. > > > > > I looked into old pf code to look for some tips. First commit worth > mentioning is 1.215 > (I took commit numbers from OpenBSD cvs). In that commit mbuf tag > PACKET_TAG_PF_ROUTED was introduced to cope with problem when > packet were routed more than once. If packet already had this tag set then > it wasn't routed any more. I suppose that this approach was good and also > prevented > packets to be duplicated more than one time. > Later on there were couple of changes in this code (1.218, 1.318, 1.385, > 1.440), but > they didn't change general approach and packets still could be routed (and > duplicated) > only once. But things changed in commit 1.447 which introduced counter > which count the > number of times that packet has already been routed. And maximum number of > route > operations on one packet was set to 4 . I think that since this change our > problem with > dup-to began to exists. And this code is still present in actual FreeBSD pf > version: > https://svnweb.freebsd.org/base/head/sys/netpfil/pf/pf.c?view=markup#l5316 > I've done a quick test with OpenBSD 5.8 and their dup-to seems to have the same issue. > The solution that at this point comes to my mind is to forbid more than one > duplication of > the same packet. We could for example set flag on every packet that pass > through > pf_route() and has pf_rule->rt == PF_DUPTO. If the same packet would be > again > processed by pf_route() then we would see that this flag is set and just > not route it once > again. Example patch is bellow: > > --- pf.c_orig 2015-11-04 08:58:00.262242000 +0100 > +++ pf.c_new 2015-11-04 08:58:13.146939000 +0100 > @@ -5310,6 +5310,12 @@ > } > > if (r->rt == PF_DUPTO) { > + if (pd->pf_mtag->flags & PF_PACKET_DUPLICATED) { > + if (s) > + PF_STATE_UNLOCK(s); > + return; > + } > + pd->pf_mtag->flags |= PF_PACKET_DUPLICATED; > if ((m0 = m_dup(*m, M_NOWAIT)) == NULL) { > if (s) > PF_STATE_UNLOCK(s); > > --- pf_mtag.h_orig 2015-11-04 08:58:27.827355000 +0100 > +++ pf_mtag.h_new 2015-11-04 08:58:39.043731000 +0100 > @@ -40,6 +40,7 @@ > #define PF_PACKET_LOOPED 0x08 > #define PF_FASTFWD_OURS_PRESENT 0x10 > #define PF_REASSEMBLED 0x20 > +#define PF_PACKET_DUPLICATED 0x40 > > struct pf_mtag { > void *hdr; /* saved hdr pos in mbuf, for ECN */ > > > That looks reasonable. > Unfortunately there are weaknesses of that solution. Theoretically I can > imagine that > someone would use nested dup-to rules in which the same packet is many time > duplicated to many interfaces, eg: > pass out on em0 dup-to (em1 10.0.0.1) > pass out on em1 dup-to (em2 10.0.2.1) > pass out on em2 dup-to (em3 10.0.3.1) > And if we would forbid more than one duplication, above ruleset would stop > to work > (packet would be only duplicated to em1, but not to em2 and em3). > Yeah, I can see what you mean. On the other hand, it's a fairly contrived scenario. > But I can't see any simple way how to make that ruleset works and in the > same time > repair our problem. > I suppose we could save the rule ID or interface or something in the pf_mtag and make a decision based on that (i.e. check if we're hitting the same rule or not). On the other hand, perhaps there's something we can do about the state matching. The problems all start because we match state on the duplicated packet. That's not correct, because the rule is set on e.g. em0, but the duplicated packet is sent out on em1. In fact, from a first reading of the code I don't actually understand why we're getting that state match. Regards, Kristof