Date: Sun, 15 Nov 2015 13:04:27 +0100 From: =?UTF-8?Q?Mi=C5=82osz_Kaniewski?= <milosz.kaniewski@gmail.com> Cc: freebsd-pf@freebsd.org Subject: Re: Creating span interface using 'dup-to' option Message-ID: <CAC4mxp6wvMe9EWqXYzNG=FEA2HO-kNqmdLrUjs8nHJUODTucUw@mail.gmail.com> In-Reply-To: <CAC4mxp7B5tYErUX%2Bh0803eQhRY2XzXCFpLP7=2ESJPQtVupczA@mail.gmail.com> References: <CAC4mxp5ar-Kvp5238VRfKEL6FiVOg7XXzmv8fE-zdEFYRk7cAw@mail.gmail.com> <SN1PR08MB18210835207E194932EBB485BA310@SN1PR08MB1821.namprd08.prod.outlook.com> <CAC4mxp77FrDvT%2B1J%2BdQqrgc_ji3vmbMZOkYnXae%2BD2L1PanK1g@mail.gmail.com> <20151108000315.GC2336@vega.codepro.be> <20151108192951.GD2336@vega.codepro.be> <CAC4mxp7B5tYErUX%2Bh0803eQhRY2XzXCFpLP7=2ESJPQtVupczA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
2015-11-08 20:29 GMT+01:00 Kristof Provost <kp@freebsd.org>: > On 2015-11-08 01:03:15 (+0100), Kristof Provost <kp@FreeBSD.org> wrote: > > It certainly looks wrong. I can also reproduce your observation that > > this doesn't happen when 'no state' is added to the rule. > > > I've been looking at this for a bit, and I think I understand what's > happening now. > > With this rule for example: > > pass out on vtnet0 dup-to (vtnet1 10.0.0.1) proto udp from any to any > port 53 > > In short, we hit pf_test() in the output path, match the rule and end up > calling into pf_route(). That's all OK. > pf_route() duplicates the packet and discovers that it's supposed to be > sent out through a different interface (We hit 'if (oifp != ifp)' in > pf_route()) so we run pf_test() again. That's still OK. > > In pf_test() we (through pf_test_state_udp()) find state for the > connection and find the rule through the state. As a result we execute > pf_route() a second time, despite the fact that the output interface is > now different. Because we run pf_route() a second time we emit the > packet a second time as well. > Yes, that's true. The problem appears because when pf_test() handle duplicated packet it finds the same state that was already found for original packet. And this state has 'dup-to' option enabled so duplicated packet is being duplicated once again. > > 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 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 */ 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). But I can't see any simple way how to make that ruleset works and in the same time repair our problem. Best regards.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAC4mxp6wvMe9EWqXYzNG=FEA2HO-kNqmdLrUjs8nHJUODTucUw>