Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> > This patch reworks template policy to instead work with existing conntrack.
> >
> > As long as such conntrack has not yet been placed into the hash table
> > (unconfirmed) we can still add the timeout extension.
> >
> > The only caveat is that we now need to update/correct ct->timeout to
> > reflect the initial/new state, otherwise the conntrack entry retains the
> > default 'new' timeout.
> >
> > Side effect of this change is that setting the policy must
> > now occur from chains that are evaluated *after* the conntrack lookup
> > has taken place.
> >
> > No released kernel contains the timeout policy feature yet, so this change
> > should be ok.
> >
> > Signed-off-by: Florian Westphal <fw@xxxxxxxxx>
> > ---
> > nft_ct_timeout_update() is ugly, but i found no better
> > solution.
>
> See below.
>
> > The alternative is to allow use of templates,
> > but that requires nasty kmemdup() games to clone the
> > template, else we'd modify some percpu/readonly template.
>
> percpu template would allow us to combine both, I mean, to use the
> template as a scratchpad area. The template is only used from the same
> hook point to pass information between hook callbacks.
I found no way to do this.
Consider this:
tcp dport 22 ct timeout set "baz" accept
meta iifname ct zone set 23 ct timeout set "foo" accept
ct zone set 42 ct timeout set "bar" accept
(ruleset is not very meaningful, only to illustrate problem)
If a pcpu template is used, we must reset that pcpu scatchpad area
back to a pristine state whenever a new skb comes in.
Otherwise, if first rule does match, pcpu template might still contain
'zone 23' or 'zone 42'.
And we don't know when the 'next' skb comes along.
nfqueue complicates this further (see the ugly refcnt check in the
nft_ct.c parts that deal with 'zone set' support).
> Probably we need to support both approaches? I mean, the legacy one
> that allows users to set the timeout from the raw table and the new
> one which doesn't need template at all. Same thing with ct helpers in
> nftables, which don't templates either.
The only "solution" that I see to support all cases for 'timeout'
is this:
if skb->nfct is a template, do a 'full' clone of the template
(need new helper function for this to deal with nf_conn->ext copy),
then mangle the copy, discard old template
if skb->nfct is NULL, allocate a new template from the packet path
and initialise it.
> > + case IPPROTO_TCP:
> > + ct->timeout = values[1];
> > + break;
> > + default:
> > + ct->timeout = values[0];
>
> I think we can avoid the assymetry by making UDP and generic trackers
> point to 1 for the default timeout, so we don't need this. It will be
> just 4 bytes more per the global timeout array and we don't need this
> function, it would need a preparation patch for this.
We could also make TCP/SCTP/DCCP use 0, as that is unused.
I can make this change.
> We can probably use nf_ct_refresh() instead here?
Oh, right, we can do that in fact.
> > - if (ct ||
> > - priv->l4proto != pkt->tprot)
> > + if (priv->l4proto != pkt->tprot)
> > return;
> >
> > - nf_ct_set(skb, priv->tmpl, IP_CT_NEW);
> > + if (!ct || nf_ct_is_template(ct))
> > + return;
> > +
> > + timeout = nf_ct_timeout_find(ct);
> > + if (timeout) {
>
> I think we should turn this into noop for unconfirmed.
OK, that makes it slightly simpler indeed.
