Hi Phil,
On Thu, Jan 10, 2019 at 11:17:36AM +0100, Phil Sutter wrote:
> Hi Pablo,
>
> On Thu, Jan 10, 2019 at 12:50:34AM +0100, Pablo Neira Ayuso wrote:
> > On Sun, Dec 30, 2018 at 08:06:11PM +0100, Phil Sutter wrote:
> > > iptables-restore allows to insert rules at a certain position which is
> > > problematic for iptables-nft to realize since rule position is not
> > > determined by number but handle of previous or following rule.
> >
> > Looking at this late postprocessing in nft_action()... a bit of
> > brainstorming.
> >
> > Why not same approach as in rule_translate_index() in nftables? I
> > guess answer is this is also incomplete, right?
>
> Yes, that is not sufficient, see below for an example.
>
> > If that is not enough, we could add support for NFTA_RULE_ID to
> > nf_tables_newrule(). This NFTA_RULE_ID attribute provides a way to
> > refer to rules coming in this batch. It is similar to NFTA_SET_ID,
> > however, currently it is only supported from the rule deletion path.
> > This ID is internal to the transaction - we can freely allocate IDs
> > for the new rules coming in the batch. My concern is that we may have
> > problems later on if we do not address limitations in the kernel
> > interface.
>
> That might help, yes. Though depending on how this will be applied and
> iptables-restore input, we may still run into ugly issues.
I'm under the impression that the problem is the lack of handle for
rules that are not in the kernel, see below.
> > Or thinking if we can do this with the existing interface... see
> > below.
> >
> > > To fix this, make use of the rule cache (which holds in-kernel rules
> > > with their associated handle already). Insert new rules at the right
> > > position into that cache, then at commit time (i.e., in nft_action())
> > > traverse each chain's rule list for new rules to add:
> > >
> > > * New rules at beginning of list are inserted in reverse order without
> > > reference of another rule, so that each consecutive rule is added
> > > before all previous ones.
> >
> > I think this refers to rules like:
> >
> > iptables -I chain 1 ...
> >
> > This case we can just handle it by doing plain rule insertion.
>
> Not quite, a user may simply do:
>
> -I chain 1 <rule 1>
> -I chain 1 <rule 2>
>
> Legacy code inserts them in order (i.e., rule 2 follows rule 1) while
> the simple approach you have in mind reverses the ordering.
I might be misunderstanding anything from above, but legacy is
reversing the ordering here:
# cat x
# Generated by xtables-save v1.8.2 on Thu Jan 10 01:10:06 2019
*filter
-I INPUT 1 -j ACCEPT
-I INPUT 1 -j DROP
COMMIT
# Completed on Thu Jan 10 01:10:06 2019
# iptables-legacy-restore < x
# iptables-legacy-save
# Generated by iptables-save v1.8.2 on Thu Jan 10 23:12:35 2019
*filter
:INPUT ACCEPT [0:0]
:FORWARD ACCEPT [0:0]
:OUTPUT ACCEPT [0:0]
-A INPUT -j DROP
-A INPUT -j ACCEPT
COMMIT
# Completed on Thu Jan 10 23:12:35 2019
So insertion semantics on the same position looks correct to me.
For iptables-nft, I would place rule 1, then look for rule in position
number 1 and insert it before.
I think we have two scenarios:
#1 normal iptables-restore: in this case, kernel rules are ignored
since we assume an implicit flush. In this case, we need a variant
of batch_rule_add_at() that allows us to place the rules at the
right position.
#2 --noflush is specified: in this case, two sub-scenarios:
#2.1 we find an existing rule with a handle at the position, we use the
rule handle that we found as NFTA_RULE_POSITION to place the rule.
We can use the existing batch_rule_add().
#2.2 we find a new rule at the position that has no handle, we use the
batch_rule_add_at() variant that allows us to place the rule at
the right position. The batch_rule_add_at() variant takes the
rule we found as parameter, so we can find it in the batch
through pointer.
> > > * Each time an "old" (i.e., in-kernel) rule is encountered, its handle
> > > is stored (overwriting any previous ones').
> > >
> > > * New rules following an old one are appended to the old one (by
> > > specifying its handle) in reverse order, so that each consecutive rule
> > > is inserted between the old one and previously appended ones.
> >
> > For existing rules, we already have an absolute reference through the
> > handle, then the NLM_F_APPEND flag tells if we want to place it before
> > or after that rule.
> >
> > What scenario is forcing us to do this late postprocessing below that
> > we cannot handle with the existing interface? :-)
>
> Kernel has this:
>
> -A chain <rule 1> # handle 10
> -A chain <rule 2> # handle 20
>
> User does:
>
> -I chain 2 <rule a>
> -I chain 3 <rule b>
>
> Handling rule a in iptables-nft-restore is easy, just insert before
> handle 20 or append to handle 10. If we don't add this rule to cache,
> rule b will be appended to ruleset (since the rulenum it refers to is
> larger than the number of rules in cache), although it should be
> inserted in between rule a and rule 2.
I agree we need to update the cache, we need this to walk over the
lists to find what rule is placed at what position, there is no other
way. So updating the cache is fine indeed :-).
But I think the postprocessing at nft_action() is not required, it
would be good if we can avoid this.
> So we need to add new rules to cache, otherwise following rule's numbers
> refer to the wrong rule. Now consider a cache of:
>
> - rule 1 # handle 10
> - rule 2 # new
> - rule 3 # new
>
> And we parse this command:
>
> -I chain 3 <rule 4>
>
> Due to missing handle, we can neither append to rule 2 nor insert before
> rule 3. How do we get rule 4 in between rule 2 and rule 3?
Using batch_rule_add_at() variant, we pass rule 2 as parameter, so we
can look up for it using the rule pointer in the batch.
We may need a batch_rule_insert_at() too.
Would this work for you?
Let me know if I'm missing any scenario, thanks!
