[nft PATCH 0/5] Changes in nft byteorder conversions

[nft PATCH 0/5] Changes in nft byteorder conversions

[Alvaro Neira Ayuso]

Currently, if we add a rule, we do some byteorder conversions in the parser,
evaluation and linealize steps. So, if we try to add rules, we do wrong
byteorder conversions. Therefore, we have matching problems or we show
the rules with wrong byteorder. Usually, we have these problems with sets
and ranges.

These series of patches changes the policy for doing byteorder conversions in our
rules. We are going to do all the conversions in two steps, parser and
evaluation. That changes remove consecutive byteorder conversions. Thus, these
patches fixing these problems for adding and for showing rules. Too, fix some
case that we don't update correctly the context for using values without
correctly byteorder conversions. These patches enables to use ranges and sets
with the rules without problems.

Comments are welcome.

Alvaro Neira Ayuso (5):
  payload: fix update context with wrong byteorder
  payload: generate dependency in the correct byteorder
  src: fix byteorder conversions in constant values
  src: fix byteorder conversions in range values
  src: fix byteorder conversions in sets

 include/datatype.h        |    1 +
 include/expression.h      |    2 ++
 include/netlink.h         |    3 +--
 src/datatype.c            |   16 +++++++++++++--
 src/evaluate.c            |   37 ++++++++++++++++++++++++++++-------
 src/expression.c          |   17 ++++++++++++++--
 src/gmputil.c             |    9 +++++++--
 src/netlink.c             |   13 +++++++++----
 src/netlink_delinearize.c |   47 +++++++++++++++++++++++++++++++++++++++++----
 src/netlink_linearize.c   |   10 +++++-----
 src/payload.c             |   10 +++++++---
 src/proto.c               |    2 +-
 12 files changed, 135 insertions(+), 32 deletions(-)

-- 
1.7.10.4

[nft PATCH 1/5] payload: fix update context with wrong byteorder

[Alvaro Neira Ayuso]

In the evaluation and delinealize steps, we update the protocol
context. The expression must be in host endian byteorder when we update the
context. However, this is not the case because we see them in network
byteorder.

Fix this by converting to the appropriate byteorder before updating
the protocol context otherwise this doesn't work.

nft add rule bridge filter input ether type ip

We have a expression like this:

[ payload load 2b @ link header + 12 => reg 1 ]
  [ cmp eq reg 1 0x00000008 ]

The byteorder of this expressions is big endian and it's in
host endian, for that when we try to update the context, we
don't find the protocol with this number. This is a output,
example:

update network layer protocol context:
 link layer          : ether
 network layer       : none <-
 transport layer     : none

Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
---
[Tested with the rules]
* nft add rule filter input ip protocol tcp counter
* nft add rule filter input ip protocol udp counter
* nft add rule filter input tcp dport 22 counter
* nft add rule filter bridge input ether type ip

