Some comments below.
On Tue, Jul 29, 2014 at 07:09:55PM +0200, Alvaro Neira Ayuso wrote:
> Currently the byteorder conversion happens in the parser,
> the evaluation and the code generation steps. This is a problem
> because the expression which are already in big endian byteorder
> are converted again.
>
> 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. 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 correctly byteorder and we change it again.
>
> Signed-off-by: Alvaro Neira Ayuso <alvaroneay@xxxxxxxxx>
> ---
> [changes in v3]
> * Added removed line in byteorder_conversion function for changing the
> byteorder of the expression if we have done any conversion.
>
> include/gmputil.h | 1 +
> include/netlink.h | 3 +--
> src/datatype.c | 8 ++++++--
> src/evaluate.c | 7 +++++--
> src/expression.c | 8 ++++++--
> src/gmputil.c | 15 ++++++++++++++-
> src/netlink.c | 13 +++++++++----
> src/netlink_delinearize.c | 16 ++++++++++++----
> src/netlink_linearize.c | 10 +++++-----
> src/payload.c | 3 +--
> 10 files changed, 60 insertions(+), 24 deletions(-)
>
> diff --git a/include/gmputil.h b/include/gmputil.h
> index 63eb0ba..566d7d7 100644
> --- a/include/gmputil.h
> +++ b/include/gmputil.h
> @@ -48,4 +48,5 @@ extern void mpz_import_data(mpz_t rop, const void *data,
> unsigned int len);
> extern void mpz_switch_byteorder(mpz_t rop, unsigned int len);
>
> +extern void mpz_switch_expr_byteorder(struct expr *expr);
> #endif /* NFTABLES_GMPUTIL_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..6b49a4a 100644
> --- a/src/datatype.c
> +++ b/src/datatype.c
> @@ -255,8 +255,11 @@ static struct error_record *integer_type_parse(const struct expr *sym,
> sym->dtype->desc);
> }
>
> + /* If we use integer type, we must to convert it to big endian for
> + * using it in internet format (big endian).
> + */
> *res = constant_expr_alloc(&sym->location, sym->dtype,
> - BYTEORDER_HOST_ENDIAN, 1, NULL);
> + BYTEORDER_BIG_ENDIAN, 1, NULL);
I guess sym->dtype->byteorder still indicates invalid byteorder when
you call datatype_parse, right?
I'm not sure this is the right place to perform the byteorder
conversion. The integer type is generic and may be used (even if not
yet) from meta/ct which always use host byteorder.
What use case is fixing this chunk? The one that you describe in the
patch description above?
Can you find a way to do this from the evaluation step? The idea is to
check if the constant right-hand side has invalid byteorder. If so,
assign the left-hand side byteorder to the right-hand side.
> mpz_set((*res)->value, v);
> mpz_clear(v);
> return NULL;
> @@ -545,7 +548,7 @@ 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)
> @@ -553,6 +556,7 @@ static struct error_record *inet_service_type_parse(const struct expr *sym,
> gai_strerror(err));
>
> port = ((struct sockaddr_in *)ai->ai_addr)->sin_port;
> + port = htons(port);
getaddrinfo() returns the port in network byte order, so this has to
be ntohs().
> freeaddrinfo(ai);
> }
>
> diff --git a/src/evaluate.c b/src/evaluate.c
> index e05473a..efef80f 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -168,9 +168,12 @@ 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)) {
> + if ((*expr)->byteorder == BYTEORDER_BIG_ENDIAN)
> + mpz_switch_expr_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..d2c61f7 100644
> --- a/src/expression.c
> +++ b/src/expression.c
> @@ -303,8 +303,12 @@ 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,
> +
> + /* All the values here have been converted to the correctly byteorder
> + * we don't need to change it again for joining it
> + */
I suggested a similar comment in your previous patch, I think this
should be something like:
/* 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,
> diff --git a/src/gmputil.c b/src/gmputil.c
> index cb46445..cc1ceca 100644
> --- a/src/gmputil.c
> +++ b/src/gmputil.c
> @@ -20,6 +20,7 @@
> #include <datatype.h>
> #include <gmputil.h>
> #include <utils.h>
> +#include <expression.h>
>
> void mpz_bitmask(mpz_t rop, unsigned int width)
> {
> @@ -125,13 +126,17 @@ void mpz_import_data(mpz_t rop, const void *data,
> enum mpz_word_order order;
> enum mpz_byte_order endian;
>
> +/* We can import data from the parser, the datatype is invalid there.
> + * Handle invalid byteorder as host endian, we'll get the real datatype
> + * in the evaluation step, then it will be converted to big endian if needed.
> + */
The coding style is not right. You have to align comments with the
indented code, eg.
/* This is a comment...
* still more info ...
*/
switch (byteorder) {
...
On top of that, I'll reformulate the comment with:
/* 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.
*/
Please, let me know if this is in the spirit of what you wanted to
express.
> 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;
> @@ -140,6 +145,14 @@ void mpz_import_data(mpz_t rop, const void *data,
> mpz_import(rop, len, order, 1, endian, 0, data);
> }
>
> +void mpz_switch_expr_byteorder(struct expr *expr)
The name for this should be expr_switch_byteorder(...) since it is not
taking as input / returning as output any mpz_t type.
And I'm not sure this function belongs to gmputil.c, most likely
expression.c?
> +{
> + uint32_t len;
> +
> + len = div_round_up(expr->len, BITS_PER_BYTE);
> + mpz_switch_byteorder(expr->value, len);
> +}
> +
> void mpz_switch_byteorder(mpz_t rop, unsigned int len)
> {
> char data[len];
> 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..c45b3b4 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.
> + */
Fix wrong unaligned comment to indentation.
> + if (tmp->byteorder != BYTEORDER_HOST_ENDIAN)
Better tmp->byteorder == BYTEORDER_BIG_ENDIAN instead?
> + mpz_switch_expr_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)
> + /* If we have value with big endian byteorder, we must to change
> + * it for showing it in host endian byteorder.
> + */
Suggestion:
/* 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/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
>
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
