Re: [iptables RFC PATCH 8/8] nft: Support compat extensions in rule userdata

[Phil Sutter <phil@nwl.cc>]

Hi Pablo,

On Wed, Aug 07, 2024 at 07:56:53PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Aug 01, 2024 at 12:27:03AM +0200, Phil Sutter wrote:
> > Add a mechanism providing forward compatibility for the current and
> > future versions of iptables-nft (and all other nft-variants) by
> > annotating nftnl rules with the extensions they were created for.
> > 
> > Upon nftnl rule parsing failure, warn about the situation and perform a
> > second attempt loading the respective compat extensions instead of the
> > native expressions which replace them. The foundational assumption is
> > that libxtables extensions are stable and thus the VM code created on
> > their behalf does not need to be.
> > 
> > Since nftnl rule userdata attributes are restricted to 255 bytes, the
> > implementation focusses on low memory consumption. Therefore, extensions
> > which remain in the rule as compat expressions are not also added to
> > userdata. In turn, extensions in userdata are annotated by start and end
> > expression number they are replacing. Also, the actual payload is
> > zipped using zlib.
> 
> What is store in the userdata extension? Is this a textual
> representation of the match/target?

The patch introduces a new attribute UDATA_TYPE_COMPAT_EXT which holds
an "array" of this data structure:

| struct rule_udata_ext {
|         uint8_t start_idx;
|         uint8_t end_idx;
|         uint8_t type;
|         uint8_t zip:1;
|         uint16_t orig_size;
|         uint16_t size;
|         unsigned char data[];
| };

start_idx/end_idx are those of expressions in the rule which are to be
replaced by this extension in fallback case. The 'type' field
distinguishes matches from targets (could be single-bit as well), the
'zip' field indicates 'data' is zlib-compressed. The remaining fields
are self-explanatory, whereat 'data' holds a (compressed) object of
either struct xt_entry_match or struct xt_entry_target.

> What is in your opinion the upside/downside of this approach?

You may recall, I tried to build a mechanism which works with old
binaries. This one does not, it requires user space support.
Distributions might backport it though, maybe even just the parser part.

The upside to this is that no kernel modifications are needed, the whole
thing is transparent to the kernel (apart from the increased rule size).

I had implemented a first approach embedding the rule in textual
representation into userdata, but it was ugly for different reasons.
Also I refrained from generating the string rep. ad-hoc from a given
iptables_command_state object because that would require refactoring of
the whole printing code to use a buffer or defined fp instead of stdout
directly. Apart from ugliness caused by reusing "whatever" the user put
into argv[], I had to overcome some obstacles:

- Space restrictions in userdata, breaking for "long" rules (e.g. having
  long comments).
- Parsing a rule from string ad-hoc (e.g. to compare user input with
  rules in cache) triggered some "funny" bugs.
- No way to omit redundant data (i.e., extensions which remain as compat
  expressions in the rule).

Vice-versa, this implementation has the following benefits:

- Rule parsing in fallback case is relatively simple, userdata bits
  parse similar to compat expression payload.
- Provide just the minimum parts of the rule in userdata. Comments will
  always remain in an extension, so will never be carried in userdata.
- Extensions compress relatively well (due to zero bytes in data
  structures).

One may assess better readability of netlink debug output when using a
string rep. This got somewhat reduced by me using NUL-chars to separate
arguments, but neither nft nor libnftnl will be able to convert the
binary payload of this approach to something user-friendly. Using
libxtables though, one could print the individual extensions into
iptables "command line parts".

I'll happily answer further questions, just shoot!

Cheers, Phil