[PATCH nft,RFC,PoC 0/2] typeof support for set / map

[PATCH nft,RFC,PoC 0/2] typeof support for set / map

[Pablo Neira Ayuso]

Hi,

One topic that keeps coming back and forth is support for using integers
from the set / map definitions, see:

https://patchwork.ozlabs.org/patch/1089728/

The following example shows how PoC/RFC patchset works:

# cat test.nft
table filter {
            set blacklist {
                    typeof ip saddr
            }

            chain input {
                    ip saddr @blacklist counter drop
            }
    }
# nft -f test.nft
# nft list ruleset
table ip filter {
        set blacklist {
                typeof ip saddr
        }

        chain input {
                ip saddr @blacklist counter packets 0 bytes 0 drop
        }
}

This patchset provides a proof-of-concept, it's a quick hack, I dislike
to deliver things in a raw shape like this, but anyway...

Support for concatenations and object maps are missing. The
representation of the expression into the TLV still needs to be defined
(it could be a structure whose first field specifies the expression
type and an union with the specific fields for this expression,
encapsulated in the TLV).

Pablo Neira Ayuso (2):
  parser: add typeof keyword for declarations
  src: restore typeof datatype when listing set definition

 include/rule.h     |  3 +++
 src/mnl.c          | 27 +++++++++++++++++++++++++++
 src/netlink.c      |  9 ++++++++-
 src/parser_bison.y | 20 ++++++++++++++++++++
 src/rule.c         |  9 +++++++--
 src/scanner.l      |  1 +
 6 files changed, 66 insertions(+), 3 deletions(-)

-- 
2.11.0

[PATCH nft,RFC,PoC 1/2] parser: add typeof keyword for declarations

[Pablo Neira Ayuso]

Add a typeof keyword to automatically use the correct type in set and map
declarations.

table filter {
	set blacklist {
		typeof ip saddr
	}

	chain input {
		ip saddr @blacklist counter drop
	}
}

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/parser_bison.y | 20 ++++++++++++++++++++
 src/scanner.l      |  1 +
 2 files changed, 21 insertions(+)

diff --git a/src/parser_bison.y b/src/parser_bison.y
index 53e669521efa..5a1a37679a29 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -206,6 +206,8 @@ int nft_lex(void *, void *, void *);
 %token WSCALE			"wscale"
 %token SACKPERM			"sack-perm"
 
+%token TYPEOF			"typeof"
+
 %token HOOK			"hook"
 %token DEVICE			"device"
 %token DEVICES			"devices"
@@ -1624,6 +1626,12 @@ set_block		:	/* empty */	{ $$ = $<set>-1; }
 				$1->key = $3;
 				$$ = $1;
 			}
+			|	set_block	TYPEOF		primary_expr	stmt_separator
+			{
+				$1->key = $3;
+				datatype_set($1->key, $3->dtype);
+				$$ = $1;
+			}
 			|	set_block	FLAGS		set_flag_list	stmt_separator
 			{
 				$1->flags = $3;
@@ -1694,6 +1702,18 @@ map_block		:	/* empty */	{ $$ = $<set>-1; }
 				$1->flags |= NFT_SET_MAP;
 				$$ = $1;
 			}
+			|	map_block	TYPEOF
+						primary_expr	COLON	primary_expr
+						stmt_separator
+			{
+				$1->key = $3;
+				datatype_set($1->key, $3->dtype);
+				$1->datatype = $5->dtype;
+
+				expr_free($5);
+				$1->flags |= NFT_SET_MAP;
+				$$ = $1;
+			}
 			|	map_block	TYPE
 						data_type_expr	COLON	COUNTER
 						stmt_separator
diff --git a/src/scanner.l b/src/scanner.l
index 4ed5f9241381..6a0f95776b38 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -238,6 +238,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "define"		{ return DEFINE; }
 "redefine"		{ return REDEFINE; }
 "undefine"		{ return UNDEFINE; }
+"typeof"		{ return TYPEOF; }
 
 "describe"		{ return DESCRIBE; }
 
-- 
2.11.0

[PATCH nft,RFC,PoC 2/2] src: restore typeof datatype when listing set definition

[Pablo Neira Ayuso]

This is a proof-of-concept.

