Re: [PATCH nftables 5/9] src: add host byte order integer type

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Mon, Feb 06, 2017 at 11:33:01PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Fri, Feb 03, 2017 at 01:35:52PM +0100, Florian Westphal wrote:
> > > diff --git a/include/datatype.h b/include/datatype.h
> > > index 9f127f2954e3..8c1c827253be 100644
> > > --- a/include/datatype.h
> > > +++ b/include/datatype.h
> > > @@ -82,6 +82,7 @@ enum datatypes {
> > >  	TYPE_DSCP,
> > >  	TYPE_ECN,
> > >  	TYPE_FIB_ADDR,
> > > +	TYPE_U32,
> > >  	__TYPE_MAX
> > >  };
> > >  #define TYPE_MAX		(__TYPE_MAX - 1)
> > 
> > Right, this is a real problem with host byteorder integer, the
> > bytecode that we generate is not correct.
> > 
> > I have a patch to avoid this, it's still incomplete. I'm attaching it.
> > 
> > Note this is still incomplete, since this doesn't solve the netlink
> > delinearize path. We can use the NFT_SET_USERDATA area and the tlv
> > infrastructure that Carlos made in summer to store this
> > metainformation that is only useful to
> > 
> > This shouldn't be a showstopper to get kernel patches in, we have a
> > bit of time ahead to solve this userspace issue.
> 
> I don't understand why all this fuss is required.
> 
> The type always enocodes/decides the endianess, so I fail to see why we
> need to store endianess also in the templates (f.e. meta_templates[],
> it just seems 100% redundant ...)
> 
> Thats why it I added this host endian thing here, we could then replace
> 
> [NFT_META_CPU]  = META_TEMPLATE("cpu", &integer_type, 4 * BITS_PER_BYTE, BYTEORDER_HOST_ENDIAN),
> with
> [NFT_META_CPU]  = META_TEMPLATE("cpu", &integer_u32, 4 * BITS_PER_BYTE),
>
> and don't need this 'endianess override' in the templates anymore.
> We might even be able to get rid of the endianess storage in the eval
> context this way.
> 
> What am I missing?

This approach will not work for more stuff coming ahead, eg.
concatenation support for integer datatypes. This currently doesn't
work since nft complains on concatenation with datatypes with not
fixed datatypes.

In that case, we will end up having integers of different sizes and
byteorder. We would need one hardcoded type for each variant.

Note that these TYPE_XYZ are ABI too, so we should avoid changes once
we push them into nftables.git. We also have a limited number of them,
6 bits since concatenations places up to 5 datatypes there using bit
shifts.

I understand the override is not nice, that's one of the reasons why I
did not push this yet. Probably we can allocate datatype instances
dynamically, so we don't need this new field.