* [PATCH 0/2 nft] Introduce variables in chain priority and policy
@ 2019-07-22 16:02 Fernando Fernandez Mancera
2019-07-22 16:02 ` [PATCH 1/2 nft] src: allow variables in the chain priority specification Fernando Fernandez Mancera
2019-07-22 16:02 ` [PATCH 2/2 nft] src: allow variable in chain policy Fernando Fernandez Mancera
0 siblings, 2 replies; 7+ messages in thread
From: Fernando Fernandez Mancera @ 2019-07-22 16:02 UTC (permalink / raw)
To: netfilter-devel; +Cc: Fernando Fernandez Mancera
This patch series introduces the use of variables in chain priority and policy
specification. It also contains tests for invalid cases.
Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1172
Fernando Fernandez Mancera (2):
src: allow variables in the chain priority specification
src: allow variable in chain policy
include/datatype.h | 1 +
include/rule.h | 10 +-
src/datatype.c | 26 ++++++
src/evaluate.c | 92 +++++++++++++++++--
src/json.c | 5 +-
src/mnl.c | 9 +-
src/netlink.c | 8 +-
src/parser_bison.y | 45 +++++++--
src/parser_json.c | 17 +++-
src/rule.c | 17 +++-
.../testcases/nft-f/0021priority_variable_0 | 17 ++++
.../testcases/nft-f/0022priority_variable_0 | 17 ++++
.../testcases/nft-f/0023priority_variable_1 | 18 ++++
.../testcases/nft-f/0024priority_variable_1 | 18 ++++
.../testcases/nft-f/0025policy_variable_0 | 17 ++++
.../testcases/nft-f/0026policy_variable_0 | 17 ++++
.../testcases/nft-f/0027policy_variable_1 | 18 ++++
.../testcases/nft-f/0028policy_variable_1 | 18 ++++
.../nft-f/dumps/0021priority_variable_0.nft | 5 +
.../nft-f/dumps/0022priority_variable_0.nft | 5 +
.../nft-f/dumps/0025policy_variable_0.nft | 5 +
.../nft-f/dumps/0026policy_variable_0.nft | 5 +
22 files changed, 350 insertions(+), 40 deletions(-)
mode change 100644 => 100755 src/evaluate.c
create mode 100755 tests/shell/testcases/nft-f/0021priority_variable_0
create mode 100755 tests/shell/testcases/nft-f/0022priority_variable_0
create mode 100755 tests/shell/testcases/nft-f/0023priority_variable_1
create mode 100755 tests/shell/testcases/nft-f/0024priority_variable_1
create mode 100755 tests/shell/testcases/nft-f/0025policy_variable_0
create mode 100755 tests/shell/testcases/nft-f/0026policy_variable_0
create mode 100755 tests/shell/testcases/nft-f/0027policy_variable_1
create mode 100755 tests/shell/testcases/nft-f/0028policy_variable_1
create mode 100644 tests/shell/testcases/nft-f/dumps/0021priority_variable_0.nft
create mode 100644 tests/shell/testcases/nft-f/dumps/0022priority_variable_0.nft
create mode 100644 tests/shell/testcases/nft-f/dumps/0025policy_variable_0.nft
create mode 100644 tests/shell/testcases/nft-f/dumps/0026policy_variable_0.nft
--
2.20.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2 nft] src: allow variables in the chain priority specification
2019-07-22 16:02 [PATCH 0/2 nft] Introduce variables in chain priority and policy Fernando Fernandez Mancera
@ 2019-07-22 16:02 ` Fernando Fernandez Mancera
2019-07-25 10:39 ` Pablo Neira Ayuso
2019-07-22 16:02 ` [PATCH 2/2 nft] src: allow variable in chain policy Fernando Fernandez Mancera
1 sibling, 1 reply; 7+ messages in thread
From: Fernando Fernandez Mancera @ 2019-07-22 16:02 UTC (permalink / raw)
To: netfilter-devel; +Cc: Fernando Fernandez Mancera
This patch introduces the use of nft input files variables in chain priority
specification. e.g.
define pri = filter
define prinum = 10
add table ip foo
add chain ip foo bar {type filter hook input priority $pri;}
add chain ip foo ber {type filter hook input priority $prinum;}
add chain ip foo bor {type filter hook input priority $pri + 10;}
table ip foo {
chain bar {
type filter hook input priority filter; policy accept;
}
chain ber {
type filter hook input priority filter + 10; policy accept;
}
chain bor {
type filter hook input priority filter + 10; policy accept;
}
}
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
---
include/datatype.h | 1 +
include/rule.h | 8 ++--
src/datatype.c | 26 ++++++++++++
src/evaluate.c | 41 ++++++++++++++-----
src/parser_bison.y | 30 ++++++++++----
src/rule.c | 4 +-
.../testcases/nft-f/0021priority_variable_0 | 17 ++++++++
.../testcases/nft-f/0022priority_variable_0 | 17 ++++++++
.../testcases/nft-f/0023priority_variable_1 | 18 ++++++++
.../testcases/nft-f/0024priority_variable_1 | 18 ++++++++
.../nft-f/dumps/0021priority_variable_0.nft | 5 +++
.../nft-f/dumps/0022priority_variable_0.nft | 5 +++
12 files changed, 166 insertions(+), 24 deletions(-)
mode change 100644 => 100755 src/evaluate.c
create mode 100755 tests/shell/testcases/nft-f/0021priority_variable_0
create mode 100755 tests/shell/testcases/nft-f/0022priority_variable_0
create mode 100755 tests/shell/testcases/nft-f/0023priority_variable_1
create mode 100755 tests/shell/testcases/nft-f/0024priority_variable_1
create mode 100644 tests/shell/testcases/nft-f/dumps/0021priority_variable_0.nft
create mode 100644 tests/shell/testcases/nft-f/dumps/0022priority_variable_0.nft
diff --git a/include/datatype.h b/include/datatype.h
index 63617eb..1b012d5 100644
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -256,6 +256,7 @@ extern const struct datatype icmpx_code_type;
extern const struct datatype igmp_type_type;
extern const struct datatype time_type;
extern const struct datatype boolean_type;
+extern const struct datatype priority_type;
void inet_service_type_print(const struct expr *expr, struct output_ctx *octx);
diff --git a/include/rule.h b/include/rule.h
index 67c3d33..c6e8716 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -174,13 +174,13 @@ enum chain_flags {
* struct prio_spec - extendend priority specification for mixed
* textual/numerical parsing.
*
- * @str: name of the standard priority value
- * @num: Numerical value. This MUST contain the parsed value of str after
+ * @expr: expr of the standard priority value
+ * @num: Numerical value. This MUST contain the parsed value of expr after
* evaluation.
*/
struct prio_spec {
- const char *str;
- int num;
+ struct expr *expr;
+ int num;
struct location loc;
};
diff --git a/src/datatype.c b/src/datatype.c
index 6d6826e..b7418e7 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -1246,3 +1246,29 @@ const struct datatype boolean_type = {
.sym_tbl = &boolean_tbl,
.json = boolean_type_json,
};
+
+static struct error_record *priority_type_parse(const struct expr *sym,
+ struct expr **res)
+{
+ int num;
+
+ if (isdigit(sym->identifier[0])) {
+ num = atoi(sym->identifier);
+ *res = constant_expr_alloc(&sym->location, &integer_type,
+ BYTEORDER_HOST_ENDIAN,
+ sizeof(int) * BITS_PER_BYTE,
+ &num);
+ } else
+ *res = constant_expr_alloc(&sym->location, &string_type,
+ BYTEORDER_HOST_ENDIAN,
+ strlen(sym->identifier) *
+ BITS_PER_BYTE, sym->identifier);
+ return NULL;
+}
+
+const struct datatype priority_type = {
+ .type = TYPE_STRING,
+ .name = "priority",
+ .desc = "priority type",
+ .parse = priority_type_parse,
+};
diff --git a/src/evaluate.c b/src/evaluate.c
old mode 100644
new mode 100755
index 69b853f..d2faee8
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3193,15 +3193,36 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
return 0;
}
-static bool evaluate_priority(struct prio_spec *prio, int family, int hook)
+static bool evaluate_priority(struct eval_ctx *ctx, struct prio_spec *prio,
+ int family, int hook)
{
int priority;
+ char prio_str[NFT_NAME_MAXLEN];
/* A numeric value has been used to specify priority. */
- if (prio->str == NULL)
+ if (prio->expr == NULL)
return true;
- priority = std_prio_lookup(prio->str, family, hook);
+ ctx->ectx.dtype = &priority_type;
+ ctx->ectx.len = NFT_NAME_MAXLEN * BITS_PER_BYTE;
+ if (expr_evaluate(ctx, &prio->expr) < 0)
+ return false;
+ if (prio->expr->etype != EXPR_VALUE) {
+ expr_error(ctx->msgs, prio->expr, "%s is not a valid "
+ "priority expression", expr_name(prio->expr));
+ return false;
+ }
+
+ if (prio->expr->dtype->type == TYPE_INTEGER) {
+ mpz_export_data(&prio->num, prio->expr->value,
+ BYTEORDER_HOST_ENDIAN, sizeof(int));
+ return true;
+ }
+ mpz_export_data(prio_str, prio->expr->value,
+ BYTEORDER_HOST_ENDIAN,
+ NFT_NAME_MAXLEN);
+
+ priority = std_prio_lookup(prio_str, family, hook);
if (priority == NF_IP_PRI_LAST)
return false;
prio->num += priority;
@@ -3223,10 +3244,10 @@ static int flowtable_evaluate(struct eval_ctx *ctx, struct flowtable *ft)
if (ft->hooknum == NF_INET_NUMHOOKS)
return chain_error(ctx, ft, "invalid hook %s", ft->hookstr);
- if (!evaluate_priority(&ft->priority, NFPROTO_NETDEV, ft->hooknum))
+ if (!evaluate_priority(ctx, &ft->priority, NFPROTO_NETDEV, ft->hooknum))
return __stmt_binary_error(ctx, &ft->priority.loc, NULL,
- "'%s' is invalid priority.",
- ft->priority.str);
+ "invalid priority expression %s.",
+ expr_name(ft->priority.expr));
if (!ft->dev_expr)
return chain_error(ctx, ft, "Unbound flowtable not allowed (must specify devices)");
@@ -3422,11 +3443,11 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
return chain_error(ctx, chain, "invalid hook %s",
chain->hookstr);
- if (!evaluate_priority(&chain->priority, chain->handle.family,
- chain->hooknum))
+ if (!evaluate_priority(ctx, &chain->priority,
+ chain->handle.family, chain->hooknum))
return __stmt_binary_error(ctx, &chain->priority.loc, NULL,
- "'%s' is invalid priority in this context.",
- chain->priority.str);
+ "invalid priority expression %s in this context.",
+ expr_name(chain->priority.expr));
}
list_for_each_entry(rule, &chain->rules, list) {
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 53e6695..ed7bd89 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -636,8 +636,8 @@ int nft_lex(void *, void *, void *);
%type <stmt> meter_stmt meter_stmt_alloc flow_stmt_legacy_alloc
%destructor { stmt_free($$); } meter_stmt meter_stmt_alloc flow_stmt_legacy_alloc
-%type <expr> symbol_expr verdict_expr integer_expr variable_expr chain_expr
-%destructor { expr_free($$); } symbol_expr verdict_expr integer_expr variable_expr chain_expr
+%type <expr> symbol_expr verdict_expr integer_expr variable_expr chain_expr prio_expr
+%destructor { expr_free($$); } symbol_expr verdict_expr integer_expr variable_expr chain_expr prio_expr
%type <expr> primary_expr shift_expr and_expr
%destructor { expr_free($$); } primary_expr shift_expr and_expr
%type <expr> exclusive_or_expr inclusive_or_expr
@@ -1969,30 +1969,44 @@ extended_prio_name : OUT
| STRING
;
+prio_expr : variable_expr
+ {
+ datatype_set($1->sym->expr, &priority_type);
+ $$ = $1;
+ }
+ | extended_prio_name
+ {
+ $$ = constant_expr_alloc(&@$, &string_type,
+ BYTEORDER_HOST_ENDIAN,
+ strlen($1) *
+ BITS_PER_BYTE, $1);
+ }
+ ;
+
extended_prio_spec : int_num
{
struct prio_spec spec = {0};
spec.num = $1;
$$ = spec;
}
- | extended_prio_name
+ | prio_expr
{
struct prio_spec spec = {0};
- spec.str = $1;
+ spec.expr = $1;
$$ = spec;
}
- | extended_prio_name PLUS NUM
+ | prio_expr PLUS NUM
{
struct prio_spec spec = {0};
spec.num = $3;
- spec.str = $1;
+ spec.expr = $1;
$$ = spec;
}
- | extended_prio_name DASH NUM
+ | prio_expr DASH NUM
{
struct prio_spec spec = {0};
spec.num = -$3;
- spec.str = $1;
+ spec.expr = $1;
$$ = spec;
}
;
diff --git a/src/rule.c b/src/rule.c
index b957b45..23011d9 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -823,7 +823,7 @@ void chain_free(struct chain *chain)
xfree(chain->type);
if (chain->dev != NULL)
xfree(chain->dev);
- xfree(chain->priority.str);
+ expr_free(chain->priority.expr);
xfree(chain);
}
@@ -2049,7 +2049,7 @@ void flowtable_free(struct flowtable *flowtable)
if (--flowtable->refcnt > 0)
return;
handle_free(&flowtable->handle);
- xfree(flowtable->priority.str);
+ expr_free(flowtable->priority.expr);
xfree(flowtable);
}
diff --git a/tests/shell/testcases/nft-f/0021priority_variable_0 b/tests/shell/testcases/nft-f/0021priority_variable_0
new file mode 100755
index 0000000..2b143db
--- /dev/null
+++ b/tests/shell/testcases/nft-f/0021priority_variable_0
@@ -0,0 +1,17 @@
+#!/bin/bash
+
+# Tests use of variables in priority specification
+
+set -e
+
+RULESET="
+define pri = filter
+
+table inet global {
+ chain prerouting {
+ type filter hook prerouting priority \$pri
+ policy accept
+ }
+}"
+
+$NFT -f - <<< "$RULESET"
diff --git a/tests/shell/testcases/nft-f/0022priority_variable_0 b/tests/shell/testcases/nft-f/0022priority_variable_0
new file mode 100755
index 0000000..51bc5eb
--- /dev/null
+++ b/tests/shell/testcases/nft-f/0022priority_variable_0
@@ -0,0 +1,17 @@
+#!/bin/bash
+
+# Tests use of variables in priority specification
+
+set -e
+
+RULESET="
+define pri = 10
+
+table inet global {
+ chain prerouting {
+ type filter hook prerouting priority \$pri
+ policy accept
+ }
+}"
+
+$NFT -f - <<< "$RULESET"
diff --git a/tests/shell/testcases/nft-f/0023priority_variable_1 b/tests/shell/testcases/nft-f/0023priority_variable_1
new file mode 100755
index 0000000..eddaf5b
--- /dev/null
+++ b/tests/shell/testcases/nft-f/0023priority_variable_1
@@ -0,0 +1,18 @@
+#!/bin/bash
+
+# Tests use of variables in priority specification
+
+set -e
+
+RULESET="
+define pri = *
+
+table inet global {
+ chain prerouting {
+ type filter hook prerouting priority \$pri
+ policy accept
+ }
+}"
+
+$NFT -f - <<< "$RULESET" && exit 1
+exit 0
diff --git a/tests/shell/testcases/nft-f/0024priority_variable_1 b/tests/shell/testcases/nft-f/0024priority_variable_1
new file mode 100755
index 0000000..592cb56
--- /dev/null
+++ b/tests/shell/testcases/nft-f/0024priority_variable_1
@@ -0,0 +1,18 @@
+#!/bin/bash
+
+# Tests use of variables in priority specification
+
+set -e
+
+RULESET="
+define pri = { 127.0.0.1 }
+
+table inet global {
+ chain prerouting {
+ type filter hook prerouting priority \$pri
+ policy accept
+ }
+}"
+
+$NFT -f - <<< "$RULESET" && exit 1
+exit 0
diff --git a/tests/shell/testcases/nft-f/dumps/0021priority_variable_0.nft b/tests/shell/testcases/nft-f/dumps/0021priority_variable_0.nft
new file mode 100644
index 0000000..f409309
--- /dev/null
+++ b/tests/shell/testcases/nft-f/dumps/0021priority_variable_0.nft
@@ -0,0 +1,5 @@
+table inet global {
+ chain prerouting {
+ type filter hook prerouting priority filter; policy accept;
+ }
+}
diff --git a/tests/shell/testcases/nft-f/dumps/0022priority_variable_0.nft b/tests/shell/testcases/nft-f/dumps/0022priority_variable_0.nft
new file mode 100644
index 0000000..2e94459
--- /dev/null
+++ b/tests/shell/testcases/nft-f/dumps/0022priority_variable_0.nft
@@ -0,0 +1,5 @@
+table inet global {
+ chain prerouting {
+ type filter hook prerouting priority filter + 10; policy accept;
+ }
+}
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2 nft] src: allow variable in chain policy
2019-07-22 16:02 [PATCH 0/2 nft] Introduce variables in chain priority and policy Fernando Fernandez Mancera
2019-07-22 16:02 ` [PATCH 1/2 nft] src: allow variables in the chain priority specification Fernando Fernandez Mancera
@ 2019-07-22 16:02 ` Fernando Fernandez Mancera
2019-07-25 10:44 ` Pablo Neira Ayuso
1 sibling, 1 reply; 7+ messages in thread
From: Fernando Fernandez Mancera @ 2019-07-22 16:02 UTC (permalink / raw)
To: netfilter-devel; +Cc: Fernando Fernandez Mancera
This patch introduces the use of nft input files variables in chain policy.
e.g.
define default_policy = "accept"
add table ip foo
add chain ip foo bar {type filter hook input priority filter; policy $default_policy}
table ip foo {
chain bar {
type filter hook input priority filter; policy accept;
}
}
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
---
include/rule.h | 2 +-
src/evaluate.c | 51 +++++++++++++++++++
src/json.c | 5 +-
src/mnl.c | 9 ++--
src/netlink.c | 8 ++-
src/parser_bison.y | 19 +++++--
src/parser_json.c | 17 +++++--
src/rule.c | 13 +++--
.../testcases/nft-f/0025policy_variable_0 | 17 +++++++
.../testcases/nft-f/0026policy_variable_0 | 17 +++++++
.../testcases/nft-f/0027policy_variable_1 | 18 +++++++
.../testcases/nft-f/0028policy_variable_1 | 18 +++++++
.../nft-f/dumps/0025policy_variable_0.nft | 5 ++
.../nft-f/dumps/0026policy_variable_0.nft | 5 ++
14 files changed, 186 insertions(+), 18 deletions(-)
create mode 100755 tests/shell/testcases/nft-f/0025policy_variable_0
create mode 100755 tests/shell/testcases/nft-f/0026policy_variable_0
create mode 100755 tests/shell/testcases/nft-f/0027policy_variable_1
create mode 100755 tests/shell/testcases/nft-f/0028policy_variable_1
create mode 100644 tests/shell/testcases/nft-f/dumps/0025policy_variable_0.nft
create mode 100644 tests/shell/testcases/nft-f/dumps/0026policy_variable_0.nft
diff --git a/include/rule.h b/include/rule.h
index c6e8716..b12165a 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -209,7 +209,7 @@ struct chain {
const char *hookstr;
unsigned int hooknum;
struct prio_spec priority;
- int policy;
+ struct expr *policy;
const char *type;
const char *dev;
struct scope scope;
diff --git a/src/evaluate.c b/src/evaluate.c
index d2faee8..4d8bfbf 100755
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3415,6 +3415,52 @@ static uint32_t str2hooknum(uint32_t family, const char *hook)
return NF_INET_NUMHOOKS;
}
+static bool evaluate_policy(struct eval_ctx *ctx, struct expr **policy)
+{
+ char policy_str[NFT_NAME_MAXLEN];
+ struct location loc;
+ int policy_num;
+
+ ctx->ectx.len = NFT_NAME_MAXLEN * BITS_PER_BYTE;
+ if (expr_evaluate(ctx, policy) < 0)
+ return false;
+ if ((*policy)->etype != EXPR_VALUE) {
+ expr_error(ctx->msgs, *policy, "%s is not a valid "
+ "policy expression", expr_name(*policy));
+ return false;
+ }
+
+ if ((*policy)->dtype->type == TYPE_STRING) {
+ mpz_export_data(policy_str, (*policy)->value,
+ BYTEORDER_HOST_ENDIAN,
+ NFT_NAME_MAXLEN);
+ loc = (*policy)->location;
+ if (!strcmp(policy_str, "accept")) {
+ expr_free(*policy);
+ policy_num = NF_ACCEPT;
+ *policy = constant_expr_alloc(&loc,
+ &integer_type,
+ BYTEORDER_HOST_ENDIAN,
+ sizeof(int) * BITS_PER_BYTE,
+ &policy_num);
+ } else if (!strcmp(policy_str, "drop")) {
+ expr_free(*policy);
+ policy_num = NF_DROP;
+ *policy = constant_expr_alloc(&(*policy)->location,
+ &integer_type,
+ BYTEORDER_HOST_ENDIAN,
+ sizeof(int) * BITS_PER_BYTE,
+ &policy_num);
+ } else {
+ expr_error(ctx->msgs, *policy, "%s is not a valid policy"
+ " value", policy_str);
+ return false;
+ }
+ }
+
+ return true;
+}
+
static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
{
struct table *table;
@@ -3448,6 +3494,11 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
return __stmt_binary_error(ctx, &chain->priority.loc, NULL,
"invalid priority expression %s in this context.",
expr_name(chain->priority.expr));
+ if (chain->policy) {
+ if (!evaluate_policy(ctx, &chain->policy))
+ return chain_error(ctx, chain, "invalid policy expression %s",
+ expr_name(chain->policy));
+ }
}
list_for_each_entry(rule, &chain->rules, list) {
diff --git a/src/json.c b/src/json.c
index 33e0ec1..ff2e06e 100644
--- a/src/json.c
+++ b/src/json.c
@@ -223,6 +223,7 @@ static json_t *rule_print_json(struct output_ctx *octx,
static json_t *chain_print_json(const struct chain *chain)
{
json_t *root, *tmp;
+ int policy;
root = json_pack("{s:s, s:s, s:s, s:I}",
"family", family2str(chain->handle.family),
@@ -231,12 +232,14 @@ static json_t *chain_print_json(const struct chain *chain)
"handle", chain->handle.handle.id);
if (chain->flags & CHAIN_F_BASECHAIN) {
+ mpz_export_data(&policy, chain->policy->value,
+ BYTEORDER_HOST_ENDIAN, sizeof(int));
tmp = json_pack("{s:s, s:s, s:i, s:s}",
"type", chain->type,
"hook", hooknum2str(chain->handle.family,
chain->hooknum),
"prio", chain->priority.num,
- "policy", chain_policy2str(chain->policy));
+ "policy", chain_policy2str(policy));
if (chain->dev)
json_object_set_new(tmp, "dev", json_string(chain->dev));
json_object_update(root, tmp);
diff --git a/src/mnl.c b/src/mnl.c
index eab8d54..ab71a00 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -518,6 +518,7 @@ int mnl_nft_chain_add(struct netlink_ctx *ctx, const struct cmd *cmd,
{
struct nftnl_chain *nlc;
struct nlmsghdr *nlh;
+ int policy;
nlc = nftnl_chain_alloc();
if (nlc == NULL)
@@ -536,9 +537,11 @@ int mnl_nft_chain_add(struct netlink_ctx *ctx, const struct cmd *cmd,
nftnl_chain_set_str(nlc, NFTNL_CHAIN_TYPE,
cmd->chain->type);
}
- if (cmd->chain->policy != -1)
- nftnl_chain_set_u32(nlc, NFTNL_CHAIN_POLICY,
- cmd->chain->policy);
+ if (cmd->chain->policy) {
+ mpz_export_data(&policy, cmd->chain->policy->value,
+ BYTEORDER_HOST_ENDIAN, sizeof(int));
+ nftnl_chain_set_u32(nlc, NFTNL_CHAIN_POLICY, policy);
+ }
if (cmd->chain->dev != NULL)
nftnl_chain_set_str(nlc, NFTNL_CHAIN_DEV,
cmd->chain->dev);
diff --git a/src/netlink.c b/src/netlink.c
index 14b0df4..d2be63c 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -369,6 +369,7 @@ struct chain *netlink_delinearize_chain(struct netlink_ctx *ctx,
const struct nftnl_chain *nlc)
{
struct chain *chain;
+ int policy;
chain = chain_alloc(nftnl_chain_get_str(nlc, NFTNL_CHAIN_NAME));
chain->handle.family =
@@ -390,7 +391,12 @@ struct chain *netlink_delinearize_chain(struct netlink_ctx *ctx,
nftnl_chain_get_s32(nlc, NFTNL_CHAIN_PRIO);
chain->type =
xstrdup(nftnl_chain_get_str(nlc, NFTNL_CHAIN_TYPE));
- chain->policy =
+ policy = nftnl_chain_get_u32(nlc, NFTNL_CHAIN_POLICY);
+ chain->policy = constant_expr_alloc(&netlink_location,
+ &string_type,
+ BYTEORDER_HOST_ENDIAN,
+ sizeof(int) * BITS_PER_BYTE,
+ &policy);
nftnl_chain_get_u32(nlc, NFTNL_CHAIN_POLICY);
if (nftnl_chain_is_set(nlc, NFTNL_CHAIN_DEV)) {
chain->dev =
diff --git a/src/parser_bison.y b/src/parser_bison.y
index ed7bd89..2a7cc9f 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -636,8 +636,8 @@ int nft_lex(void *, void *, void *);
%type <stmt> meter_stmt meter_stmt_alloc flow_stmt_legacy_alloc
%destructor { stmt_free($$); } meter_stmt meter_stmt_alloc flow_stmt_legacy_alloc
-%type <expr> symbol_expr verdict_expr integer_expr variable_expr chain_expr prio_expr
-%destructor { expr_free($$); } symbol_expr verdict_expr integer_expr variable_expr chain_expr prio_expr
+%type <expr> symbol_expr verdict_expr integer_expr variable_expr chain_expr prio_expr policy_expr
+%destructor { expr_free($$); } symbol_expr verdict_expr integer_expr variable_expr chain_expr prio_expr policy_expr
%type <expr> primary_expr shift_expr and_expr
%destructor { expr_free($$); } primary_expr shift_expr and_expr
%type <expr> exclusive_or_expr inclusive_or_expr
@@ -2019,17 +2019,28 @@ dev_spec : DEVICE string { $$ = $2; }
| /* empty */ { $$ = NULL; }
;
-policy_spec : POLICY chain_policy
+policy_spec : POLICY policy_expr
{
- if ($<chain>0->policy != -1) {
+ if ($<chain>0->policy) {
erec_queue(error(&@$, "you cannot set chain policy twice"),
state->msgs);
+ expr_free($2);
YYERROR;
}
$<chain>0->policy = $2;
}
;
+policy_expr : variable_expr
+ | chain_policy
+ {
+ $$ = constant_expr_alloc(&@$, &integer_type,
+ BYTEORDER_HOST_ENDIAN,
+ sizeof(int) *
+ BITS_PER_BYTE, &$1);
+ }
+ ;
+
chain_policy : ACCEPT { $$ = NF_ACCEPT; }
| DROP { $$ = NF_DROP; }
;
diff --git a/src/parser_json.c b/src/parser_json.c
index 76c0a5c..a227b17 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -2530,13 +2530,20 @@ static struct cmd *json_parse_cmd_add_table(struct json_ctx *ctx, json_t *root,
return cmd_alloc(op, obj, &h, int_loc, NULL);
}
-static int parse_policy(const char *policy)
+static struct expr *parse_policy(const char *policy)
{
+ int policy_num;
+
if (!strcmp(policy, "accept"))
- return NF_ACCEPT;
- if (!strcmp(policy, "drop"))
- return NF_DROP;
- return -1;
+ policy_num = NF_ACCEPT;
+ else if (!strcmp(policy, "drop"))
+ policy_num = NF_DROP;
+ else
+ return NULL;
+
+ return constant_expr_alloc(int_loc, &integer_type,
+ BYTEORDER_HOST_ENDIAN, sizeof(int)
+ * BITS_PER_BYTE, &policy_num);
}
static struct cmd *json_parse_cmd_add_chain(struct json_ctx *ctx, json_t *root,
diff --git a/src/rule.c b/src/rule.c
index 23011d9..afa46e9 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -800,7 +800,7 @@ struct chain *chain_alloc(const char *name)
if (name != NULL)
chain->handle.chain.name = xstrdup(name);
- chain->policy = -1;
+ chain->policy = NULL;
return chain;
}
@@ -824,6 +824,7 @@ void chain_free(struct chain *chain)
if (chain->dev != NULL)
xfree(chain->dev);
expr_free(chain->priority.expr);
+ expr_free(chain->policy);
xfree(chain);
}
@@ -1099,12 +1100,15 @@ static void chain_print_declaration(const struct chain *chain,
struct output_ctx *octx)
{
char priobuf[STD_PRIO_BUFSIZE];
+ int policy;
nft_print(octx, "\tchain %s {", chain->handle.chain.name);
if (nft_output_handle(octx))
nft_print(octx, " # handle %" PRIu64, chain->handle.handle.id);
nft_print(octx, "\n");
if (chain->flags & CHAIN_F_BASECHAIN) {
+ mpz_export_data(&policy, chain->policy->value,
+ BYTEORDER_HOST_ENDIAN, sizeof(int));
nft_print(octx, "\t\ttype %s hook %s", chain->type,
hooknum2str(chain->handle.family, chain->hooknum));
if (chain->dev != NULL)
@@ -1113,7 +1117,7 @@ static void chain_print_declaration(const struct chain *chain,
prio2str(octx, priobuf, sizeof(priobuf),
chain->handle.family, chain->hooknum,
chain->priority.num),
- chain_policy2str(chain->policy));
+ chain_policy2str(policy));
}
}
@@ -1134,17 +1138,20 @@ static void chain_print(const struct chain *chain, struct output_ctx *octx)
void chain_print_plain(const struct chain *chain, struct output_ctx *octx)
{
char priobuf[STD_PRIO_BUFSIZE];
+ int policy;
nft_print(octx, "chain %s %s %s", family2str(chain->handle.family),
chain->handle.table.name, chain->handle.chain.name);
if (chain->flags & CHAIN_F_BASECHAIN) {
+ mpz_export_data(&policy, chain->policy->value,
+ BYTEORDER_HOST_ENDIAN, 4);
nft_print(octx, " { type %s hook %s priority %s; policy %s; }",
chain->type, chain->hookstr,
prio2str(octx, priobuf, sizeof(priobuf),
chain->handle.family, chain->hooknum,
chain->priority.num),
- chain_policy2str(chain->policy));
+ chain_policy2str(policy));
}
if (nft_output_handle(octx))
nft_print(octx, " # handle %" PRIu64, chain->handle.handle.id);
diff --git a/tests/shell/testcases/nft-f/0025policy_variable_0 b/tests/shell/testcases/nft-f/0025policy_variable_0
new file mode 100755
index 0000000..b88e968
--- /dev/null
+++ b/tests/shell/testcases/nft-f/0025policy_variable_0
@@ -0,0 +1,17 @@
+#!/bin/bash
+
+# Tests use of variables in chain policy
+
+set -e
+
+RULESET="
+define default_policy = \"accept\"
+
+table inet global {
+ chain prerouting {
+ type filter hook prerouting priority filter
+ policy \$default_policy
+ }
+}"
+
+$NFT -f - <<< "$RULESET"
diff --git a/tests/shell/testcases/nft-f/0026policy_variable_0 b/tests/shell/testcases/nft-f/0026policy_variable_0
new file mode 100755
index 0000000..d4d98ed
--- /dev/null
+++ b/tests/shell/testcases/nft-f/0026policy_variable_0
@@ -0,0 +1,17 @@
+#!/bin/bash
+
+# Tests use of variables in chain policy
+
+set -e
+
+RULESET="
+define default_policy = \"drop\"
+
+table inet global {
+ chain prerouting {
+ type filter hook prerouting priority filter
+ policy \$default_policy
+ }
+}"
+
+$NFT -f - <<< "$RULESET"
diff --git a/tests/shell/testcases/nft-f/0027policy_variable_1 b/tests/shell/testcases/nft-f/0027policy_variable_1
new file mode 100755
index 0000000..ae35516
--- /dev/null
+++ b/tests/shell/testcases/nft-f/0027policy_variable_1
@@ -0,0 +1,18 @@
+#!/bin/bash
+
+# Tests use of variables in chain policy
+
+set -e
+
+RULESET="
+define default_policy = { 127.0.0.1 }
+
+table inet global {
+ chain prerouting {
+ type filter hook prerouting priority filter
+ policy \$default_policy
+ }
+}"
+
+$NFT -f - <<< "$RULESET" && exit 1
+exit 0
diff --git a/tests/shell/testcases/nft-f/0028policy_variable_1 b/tests/shell/testcases/nft-f/0028policy_variable_1
new file mode 100755
index 0000000..027eb01
--- /dev/null
+++ b/tests/shell/testcases/nft-f/0028policy_variable_1
@@ -0,0 +1,18 @@
+#!/bin/bash
+
+# Tests use of variables in priority specification
+
+set -e
+
+RULESET="
+define default_policy = *
+
+table inet global {
+ chain prerouting {
+ type filter hook prerouting priority filter
+ policy \$default_policy
+ }
+}"
+
+$NFT -f - <<< "$RULESET" && exit 1
+exit 0
diff --git a/tests/shell/testcases/nft-f/dumps/0025policy_variable_0.nft b/tests/shell/testcases/nft-f/dumps/0025policy_variable_0.nft
new file mode 100644
index 0000000..f409309
--- /dev/null
+++ b/tests/shell/testcases/nft-f/dumps/0025policy_variable_0.nft
@@ -0,0 +1,5 @@
+table inet global {
+ chain prerouting {
+ type filter hook prerouting priority filter; policy accept;
+ }
+}
diff --git a/tests/shell/testcases/nft-f/dumps/0026policy_variable_0.nft b/tests/shell/testcases/nft-f/dumps/0026policy_variable_0.nft
new file mode 100644
index 0000000..d729e1e
--- /dev/null
+++ b/tests/shell/testcases/nft-f/dumps/0026policy_variable_0.nft
@@ -0,0 +1,5 @@
+table inet global {
+ chain prerouting {
+ type filter hook prerouting priority filter; policy drop;
+ }
+}
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2 nft] src: allow variables in the chain priority specification
2019-07-22 16:02 ` [PATCH 1/2 nft] src: allow variables in the chain priority specification Fernando Fernandez Mancera
@ 2019-07-25 10:39 ` Pablo Neira Ayuso
2019-07-25 12:22 ` Fernando Fernandez Mancera
0 siblings, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2019-07-25 10:39 UTC (permalink / raw)
To: Fernando Fernandez Mancera; +Cc: netfilter-devel
On Mon, Jul 22, 2019 at 06:02:37PM +0200, Fernando Fernandez Mancera wrote:
> diff --git a/include/rule.h b/include/rule.h
> index 67c3d33..c6e8716 100644
[...]
>+const struct datatype priority_type = {
Please, add here something like on top of the definition:
/* This datatype is not registered via datatype_register()
* since this datatype should not ever be used from either
* rules or elements.
*/
or alike, so we don't forget that missing datatype_register() is not a
bug.
[...]
> --- a/include/rule.h
> +++ b/include/rule.h
> @@ -174,13 +174,13 @@ enum chain_flags {
> * struct prio_spec - extendend priority specification for mixed
> * textual/numerical parsing.
> *
> - * @str: name of the standard priority value
> - * @num: Numerical value. This MUST contain the parsed value of str after
> + * @expr: expr of the standard priority value
> + * @num: Numerical value. This MUST contain the parsed value of expr after
> * evaluation.
There must be a way to avoid this num field.
> */
> struct prio_spec {
> - const char *str;
> - int num;
> + struct expr *expr;
> + int num;
> struct location loc;
> };
>
> diff --git a/src/datatype.c b/src/datatype.c
> index 6d6826e..b7418e7 100644
> --- a/src/datatype.c
> +++ b/src/datatype.c
> @@ -1246,3 +1246,29 @@ const struct datatype boolean_type = {
> .sym_tbl = &boolean_tbl,
> .json = boolean_type_json,
> };
> +
> +static struct error_record *priority_type_parse(const struct expr *sym,
> + struct expr **res)
> +{
> + int num;
> +
> + if (isdigit(sym->identifier[0])) {
> + num = atoi(sym->identifier);
> + *res = constant_expr_alloc(&sym->location, &integer_type,
> + BYTEORDER_HOST_ENDIAN,
> + sizeof(int) * BITS_PER_BYTE,
> + &num);
> + } else
> + *res = constant_expr_alloc(&sym->location, &string_type,
> + BYTEORDER_HOST_ENDIAN,
> + strlen(sym->identifier) *
> + BITS_PER_BYTE, sym->identifier);
I'd suggest you call integer_datatype_parse() first, check if erec ==
NULL, then all good, this is an integer. Otherwise, release erec
object and parse this as a string via string_datatype_parse().
> + return NULL;
> +}
> +
> +const struct datatype priority_type = {
> + .type = TYPE_STRING,
> + .name = "priority",
> + .desc = "priority type",
> + .parse = priority_type_parse,
> +};
> diff --git a/src/evaluate.c b/src/evaluate.c
> old mode 100644
> new mode 100755
> index 69b853f..d2faee8
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -3193,15 +3193,36 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
> return 0;
> }
>
> -static bool evaluate_priority(struct prio_spec *prio, int family, int hook)
> +static bool evaluate_priority(struct eval_ctx *ctx, struct prio_spec *prio,
> + int family, int hook)
> {
> int priority;
> + char prio_str[NFT_NAME_MAXLEN];
>
> /* A numeric value has been used to specify priority. */
> - if (prio->str == NULL)
> + if (prio->expr == NULL)
If we always use an expression, as described below, this won't be
needed.
> return true;
>
> - priority = std_prio_lookup(prio->str, family, hook);
> + ctx->ectx.dtype = &priority_type;
> + ctx->ectx.len = NFT_NAME_MAXLEN * BITS_PER_BYTE;
> + if (expr_evaluate(ctx, &prio->expr) < 0)
> + return false;
> + if (prio->expr->etype != EXPR_VALUE) {
> + expr_error(ctx->msgs, prio->expr, "%s is not a valid "
> + "priority expression", expr_name(prio->expr));
> + return false;
> + }
> +
> + if (prio->expr->dtype->type == TYPE_INTEGER) {
> + mpz_export_data(&prio->num, prio->expr->value,
> + BYTEORDER_HOST_ENDIAN, sizeof(int));
> + return true;
> + }
> + mpz_export_data(prio_str, prio->expr->value,
If you use symbol expression, as I describe below. You don't need to
convert this constant expression back to string again. You can just
use the symbol identify sym->identifier.
> + BYTEORDER_HOST_ENDIAN,
> + NFT_NAME_MAXLEN);
> +
> + priority = std_prio_lookup(prio_str, family, hook);
> if (priority == NF_IP_PRI_LAST)
> return false;
> prio->num += priority;
> @@ -3223,10 +3244,10 @@ static int flowtable_evaluate(struct eval_ctx *ctx, struct flowtable *ft)
> if (ft->hooknum == NF_INET_NUMHOOKS)
> return chain_error(ctx, ft, "invalid hook %s", ft->hookstr);
>
> - if (!evaluate_priority(&ft->priority, NFPROTO_NETDEV, ft->hooknum))
> + if (!evaluate_priority(ctx, &ft->priority, NFPROTO_NETDEV, ft->hooknum))
> return __stmt_binary_error(ctx, &ft->priority.loc, NULL,
> - "'%s' is invalid priority.",
> - ft->priority.str);
> + "invalid priority expression %s.",
> + expr_name(ft->priority.expr));
>
> if (!ft->dev_expr)
> return chain_error(ctx, ft, "Unbound flowtable not allowed (must specify devices)");
> @@ -3422,11 +3443,11 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
> return chain_error(ctx, chain, "invalid hook %s",
> chain->hookstr);
>
> - if (!evaluate_priority(&chain->priority, chain->handle.family,
> - chain->hooknum))
> + if (!evaluate_priority(ctx, &chain->priority,
> + chain->handle.family, chain->hooknum))
> return __stmt_binary_error(ctx, &chain->priority.loc, NULL,
> - "'%s' is invalid priority in this context.",
> - chain->priority.str);
> + "invalid priority expression %s in this context.",
> + expr_name(chain->priority.expr));
> }
>
> list_for_each_entry(rule, &chain->rules, list) {
> diff --git a/src/parser_bison.y b/src/parser_bison.y
> index 53e6695..ed7bd89 100644
> --- a/src/parser_bison.y
> +++ b/src/parser_bison.y
> @@ -636,8 +636,8 @@ int nft_lex(void *, void *, void *);
> %type <stmt> meter_stmt meter_stmt_alloc flow_stmt_legacy_alloc
> %destructor { stmt_free($$); } meter_stmt meter_stmt_alloc flow_stmt_legacy_alloc
>
> -%type <expr> symbol_expr verdict_expr integer_expr variable_expr chain_expr
> -%destructor { expr_free($$); } symbol_expr verdict_expr integer_expr variable_expr chain_expr
> +%type <expr> symbol_expr verdict_expr integer_expr variable_expr chain_expr prio_expr
> +%destructor { expr_free($$); } symbol_expr verdict_expr integer_expr variable_expr chain_expr prio_expr
> %type <expr> primary_expr shift_expr and_expr
> %destructor { expr_free($$); } primary_expr shift_expr and_expr
> %type <expr> exclusive_or_expr inclusive_or_expr
> @@ -1969,30 +1969,44 @@ extended_prio_name : OUT
> | STRING
> ;
>
> +prio_expr : variable_expr
> + {
> + datatype_set($1->sym->expr, &priority_type);
Can you use symbol_expr_alloc() here, both for variable_expr and
extended_prio_name.
Then, from the evaluation phase you can turn this symbol expression
into a constant using the priority_type_parse() function.
I think this will allow you to skip the num field in prio_spec.
Thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2 nft] src: allow variable in chain policy
2019-07-22 16:02 ` [PATCH 2/2 nft] src: allow variable in chain policy Fernando Fernandez Mancera
@ 2019-07-25 10:44 ` Pablo Neira Ayuso
2019-07-25 10:52 ` Fernando Fernandez Mancera
0 siblings, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2019-07-25 10:44 UTC (permalink / raw)
To: Fernando Fernandez Mancera; +Cc: netfilter-devel
On Mon, Jul 22, 2019 at 06:02:39PM +0200, Fernando Fernandez Mancera wrote:
> This patch introduces the use of nft input files variables in chain policy.
> e.g.
>
> define default_policy = "accept"
>
> add table ip foo
> add chain ip foo bar {type filter hook input priority filter; policy $default_policy}
>
> table ip foo {
> chain bar {
> type filter hook input priority filter; policy accept;
> }
> }
>
> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
> ---
> include/rule.h | 2 +-
> src/evaluate.c | 51 +++++++++++++++++++
> src/json.c | 5 +-
> src/mnl.c | 9 ++--
> src/netlink.c | 8 ++-
> src/parser_bison.y | 19 +++++--
> src/parser_json.c | 17 +++++--
> src/rule.c | 13 +++--
> .../testcases/nft-f/0025policy_variable_0 | 17 +++++++
> .../testcases/nft-f/0026policy_variable_0 | 17 +++++++
> .../testcases/nft-f/0027policy_variable_1 | 18 +++++++
> .../testcases/nft-f/0028policy_variable_1 | 18 +++++++
> .../nft-f/dumps/0025policy_variable_0.nft | 5 ++
> .../nft-f/dumps/0026policy_variable_0.nft | 5 ++
> 14 files changed, 186 insertions(+), 18 deletions(-)
> create mode 100755 tests/shell/testcases/nft-f/0025policy_variable_0
> create mode 100755 tests/shell/testcases/nft-f/0026policy_variable_0
> create mode 100755 tests/shell/testcases/nft-f/0027policy_variable_1
> create mode 100755 tests/shell/testcases/nft-f/0028policy_variable_1
> create mode 100644 tests/shell/testcases/nft-f/dumps/0025policy_variable_0.nft
> create mode 100644 tests/shell/testcases/nft-f/dumps/0026policy_variable_0.nft
>
> diff --git a/include/rule.h b/include/rule.h
> index c6e8716..b12165a 100644
> --- a/include/rule.h
> +++ b/include/rule.h
> @@ -209,7 +209,7 @@ struct chain {
> const char *hookstr;
> unsigned int hooknum;
> struct prio_spec priority;
> - int policy;
> + struct expr *policy;
> const char *type;
> const char *dev;
> struct scope scope;
> diff --git a/src/evaluate.c b/src/evaluate.c
> index d2faee8..4d8bfbf 100755
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -3415,6 +3415,52 @@ static uint32_t str2hooknum(uint32_t family, const char *hook)
> return NF_INET_NUMHOOKS;
> }
>
> +static bool evaluate_policy(struct eval_ctx *ctx, struct expr **policy)
better rename static bool ...(..., struct expr **exprp)
> +{
> + char policy_str[NFT_NAME_MAXLEN];
> + struct location loc;
> + int policy_num;
so you can use 'int policy;'.
> + ctx->ectx.len = NFT_NAME_MAXLEN * BITS_PER_BYTE;
> + if (expr_evaluate(ctx, policy) < 0)
> + return false;
probably cache this here:
expr = *exprp;
so you don't need:
(*expr)->...
in all this code below.
> + if ((*policy)->etype != EXPR_VALUE) {
> + expr_error(ctx->msgs, *policy, "%s is not a valid "
> + "policy expression", expr_name(*policy));
> + return false;
> + }
> +
> + if ((*policy)->dtype->type == TYPE_STRING) {
> + mpz_export_data(policy_str, (*policy)->value,
> + BYTEORDER_HOST_ENDIAN,
> + NFT_NAME_MAXLEN);
> + loc = (*policy)->location;
> + if (!strcmp(policy_str, "accept")) {
> + expr_free(*policy);
> + policy_num = NF_ACCEPT;
> + *policy = constant_expr_alloc(&loc,
> + &integer_type,
> + BYTEORDER_HOST_ENDIAN,
> + sizeof(int) * BITS_PER_BYTE,
> + &policy_num);
> + } else if (!strcmp(policy_str, "drop")) {
> + expr_free(*policy);
> + policy_num = NF_DROP;
> + *policy = constant_expr_alloc(&(*policy)->location,
> + &integer_type,
> + BYTEORDER_HOST_ENDIAN,
> + sizeof(int) * BITS_PER_BYTE,
> + &policy_num);
Probably:
if (!strcmp(policy_str, "accept")) {
policy = NF_ACCEPT;
else (!strmp(policy_str, "drop")) {
policy = NF_DROP;
} else {
...
}
expr = constant_expr_alloc(...);
so this code becomes shorter and easier to read.
And I think you should do all this from the new policy_datype, not
from the evaluation phase itself.
Thanks.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2 nft] src: allow variable in chain policy
2019-07-25 10:44 ` Pablo Neira Ayuso
@ 2019-07-25 10:52 ` Fernando Fernandez Mancera
0 siblings, 0 replies; 7+ messages in thread
From: Fernando Fernandez Mancera @ 2019-07-25 10:52 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
Hi Pablo,
El 25 de julio de 2019 12:44:25 CEST, Pablo Neira Ayuso <pablo@netfilter.org> escribió:
>On Mon, Jul 22, 2019 at 06:02:39PM +0200, Fernando Fernandez Mancera
>wrote:
>> This patch introduces the use of nft input files variables in chain
>policy.
>> e.g.
>>
>> define default_policy = "accept"
>>
>> add table ip foo
>> add chain ip foo bar {type filter hook input priority filter; policy
>$default_policy}
>>
>> table ip foo {
>> chain bar {
>> type filter hook input priority filter; policy accept;
>> }
>> }
>>
>> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
>> ---
>> include/rule.h | 2 +-
>> src/evaluate.c | 51
>+++++++++++++++++++
>> src/json.c | 5 +-
>> src/mnl.c | 9 ++--
>> src/netlink.c | 8 ++-
>> src/parser_bison.y | 19 +++++--
>> src/parser_json.c | 17 +++++--
>> src/rule.c | 13 +++--
>> .../testcases/nft-f/0025policy_variable_0 | 17 +++++++
>> .../testcases/nft-f/0026policy_variable_0 | 17 +++++++
>> .../testcases/nft-f/0027policy_variable_1 | 18 +++++++
>> .../testcases/nft-f/0028policy_variable_1 | 18 +++++++
>> .../nft-f/dumps/0025policy_variable_0.nft | 5 ++
>> .../nft-f/dumps/0026policy_variable_0.nft | 5 ++
>> 14 files changed, 186 insertions(+), 18 deletions(-)
>> create mode 100755 tests/shell/testcases/nft-f/0025policy_variable_0
>> create mode 100755 tests/shell/testcases/nft-f/0026policy_variable_0
>> create mode 100755 tests/shell/testcases/nft-f/0027policy_variable_1
>> create mode 100755 tests/shell/testcases/nft-f/0028policy_variable_1
>> create mode 100644
>tests/shell/testcases/nft-f/dumps/0025policy_variable_0.nft
>> create mode 100644
>tests/shell/testcases/nft-f/dumps/0026policy_variable_0.nft
>>
>> diff --git a/include/rule.h b/include/rule.h
>> index c6e8716..b12165a 100644
>> --- a/include/rule.h
>> +++ b/include/rule.h
>> @@ -209,7 +209,7 @@ struct chain {
>> const char *hookstr;
>> unsigned int hooknum;
>> struct prio_spec priority;
>> - int policy;
>> + struct expr *policy;
>> const char *type;
>> const char *dev;
>> struct scope scope;
>> diff --git a/src/evaluate.c b/src/evaluate.c
>> index d2faee8..4d8bfbf 100755
>> --- a/src/evaluate.c
>> +++ b/src/evaluate.c
>> @@ -3415,6 +3415,52 @@ static uint32_t str2hooknum(uint32_t family,
>const char *hook)
>> return NF_INET_NUMHOOKS;
>> }
>>
>> +static bool evaluate_policy(struct eval_ctx *ctx, struct expr
>**policy)
>
>better rename static bool ...(..., struct expr **exprp)
>
>> +{
>> + char policy_str[NFT_NAME_MAXLEN];
>> + struct location loc;
>> + int policy_num;
>
>so you can use 'int policy;'.
>
>> + ctx->ectx.len = NFT_NAME_MAXLEN * BITS_PER_BYTE;
>> + if (expr_evaluate(ctx, policy) < 0)
>> + return false;
>
>probably cache this here:
>
> expr = *exprp;
>
>so you don't need:
>
> (*expr)->...
>
>in all this code below.
>
>> + if ((*policy)->etype != EXPR_VALUE) {
>> + expr_error(ctx->msgs, *policy, "%s is not a valid "
>> + "policy expression", expr_name(*policy));
>> + return false;
>> + }
>> +
>> + if ((*policy)->dtype->type == TYPE_STRING) {
>> + mpz_export_data(policy_str, (*policy)->value,
>> + BYTEORDER_HOST_ENDIAN,
>> + NFT_NAME_MAXLEN);
>> + loc = (*policy)->location;
>> + if (!strcmp(policy_str, "accept")) {
>> + expr_free(*policy);
>> + policy_num = NF_ACCEPT;
>> + *policy = constant_expr_alloc(&loc,
>> + &integer_type,
>> + BYTEORDER_HOST_ENDIAN,
>> + sizeof(int) * BITS_PER_BYTE,
>> + &policy_num);
>> + } else if (!strcmp(policy_str, "drop")) {
>> + expr_free(*policy);
>> + policy_num = NF_DROP;
>> + *policy = constant_expr_alloc(&(*policy)->location,
>> + &integer_type,
>> + BYTEORDER_HOST_ENDIAN,
>> + sizeof(int) * BITS_PER_BYTE,
>> + &policy_num);
>
>Probably:
>
> if (!strcmp(policy_str, "accept")) {
> policy = NF_ACCEPT;
> else (!strmp(policy_str, "drop")) {
> policy = NF_DROP;
> } else {
> ...
> }
>
> expr = constant_expr_alloc(...);
>
>so this code becomes shorter and easier to read.
>
>And I think you should do all this from the new policy_datype, not
>from the evaluation phase itself.
>
>Thanks.
I agree with everything except this part. There is no policy datatype. I could create it if you prefer it. Thanks! :-)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2 nft] src: allow variables in the chain priority specification
2019-07-25 10:39 ` Pablo Neira Ayuso
@ 2019-07-25 12:22 ` Fernando Fernandez Mancera
0 siblings, 0 replies; 7+ messages in thread
From: Fernando Fernandez Mancera @ 2019-07-25 12:22 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
Hi Pablo, I have a few comments below.
On 7/25/19 12:39 PM, Pablo Neira Ayuso wrote:
> On Mon, Jul 22, 2019 at 06:02:37PM +0200, Fernando Fernandez Mancera wrote:
>> diff --git a/include/rule.h b/include/rule.h
>> index 67c3d33..c6e8716 100644
> [...]
>> +const struct datatype priority_type = {
>
> Please, add here something like on top of the definition:
>
> /* This datatype is not registered via datatype_register()
> * since this datatype should not ever be used from either
> * rules or elements.
> */
>
> or alike, so we don't forget that missing datatype_register() is not a
> bug.
>
> [...]
>> --- a/include/rule.h
>> +++ b/include/rule.h
>> @@ -174,13 +174,13 @@ enum chain_flags {
>> * struct prio_spec - extendend priority specification for mixed
>> * textual/numerical parsing.
>> *
>> - * @str: name of the standard priority value
>> - * @num: Numerical value. This MUST contain the parsed value of str after
>> + * @expr: expr of the standard priority value
>> + * @num: Numerical value. This MUST contain the parsed value of expr after
>> * evaluation.
>
> There must be a way to avoid this num field.
>
I have been thinking about that for a while. Right now it is needed
because we are supporting the following priorities:
0 /* only numbers */
filter /* only string */
filter + 10 /* combination of a string plus a number */
filter - 10 /* combination of a string minus a number */
The proposed design will allow us to continue supporting that. Having
said that, the only way to get rid of this num field is to parse the
string in parsing time and then store the resulting number as a symbol
expression in my opinion. Maybe I am missing something here. Have you
any idea? Thanks!
>> */
>> struct prio_spec {
>> - const char *str;
>> - int num;
>> + struct expr *expr;
>> + int num;
>> struct location loc;
>> };
>>
>> diff --git a/src/datatype.c b/src/datatype.c
>> index 6d6826e..b7418e7 100644
>> --- a/src/datatype.c
>> +++ b/src/datatype.c
>> @@ -1246,3 +1246,29 @@ const struct datatype boolean_type = {
>> .sym_tbl = &boolean_tbl,
>> .json = boolean_type_json,
>> };
>> +
>> +static struct error_record *priority_type_parse(const struct expr *sym,
>> + struct expr **res)
>> +{
>> + int num;
>> +
>> + if (isdigit(sym->identifier[0])) {
>> + num = atoi(sym->identifier);
>> + *res = constant_expr_alloc(&sym->location, &integer_type,
>> + BYTEORDER_HOST_ENDIAN,
>> + sizeof(int) * BITS_PER_BYTE,
>> + &num);
>> + } else
>> + *res = constant_expr_alloc(&sym->location, &string_type,
>> + BYTEORDER_HOST_ENDIAN,
>> + strlen(sym->identifier) *
>> + BITS_PER_BYTE, sym->identifier);
>
> I'd suggest you call integer_datatype_parse() first, check if erec ==
> NULL, then all good, this is an integer. Otherwise, release erec
> object and parse this as a string via string_datatype_parse().
> >> + return NULL;
>> +}
>> +
>> +const struct datatype priority_type = {
>> + .type = TYPE_STRING,
>> + .name = "priority",
>> + .desc = "priority type",
>> + .parse = priority_type_parse,
>> +};
>> diff --git a/src/evaluate.c b/src/evaluate.c
>> old mode 100644
>> new mode 100755
>> index 69b853f..d2faee8
>> --- a/src/evaluate.c
>> +++ b/src/evaluate.c
>> @@ -3193,15 +3193,36 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
>> return 0;
>> }
>>
>> -static bool evaluate_priority(struct prio_spec *prio, int family, int hook)
>> +static bool evaluate_priority(struct eval_ctx *ctx, struct prio_spec *prio,
>> + int family, int hook)
>> {
>> int priority;
>> + char prio_str[NFT_NAME_MAXLEN];
>>
>> /* A numeric value has been used to specify priority. */
>> - if (prio->str == NULL)
>> + if (prio->expr == NULL)
>
> If we always use an expression, as described below, this won't be
> needed.
>
>> return true;
>>
>> - priority = std_prio_lookup(prio->str, family, hook);
>> + ctx->ectx.dtype = &priority_type;
>> + ctx->ectx.len = NFT_NAME_MAXLEN * BITS_PER_BYTE;
>> + if (expr_evaluate(ctx, &prio->expr) < 0)
>> + return false;
>> + if (prio->expr->etype != EXPR_VALUE) {
>> + expr_error(ctx->msgs, prio->expr, "%s is not a valid "
>> + "priority expression", expr_name(prio->expr));
>> + return false;
>> + }
>> +
>> + if (prio->expr->dtype->type == TYPE_INTEGER) {
>> + mpz_export_data(&prio->num, prio->expr->value,
>> + BYTEORDER_HOST_ENDIAN, sizeof(int));
>> + return true;
>> + }
>> + mpz_export_data(prio_str, prio->expr->value,
>
> If you use symbol expression, as I describe below. You don't need to
> convert this constant expression back to string again. You can just
> use the symbol identify sym->identifier.
>
>> + BYTEORDER_HOST_ENDIAN,
>> + NFT_NAME_MAXLEN);
>> +
>> + priority = std_prio_lookup(prio_str, family, hook);
>> if (priority == NF_IP_PRI_LAST)
>> return false;
>> prio->num += priority;
>> @@ -3223,10 +3244,10 @@ static int flowtable_evaluate(struct eval_ctx *ctx, struct flowtable *ft)
>> if (ft->hooknum == NF_INET_NUMHOOKS)
>> return chain_error(ctx, ft, "invalid hook %s", ft->hookstr);
>>
>> - if (!evaluate_priority(&ft->priority, NFPROTO_NETDEV, ft->hooknum))
>> + if (!evaluate_priority(ctx, &ft->priority, NFPROTO_NETDEV, ft->hooknum))
>> return __stmt_binary_error(ctx, &ft->priority.loc, NULL,
>> - "'%s' is invalid priority.",
>> - ft->priority.str);
>> + "invalid priority expression %s.",
>> + expr_name(ft->priority.expr));
>>
>> if (!ft->dev_expr)
>> return chain_error(ctx, ft, "Unbound flowtable not allowed (must specify devices)");
>> @@ -3422,11 +3443,11 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
>> return chain_error(ctx, chain, "invalid hook %s",
>> chain->hookstr);
>>
>> - if (!evaluate_priority(&chain->priority, chain->handle.family,
>> - chain->hooknum))
>> + if (!evaluate_priority(ctx, &chain->priority,
>> + chain->handle.family, chain->hooknum))
>> return __stmt_binary_error(ctx, &chain->priority.loc, NULL,
>> - "'%s' is invalid priority in this context.",
>> - chain->priority.str);
>> + "invalid priority expression %s in this context.",
>> + expr_name(chain->priority.expr));
>> }
>>
>> list_for_each_entry(rule, &chain->rules, list) {
>> diff --git a/src/parser_bison.y b/src/parser_bison.y
>> index 53e6695..ed7bd89 100644
>> --- a/src/parser_bison.y
>> +++ b/src/parser_bison.y
>> @@ -636,8 +636,8 @@ int nft_lex(void *, void *, void *);
>> %type <stmt> meter_stmt meter_stmt_alloc flow_stmt_legacy_alloc
>> %destructor { stmt_free($$); } meter_stmt meter_stmt_alloc flow_stmt_legacy_alloc
>>
>> -%type <expr> symbol_expr verdict_expr integer_expr variable_expr chain_expr
>> -%destructor { expr_free($$); } symbol_expr verdict_expr integer_expr variable_expr chain_expr
>> +%type <expr> symbol_expr verdict_expr integer_expr variable_expr chain_expr prio_expr
>> +%destructor { expr_free($$); } symbol_expr verdict_expr integer_expr variable_expr chain_expr prio_expr
>> %type <expr> primary_expr shift_expr and_expr
>> %destructor { expr_free($$); } primary_expr shift_expr and_expr
>> %type <expr> exclusive_or_expr inclusive_or_expr
>> @@ -1969,30 +1969,44 @@ extended_prio_name : OUT
>> | STRING
>> ;
>>
>> +prio_expr : variable_expr
>> + {
>> + datatype_set($1->sym->expr, &priority_type);
>
> Can you use symbol_expr_alloc() here, both for variable_expr and
> extended_prio_name.
>
> Then, from the evaluation phase you can turn this symbol expression
> into a constant using the priority_type_parse() function.
> > I think this will allow you to skip the num field in prio_spec.
Do you mean to store "filter + 10" in the symbol and parse the string
manually using priority_type_parse() function? Thanks and sorry if I am
making a big mess out of this.
> Thanks!
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-07-25 12:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-22 16:02 [PATCH 0/2 nft] Introduce variables in chain priority and policy Fernando Fernandez Mancera
2019-07-22 16:02 ` [PATCH 1/2 nft] src: allow variables in the chain priority specification Fernando Fernandez Mancera
2019-07-25 10:39 ` Pablo Neira Ayuso
2019-07-25 12:22 ` Fernando Fernandez Mancera
2019-07-22 16:02 ` [PATCH 2/2 nft] src: allow variable in chain policy Fernando Fernandez Mancera
2019-07-25 10:44 ` Pablo Neira Ayuso
2019-07-25 10:52 ` Fernando Fernandez Mancera
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.