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. > 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. > > * 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. 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? Given the freedom users have when creating input to iptables-nft-restore, all this quickly turns into a can of worms. At least I am not able to keep all possible situations in mind in order to get the code right. That's why I chose the most straight forward approach that occurred to me: Put all rules in cache at the right spot and do the insert/append with handle dance later when all rules are in place. Cheers, Phil