src/payload.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/payload.c b/src/payload.c
index a1785a5..be3d610 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -69,13 +69,18 @@ static void payload_expr_pctx_update(struct proto_ctx *ctx,
 {
 	const struct expr *left = expr->left, *right = expr->right;
 	const struct proto_desc *base, *desc;
+	const struct proto_hdr_template *tmpl;
+	uint32_t value = 0;
 
 	if (!(left->flags & EXPR_F_PROTOCOL))
 		return;
 
 	assert(expr->op == OP_EQ);
 	base = ctx->protocol[left->payload.base].desc;
-	desc = proto_find_upper(base, mpz_get_uint32(right->value));
+	tmpl = left->payload.tmpl;
+	mpz_export_data(&value, right->value, tmpl->dtype->byteorder,
+			div_round_up(tmpl->len, BITS_PER_BYTE));
+	desc = proto_find_upper(base, value);
 
 	proto_ctx_update(ctx, left->payload.base + 1, &expr->location, desc);
 }
-- 
1.7.10.4

[nft PATCH 2/5] payload: generate dependency in the correct byteorder

[Alvaro Neira Ayuso]

Currently, we suppose that all the dependency are in host endian. The problem
is if we try to add a dependency with big endian byteorder, we create
this dependency wrong. For example, if we add a ether type dependency in bridge,
we must use big endian byteorder and we use host endian and we create wrong the
payload.

Now, we are going to create the dependency taking account the byteorder.

Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
---
 src/payload.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/payload.c b/src/payload.c
index be3d610..8b10a79 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -213,8 +213,7 @@ int payload_gen_dependency(struct eval_ctx *ctx, const struct expr *expr,
 		left = payload_expr_alloc(&expr->location, desc, desc->protocol_key);
 
 	right = constant_expr_alloc(&expr->location, tmpl->dtype,
-				    BYTEORDER_HOST_ENDIAN,
-				    tmpl->len,
+				    tmpl->dtype->byteorder, tmpl->len,
 				    constant_data_ptr(protocol, tmpl->len));
 
 	dep = relational_expr_alloc(&expr->location, OP_EQ, left, right);
-- 
1.7.10.4

[nft PATCH 3/5 v3] src: fix byteorder conversions in constant values

[Alvaro Neira Ayuso]

Currently the byteorder conversion happens in many places: the parser,
the evaluation and the code generation steps. This a problem because
the constant expression is converted several times into big endian
byteorder at different steps.

With this patch, we don't change byteorder in the code generation
step anymore because we assume that all expressions are in the
appropriate byteorder after the parsing and the evaluation steps.
This patch solves rules like:

nft add rule bridge filter input ether type ip

With this rule we create a expression like:

[ payload load 2b @ link header + 12 => reg 1 ]
  [ cmp eq reg 1 0x00000800 ]

With this patch, we have a expresion with the correct conversion:

[ payload load 2b @ link header + 12 => reg 1 ]
  [ cmp eq reg 1 0x00000008 ]

This is also a problem in the delinearize step because the expression
are already converted to the correct byteorder and we change it again.

Also this patch, add a new datatype integer_big_endian for fixing some case
that we have use integer_type with a invalid byteorder when we must associate
a big endian byteorder. This solution is based in a previously patch of Yuxuan
Shui.

Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
---
[Changes in v3]
* Fixed the expressions with invalid byteorder and renamed the patch name.

 include/datatype.h        |    1 +
 include/expression.h      |    2 ++
 include/netlink.h         |    3 +--
 src/datatype.c            |   16 ++++++++++++++--
 src/evaluate.c            |    5 +++--
 src/expression.c          |   17 +++++++++++++++--
 src/gmputil.c             |    9 +++++++--
 src/netlink.c             |   13 +++++++++----
 src/netlink_delinearize.c |   16 ++++++++++++----
 src/netlink_linearize.c   |   10 +++++-----
 src/proto.c               |    2 +-
 11 files changed, 70 insertions(+), 24 deletions(-)

diff --git a/include/datatype.h b/include/datatype.h
index 2c66e9d..0a29506 100644
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -182,6 +182,7 @@ extern const struct datatype verdict_type;
 extern const struct datatype nfproto_type;
 extern const struct datatype bitmask_type;
 extern const struct datatype integer_type;
+extern const struct datatype big_endian_integer_type;
 extern const struct datatype string_type;
 extern const struct datatype lladdr_type;
 extern const struct datatype ipaddr_type;
diff --git a/include/expression.h b/include/expression.h
index edb6dc5..4679316 100644
--- a/include/expression.h
+++ b/include/expression.h
@@ -351,4 +351,6 @@ extern struct expr *map_expr_alloc(const struct location *loc,
 extern struct expr *set_ref_expr_alloc(const struct location *loc,
 				       struct set *set);
 
+extern void expr_switch_byteorder(struct expr *expr);
+
 #endif /* NFTABLES_EXPRESSION_H */
diff --git a/include/netlink.h b/include/netlink.h
index af5dcd9..603da80 100644
--- a/include/netlink.h
+++ b/include/netlink.h
@@ -55,8 +55,7 @@ struct nft_data_delinearize {
 
 extern void netlink_gen_data(const struct expr *expr,
 			     struct nft_data_linearize *data);
-extern void netlink_gen_raw_data(const mpz_t value, enum byteorder byteorder,
-				 unsigned int len,
+extern void netlink_gen_raw_data(const mpz_t value, unsigned int len,
 				 struct nft_data_linearize *data);
 
 extern struct expr *netlink_alloc_value(const struct location *loc,
diff --git a/src/datatype.c b/src/datatype.c
index 55af227..96bf915 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -270,6 +270,14 @@ const struct datatype integer_type = {
 	.parse		= integer_type_parse,
 };
 
+const struct datatype big_endian_integer_type = {
+	.type = TYPE_INTEGER,
+	.byteorder = BYTEORDER_BIG_ENDIAN,
+	.name = "big_endian_integer",
+	.desc = "big endian integer",
+	.basetype = &integer_type,
+};
+
 static void string_type_print(const struct expr *expr)
 {
 	unsigned int len = div_round_up(expr->len, BITS_PER_BYTE);
@@ -545,14 +553,18 @@ static struct error_record *inet_service_type_parse(const struct expr *sym,
 		if (errno == ERANGE || i > UINT16_MAX)
 			return error(&sym->location, "Service out of range");
 
-		port = htons(i);
+		port = i;
 	} else {
 		err = getaddrinfo(NULL, sym->identifier, NULL, &ai);
 		if (err != 0)
 			return error(&sym->location, "Could not resolve service: %s",
 				     gai_strerror(err));
 
-		port = ((struct sockaddr_in *)ai->ai_addr)->sin_port;
+		/* The port is in network byteorder, we must convert it because
+		 * we create a big endian constant and we make a wrong
+		 * reconversion.
+		 */
+		port = ntohs(((struct sockaddr_in *)ai->ai_addr)->sin_port);
 		freeaddrinfo(ai);
 	}
 
diff --git a/src/evaluate.c b/src/evaluate.c
index e05473a..8aaf1bf 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -168,9 +168,10 @@ static int byteorder_conversion(struct eval_ctx *ctx, struct expr **expr,
 				  byteorder_names[byteorder],
 				  byteorder_names[(*expr)->byteorder]);
 
-	if (expr_is_constant(*expr))
+	if (expr_is_constant(*expr)) {
+		expr_switch_byteorder(*expr);
 		(*expr)->byteorder = byteorder;
-	else {
+	} else {
 		op = byteorder_conversion_op(*expr, byteorder);
 		*expr = unary_expr_alloc(&(*expr)->location, op, *expr);
 		if (expr_evaluate(ctx, expr) < 0)
diff --git a/src/expression.c b/src/expression.c
index fa14d99..ebd0ebb 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -303,8 +303,13 @@ struct expr *constant_expr_join(const struct expr *e1, const struct expr *e2)
 	assert(e2->ops->type == EXPR_VALUE);
 
 	tmp = e1->len / BITS_PER_BYTE;
-	mpz_export_data(data, e1->value, e1->byteorder, tmp);
-	mpz_export_data(data + tmp, e2->value, e2->byteorder,
+
+	/* We assume that the expression already comes in the
+	 * appropriate byteorder from the parse/evaluation steps, so
+	 * use BYTEORDER_HOST_ENDIAN to leave it as is.
+	 */
+	mpz_export_data(data, e1->value, BYTEORDER_HOST_ENDIAN, tmp);
+	mpz_export_data(data + tmp, e2->value, BYTEORDER_HOST_ENDIAN,
 			e2->len / BITS_PER_BYTE);
 
 	return constant_expr_alloc(&e1->location, &invalid_type,
@@ -863,6 +868,14 @@ static void set_ref_expr_destroy(struct expr *expr)
 	set_free(expr->set);
 }
 
+void expr_switch_byteorder(struct expr *expr)
+{
+	uint32_t len;
+
+	len = div_round_up(expr->len, BITS_PER_BYTE);
+	mpz_switch_byteorder(expr->value, len);
+}
+
 static const struct expr_ops set_ref_expr_ops = {
 	.type		= EXPR_SET_REF,
 	.name		= "set reference",
diff --git a/src/gmputil.c b/src/gmputil.c
index cb46445..2d1ee3c 100644
--- a/src/gmputil.c
+++ b/src/gmputil.c
@@ -103,11 +103,11 @@ void *mpz_export_data(void *data, const mpz_t op,
 
 	switch (byteorder) {
 	case BYTEORDER_BIG_ENDIAN:
-	default:
 		order = MPZ_MSWF;
 		endian = MPZ_BIG_ENDIAN;
 		break;
 	case BYTEORDER_HOST_ENDIAN:
+	default:
 		order = MPZ_HWO;
 		endian = MPZ_HOST_ENDIAN;
 		break;
@@ -125,13 +125,18 @@ void mpz_import_data(mpz_t rop, const void *data,
 	enum mpz_word_order order;
 	enum mpz_byte_order endian;
 
+	/* This function is called from the parser. At that stage,
+	 * some constant values may still have BYTEORDER_INVALID. In
+	 * that case, fall back to BYTEORDER_HOST_ENDIAN which leave
+	 * it as is until this is passed to the evaluation step.
+	 */
 	switch (byteorder) {
 	case BYTEORDER_BIG_ENDIAN:
-	default:
 		order  = MPZ_MSWF;
 		endian = MPZ_BIG_ENDIAN;
 		break;
 	case BYTEORDER_HOST_ENDIAN:
+	default:
 		order  = MPZ_HWO;
 		endian = MPZ_HOST_ENDIAN;
 		break;
diff --git a/src/netlink.c b/src/netlink.c
index e149215..35ef4d8 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -239,11 +239,16 @@ static struct nft_set_elem *alloc_nft_setelem(const struct expr *expr)
 	return nlse;
 }
 
-void netlink_gen_raw_data(const mpz_t value, enum byteorder byteorder,
-			  unsigned int len, struct nft_data_linearize *data)
+void netlink_gen_raw_data(const mpz_t value, unsigned int len,
+			  struct nft_data_linearize *data)
 {
 	assert(len > 0);
-	mpz_export_data(data->value, value, byteorder, len);
+
+	/* After the evaluation step we assume that the values are always in
+	 * the appropriate byteorder. Use BYTEORDER_HOST_ENDIAN here not to
+	 * alter endianness.
+	 */
+	mpz_export_data(data->value, value, BYTEORDER_HOST_ENDIAN, len);
 	data->len = len;
 }
 
@@ -277,7 +282,7 @@ static void netlink_gen_constant_data(const struct expr *expr,
 				      struct nft_data_linearize *data)
 {
 	assert(expr->ops->type == EXPR_VALUE);
-	netlink_gen_raw_data(expr->value, expr->byteorder,
+	netlink_gen_raw_data(expr->value,
 			     div_round_up(expr->len, BITS_PER_BYTE), data);
 }
 
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 5c6ca80..1035e32 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -642,14 +642,20 @@ static void payload_match_postprocess(struct rule_pp_ctx *ctx,
 		list_for_each_entry(left, &list, list) {
 			tmp = constant_expr_splice(right, left->len);
 			expr_set_type(tmp, left->dtype, left->byteorder);
-			if (tmp->byteorder == BYTEORDER_HOST_ENDIAN)
-				mpz_switch_byteorder(tmp->value, tmp->len / BITS_PER_BYTE);
 
 			nexpr = relational_expr_alloc(&expr->location, expr->op,
 						      left, tmp);
 			if (expr->op == OP_EQ)
 				left->ops->pctx_update(&ctx->pctx, nexpr);
 
+			/* We have to convert the values that we obtained from
+			 * the kernel to host byteorder. Therefore, we assume
+			 * that the values are in host endian after the
+			 * delinearization.
+			 */
+			if (tmp->byteorder == BYTEORDER_BIG_ENDIAN)
+				expr_switch_byteorder(nexpr->right);
+
 			nstmt = expr_stmt_alloc(&stmt->location, nexpr);
 			list_add_tail(&nstmt->list, &stmt->list);
 
@@ -831,8 +837,10 @@ static void expr_postprocess(struct rule_pp_ctx *ctx,
 		payload_dependency_kill(ctx, expr);
 		break;
 	case EXPR_VALUE:
-		// FIXME
-		if (expr->byteorder == BYTEORDER_HOST_ENDIAN)
+		/* Everything has to be in host byteorder after the
+		 * delinearization step.
+		 */
+		if (expr->byteorder == BYTEORDER_BIG_ENDIAN)
 			mpz_switch_byteorder(expr->value, expr->len / BITS_PER_BYTE);
 
 		// Quite a hack :)
diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index 5c1b46d..8788813 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -206,8 +206,8 @@ static void netlink_gen_cmp(struct netlink_linearize_ctx *ctx,
 
 		mpz_init(mask);
 		mpz_prefixmask(mask, expr->right->len, expr->right->prefix_len);
-		netlink_gen_raw_data(mask, expr->right->byteorder,
-				     expr->right->len / BITS_PER_BYTE, &nld);
+		netlink_gen_raw_data(mask, expr->right->len / BITS_PER_BYTE,
+				     &nld);
 		mpz_clear(mask);
 
 		zero.len = nld.len;
@@ -315,7 +315,7 @@ static void netlink_gen_flagcmp(struct netlink_linearize_ctx *ctx,
 
 	mpz_init_set_ui(zero, 0);
 
-	netlink_gen_raw_data(zero, expr->right->byteorder, len, &nld);
+	netlink_gen_raw_data(zero, len, &nld);
 	netlink_gen_data(expr->right, &nld2);
 
 	nle = alloc_nft_expr("bitwise");
@@ -423,9 +423,9 @@ static void netlink_gen_binop(struct netlink_linearize_ctx *ctx,
 	nft_rule_expr_set_u32(nle, NFT_EXPR_BITWISE_DREG, dreg);
 	nft_rule_expr_set_u32(nle, NFT_EXPR_BITWISE_LEN, len);
 
-	netlink_gen_raw_data(mask, expr->byteorder, len, &nld);
+	netlink_gen_raw_data(mask, len, &nld);
 	nft_rule_expr_set(nle, NFT_EXPR_BITWISE_MASK, nld.value, nld.len);
-	netlink_gen_raw_data(xor, expr->byteorder, len, &nld);
+	netlink_gen_raw_data(xor, len, &nld);
 	nft_rule_expr_set(nle, NFT_EXPR_BITWISE_XOR, nld.value, nld.len);
 
 	mpz_clear(tmp);
diff --git a/src/proto.c b/src/proto.c
index e5f49cb..b8eb6f7 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -190,7 +190,7 @@ void proto_ctx_update(struct proto_ctx *ctx, enum proto_bases base,
 			   field_sizeof(__type, __member) * 8)
 
 #define HDR_FIELD(__name, __struct, __member)				\
-	HDR_TEMPLATE(__name, &integer_type, __struct, __member)
+	HDR_TEMPLATE(__name, &big_endian_integer_type, __struct, __member)
 #define HDR_BITFIELD(__name, __dtype,  __offset, __len)			\
 	PROTO_HDR_TEMPLATE(__name, __dtype, __offset, __len)
 #define HDR_TYPE(__name, __dtype, __struct, __member)			\
-- 
1.7.10.4

[nft PATCH 4/5 v2] src: fix byteorder conversions in range values

[Alvaro Neira Ayuso]

Currently when we try to use range values in nft rules doesn't
work correctly. Usually this problem is related to incorrect byteorder
conversion. I make the following solution for showing the range in
the correct byteorder.

Example:
* nft add rule filter input tcp checksum 22-55
* nft list table filter
    tcp checksum >= 5632 tcp checksum <= 14080

And now, if we show it:
* nft add rule filter input tcp checksum 22-55
* nft list table filter
    tcp checksum >= 22 tcp checksum <= 55

Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
---
[changes in v2]
* Changed the solution for big endian and host endian cases.

 src/netlink_delinearize.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 1035e32..af18dcc 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -627,6 +627,17 @@ static void payload_dependency_store(struct rule_pp_ctx *ctx,
 	ctx->pdep  = stmt;
 }
 
+static void payload_elem_postprocess(struct expr *expr)
+{
+	switch (expr->ops->type) {
+	case EXPR_VALUE:
+		expr_switch_byteorder(expr);
+		break;
+	default:
+		break;
+	}
+}
+
 static void payload_match_postprocess(struct rule_pp_ctx *ctx,
 				      struct stmt *stmt, struct expr *expr)
 {
@@ -677,6 +688,14 @@ static void payload_match_postprocess(struct rule_pp_ctx *ctx,
 		payload_expr_complete(left, &ctx->pctx);
 		expr_set_type(expr->right, expr->left->dtype,
 			      expr->left->byteorder);
+
+		/* If we have rules that we have used payload with ranges or set
+		 * we must to convert it to host endian for representing it
+		 * correctly
+		 */
+		if (left->dtype->byteorder == BYTEORDER_BIG_ENDIAN)
+			payload_elem_postprocess(expr->right);
+
 		payload_dependency_kill(ctx, expr->left);
 		break;
 	}
-- 
1.7.10.4

[nft PATCH 5/5 v2] src: fix byteorder conversions in sets

[Alvaro Neira Ayuso]

In some rules if we use sets, we don't convert the values inside the set.
Usually, rule like the datatype is integer_type. For example:

nft add rule filter input tcp checksum {22-55}

set%d filter 7
set%d filter 0
element 00000000:1 [end] element 00000016: 0 [end] element 00000038  : 1 [end]
ip filter input
[ payload load 1b @ network header + 9 => reg 1 ]
[ cmp eq reg 1 0x00000006 ]
[ payload load 2b @ transport header + 16 => reg 1 ]
[ lookup reg 1 set set%d ]

Currently, we are going to do the byteorder conversion the values inside the set
like:

set%d filter 7
set%d filter 0
element 00000000 : 1 [end] element 00001600 : 0 [end] element 00003701: 1 [end]
ip filter input
[ payload load 1b @ network header + 9 => reg 1 ]
[ cmp eq reg 1 0x00000006 ]
[ payload load 2b @ transport header + 16 => reg 1 ]
[ lookup reg 1 set set%d ]

Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
---
[changes in v2]
* Changed the solution for big endian and host endian cases.

 src/evaluate.c            |   32 +++++++++++++++++++++++++++-----
 src/netlink_delinearize.c |   12 ++++++++++++
 2 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 8aaf1bf..d973cb8 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -670,6 +670,29 @@ static int expr_evaluate_list(struct eval_ctx *ctx, struct expr **expr)
 	return 0;
 }
 
+static int expr_evaluate_set_elem(struct eval_ctx *ctx, struct expr *expr)
+{
+	switch (expr->ops->type) {
+	case EXPR_VALUE:
+		if (byteorder_conversion(ctx, &expr,
+					 BYTEORDER_BIG_ENDIAN) < 0)
+			return -1;
+		break;
+	case EXPR_RANGE:
+		if (byteorder_conversion(ctx, &expr->right,
+					 BYTEORDER_BIG_ENDIAN) < 0)
+			return -1;
+		if (byteorder_conversion(ctx, &expr->left,
+					 BYTEORDER_BIG_ENDIAN) < 0)
+			return -1;
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 static int expr_evaluate_set(struct eval_ctx *ctx, struct expr **expr)
 {
 	struct expr *set = *expr, *i, *next;
@@ -691,6 +714,10 @@ static int expr_evaluate_set(struct eval_ctx *ctx, struct expr **expr)
 			expr_free(i);
 		} else if (!expr_is_singleton(i))
 			set->set_flags |= SET_F_INTERVAL;
+
+		/* Byteorder conversion of the set elements */
+		if (i->ops->type != EXPR_SET)
+			expr_evaluate_set_elem(ctx, i);
 	}
 
 	set->dtype = ctx->ectx.dtype;
@@ -927,11 +954,6 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 						 left->dtype->desc,
 						 right->dtype->desc);
 
-		/* Data for range lookups needs to be in big endian order */
-		if (right->set->flags & SET_F_INTERVAL &&
-		    byteorder_conversion(ctx, &rel->left,
-					 BYTEORDER_BIG_ENDIAN) < 0)
-			return -1;
 		left = rel->left;
 		break;
 	case OP_EQ:
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index af18dcc..f7961ad 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -629,10 +629,20 @@ static void payload_dependency_store(struct rule_pp_ctx *ctx,
 
 static void payload_elem_postprocess(struct expr *expr)
 {
+	struct expr *i;
+
 	switch (expr->ops->type) {
 	case EXPR_VALUE:
 		expr_switch_byteorder(expr);
 		break;
+	case EXPR_SET_REF:
+		list_for_each_entry(i, &expr->set->init->expressions, list)
+			payload_elem_postprocess(i);
+		break;
+	case EXPR_RANGE:
+		payload_elem_postprocess(expr->right);
+		payload_elem_postprocess(expr->left);
+		break;
 	default:
 		break;
 	}
@@ -889,6 +899,8 @@ static void expr_postprocess(struct rule_pp_ctx *ctx,
 		expr_postprocess(ctx, stmt, &expr->right);
 		break;
 	case EXPR_SET_REF:
+		expr_postprocess(ctx, stmt, &expr->set->init);
+		break;
 	case EXPR_EXTHDR:
 	case EXPR_META:
 	case EXPR_CT:
-- 
1.7.10.4

Re: [nft PATCH 5/5 v2] src: fix byteorder conversions in sets

[Patrick McHardy]

On 4. August 2014 17:00:11 GMT+01:00, Alvaro Neira Ayuso <alvaroneay@gmail.com> wrote:
>In some rules if we use sets, we don't convert the values inside the
>set.
>Usually, rule like the datatype is integer_type. For example:
>
>nft add rule filter input tcp checksum {22-55}
>
>set%d filter 7
>set%d filter 0
>element 00000000:1 [end] element 00000016: 0 [end] element 00000038  :
>1 [end]
>ip filter input
>[ payload load 1b @ network header + 9 => reg 1 ]
>[ cmp eq reg 1 0x00000006 ]
>[ payload load 2b @ transport header + 16 => reg 1 ]
>[ lookup reg 1 set set%d ]
>
>Currently, we are going to do the byteorder conversion the values
>inside the set
>like:
>
>set%d filter 7
>set%d filter 0
>element 00000000 : 1 [end] element 00001600 : 0 [end] element 00003701:
>1 [end]
>ip filter input
>[ payload load 1b @ network header + 9 => reg 1 ]
>[ cmp eq reg 1 0x00000006 ]
>[ payload load 2b @ transport header + 16 => reg 1 ]
>[ lookup reg 1 set set%d ]
>
>Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
>---
>[changes in v2]
>* Changed the solution for big endian and host endian cases.

I don't unterstand the problem, you seem to be converting values independently of wether we have a range lookup or not.

Could you please specify a case where we are currently not converting values when necessary?



> src/evaluate.c            |   32 +++++++++++++++++++++++++++-----
> src/netlink_delinearize.c |   12 ++++++++++++
> 2 files changed, 39 insertions(+), 5 deletions(-)
>
>diff --git a/src/evaluate.c b/src/evaluate.c
>index 8aaf1bf..d973cb8 100644
>--- a/src/evaluate.c
>+++ b/src/evaluate.c
>@@ -670,6 +670,29 @@ static int expr_evaluate_list(struct eval_ctx
>*ctx, struct expr **expr)
> 	return 0;
> }
> 
>+static int expr_evaluate_set_elem(struct eval_ctx *ctx, struct expr
>*expr)
>+{
>+	switch (expr->ops->type) {
>+	case EXPR_VALUE:
>+		if (byteorder_conversion(ctx, &expr,
>+					 BYTEORDER_BIG_ENDIAN) < 0)
>+			return -1;
>+		break;
>+	case EXPR_RANGE:
>+		if (byteorder_conversion(ctx, &expr->right,
>+					 BYTEORDER_BIG_ENDIAN) < 0)
>+			return -1;
>+		if (byteorder_conversion(ctx, &expr->left,
>+					 BYTEORDER_BIG_ENDIAN) < 0)
>+			return -1;
>+		break;
>+	default:
>+		break;
>+	}
>+
>+	return 0;
>+}
>+
> static int expr_evaluate_set(struct eval_ctx *ctx, struct expr **expr)
> {
> 	struct expr *set = *expr, *i, *next;
>@@ -691,6 +714,10 @@ static int expr_evaluate_set(struct eval_ctx *ctx,
>struct expr **expr)
> 			expr_free(i);
> 		} else if (!expr_is_singleton(i))
> 			set->set_flags |= SET_F_INTERVAL;
>+
>+		/* Byteorder conversion of the set elements */
>+		if (i->ops->type != EXPR_SET)
>+			expr_evaluate_set_elem(ctx, i);
> 	}
> 
> 	set->dtype = ctx->ectx.dtype;
>@@ -927,11 +954,6 @@ static int expr_evaluate_relational(struct
>eval_ctx *ctx, struct expr **expr)
> 						 left->dtype->desc,
> 						 right->dtype->desc);
> 
>-		/* Data for range lookups needs to be in big endian order */
>-		if (right->set->flags & SET_F_INTERVAL &&
>-		    byteorder_conversion(ctx, &rel->left,
>-					 BYTEORDER_BIG_ENDIAN) < 0)
>-			return -1;
> 		left = rel->left;
> 		break;
> 	case OP_EQ:
>diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
>index af18dcc..f7961ad 100644
>--- a/src/netlink_delinearize.c
>+++ b/src/netlink_delinearize.c
>@@ -629,10 +629,20 @@ static void payload_dependency_store(struct
>rule_pp_ctx *ctx,
> 
> static void payload_elem_postprocess(struct expr *expr)
> {
>+	struct expr *i;
>+
> 	switch (expr->ops->type) {
> 	case EXPR_VALUE:
> 		expr_switch_byteorder(expr);
> 		break;
>+	case EXPR_SET_REF:
>+		list_for_each_entry(i, &expr->set->init->expressions, list)
>+			payload_elem_postprocess(i);
>+		break;
>+	case EXPR_RANGE:
>+		payload_elem_postprocess(expr->right);
>+		payload_elem_postprocess(expr->left);
>+		break;
> 	default:
> 		break;
> 	}
>@@ -889,6 +899,8 @@ static void expr_postprocess(struct rule_pp_ctx
>*ctx,
> 		expr_postprocess(ctx, stmt, &expr->right);
> 		break;
> 	case EXPR_SET_REF:
>+		expr_postprocess(ctx, stmt, &expr->set->init);
>+		break;
> 	case EXPR_EXTHDR:
> 	case EXPR_META:
> 	case EXPR_CT:

Re: [nft PATCH 1/5] payload: fix update context with wrong byteorder

[Patrick McHardy]

On Mon, Aug 04, 2014 at 06:00:07PM +0200, Alvaro Neira Ayuso wrote:
> In the evaluation and delinealize steps, we update the protocol
> context. The expression must be in host endian byteorder when we update the
> context. However, this is not the case because we see them in network
> byteorder.
> 
> Fix this by converting to the appropriate byteorder before updating
> the protocol context otherwise this doesn't work.
> 
> nft add rule bridge filter input ether type ip
> 
> We have a expression like this:
> 
> [ payload load 2b @ link header + 12 => reg 1 ]
>   [ cmp eq reg 1 0x00000008 ]
> 
> The byteorder of this expressions is big endian and it's in
> host endian, for that when we try to update the context, we
> don't find the protocol with this number. This is a output,
> example:
> 
> update network layer protocol context:
>  link layer          : ether
>  network layer       : none <-
>  transport layer     : none

So after looking into this, the problem seems really simple, the ETH_P_*
values are in host byteorder and we incorrectly mark them as being in
network byteorder, so they are not converted automatically.

Could you please test whether this fixes the problem you're seeing?

diff --git a/src/proto.c b/src/proto.c
index e5f49cb..050dd90 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -772,7 +779,7 @@ const struct datatype ethertype_type = {
 	.type		= TYPE_ETHERTYPE,
 	.name		= "ether_type",
 	.desc		= "Ethernet protocol",
-	.byteorder	= BYTEORDER_BIG_ENDIAN,
+	.byteorder	= BYTEORDER_HOST_ENDIAN,
 	.size		= 2 * BITS_PER_BYTE,
 	.basetype	= &integer_type,
 	.basefmt	= "0x%.4Zx",

Re: [nft PATCH 2/5] payload: generate dependency in the correct byteorder

[Patrick McHardy]

On Mon, Aug 04, 2014 at 06:00:08PM +0200, Alvaro Neira Ayuso wrote:
> Currently, we suppose that all the dependency are in host endian. The problem
> is if we try to add a dependency with big endian byteorder, we create
> this dependency wrong. For example, if we add a ether type dependency in bridge,
> we must use big endian byteorder and we use host endian and we create wrong the
> payload.
> 
> Now, we are going to create the dependency taking account the byteorder.

This seems correct to me.

> 
> Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
> ---
>  src/payload.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/src/payload.c b/src/payload.c
> index be3d610..8b10a79 100644
> --- a/src/payload.c
> +++ b/src/payload.c
> @@ -213,8 +213,7 @@ int payload_gen_dependency(struct eval_ctx *ctx, const struct expr *expr,
>  		left = payload_expr_alloc(&expr->location, desc, desc->protocol_key);
>  
>  	right = constant_expr_alloc(&expr->location, tmpl->dtype,
> -				    BYTEORDER_HOST_ENDIAN,
> -				    tmpl->len,
> +				    tmpl->dtype->byteorder, tmpl->len,
>  				    constant_data_ptr(protocol, tmpl->len));
>  
>  	dep = relational_expr_alloc(&expr->location, OP_EQ, left, right);
> -- 
> 1.7.10.4
> 

Re: [nft PATCH 3/5 v3] src: fix byteorder conversions in constant values

[Patrick McHardy]

On Mon, Aug 04, 2014 at 06:00:09PM +0200, Alvaro Neira Ayuso wrote:
> Currently the byteorder conversion happens in many places: the parser,
> the evaluation and the code generation steps. This a problem because
> the constant expression is converted several times into big endian
> byteorder at different steps.
> 
> With this patch, we don't change byteorder in the code generation
> step anymore because we assume that all expressions are in the
> appropriate byteorder after the parsing and the evaluation steps.
> This patch solves rules like:
> 
> nft add rule bridge filter input ether type ip
> 
> With this rule we create a expression like:
> 
> [ payload load 2b @ link header + 12 => reg 1 ]
>   [ cmp eq reg 1 0x00000800 ]
> 
> With this patch, we have a expresion with the correct conversion:
> 
> [ payload load 2b @ link header + 12 => reg 1 ]
>   [ cmp eq reg 1 0x00000008 ]
> 
> This is also a problem in the delinearize step because the expression
> are already converted to the correct byteorder and we change it again.

There conversions are actually only done in the evaluation phase. It usually
only invokes setting the value to the correct byteorder. During netlink
linearization, we don't convert byteorder, we export data in the correct
byteorder.

This patch seems to complicate things more than it simplifies them.

> Also this patch, add a new datatype integer_big_endian for fixing some case
> that we have use integer_type with a invalid byteorder when we must associate
> a big endian byteorder. This solution is based in a previously patch of Yuxuan
> Shui.

That is actually a valid problem. However I don't like the solution of adding
a new generic type that encodes the byteorder and additionally breaks the
assumption that type identifiers are unique.

The proper solutions seems to me to simply initialize to the correct byteorder
in the HDR* macros. I'll work on doing that.