The idea behind this patch is to store the typeof definition
so it can be restored when listing it back.

Better way to do this would be to store the typeof expression
definition in a way that the set->key expression can be rebuilt.

Particularly, the code to print into the buffer is a quick and
dirty hack.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/rule.h |  3 +++
 src/mnl.c      | 27 +++++++++++++++++++++++++++
 src/netlink.c  |  9 ++++++++-
 src/rule.c     |  9 +++++++--
 4 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/include/rule.h b/include/rule.h
index ee881b9ccd17..15b0fc684726 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -278,6 +278,7 @@ extern struct rule *rule_lookup_by_index(const struct chain *chain,
  * @timeout:	default timeout value
  * @key:	key expression (data type, length))
  * @datatype:	mapping data type
+ * @datatypeof:	data type of expression
  * @datalen:	mapping data len
  * @objtype:	mapping object type
  * @init:	initializer
@@ -295,7 +296,9 @@ struct set {
 	uint32_t		gc_int;
 	uint64_t		timeout;
 	struct expr		*key;
+	const char		*key_str; /* XXX a hack, use struct expr */
 	const struct datatype	*datatype;
+	const struct expr	*datatypeof;
 	unsigned int		datalen;
 	uint32_t		objtype;
 	struct expr		*init;
diff --git a/src/mnl.c b/src/mnl.c
index eab8d5486437..2961bf6181a3 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -843,6 +843,33 @@ int mnl_nft_set_add(struct netlink_ctx *ctx, const struct cmd *cmd,
 				 set->automerge))
 		memory_allocation_error();
 
+	/* Set definition uses typeof to define datatype. */
+	if (!(set->key->flags & EXPR_F_CONSTANT)) {
+		struct output_ctx octx = {};
+		char buf[64];
+		int fds[2];
+
+		/* XXX a huge hack here below...
+		 *
+		 * Instead of storing the string, please store the expression
+		 * type and fields, ie. [ payload, desc->name, tmpl->token,
+		 * base, offset ]. This allows us to rebuild the expression
+		 * from the delinearize path. Similarly for other expressions.
+		 * Add new indirection to expr_ops to store a structure in the
+		 * TLV.
+		 */
+		assert(pipe(fds) == 0);
+		octx.output_fp = fdopen(fds[1], "w");
+		expr_print(set->key, &octx);
+		read(fds[0], buf, sizeof(buf));
+		close(fds[0]);
+		close(fds[1]);
+
+		if (!nftnl_udata_put(udbuf, NFTNL_UDATA_SET_MERGE_ELEMENTS + 1,
+				     strlen(buf) + 1, buf))
+			memory_allocation_error();
+	}
+
 	nftnl_set_set_data(nls, NFTNL_SET_USERDATA, nftnl_udata_buf_data(udbuf),
 			   nftnl_udata_buf_len(udbuf));
 	nftnl_udata_buf_free(udbuf);
diff --git a/src/netlink.c b/src/netlink.c
index 14b0df410726..1ccc98c3512a 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -534,6 +534,8 @@ static int set_parse_udata_cb(const struct nftnl_udata *attr, void *data)
 		if (len != sizeof(uint32_t))
 			return -1;
 		break;
+	case NFTNL_UDATA_SET_MERGE_ELEMENTS + 1:
+		break;
 	default:
 		return 0;
 	}
@@ -544,11 +546,12 @@ static int set_parse_udata_cb(const struct nftnl_udata *attr, void *data)
 struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
 				    const struct nftnl_set *nls)
 {
-	const struct nftnl_udata *ud[NFTNL_UDATA_SET_MAX + 1] = {};
+	const struct nftnl_udata *ud[NFTNL_UDATA_SET_MAX + 1 + 1] = {};
 	uint32_t flags, key, data, data_len, objtype = 0;
 	enum byteorder keybyteorder = BYTEORDER_INVALID;
 	enum byteorder databyteorder = BYTEORDER_INVALID;
 	const struct datatype *keytype, *datatype;
+	const char *key_str = NULL;
 	bool automerge = false;
 	const char *udata;
 	struct set *set;
@@ -569,6 +572,9 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
 		GET_U32_UDATA(databyteorder, NFTNL_UDATA_SET_DATABYTEORDER);
 		GET_U32_UDATA(automerge, NFTNL_UDATA_SET_MERGE_ELEMENTS);
 
+		if (ud[NFTNL_UDATA_SET_MERGE_ELEMENTS + 1])
+			key_str = xstrdup(nftnl_udata_get(ud[NFTNL_UDATA_SET_MERGE_ELEMENTS + 1]));
+
 #undef GET_U32_UDATA
 	}
 
