Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Phil Sutter <phil@nwl.cc>]

Hi Pablo,

On Thu, Feb 02, 2023 at 10:31:58PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Jan 18, 2023 at 02:48:20PM +0100, Phil Sutter wrote:
> [...]
> > The crucial aspect of this implementation is to provide a compatible
> > rule representation for old software which is not aware of it. This is
> > only possible by dumping the compat representation in the well-known
> > NFTA_RULE_EXPRESSIONS attribute.
> 
> OK, so NFTA_RULE_EXPRESSIONS contains the xt expressions.
> 
> Then, _ACTUAL_EXPR is taken if kernel supports it and these are
> expressions that run from datapath, if present.

Yes, this is indeed somewhat of a downside of this approach: a kernel
which doesn't support the new attribute will use the compatible version
of the rule instead of the improved one. But apart from that, everything
just works.

> > This means what is contained in NFTA_RULE_EXPRESSIONS may not be what
> > the kernel actually executes. To make this less scary, the kernel should
> > dump the actual rule in a second attribute for the sake of verification
> > in user space.
> >
> > While rule dumps are pretty much fixed given the above, there is
> > flexibility when it comes to loading the rule:
> > 
> > A) Submit the compat representation as additional attribute
> > 
> > This was my initial approach, but Florian objected because the changing
> > content of NFTA_RULE_EXPRESSIONS attribute may be confusing:
> 
> It is indeed.

Sorry. :(

> > On input, NFTA_RULE_EXPRESSIONS contains the new rule representation, on
> > output it contains the compat one. The extra attribute I introduced
> > behaves identical, i.e. on input it holds the compat representation
> > while on output it holds the new one.
> > 
> > B) Submit the new representation as additional attribute
> > 
> > This is the current approach: If the additional attribute is present,
> > the kernel will use it to build the rule and leave NFTA_RULE_EXPRESSIONS
> > alone (actually: store it for dumps). Otherwise it will "fall back" to
> > using NFTA_RULE_EXPRESSIONS just as usual.
> >
> > When dumping, if a stored NFTA_RULE_EXPRESSIONS content is present, it
> > will dump that as-is and serialize the active rule into an additional
> > attribute. Otherwise the active rule will go into NFTA_RULE_EXPRESSIONS
> > just as usual.
> 
> So this is not swapping things, right? Probably I am still getting
> confused but the initial approach described in A.

No swap: The kernel will dump in NFTA_RULE_EXPRESSIONS exactly what it
got in that attribute, same for the new one.

> When, dumping back to userspace, NFTA_RULE_EXPRESSIONS still stores
> the xt compat representation and NFTA_RULE_ACTUAL_EXPRS the one that
> runs from kernel datapath (if the kernel supports this attribute).

Yes, exactly. And old user space or nft will put the "new"
representation into NFTA_RULE_EXPRESSIONS, not attach
NFTA_RULE_ACTUAL_EXPRS and thus the kernel will use the former in its
data path.

> [...]
> > I am swapping things around in libnftnl - it uses NFTA_RULE_ACTUAL_EXPRS
> > if present and puts NFTA_RULE_EXPRESSIONS into a second list for
> > verification only. In iptables, I parse both lists separately into
> > iptables_command_state objects and compare them. If not identical,
> > there's a bug.
> 
> Old kernels would simply discard the ACTUAL_ attribute. Maybe _ALT_
> standing by alternative is a better name?

Fine with me! "ACTUAL" was suggested by Florian, probably to point out
that it's what should take precedence if present. In my understanding,
"ALT" means "as good as".

> Sorry, this is a bit confusing but I understand something like this is
> required as you explained during the NFWS.

Thanks. Irrespective of the "crazy container people" mixing iptables
versions and variants like mad, I believe it will allow us to make more
drastic changes in future.

Cheers, Phil