Hi Pablo, On Thu, Jan 10, 2019 at 11:53:15PM +0100, Pablo Neira Ayuso wrote: > On Thu, Jan 10, 2019 at 11:17:36AM +0100, Phil Sutter wrote: > > 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: [...] > > > 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. Yes, this is probably the best choice. > > > 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: Oh, crap. You're right of course, I wrote the above having my postprocessing algorithm in mind: When adding the above two rules to the batch, they will be reversed (so cache contains them in correct order). During postprocessing, I have to reconstruct the user-provided ordering, i.e. do the insert without reference in reverse. Of course this is not required when sticking to the old code which generates the batch jobs right away. [...] > 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. Since this is the same as restore with --noflush and an empty ruleset in kernel, there is no need to treat this separately - the existing code which creates a flush job and removes any existing rules from cache is fine. > #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. Sounds good, I'll give it a go. Thanks, Phil
