diff mbox series

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

Message ID 20190730141620.2129-3-pablo@netfilter.org
State RFC
Delegated to: Pablo Neira
Headers show
Series typeof support for set / map | expand

Commit Message

Pablo Neira Ayuso July 30, 2019, 2:16 p.m. UTC 
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(-)

Comments

Florian Westphal July 30, 2019, 2:41 p.m. UTC | #1 
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.
Pablo Neira Ayuso July 30, 2019, 2:48 p.m. UTC | #2 
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.
Florian Westphal July 30, 2019, 2:56 p.m. UTC | #3 
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.
diff mbox series

Patch

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))