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

[Pablo Neira Ayuso]

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  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", _type, 4 * BITS_PER_BYTE, 
> > BYTEORDER_HOST_ENDIAN),
> > with
> > [NFT_META_CPU]  = META_TEMPLATE("cpu", _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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pablo Neira Ayuso]

On Mon, Feb 06, 2017 at 11:33:01PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso  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", _type, 4 * BITS_PER_BYTE, 
> BYTEORDER_HOST_ENDIAN),
> with
> [NFT_META_CPU]  = META_TEMPLATE("cpu", _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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Florian Westphal]

Pablo Neira Ayuso  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", _type, 4 * BITS_PER_BYTE, 
BYTEORDER_HOST_ENDIAN),
with
[NFT_META_CPU]  = META_TEMPLATE("cpu", _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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pablo Neira Ayuso]

On Mon, Feb 06, 2017 at 06:31:10PM +0100, Pablo Neira Ayuso 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.

Forgot attachment, here it comes.
>From 52594fb4130560e2072524193296019d209e3bf8 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso 
Date: Tue, 6 Sep 2016 19:49:05 +0200
Subject: [PATCH] evaluate: store byteorder for set keys

Selectors that rely on the integer type and expect host endian
byteorder don't work properly.

We need to keep the byteorder around based on the left hand size
expression that provides the context, so store the byteorder when
evaluating the map.

Before this patch.

 # nft --debug=netlink add rule x y meta mark set meta cpu map { 0 : 1, 1 : 2 }
 __map%d x b
 __map%d x 0
element   : 0001 0 [end]element 0100  : 0002 0 
[end]


This is expressed in network byteorder, because the invalid byteorder
defaults on this.

After this patch:

 # nft --debug=netlink add rule x y meta mark set meta cpu map { 0 : 1, 1 : 2 }
 __map%d x b
 __map%d x 0
element   : 0001 0 [end]element 0001  : 0002 0 
[end]


This is in host byteorder, as the key selector in the map mandates.

Signed-off-by: Pablo Neira Ayuso 
---
 include/rule.h |  3 +++
 src/evaluate.c | 23 +++
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/include/rule.h b/include/rule.h
index 99e92ee..ae9e462 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -209,6 +209,8 @@ enum set_flags {
SET_F_EVAL  = 0x20,
 };
 
+#include 
+
 /**
  * struct set - nftables set
  *
@@ -237,6 +239,7 @@ struct set {
uint64_ttimeout;
const struct datatype   *keytype;
unsigned intkeylen;
+   enum byteorder  keybyteorder;
const struct datatype   *datatype;
unsigned intdatalen;
struct expr *init;
diff --git a/src/evaluate.c b/src/evaluate.c
index 45af329..b64dfc1 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -63,6 +63,7 @@ static struct expr *implicit_set_declaration(struct eval_ctx 
*ctx,
 const char *name,
 const struct datatype *keytype,
 unsigned int keylen,
+enum byteorder byteorder,
 struct expr *expr)
 {
struct cmd *cmd;
@@ -70,11 +71,12 @@ static struct expr *implicit_set_declaration(struct 
eval_ctx *ctx,
struct handle h;
 
set = set_alloc(>location);
-   set->flags  = SET_F_ANONYMOUS | expr->set_flags;
-   set->handle.set = xstrdup(name),
-   set->keytype= keytype;
-   set->keylen = keylen;
-   set->init   = expr;
+   set->flags= SET_F_ANONYMOUS | expr->set_flags;
+   set->handle.set   = xstrdup(name),
+   set->keytype  = keytype;
+   set->keylen   = keylen;
+   set->keybyteorder = byteorder;
+   set->init = expr;
 
if (ctx->table != NULL)
list_add_tail(>list, >table->sets);
@@ -1104,7 +1106,9 @@ static int expr_evaluate_map(struct eval_ctx *ctx, struct 
expr **expr)
case EXPR_SET:
mappings = implicit_set_declaration(ctx, "__map%d",
ctx->ectx.dtype,
-   ctx->ectx.len, mappings);
+   ctx->ectx.len,
+   ctx->ectx.byteorder,
+   mappings);
mappings->set->datatype = ectx.dtype;
mappings->set->datalen  = ectx.len;
 
@@ -1159,7 +1163,8 @@ static int expr_evaluate_mapping(struct eval_ctx *ctx, 
struct 

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

[Pablo Neira Ayuso]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Florian Westphal]

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 
---
 include/datatype.h |  2 ++
 src/datatype.c | 10 ++
 2 files changed, 12 insertions(+)

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]= _code_type,
[TYPE_ICMPV6_CODE]  = _code_type,
[TYPE_ICMPX_CODE]   = _code_type,
+   [TYPE_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   = _type,
+};
-- 
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html