From: Pablo Neira Ayuso <pablo@netfilter.org>
To: Florian Westphal <fw@strlen.de>
Cc: Dirk Morris <dmorris@metaloft.com>, netfilter-devel@vger.kernel.org
Subject: Re: [PATCH net] netfilter: Use consistent ct id hash calculation
Date: Wed, 7 Aug 2019 22:31:46 +0200 [thread overview]
Message-ID: <20190807203146.bmlvjw4kvkbd5dns@salvia> (raw)
In-Reply-To: <20190807180157.ogsx435gxih7wo7r@breakpoint.cc>
On Wed, Aug 07, 2019 at 08:01:57PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > @Florian: by mangling this patch not to use ct->ext, including Dirk's
> > update, conntrackd works again (remember that bug we discussed during
> > NFWS).
>
> But conntrackd is still borken.
> It can't rely on id recycling -- it will just take a lot
> longer before it starts to fill up.
Conntrackd does not rely on ID recycling. Conntrackd is in trouble
because of event loss. It seems the event re-delivery routine is
buggy, if the destroy event gets to userspace sooner or later, then
this entry would not get stuck in the cache forever. I can just remove
the check for the ID in userspace, so conntrackd would get rid of the
stale entry by when a new entry with the same tuple shows up (lazy
garbage collection).
> > @@ -470,8 +470,8 @@ u32 nf_ct_get_id(const struct nf_conn *ct)
> >
> > a = (unsigned long)ct;
> > b = (unsigned long)ct->master ^ net_hash_mix(nf_ct_net(ct));
> > - c = (unsigned long)ct->ext;
> > - d = (unsigned long)siphash(&ct->tuplehash[IP_CT_DIR_ORIGINAL], sizeof(ct->tuplehash[IP_CT_DIR_ORIGINAL]),
> > + c = (unsigned long)0;
> > + d = (unsigned long)siphash(&ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple, sizeof(ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple);
>
> > I think it's safe to turn this into:
> >
> > a = (unsigned long)ct;
> > b = (unsigned long)ct->master;
> > c = (unsigned long)nf_ct_net(ct));
> > d = (unsigned long)siphash(&ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple, sizeof(ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple);
>
> No, not if we allow using the function before confirmation, the tuple
> can also change in original dir when e.g. queuing before NAT hooks.
Tuple could be artificially built from original source as source and
reply source as destination, those never change IIRC.
This hash-based ID calculation is a simple approach, but it looks weak
/ easy to break.
next prev parent reply other threads:[~2019-08-07 20:31 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-07 0:25 [PATCH net] netfilter: Use consistent ct id hash calculation Dirk Morris
2019-08-07 0:34 ` Florian Westphal
2019-08-07 0:57 ` Dirk Morris
2019-08-07 16:36 ` Pablo Neira Ayuso
2019-08-07 18:01 ` Florian Westphal
2019-08-07 20:31 ` Pablo Neira Ayuso [this message]
2019-08-07 23:45 ` Florian Westphal
2019-08-08 5:01 ` Dirk Morris
2019-08-08 10:28 ` Florian Westphal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190807203146.bmlvjw4kvkbd5dns@salvia \
--to=pablo@netfilter.org \
--cc=dmorris@metaloft.com \
--cc=fw@strlen.de \
--cc=netfilter-devel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).