Re: [nf-next PATCH v2 1/5] netfilter: bitwise: keep track of bit-length of expressions

[Florian Westphal <fw@strlen.de>]

Jeremy Sowden <jeremy@azazel.net> wrote:
> It wouldn't be straightforward.  Expression udata might make more sense
> than adding a new bitwise attribute, but that doesn't currently exist.
> Would it be worth adding?  I seem to recall considering something along
> those lines for passing type information with expressions as a way to
> implement casting.

Had not thought of casting, good point.
Given bitwise needs to be touched anyway to get the second register
operations I think the proposed patch isn't too bad.

For casts and other use cases (including bitlen), I think its
not needed to add special udata for expressions, as userspace can't
zap them selectively.

We already do something similar for sets (to embed 'typeof' info
for key and data).

Probably extend nftnl_udata_rule_types in libnftnl to add a
NFTNL_UDATA_RULE_EXPR_INFO.

NFTNL_UDATA_RULE_EXPR_INFO would be nested and contain
expression specific (nested) attributes.

i.e., if you have something like

meta mark -> reg 1
binop reg1 &= 0x0000ffff
ct mark -> reg 2
binop and reg1 &= reg2  // ulen 16

Then rule udata would have:
NFTNL_UDATA_RULE_EXPR_INFO (nested)
   type 4 (nested, 4 refers to the last expression above,
 	   type '0' is reserved).
      type 1 // nla_u32  -> for binop, 1 is 'len', it would be
               defined privately in src/bitwise.c
END

because only expression 4 needs annotations, so we don't waste
space for expressions that do not need extra data.

We could reserve a few nested numbers for things that might make sense
for all (or many) expresssions, e.g. 'cast to type x'.

We could of course place expr specific structs in there too but so
far we managed to avoid this and it would be not-so-nice to break
nft userspace when listing a ruleset added by an older version.

Probably could extend struct netlink_linearize_ctx with a memory
blob pointer that netlink_linearize_rule()/netlink_gen_stmt() can use
to add extra data.

My problem is that its a lot of (userspace) code for something that can
easily be done by a small kernel patch such as this one and so far we
don't have a real need for something like this.