On Thu, Jan 12, 2017 at 02:29:30PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> > On Thu, Jan 12, 2017 at 12:01:41AM +0100, Florian Westphal wrote:
> > > ct zone set meta mark # as per my proposal -- set template
> > > ct eventmask set new # as per my proposal -- lookup&change real conntrack
> > > ct helper set "foo" # as per my proposal, lookup and change real
> > > conntrack if its unconfirmed, EXCEPT helper is not looked up based on skb
> > > payload and name "foo" but instead taken from objref infrastructure.
> > > Same for timeout.
> >
> > Yes, something like the one above. By look&change, you refer to this?
> >
> > ct = nf_ct_get(...);
> > if (!ct || !nf_ct_unconfirmed(ct))
> > return;
> >
> > /*
> > * ... update ct with helper here.
> > */
>
> No, I meant more intrusive version:
>
> ct = nf_ct_get(...);
> if (!ct || nf_ct_is_template(ct))))
> nf_conntrack_in(net, info->pf, hook, skb);
OK, then we have to defrag first. Let me give another twist to this
discussion, still brainstorming.
Did you consider to move this logic into a explicit 'track' statement.
Instead of this implicit lookup hidden in the helper/timeout
assignment, syntax would be something like:
tcp dport 21 track with helper "ftp-standalone" timeout "tcp-short-timeout"
Note, that:
track with helper "ftp-standalone" timeout "tcp-short-timeout"
performs this lookup&change as you need, but in one go, only for
unconfirmed conntrack. And this would be achieved with one single
kernel expression, in nft --debug=netlink representation:
[ track helper "ftp-standalone" timeout "tcp-short-timeout" ]
For zone ID, we can use the same thing:
track with zone 1 [ This can be combined with helper/timeouts too. ]
In this case, we pass the zone ID via nf_conntrack_in() [ or a new
function that is called from nf_conntrack_in() that takes the zone ID
and that doesn't depend on templates anymore, we can strip off
nf_conntrack_in() from the template logic ].
The new track kernel expression would need to perform similar tricks
as 'objref', as we also want map support there. So we can add
_SREG_ZONE, _SREG_TIMEOUT and _SREG_HELPER attribute to indicate the
set key that we use to perform the lookups to fetch the data/objects.
Syntax would be:
track with helper map tcp dport { 21 : "ftp-standalone",
2121 : "ftp-standaline" } \
timeout "tcp-short-timeout"
In nft --debug=netlink representation, this looks like:
[ track helper sreg X timeout "tcp-short-timeout" ]
_SREG_TIMEOUT and _SREG_HELPER would require a NFT_SET_OBJECT set
type. _SREG_ZONE will be just fine with a NFT_SET_MAP that contains
simple data maps, not pointers.
With this proposal, I'm moving to the opposite side than in my
previous email 8): I think we could completely get rid of templates.
Look, and we could also allow a simple:
track
alone itself with no options, that means, track this. This results in
creation of conntrack if it doesn't exists, otherwise lookup and
attach to skb->nfct. I know this is different front as this would be
only useful for bridge and ingress at this stage. For existing ip/ip6
families, this would nf_conntrack_in() into a no-op if skb->nfct is
set.
> ct = nf_ct_get(...);
> if (!ct)
> return;
>
> switch (priv->key) {
> case NFT_CT_MARK:
> ct->mark = sreg32;
> break;
> case NFT_CT_HELPER:
> if (nf_ct_is_confirmed(ct))
> return; /* too late */
>
> /* update helper here. sreg contains name of objectref,
> * so we'd use that to obtain a helper template, then set/add
> * helper extension to ct based on that.
> * But we don't set skb->nfct to the template, we only use
> * it as source to get the needed helper information.
> */
> break;
> case NFT_CT_EVENTMASK:
> struct nf_conntrack_ecache *e = nf_ct_ecache_find(ct);
>
> if (!e) {
> if (!nf_ct_is_confirmed(ct))
> nf_ct_ecache_ext_add(ct, sreg16, ~0, GFP_ATOMIC);
> return;
> }
>
> e->ctmask = sreg16;
> break;
> case ...
> }
>
> NFT_CT_ZONE would have its own eval() version that doesn't
> call nf_conntrack_in but instead sets up skb->nfct to a percpu template
> that will then be used to transport the zoneinfo until nf_conntrack_in
> is called (either by later hook or by later call via 'ct foo set bar').
>
> [ unless skb->nfct is != NULL, then its a no-op ]
I see.
> > For timeouts, after a second look I think this is more complicated: we
> > set them from nf_conntrack_in() via l4proto->packet(). Thus, if we add
> > the conntrack extension to store timeouts later on, the first
> > conntrack state will not get refreshed with the right timeout.
> >
> > Assuming this is the first packet of a flow, that should be easy to
> > amend later. But if we pickup a flow from the middle, timeout
> > amendment gets a bit more tricky. Calling l4proto->packet() is not
> > possible either since this sets packets as invalid, so we can drop it
> > from the ruleset.
>
> Ok. However I don't see why this isn't solveable in some way.
>
> We could e.g. add a new l4proto callback, something like
> l4proto->update_timeouts(ct, timeouts);
Yes, timeout logic happens at the end of TCP tracker timeout which
seems to perform a few tricks there. And matching 'ct expiration' will
change semantics for unconfirmed ct, but that should be OK, I don't
know of anyone using it.
> > Back to helpers, users are familiar with the current way to attach
> > helpers, ie. from the raw chain.
> >
> > Am I missing anything? I'm starting to think we can't escape the
> > conntrack template.
>
> For Helpers? Why not? As long as ct isn't in the main table it should
> be fine afaics? (Unless you mean "can't escape conntrack template to
> read to helper info that we need to assign to ct from".
>
> For zones, yes, I don't see a way to avoid a template for them.
> But thats the only ct key where I think that a template has to be used.
Yes, following the approach you propose zone would be the only one
that requires the template. So this needs to happen before the
conntrack hook.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
