Date: Tue, 17 Apr 2012 12:46:08 +0400 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Ermal Lu?i <eri@FreeBSD.org> Cc: freebsd-pf@FreeBSD.org Subject: Re: kern/164402: [pf] pf crashes with a particular set of rules when first matching packet arrives Message-ID: <20120417084608.GA99119@glebius.int.ru> In-Reply-To: <CAPBZQG2gF8GSx6eE4jkFuOf29c-jB09Gz6=%2BkbpXprN8XiEE4w@mail.gmail.com> References: <201204151200.q3FC0LT5085161@freefall.freebsd.org> <20120416185949.GC92286@FreeBSD.org> <CAPBZQG2Tjg36GNCBetRZ20FhQnL1sK9i_-oQDDb97bcb4N=sLA@mail.gmail.com> <20120417081406.GA93887@glebius.int.ru> <CAPBZQG2gF8GSx6eE4jkFuOf29c-jB09Gz6=%2BkbpXprN8XiEE4w@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Apr 17, 2012 at 10:38:31AM +0200, Ermal Lu?i wrote:
E> 2012/4/17 Gleb Smirnoff <glebius@freebsd.org>:
E> > On Tue, Apr 17, 2012 at 10:06:15AM +0200, Ermal Lu?i wrote:
E> > E> 2012/4/16 Gleb Smirnoff <glebius@freebsd.org>:
E> > E> > On Sun, Apr 15, 2012 at 12:00:21PM +0000, Gleb Smirnoff wrote:
E> > E> > T> On Sun, Apr 15, 2012 at 11:10:03AM +0000, Gleb Smirnoff wrote:
E> > E> > T> T> I have a vague suspicion on what is happening. Your description of
E> > E> > T> T> the problem looks like if a packet processing in the kernel has entered
E> > E> > T> T> an endless loop.
E> > E> > T> T>
E> > E> > T> T> Looking at pf_route() I see such possibility. From OpenBSD we have
E> > E> > T> T> this protection against endless looping:
E> > E> > T> T>
E> > E> > T> T> if ((*m)->m_pkthdr.pf.routed++ > 3) {
E> > E> > T> T> m0 = *m;
E> > E> > T> T> *m = NULL;
E> > E> > T> T> goto bad;
E> > E> > T> T> }
E> > E> > T> T>
E> > E> > T> T> In our code this transforms to:
E> > E> > T> T>
E> > E> > T> T> if (pd->pf_mtag->routed++ > 3) {
E> > E> > T> T> m0 = *m;
E> > E> > T> T> *m = NULL;
E> > E> > T> T> goto bad;
E> > E> > T> T> }
E> > E> > T> T>
E> > E> > T> T> The root difference between storing the tag on mbuf and on pfdesc
E> > E> > T> T> is that we lose pfdesc, and thus the tag, when we enter pf_test()
E> > E> > T> T> recursively. And pf_route() does this recursion:
E> > E> > T> T>
E> > E> > T> T> if (oifp != ifp) {
E> > E> > T> T> if (pf_test(PF_OUT, ifp, &m0, NULL) != PF_PASS) {
E> > E> > T> T> goto bad;
E> > E> > T> T> ....
E> > E> > T>
E> > E> > T> On second look I see that my suspicion may not be true. In the
E> > E> > T> beginning of pf_test() we do pf_get_mtag() which preserves already
E> > E> > T> present tag if there is one.
E> > E> >
E> > E> > Further investigation showed that problem exist when route applied
E> > E> > ends in lo0, and packet passes to if_simloop(). There all mtags are
E> > E> > stripped from the mbuf, including the pf mtag. Then packet is again
E> > E> > processed by ip_input() again entering pf(4), if it again matches
E> > E> > a routing rule, then we got an endless loop.
E> > E> >
E> > E> > We can try to fix this applying MTAG_PERSISTENT to the pf(4) tag id.
E> > E>
E> > E> That seems like the best fix for this case.
E> >
E> > In this case crash or freeze is fixed, but still packet is dropped. Example
E> > of such rule:
E> >
E> > pass in on igb0 fastroute proto tcp from any to $localip
E> >
E> > Anyway, dropping packets is much better than crashing.
E> >
E>
E> Actually after some coffee :) i think its better marking the packet
E> with M_SKIP_FIREWALL since
E> it has already taken its decision on this packet.
E>
E> The simloop consumers seem to be just facilities of how things work
E> from what i can see.
E>
E> So just delivering the packet by sending skipping the firewalls seems
E> more sensibile!
E>
E> Though the persistent case for the tags should be revisited since it
E> may fix some other issues with pf(4) tags, and some others.
We can make the assignment like:
if (ifp->if_flags & IFF_LOOPBACK)
m->m_flags |= M_SKIP_FIREWALL;
Because only loopback interface is special: processing its ifp->if_output()
leads to re-entering ip_input().
I'm afraid that if we mark all pf-routed packets as M_SKIP_FIREWALL, that
can lead to a case when packet is routed by pf processing on input hook, and
then it skips processing the output hook, and that can lead to state
not being updated and session stuck.
Still, I think that pf tag deserves MTAG_PERSISTENT. That would keep us
in-line with OpenBSD, since they store pf information right in the pkthdr,
and I don't think that they erase it anywhere until mbuf is freed.
--
Totus tuus, Glebius.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120417084608.GA99119>