[PATCH nft 0/3] fix map update with concatenation and timeouts

[PATCH nft 0/3] fix map update with concatenation and timeouts

[Florian Westphal]

When "update" is used with a map, nft will ignore a given timeout.
Futhermore, listing is broken, only the first data expression
gets decoded:

in:
 meta l4proto tcp update @pinned { ip saddr . ct original proto-src : ip daddr . ct original proto-dst timeout 90s }
out:
 meta l4proto tcp update @pinned { ip saddr . ct original proto-src : ip daddr }

Missing timeout is input bug (never passed to kernel), mussing
"proto-dst" is output bug.

Also add a test case.

Florian Westphal (3):
  netlink_delinearize: fix decoding of concat data element
  netlink_linearize: fix timeout with map updates
  tests: add a test case for map update from packet path with concat

 src/netlink_delinearize.c                      |  8 ++++++++
 src/netlink_linearize.c                        |  7 +++++++
 .../maps/dumps/typeof_maps_concat_update_0.nft | 12 ++++++++++++
 .../testcases/maps/typeof_maps_concat_update_0 | 18 ++++++++++++++++++
 4 files changed, 45 insertions(+)
 create mode 100644 tests/shell/testcases/maps/dumps/typeof_maps_concat_update_0.nft
 create mode 100755 tests/shell/testcases/maps/typeof_maps_concat_update_0

-- 
2.38.1

[PATCH nft 1/3] netlink_delinearize: fix decoding of concat data element

[Florian Westphal]

Its possible to use update as follows:

 meta l4proto tcp update @pinned { ip saddr . ct original proto-src : ip daddr . ct original proto-dst }

... but when listing, only the first element of the concatenation is
shown.

