| Message ID | 20170203123556.17357-6-fw@strlen.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 07, 2017 at 12:58:56PM +0100, Pablo Neira Ayuso wrote: > 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. Oh, to add more info: There is one more corner case we have to support: add rule x y ip saddr . meta cpu eq 1.1.1.1 . 10 When using sets, we can stash the datatype, byteorder and size in NFTA_SET_USERDATA. In this case above, we have to infer it from the LHS of the relational that defines the concatenation. I guess this will require a bit of code from rule_postprocess(). -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) @@ -231,6 +232,7 @@ extern const struct datatype icmp_code_type; extern const struct datatype icmpv6_code_type; extern const struct datatype icmpx_code_type; extern const struct datatype time_type; +extern const struct datatype u32_type; extern const struct datatype *concat_type_alloc(uint32_t type); extern void concat_type_destroy(const struct datatype *dtype); diff --git a/src/datatype.c b/src/datatype.c index 1518606a3f89..cab42d47f0f0 100644 --- a/src/datatype.c +++ b/src/datatype.c @@ -48,6 +48,7 @@ static const struct datatype *datatypes[TYPE_MAX + 1] = { [TYPE_ICMP_CODE] = &icmp_code_type, [TYPE_ICMPV6_CODE] = &icmpv6_code_type, [TYPE_ICMPX_CODE] = &icmpx_code_type, + [TYPE_U32] = &u32_type, }; void datatype_register(const struct datatype *dtype) @@ -1057,3 +1058,12 @@ struct error_record *rate_parse(const struct location *loc, const char *str, return NULL; } + +const struct datatype u32_type = { + .type = TYPE_U32, + .name = "u32", + .desc = "32bit host endian integer", + .size = 4 * BITS_PER_BYTE, + .byteorder = BYTEORDER_HOST_ENDIAN, + .basetype = &integer_type, +};
This is needed once we add support to set a zone, as in ct zone set 42 Using integer_type makes nft use big-endian representation of the zone id instead of the required host byte order. When using 'ct zone 1', things will work because the (implicit) relational operation makes sure that the left and right sides have same byte order. In the statement case the lack of relop means we either need to convert ourselves (the ct template contains endianess info), or use a dedicated type (the latter is the reason why setting a mark will 'just work' since the mark type takes care of it). The dedicated type has the advantage that it also works when maps are used: ct zone set mark map { 1 : 10, 2 : 20, 3 : 30 } ... which is not easy to do with current map/set code, its endianess settings rely on dtype->byteorder (i.e., it will always set BIG_ENDIAN when we'd use integer_type for the zone). Using evaluation context seems like a nightmare because several places during eval steps can re-set this information, and propagating the template info means to pollute generic code with something specific to ct. It seems like a future removal of all .byteorder members in the templates in favor of using appropriate types might be a good idea. Signed-off-by: Florian Westphal <fw@strlen.de> --- include/datatype.h | 2 ++ src/datatype.c | 10 ++++++++++ 2 files changed, 12 insertions(+)