* [PATCH 0/2 nft] mptcp: add mptcp subtype mnemonics
@ 2021-12-09 15:11 Florian Westphal
2021-12-09 15:11 ` [PATCH nft 1/2] tcpopt: add phony mptcp suboption datatype Florian Westphal
2021-12-09 15:11 ` [PATCH nft 2/2] src: propagate key datatype for anonymous sets Florian Westphal
0 siblings, 2 replies; 5+ messages in thread
From: Florian Westphal @ 2021-12-09 15:11 UTC (permalink / raw)
To: netfilter-devel
This allows use of mnemonics, e.g.
tcp option mptcp subtype mp-capable
The new datatype is phony: on kernel-side its represented as
TYPE_INTEGER.
It can only be used a set key via the 'typeof' expression.
This avoids bloating the (finite) list of data types just to
handle the extra symbol table.
include/datatype.h | 5 +++-
src/expression.c | 34 ++++++++++++++++++++++
src/tcpopt.c | 30 ++++++++++++++++++-
tests/py/any/tcpopt.t | 6 ++--
tests/py/any/tcpopt.t.json | 2 +-
tests/py/any/tcpopt.t.json.output | 31 ++++++++++++++++++++
tests/py/any/tcpopt.t.payload | 12 ++++----
tests/shell/testcases/sets/dumps/typeof_sets_0.nft | 9 ++++++
tests/shell/testcases/sets/typeof_sets_0 | 9 ++++++
9 files changed, 126 insertions(+), 12 deletions(-)
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH nft 1/2] tcpopt: add phony mptcp suboption datatype
2021-12-09 15:11 [PATCH 0/2 nft] mptcp: add mptcp subtype mnemonics Florian Westphal
@ 2021-12-09 15:11 ` Florian Westphal
2021-12-09 15:11 ` [PATCH nft 2/2] src: propagate key datatype for anonymous sets Florian Westphal
1 sibling, 0 replies; 5+ messages in thread
From: Florian Westphal @ 2021-12-09 15:11 UTC (permalink / raw)
To: netfilter-devel; +Cc: Florian Westphal
Because the number of unique 'enum datatypes' is limited by ABI
contraints this adds a new mptcp suboption type as integer alias.
This means that input side will work fine, but output won't, e.g.
tcp option mptcp subtype mp-capable
works, but will be shown as 'subtype 0'.
A followup patch will augment delinearization to infer the correct
type.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
include/datatype.h | 5 ++-
src/tcpopt.c | 30 +++++++++++++++++-
tests/py/any/tcpopt.t | 4 +--
tests/py/any/tcpopt.t.json | 2 +-
tests/py/any/tcpopt.t.json.output | 31 +++++++++++++++++++
tests/py/any/tcpopt.t.payload | 2 +-
.../testcases/sets/dumps/typeof_sets_0.nft | 9 ++++++
tests/shell/testcases/sets/typeof_sets_0 | 9 ++++++
8 files changed, 86 insertions(+), 6 deletions(-)
diff --git a/include/datatype.h b/include/datatype.h
index f5bb9dc4d937..70a0a8ac8f5d 100644
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -254,7 +254,6 @@ 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 xinteger_type;
extern const struct datatype string_type;
extern const struct datatype lladdr_type;
extern const struct datatype ipaddr_type;
@@ -276,6 +275,10 @@ extern const struct datatype priority_type;
extern const struct datatype policy_type;
extern const struct datatype cgroupv2_type;
+/* TYPE_INTEGER aliases: */
+extern const struct datatype xinteger_type;
+extern const struct datatype mptcpopt_subtype;
+
void inet_service_type_print(const struct expr *expr, struct output_ctx *octx);
extern const struct datatype *concat_type_alloc(uint32_t type);
diff --git a/src/tcpopt.c b/src/tcpopt.c
index c3e07d7889ab..9f2f082ec333 100644
--- a/src/tcpopt.c
+++ b/src/tcpopt.c
@@ -109,6 +109,31 @@ static const struct exthdr_desc tcpopt_md5sig = {
},
};
+static const struct symbol_table mptcp_subtype_tbl = {
+ .base = BASE_DECIMAL,
+ .symbols = {
+ SYMBOL("mp-capable", 0),
+ SYMBOL("mp-join", 1),
+ SYMBOL("dss", 2),
+ SYMBOL("add-addr", 3),
+ SYMBOL("remove-addr", 4),
+ SYMBOL("mp-prio", 5),
+ SYMBOL("mp-fail", 6),
+ SYMBOL("mp-fastclose", 7),
+ SYMBOL("mp-tcprst", 8),
+ SYMBOL_LIST_END
+ },
+};
+
+/* alias of integer_type to parse mptcp subtypes */
+const struct datatype mptcpopt_subtype = {
+ .type = TYPE_INTEGER,
+ .name = "integer",
+ .desc = "mptcp option subtype",
+ .size = 4,
+ .basetype = &integer_type,
+ .sym_tbl = &mptcp_subtype_tbl,
+};
static const struct exthdr_desc tcpopt_mptcp = {
.name = "mptcp",
@@ -116,7 +141,10 @@ static const struct exthdr_desc tcpopt_mptcp = {
.templates = {
[TCPOPT_MPTCP_KIND] = PHT("kind", 0, 8),
[TCPOPT_MPTCP_LENGTH] = PHT("length", 8, 8),
- [TCPOPT_MPTCP_SUBTYPE] = PHT("subtype", 16, 4),
+ [TCPOPT_MPTCP_SUBTYPE] = PROTO_HDR_TEMPLATE("subtype",
+ &mptcpopt_subtype,
+ BYTEORDER_BIG_ENDIAN,
+ 16, 4),
},
};
#undef PHT
diff --git a/tests/py/any/tcpopt.t b/tests/py/any/tcpopt.t
index 3d4be2a274df..a5ac1e86e207 100644
--- a/tests/py/any/tcpopt.t
+++ b/tests/py/any/tcpopt.t
@@ -51,6 +51,6 @@ tcp option md5sig exists;ok
tcp option fastopen exists;ok
tcp option mptcp exists;ok
-tcp option mptcp subtype 0;ok
-tcp option mptcp subtype 1;ok
+tcp option mptcp subtype mp-capable;ok
+tcp option mptcp subtype 1;ok;tcp option mptcp subtype mp-join
tcp option mptcp subtype { 0, 2};ok
diff --git a/tests/py/any/tcpopt.t.json b/tests/py/any/tcpopt.t.json
index 5cc6f8f42446..dc1e7a279c4a 100644
--- a/tests/py/any/tcpopt.t.json
+++ b/tests/py/any/tcpopt.t.json
@@ -533,7 +533,7 @@
}
]
-# tcp option mptcp subtype 0
+# tcp option mptcp subtype mp-capable
[
{
"match": {
diff --git a/tests/py/any/tcpopt.t.json.output b/tests/py/any/tcpopt.t.json.output
index ad0d25f4d56c..fd17cc2ace34 100644
--- a/tests/py/any/tcpopt.t.json.output
+++ b/tests/py/any/tcpopt.t.json.output
@@ -30,3 +30,34 @@
}
]
+# tcp option mptcp subtype mp-capable
+[
+ {
+ "match": {
+ "left": {
+ "tcp option": {
+ "field": "subtype",
+ "name": "mptcp"
+ }
+ },
+ "op": "==",
+ "right": "mp-capable"
+ }
+ }
+]
+
+# tcp option mptcp subtype 1
+[
+ {
+ "match": {
+ "left": {
+ "tcp option": {
+ "field": "subtype",
+ "name": "mptcp"
+ }
+ },
+ "op": "==",
+ "right": "mp-join"
+ }
+ }
+]
diff --git a/tests/py/any/tcpopt.t.payload b/tests/py/any/tcpopt.t.payload
index 121cc97fac09..269dee0aedb6 100644
--- a/tests/py/any/tcpopt.t.payload
+++ b/tests/py/any/tcpopt.t.payload
@@ -168,7 +168,7 @@ inet
[ exthdr load tcpopt 1b @ 30 + 0 present => reg 1 ]
[ cmp eq reg 1 0x00000001 ]
-# tcp option mptcp subtype 0
+# tcp option mptcp subtype mp-capable
inet
[ exthdr load tcpopt 1b @ 30 + 2 => reg 1 ]
[ bitwise reg 1 = ( reg 1 & 0x000000f0 ) ^ 0x00000000 ]
diff --git a/tests/shell/testcases/sets/dumps/typeof_sets_0.nft b/tests/shell/testcases/sets/dumps/typeof_sets_0.nft
index e397a6345462..52dc583b139c 100644
--- a/tests/shell/testcases/sets/dumps/typeof_sets_0.nft
+++ b/tests/shell/testcases/sets/dumps/typeof_sets_0.nft
@@ -45,6 +45,11 @@ table inet t {
15 }
}
+ set s10 {
+ typeof tcp option mptcp subtype
+ elements = { mp-join, dss }
+ }
+
chain c1 {
osf name @s1 accept
}
@@ -76,4 +81,8 @@ table inet t {
chain c9 {
ip hdrlength @s9 accept
}
+
+ chain c10 {
+ tcp option mptcp subtype @s10 accept
+ }
}
diff --git a/tests/shell/testcases/sets/typeof_sets_0 b/tests/shell/testcases/sets/typeof_sets_0
index be906cdcc842..5a40fe4f20c2 100755
--- a/tests/shell/testcases/sets/typeof_sets_0
+++ b/tests/shell/testcases/sets/typeof_sets_0
@@ -50,6 +50,11 @@ EXPECTED="table inet t {
elements = { 0, 1, 2, 3, 4, 15 }
}
+ set s10 {
+ typeof tcp option mptcp subtype
+ elements = { mp-join, dss }
+ }
+
chain c1 {
osf name @s1 accept
}
@@ -81,6 +86,10 @@ EXPECTED="table inet t {
chain c9 {
ip hdrlength @s9 accept
}
+
+ chain c10 {
+ tcp option mptcp subtype @s10 accept
+ }
}"
set -e
--
2.32.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH nft 2/2] src: propagate key datatype for anonymous sets
2021-12-09 15:11 [PATCH 0/2 nft] mptcp: add mptcp subtype mnemonics Florian Westphal
2021-12-09 15:11 ` [PATCH nft 1/2] tcpopt: add phony mptcp suboption datatype Florian Westphal
@ 2021-12-09 15:11 ` Florian Westphal
2021-12-15 23:22 ` Pablo Neira Ayuso
1 sibling, 1 reply; 5+ messages in thread
From: Florian Westphal @ 2021-12-09 15:11 UTC (permalink / raw)
To: netfilter-devel; +Cc: Florian Westphal
set s10 {
typeof tcp option mptcp subtype
elements = { mp-join, dss }
}
is listed correctly: typeof provides the 'mptcpopt_subtype'
datatype, so listing will print the elements with their sybolic types.
In anon case this doesn't work:
tcp option mptcp subtype { mp-join, dss }
gets shown as 'tcp option mptcp subtype { 1, 2}' because the anon
set has integer type.
This change propagates the datatype to the individual members
of the anon set.
After this change, multiple existing data types such as
TYPE_ICMP_TYPE could be replaced by integer-type aliases, but those
data types are already exposed to userspace via the 'set type'
directive so doing this may break existing set definitions.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
src/expression.c | 34 ++++++++++++++++++++++++++++++++++
tests/py/any/tcpopt.t | 2 +-
tests/py/any/tcpopt.t.payload | 10 +++++-----
3 files changed, 40 insertions(+), 6 deletions(-)
diff --git a/src/expression.c b/src/expression.c
index f1cca8845376..9de70c6cc1a4 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -1249,6 +1249,31 @@ static void set_ref_expr_destroy(struct expr *expr)
set_free(expr->set);
}
+static void set_ref_expr_set_type(const struct expr *expr,
+ const struct datatype *dtype,
+ enum byteorder byteorder)
+{
+ const struct set *s = expr->set;
+
+ /* normal sets already have a precise datatype that is given in
+ * the set definition via type foo.
+ *
+ * Anon sets do not have this, and need to rely on type info
+ * generated at rule creation time.
+ *
+ * For most cases, the type info is correct.
+ * In some cases however, the kernel only stores TYPE_INTEGER.
+ *
+ * This happens with expressions that only use an integer alias
+ * type, such as mptcp_suboption.
+ *
+ * In this case nft would print '1', '2', etc. instead of symbolic
+ * names because the base type lacks ->sym_tbl information.
+ */
+ if (s->init && set_is_anonymous(s->flags))
+ expr_set_type(s->init, dtype, byteorder);
+}
+
static const struct expr_ops set_ref_expr_ops = {
.type = EXPR_SET_REF,
.name = "set reference",
@@ -1256,6 +1281,7 @@ static const struct expr_ops set_ref_expr_ops = {
.json = set_ref_expr_json,
.clone = set_ref_expr_clone,
.destroy = set_ref_expr_destroy,
+ .set_type = set_ref_expr_set_type,
};
struct expr *set_ref_expr_alloc(const struct location *loc, struct set *set)
@@ -1310,6 +1336,13 @@ static void set_elem_expr_clone(struct expr *new, const struct expr *expr)
init_list_head(&new->stmt_list);
}
+static void set_elem_expr_set_type(const struct expr *expr,
+ const struct datatype *dtype,
+ enum byteorder byteorder)
+{
+ expr_set_type(expr->key, dtype, byteorder);
+}
+
static const struct expr_ops set_elem_expr_ops = {
.type = EXPR_SET_ELEM,
.name = "set element",
@@ -1317,6 +1350,7 @@ static const struct expr_ops set_elem_expr_ops = {
.print = set_elem_expr_print,
.json = set_elem_expr_json,
.destroy = set_elem_expr_destroy,
+ .set_type = set_elem_expr_set_type,
};
struct expr *set_elem_expr_alloc(const struct location *loc, struct expr *key)
diff --git a/tests/py/any/tcpopt.t b/tests/py/any/tcpopt.t
index a5ac1e86e207..f57532c3016d 100644
--- a/tests/py/any/tcpopt.t
+++ b/tests/py/any/tcpopt.t
@@ -53,4 +53,4 @@ tcp option mptcp exists;ok
tcp option mptcp subtype mp-capable;ok
tcp option mptcp subtype 1;ok;tcp option mptcp subtype mp-join
-tcp option mptcp subtype { 0, 2};ok
+tcp option mptcp subtype { mp-capable, mp-join, remove-addr, mp-prio, mp-fail, mp-fastclose, mp-tcprst };ok
diff --git a/tests/py/any/tcpopt.t.payload b/tests/py/any/tcpopt.t.payload
index 269dee0aedb6..5a3102f9b459 100644
--- a/tests/py/any/tcpopt.t.payload
+++ b/tests/py/any/tcpopt.t.payload
@@ -180,11 +180,11 @@ inet
[ bitwise reg 1 = ( reg 1 & 0x000000f0 ) ^ 0x00000000 ]
[ cmp eq reg 1 0x00000010 ]
-# tcp option mptcp subtype { 0, 2}
-__set%d test-inet 3 size 2
-__set%d test-inet 0
- element 00000000 : 0 [end] element 00000020 : 0 [end]
-inet
+# tcp option mptcp subtype { mp-capable, mp-join, remove-addr, mp-prio, mp-fail, mp-fastclose, mp-tcprst }
+__set%d test-ip4 3 size 7
+__set%d test-ip4 0
+ element 00000000 : 0 [end] element 00000010 : 0 [end] element 00000040 : 0 [end] element 00000050 : 0 [end] element 00000060 : 0 [end] element 00000070 : 0 [end] element 00000080 : 0 [end]
+ip test-ip4 input
[ exthdr load tcpopt 1b @ 30 + 2 => reg 1 ]
[ bitwise reg 1 = ( reg 1 & 0x000000f0 ) ^ 0x00000000 ]
[ lookup reg 1 set __set%d ]
--
2.32.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH nft 2/2] src: propagate key datatype for anonymous sets
2021-12-09 15:11 ` [PATCH nft 2/2] src: propagate key datatype for anonymous sets Florian Westphal
@ 2021-12-15 23:22 ` Pablo Neira Ayuso
2021-12-17 11:22 ` Florian Westphal
0 siblings, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2021-12-15 23:22 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
On Thu, Dec 09, 2021 at 04:11:31PM +0100, Florian Westphal wrote:
> set s10 {
> typeof tcp option mptcp subtype
> elements = { mp-join, dss }
> }
>
> is listed correctly: typeof provides the 'mptcpopt_subtype'
> datatype, so listing will print the elements with their sybolic types.
>
> In anon case this doesn't work:
> tcp option mptcp subtype { mp-join, dss }
>
> gets shown as 'tcp option mptcp subtype { 1, 2}' because the anon
> set has integer type.
>
> This change propagates the datatype to the individual members
> of the anon set.
>
> After this change, multiple existing data types such as
> TYPE_ICMP_TYPE could be replaced by integer-type aliases, but those
> data types are already exposed to userspace via the 'set type'
> directive so doing this may break existing set definitions.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
> src/expression.c | 34 ++++++++++++++++++++++++++++++++++
> tests/py/any/tcpopt.t | 2 +-
> tests/py/any/tcpopt.t.payload | 10 +++++-----
> 3 files changed, 40 insertions(+), 6 deletions(-)
>
> diff --git a/src/expression.c b/src/expression.c
> index f1cca8845376..9de70c6cc1a4 100644
> --- a/src/expression.c
> +++ b/src/expression.c
> @@ -1249,6 +1249,31 @@ static void set_ref_expr_destroy(struct expr *expr)
> set_free(expr->set);
> }
>
> +static void set_ref_expr_set_type(const struct expr *expr,
> + const struct datatype *dtype,
> + enum byteorder byteorder)
> +{
> + const struct set *s = expr->set;
> +
> + /* normal sets already have a precise datatype that is given in
> + * the set definition via type foo.
> + *
> + * Anon sets do not have this, and need to rely on type info
> + * generated at rule creation time.
> + *
> + * For most cases, the type info is correct.
> + * In some cases however, the kernel only stores TYPE_INTEGER.
> + *
> + * This happens with expressions that only use an integer alias
> + * type, such as mptcp_suboption.
> + *
> + * In this case nft would print '1', '2', etc. instead of symbolic
> + * names because the base type lacks ->sym_tbl information.
> + */
> + if (s->init && set_is_anonymous(s->flags))
> + expr_set_type(s->init, dtype, byteorder);
Will this work with concatenations?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH nft 2/2] src: propagate key datatype for anonymous sets
2021-12-15 23:22 ` Pablo Neira Ayuso
@ 2021-12-17 11:22 ` Florian Westphal
0 siblings, 0 replies; 5+ messages in thread
From: Florian Westphal @ 2021-12-17 11:22 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Dec 09, 2021 at 04:11:31PM +0100, Florian Westphal wrote:
> > set s10 {
> > typeof tcp option mptcp subtype
> > elements = { mp-join, dss }
> > }
> >
> > is listed correctly: typeof provides the 'mptcpopt_subtype'
> > datatype, so listing will print the elements with their sybolic types.
> >
> > In anon case this doesn't work:
> > tcp option mptcp subtype { mp-join, dss }
> >
> > gets shown as 'tcp option mptcp subtype { 1, 2}' because the anon
> > set has integer type.
> >
> > This change propagates the datatype to the individual members
> > of the anon set.
> >
> > After this change, multiple existing data types such as
> > TYPE_ICMP_TYPE could be replaced by integer-type aliases, but those
> > data types are already exposed to userspace via the 'set type'
> > directive so doing this may break existing set definitions.
> >
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> > src/expression.c | 34 ++++++++++++++++++++++++++++++++++
> > tests/py/any/tcpopt.t | 2 +-
> > tests/py/any/tcpopt.t.payload | 10 +++++-----
> > 3 files changed, 40 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/expression.c b/src/expression.c
> > index f1cca8845376..9de70c6cc1a4 100644
> > --- a/src/expression.c
> > +++ b/src/expression.c
> > @@ -1249,6 +1249,31 @@ static void set_ref_expr_destroy(struct expr *expr)
> > set_free(expr->set);
> > }
> >
> > +static void set_ref_expr_set_type(const struct expr *expr,
> > + const struct datatype *dtype,
> > + enum byteorder byteorder)
> > +{
> > + const struct set *s = expr->set;
> > +
> > + /* normal sets already have a precise datatype that is given in
> > + * the set definition via type foo.
> > + *
> > + * Anon sets do not have this, and need to rely on type info
> > + * generated at rule creation time.
> > + *
> > + * For most cases, the type info is correct.
> > + * In some cases however, the kernel only stores TYPE_INTEGER.
> > + *
> > + * This happens with expressions that only use an integer alias
> > + * type, such as mptcp_suboption.
> > + *
> > + * In this case nft would print '1', '2', etc. instead of symbolic
> > + * names because the base type lacks ->sym_tbl information.
> > + */
> > + if (s->init && set_is_anonymous(s->flags))
> > + expr_set_type(s->init, dtype, byteorder);
>
> Will this work with concatenations?
No. But concatenations won't work for any of these, e.g.
ip version . ip daddr
tcp option mptcp subtype . tcp dport
... and so on because integer type (unspecific length) can't be
used with concat types so far.
So I think the problem is related but not added in this patch.
However, I think I need to withdraw this propsed patch for a different
reason.
If we'd try to expose mptcp option matching for specific sub fields,
the symbol expression path might not be ideal, for example consider:
mptcp option subtype mp-join address-id gt 4 drop
(illustrative example with made-up syntax).
From parser perspective it might be better to make this
MPTCP OPTION SUBTYPE mptcp_option_fields : { ...
mptcp_option_fields : MP_JOIN mptcp_option_mpjoin_fields ...
| MP_CAPABLE mptcp_option_capable_fields ...
rather than
mptcp option subtype STRING STRING
... where parser calls helpers to 'translate' $4 and $5 to
the desired needed offset-length values (plus dependencies
to avoid bogus match on different suboption).
What do you think?
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-12-17 11:22 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-09 15:11 [PATCH 0/2 nft] mptcp: add mptcp subtype mnemonics Florian Westphal
2021-12-09 15:11 ` [PATCH nft 1/2] tcpopt: add phony mptcp suboption datatype Florian Westphal
2021-12-09 15:11 ` [PATCH nft 2/2] src: propagate key datatype for anonymous sets Florian Westphal
2021-12-15 23:22 ` Pablo Neira Ayuso
2021-12-17 11:22 ` Florian Westphal
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.