Check if the element size is too small and parse subsequent registers as
well.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/netlink_delinearize.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 0b6cf1072294..376b3550f9e2 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -1660,6 +1660,14 @@ static void netlink_parse_dynset(struct netlink_parse_ctx *ctx,
 	if (nftnl_expr_is_set(nle, NFTNL_EXPR_DYNSET_SREG_DATA)) {
 		sreg_data = netlink_parse_register(nle, NFTNL_EXPR_DYNSET_SREG_DATA);
 		expr_data = netlink_get_register(ctx, loc, sreg_data);
+
+		if (expr_data->len < set->data->len) {
+			expr_free(expr_data);
+			expr_data = netlink_parse_concat_expr(ctx, loc, sreg_data, set->data->len);
+			if (expr_data == NULL)
+				netlink_error(ctx, loc,
+					      "Could not parse dynset map data expessions");
+		}
 	}
 
 	if (expr_data != NULL) {
-- 
2.38.1

[PATCH nft 2/3] netlink_linearize: fix timeout with map updates

[Florian Westphal]

Map updates can use timeouts, just like with sets, but the
linearization step did not pass this info to the kernel.

meta l4proto tcp update @pinned { ip saddr . ct original proto-src : ip daddr . ct original proto-dst timeout 90s

Listing this won't show the "timeout 90s" because kernel never saw it to
begin with.

NB: The above line attaches the timeout to the data element,
but there are no separate timeouts for the key and the value.

An alternative is to reject "key : value timeout X" from the parser
or evaluation step.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/netlink_linearize.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index c8bbcb7452b0..765b12263fa3 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -1520,6 +1520,13 @@ static void netlink_gen_map_stmt(struct netlink_linearize_ctx *ctx,
 	nftnl_expr_set_u32(nle, NFTNL_EXPR_DYNSET_SET_ID, set->handle.set_id);
 	nft_rule_add_expr(ctx, nle, &stmt->location);
 
+	if (stmt->map.key->timeout > 0)
+		nftnl_expr_set_u64(nle, NFTNL_EXPR_DYNSET_TIMEOUT,
+				   stmt->map.key->timeout);
+	else if (stmt->map.data->timeout > 0)
+		nftnl_expr_set_u64(nle, NFTNL_EXPR_DYNSET_TIMEOUT,
+				   stmt->map.data->timeout);
+
 	list_for_each_entry(this, &stmt->map.stmt_list, list)
 		num_stmts++;
 
-- 
2.38.1

Re: [PATCH nft 2/3] netlink_linearize: fix timeout with map updates

[Pablo Neira Ayuso]

On Mon, Dec 12, 2022 at 11:04:35AM +0100, Florian Westphal wrote:
> Map updates can use timeouts, just like with sets, but the
> linearization step did not pass this info to the kernel.
> 
> meta l4proto tcp update @pinned { ip saddr . ct original proto-src : ip daddr . ct original proto-dst timeout 90s
> 
> Listing this won't show the "timeout 90s" because kernel never saw it to
> begin with.
> 
> NB: The above line attaches the timeout to the data element,
> but there are no separate timeouts for the key and the value.
> 
> An alternative is to reject "key : value timeout X" from the parser
> or evaluation step.

You mean, timeout is accepted both from key : value sides of the
mapping, right?

It makes more sense to restrict it to the key side, that would require
a follow up patch.

Thanks.

Re: [PATCH nft 2/3] netlink_linearize: fix timeout with map updates

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Dec 12, 2022 at 11:04:35AM +0100, Florian Westphal wrote:
> > Map updates can use timeouts, just like with sets, but the
> > linearization step did not pass this info to the kernel.
> > 
> > meta l4proto tcp update @pinned { ip saddr . ct original proto-src : ip daddr . ct original proto-dst timeout 90s
> > 
> > Listing this won't show the "timeout 90s" because kernel never saw it to
> > begin with.
> > 
> > NB: The above line attaches the timeout to the data element,
> > but there are no separate timeouts for the key and the value.
> > 
> > An alternative is to reject "key : value timeout X" from the parser
> > or evaluation step.
> 
> You mean, timeout is accepted both from key : value sides of the
> mapping, right?

Yes, exactly, you can even to

ip saddr timeout 1m : 0x42 timeout 1s

> It makes more sense to restrict it to the key side, that would require
> a follow up patch.

Ok, works for me, should be easy to do.

[PATCH nft 3/3] tests: add a test case for map update from packet path with concat

[Florian Westphal]

add a second test case for map updates, this time with both
a timeout and a data element that consists of a concatenation.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 .../maps/dumps/typeof_maps_concat_update_0.nft | 12 ++++++++++++
 .../testcases/maps/typeof_maps_concat_update_0 | 18 ++++++++++++++++++
 2 files changed, 30 insertions(+)
 create mode 100644 tests/shell/testcases/maps/dumps/typeof_maps_concat_update_0.nft
 create mode 100755 tests/shell/testcases/maps/typeof_maps_concat_update_0

diff --git a/tests/shell/testcases/maps/dumps/typeof_maps_concat_update_0.nft b/tests/shell/testcases/maps/dumps/typeof_maps_concat_update_0.nft
new file mode 100644
index 000000000000..0963668629e5
--- /dev/null
+++ b/tests/shell/testcases/maps/dumps/typeof_maps_concat_update_0.nft
@@ -0,0 +1,12 @@
+table ip foo {
+	map pinned {
+		typeof ip daddr . tcp dport : ip daddr . tcp dport
+		size 65535
+		flags dynamic,timeout
+		timeout 6m
+	}
+
+	chain pr {
+		meta l4proto tcp update @pinned { ip saddr . ct original proto-dst timeout 1m30s : ct original ip daddr . ct reply proto-src }
+	}
+}
diff --git a/tests/shell/testcases/maps/typeof_maps_concat_update_0 b/tests/shell/testcases/maps/typeof_maps_concat_update_0
new file mode 100755
index 000000000000..357594ad55e8
--- /dev/null
+++ b/tests/shell/testcases/maps/typeof_maps_concat_update_0
@@ -0,0 +1,18 @@
+#!/bin/bash
+
+# check update statement does print both concatentations (key and data).
+
+EXPECTED="table ip foo {
+ map pinned {
+	typeof ip daddr . tcp dport : ip daddr . tcp dport
+	size 65535
+	flags dynamic,timeout
+        timeout 6m
+  }
+  chain pr {
+     meta l4proto tcp update @pinned { ip saddr . ct original proto-dst : ct original ip daddr . ct reply proto-src timeout 1m30s }
+  }
+}"
+
+set -e
+$NFT -f - <<< $EXPECTED
-- 
2.38.1

Re: [PATCH nft 0/3] fix map update with concatenation and timeouts

[Pablo Neira Ayuso]

On Mon, Dec 12, 2022 at 11:04:33AM +0100, Florian Westphal wrote:
> When "update" is used with a map, nft will ignore a given timeout.
> Futhermore, listing is broken, only the first data expression
> gets decoded:
> 
> in:
>  meta l4proto tcp update @pinned { ip saddr . ct original proto-src : ip daddr . ct original proto-dst timeout 90s }
> out:
>  meta l4proto tcp update @pinned { ip saddr . ct original proto-src : ip daddr }
> 
> Missing timeout is input bug (never passed to kernel), mussing
> "proto-dst" is output bug.
> 
> Also add a test case.

Series LGTM, thanks.

I might follow up to restrict the timeout to the key side unless you
would like to look into this.

Re: [PATCH nft 0/3] fix map update with concatenation and timeouts

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Dec 12, 2022 at 11:04:33AM +0100, Florian Westphal wrote:
> > When "update" is used with a map, nft will ignore a given timeout.
> > Futhermore, listing is broken, only the first data expression
> > gets decoded:
> > 
> > in:
> >  meta l4proto tcp update @pinned { ip saddr . ct original proto-src : ip daddr . ct original proto-dst timeout 90s }
> > out:
> >  meta l4proto tcp update @pinned { ip saddr . ct original proto-src : ip daddr }
> > 
> > Missing timeout is input bug (never passed to kernel), mussing
> > "proto-dst" is output bug.
> > 
> > Also add a test case.
> 
> Series LGTM, thanks.
> 
> I might follow up to restrict the timeout to the key side unless you
> would like to look into this.

I've pushed this series out; I fixed the typo in patch 1 and I
mangled patch 2 to reject data-element timeouts from the eval phase.