@@ -604,6 +610,7 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
 	set->handle.set.name = xstrdup(nftnl_set_get_str(nls, NFTNL_SET_NAME));
 	set->automerge	   = automerge;
 
+	set->key_str = key_str;
 	set->key     = constant_expr_alloc(&netlink_location,
 					   set_datatype_alloc(keytype, keybyteorder),
 					   keybyteorder,
diff --git a/src/rule.c b/src/rule.c
index 293606576044..c21a550c8712 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -465,8 +465,13 @@ static void set_print_declaration(const struct set *set,
 	if (nft_output_handle(octx))
 		nft_print(octx, " # handle %" PRIu64, set->handle.handle.id);
 	nft_print(octx, "%s", opts->nl);
-	nft_print(octx, "%s%stype %s",
-		  opts->tab, opts->tab, set->key->dtype->name);
+	if (set->key_str) {
+		nft_print(octx, "%s%stypeof %s",
+			  opts->tab, opts->tab, set->key_str);
+	} else {
+		nft_print(octx, "%s%stype %s",
+			  opts->tab, opts->tab, set->key->dtype->name);
+	}
 	if (set_is_datamap(set->flags))
 		nft_print(octx, " : %s", set->datatype->name);
 	else if (set_is_objmap(set->flags))
-- 
2.11.0

Re: [PATCH nft,RFC,PoC 2/2] src: restore typeof datatype when listing set definition

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> This is a proof-of-concept.
> 
> The idea behind this patch is to store the typeof definition
> so it can be restored when listing it back.
> 
> Better way to do this would be to store the typeof expression
> definition in a way that the set->key expression can be rebuilt.

Maybe we can store the raw netlink data that makes up the expression
in the tlv area?

That would probably allow more code reuse to get back the "proper"
type.

One problem with my patch is that while you can add a map that
returns "osf name", I could not find a way to easily re-lookup
a suitable expression.  Storing a string would work of course,
but I don't like it because we have no way to revalidate this.

If we can reuse libnftnl/libmnl to have the basic netlink validation
run on the blob we can at least be sure that its not complete garbage
before we attempt to interpret the blob.

Re: [PATCH nft,RFC,PoC 2/2] src: restore typeof datatype when listing set definition

[Pablo Neira Ayuso]

On Tue, Jul 30, 2019 at 04:41:41PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > This is a proof-of-concept.
> > 
> > The idea behind this patch is to store the typeof definition
> > so it can be restored when listing it back.
> > 
> > Better way to do this would be to store the typeof expression
> > definition in a way that the set->key expression can be rebuilt.
> 
> Maybe we can store the raw netlink data that makes up the expression
> in the tlv area?

That's another possibility to explore.

> That would probably allow more code reuse to get back the "proper"
> type.

Just make sure there's sufficient context around to rebuild the
expression. Think of more complex fields that require bitmask
operations.

> One problem with my patch is that while you can add a map that
> returns "osf name", I could not find a way to easily re-lookup
> a suitable expression.  Storing a string would work of course,
> but I don't like it because we have no way to revalidate this.

I'm not advocating for storing the string. This was just a quick PoC
given the discussions after NFWS, and I wasn't sure everyone was on
the same page after it.

I agree with you in that the string is not the way to go.

> If we can reuse libnftnl/libmnl to have the basic netlink validation
> run on the blob we can at least be sure that its not complete garbage
> before we attempt to interpret the blob.

Please, go ahead explore that possibility. I'd try with payload
expressions, which are the most complex one. For thing like meta, it
should be simple to follow the approach you describe. Thanks.

Re: [PATCH nft,RFC,PoC 2/2] src: restore typeof datatype when listing set definition

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Just make sure there's sufficient context around to rebuild the
> expression. Think of more complex fields that require bitmask
> operations.

Indeed, I had forgotten about those.
I agree that this should work as well.