[PATCH nft 0/3] typeof incremental enhancements

[PATCH nft 0/3] typeof incremental enhancements

[Pablo Neira Ayuso]

Hi Florian,

This patchset removes the need to self invoke the parser and the
evaluation to fetch the datatype. Instead, the expression type and
the expression description are stored into the userdata area.

This patch only supports for the payload expression, but it should be
relatively easy to extend it to support for other existing expressions
types.

This patch could be squashed into 06/11 src: add "typeof" print support
of your patch series, which is actually not just adding support for
printing but also for building the userdata.

Thanks.

Pablo Neira Ayuso (3):
  proto: add proto_desc_id enumeration
  expr: add expr_ops_by_type()
  expr: add parse and build userdata interface

 include/expression.h |   5 +++
 include/proto.h      |  27 +++++++++++++
 src/expression.c     |  12 ++++++
 src/mnl.c            |  28 +++++---------
 src/netlink.c        | 105 +++++++++++++++++----------------------------------
 src/payload.c        |  80 +++++++++++++++++++++++++++++++++++++++
 src/proto.c          |  46 ++++++++++++++++++++++
 7 files changed, 214 insertions(+), 89 deletions(-)

-- 
2.11.0

[PATCH nft 1/3] proto: add proto_desc_id enumeration

[Pablo Neira Ayuso]

This allows to uniquely identify the protocol description.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/proto.h | 27 +++++++++++++++++++++++++++
 src/proto.c     | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)

diff --git a/include/proto.h b/include/proto.h
index fab48c1bb30c..1771ba8e8d8c 100644
--- a/include/proto.h
+++ b/include/proto.h
@@ -63,10 +63,34 @@ struct proto_hdr_template {
 #define PROTO_UPPER_MAX		16
 #define PROTO_HDRS_MAX		20
 
+enum proto_desc_id {
+	PROTO_DESC_UNKNOWN	= 0,
+	PROTO_DESC_AH,
+	PROTO_DESC_ESP,
+	PROTO_DESC_COMP,
+	PROTO_DESC_ICMP,
+	PROTO_DESC_IGMP,
+	PROTO_DESC_UDP,
+	PROTO_DESC_UDPLITE,
+	PROTO_DESC_TCP,
+	PROTO_DESC_DCCP,
+	PROTO_DESC_SCTP,
+	PROTO_DESC_TH,
+	PROTO_DESC_IP,
+	PROTO_DESC_IP6,
+	PROTO_DESC_ICMPV6,
+	PROTO_DESC_ARP,
+	PROTO_DESC_VLAN,
+	PROTO_DESC_ETHER,
+	__PROTO_DESC_MAX
+};
+#define PROTO_DESC_MAX	(__PROTO_DESC_MAX - 1)
+
 /**
  * struct proto_desc - protocol header description
  *
  * @name:	protocol name
+ * @id:		protocol identifier
  * @base:	header base
  * @checksum_key: key of template containing checksum
  * @protocol_key: key of template containing upper layer protocol description
@@ -77,6 +101,7 @@ struct proto_hdr_template {
  */
 struct proto_desc {
 	const char			*name;
+	enum proto_desc_id		id;
 	enum proto_bases		base;
 	unsigned int			checksum_key;
 	unsigned int			protocol_key;
@@ -160,6 +185,8 @@ extern const struct proto_desc *proto_find_upper(const struct proto_desc *base,
 extern int proto_find_num(const struct proto_desc *base,
 			  const struct proto_desc *desc);
 
+extern const struct proto_desc *proto_find_desc(enum proto_desc_id desc_id);
+
 enum eth_hdr_fields {
 	ETHHDR_INVALID,
 	ETHHDR_DADDR,
diff --git a/src/proto.c b/src/proto.c
index 40ce590efd12..7d001501d7d2 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -227,6 +227,7 @@ void proto_ctx_update(struct proto_ctx *ctx, enum proto_bases base,
 
 const struct proto_desc proto_ah = {
 	.name		= "ah",
+	.id		= PROTO_DESC_AH,
 	.base		= PROTO_BASE_TRANSPORT_HDR,
 	.protocol_key	= AHHDR_NEXTHDR,
 	.protocols	= {
@@ -263,6 +264,7 @@ const struct proto_desc proto_ah = {
 
 const struct proto_desc proto_esp = {
 	.name		= "esp",
+	.id		= PROTO_DESC_ESP,
 	.base		= PROTO_BASE_TRANSPORT_HDR,
 	.templates	= {
 		[ESPHDR_SPI]		= ESPHDR_FIELD("spi", spi),
@@ -279,6 +281,7 @@ const struct proto_desc proto_esp = {
 
 const struct proto_desc proto_comp = {
 	.name		= "comp",
+	.id		= PROTO_DESC_COMP,
 	.base		= PROTO_BASE_TRANSPORT_HDR,
 	.protocol_key	= COMPHDR_NEXTHDR,
 	.protocols	= {
@@ -343,6 +346,7 @@ const struct datatype icmp_type_type = {
 
 const struct proto_desc proto_icmp = {
 	.name		= "icmp",
+	.id		= PROTO_DESC_ICMP,
 	.base		= PROTO_BASE_TRANSPORT_HDR,
 	.checksum_key	= ICMPHDR_CHECKSUM,
 	.templates	= {
@@ -395,6 +399,7 @@ const struct datatype igmp_type_type = {
 
 const struct proto_desc proto_igmp = {
 	.name		= "igmp",
+	.id		= PROTO_DESC_IGMP,
 	.base		= PROTO_BASE_TRANSPORT_HDR,
 	.checksum_key	= IGMPHDR_CHECKSUM,
 	.templates	= {
@@ -415,6 +420,7 @@ const struct proto_desc proto_igmp = {
 
 const struct proto_desc proto_udp = {
 	.name		= "udp",
+	.id		= PROTO_DESC_UDP,
 	.base		= PROTO_BASE_TRANSPORT_HDR,
 	.checksum_key	= UDPHDR_CHECKSUM,
 	.templates	= {
@@ -427,6 +433,7 @@ const struct proto_desc proto_udp = {
 
 const struct proto_desc proto_udplite = {
 	.name		= "udplite",
+	.id		= PROTO_DESC_UDPLITE,
 	.base		= PROTO_BASE_TRANSPORT_HDR,
 	.templates	= {
 		[UDPHDR_SPORT]		= INET_SERVICE("sport", struct udphdr, source),
@@ -472,6 +479,7 @@ const struct datatype tcp_flag_type = {
 
 const struct proto_desc proto_tcp = {
 	.name		= "tcp",
+	.id		= PROTO_DESC_TCP,
 	.base		= PROTO_BASE_TRANSPORT_HDR,
 	.checksum_key	= TCPHDR_CHECKSUM,
 	.templates	= {
@@ -534,6 +542,7 @@ const struct datatype dccp_pkttype_type = {
 
 const struct proto_desc proto_dccp = {
 	.name		= "dccp",
+	.id		= PROTO_DESC_DCCP,
 	.base		= PROTO_BASE_TRANSPORT_HDR,
 	.templates	= {
 		[DCCPHDR_SPORT]		= INET_SERVICE("sport", struct dccp_hdr, dccph_sport),
@@ -552,6 +561,7 @@ const struct proto_desc proto_dccp = {
 
 const struct proto_desc proto_sctp = {
 	.name		= "sctp",
+	.id		= PROTO_DESC_SCTP,
 	.base		= PROTO_BASE_TRANSPORT_HDR,
 	.templates	= {
 		[SCTPHDR_SPORT]		= INET_SERVICE("sport", struct sctphdr, source),
@@ -566,6 +576,7 @@ const struct proto_desc proto_sctp = {
  */
 const struct proto_desc proto_th = {
 	.name		= "th",
+	.id		= PROTO_DESC_TH,
 	.base		= PROTO_BASE_TRANSPORT_HDR,
 	.templates	= {
 		[THDR_SPORT]		= INET_SERVICE("sport", struct udphdr, source),
@@ -648,6 +659,7 @@ const struct datatype ecn_type = {
 
 const struct proto_desc proto_ip = {
 	.name		= "ip",
+	.id		= PROTO_DESC_IP,
 	.base		= PROTO_BASE_NETWORK_HDR,
 	.checksum_key	= IPHDR_CHECKSUM,
 	.protocols	= {
@@ -744,6 +756,7 @@ const struct datatype icmp6_type_type = {
 
 const struct proto_desc proto_icmp6 = {
 	.name		= "icmpv6",
+	.id		= PROTO_DESC_ICMPV6,
 	.base		= PROTO_BASE_TRANSPORT_HDR,
 	.checksum_key	= ICMP6HDR_CHECKSUM,
 	.templates	= {
@@ -771,6 +784,7 @@ const struct proto_desc proto_icmp6 = {
 
 const struct proto_desc proto_ip6 = {
 	.name		= "ip6",
+	.id		= PROTO_DESC_IP6,
 	.base		= PROTO_BASE_NETWORK_HDR,
 	.protocols	= {
 		PROTO_LINK(IPPROTO_ESP,		&proto_esp),
@@ -892,6 +906,7 @@ const struct datatype arpop_type = {
 
 const struct proto_desc proto_arp = {
 	.name		= "arp",
+	.id		= PROTO_DESC_ARP,
 	.base		= PROTO_BASE_NETWORK_HDR,
 	.templates	= {
 		[ARPHDR_HRD]		= ARPHDR_FIELD("htype",	htype),
@@ -925,6 +940,7 @@ const struct proto_desc proto_arp = {
 
 const struct proto_desc proto_vlan = {
 	.name		= "vlan",
+	.id		= PROTO_DESC_VLAN,
 	.base		= PROTO_BASE_LL_HDR,
 	.protocol_key	= VLANHDR_TYPE,
 	.length		= sizeof(struct vlan_hdr) * BITS_PER_BYTE,
@@ -996,6 +1012,7 @@ const struct datatype ethertype_type = {
 
 const struct proto_desc proto_eth = {
 	.name		= "ether",
+	.id		= PROTO_DESC_ETHER,
 	.base		= PROTO_BASE_LL_HDR,
 	.protocol_key	= ETHHDR_TYPE,
 	.length		= sizeof(struct ether_header) * BITS_PER_BYTE,
@@ -1034,3 +1051,32 @@ const struct proto_desc proto_netdev = {
 		[0]	= PROTO_META_TEMPLATE("protocol", &ethertype_type, NFT_META_PROTOCOL, 16),
 	},
 };
+
+static const struct proto_desc *proto_definitions[PROTO_DESC_MAX + 1] = {
+	[PROTO_DESC_AH]		= &proto_ah,
+	[PROTO_DESC_ESP]	= &proto_esp,
+	[PROTO_DESC_COMP]	= &proto_comp,
+	[PROTO_DESC_ICMP]	= &proto_icmp,
+	[PROTO_DESC_IGMP]	= &proto_igmp,
+	[PROTO_DESC_UDP]	= &proto_udp,
+	[PROTO_DESC_UDPLITE]	= &proto_udplite,
+	[PROTO_DESC_TCP]	= &proto_tcp,
+	[PROTO_DESC_DCCP]	= &proto_dccp,
+	[PROTO_DESC_SCTP]	= &proto_sctp,
+	[PROTO_DESC_TH]		= &proto_th,
+	[PROTO_DESC_IP]		= &proto_ip,
+	[PROTO_DESC_IP6]	= &proto_ip6,
+	[PROTO_DESC_ICMPV6]	= &proto_icmp6,
+	[PROTO_DESC_ARP]	= &proto_arp,
+	[PROTO_DESC_VLAN]	= &proto_vlan,
+	[PROTO_DESC_ETHER]	= &proto_eth,
+};
+
+const struct proto_desc *proto_find_desc(enum proto_desc_id desc_id)
+{
+	if (desc_id >= PROTO_DESC_UNKNOWN &&
+	    desc_id <= PROTO_DESC_MAX)
+		return proto_definitions[desc_id];
+
+	return NULL;
+}
-- 
2.11.0

[PATCH nft 2/3] expr: add expr_ops_by_type()

[Pablo Neira Ayuso]

Fetch expression operation from the expression type.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/expression.h |  1 +
 src/expression.c     | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/include/expression.h b/include/expression.h
index 717b67550381..d502fc2a8611 100644
--- a/include/expression.h
+++ b/include/expression.h
@@ -169,6 +169,7 @@ struct expr_ops {
 };
 
 const struct expr_ops *expr_ops(const struct expr *e);
+const struct expr_ops *expr_ops_by_type(enum expr_types etype);
 
 /**
  * enum expr_flags
diff --git a/src/expression.c b/src/expression.c
index 6fa2f1dd9b12..a7bbde7eec1a 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -1222,3 +1222,15 @@ const struct expr_ops *expr_ops(const struct expr *e)
 
 	BUG("Unknown expression type %d\n", e->etype);
 }
+
+const struct expr_ops *expr_ops_by_type(enum expr_types etype)
+{
+	switch (etype) {
+	case EXPR_PAYLOAD:
+		return &payload_expr_ops;
+	default:
+		break;
+	}
+
+	BUG("Unknown expression type %d\n", etype);
+}
-- 
2.11.0

[PATCH nft 3/3] expr: add parse and build userdata interface

[Pablo Neira Ayuso]

This patch adds two new expression operations to build and to parse the
userdata area that describe the set key and data typeof definitions.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/expression.h |   4 ++
 src/mnl.c            |  28 +++++---------
 src/netlink.c        | 105 +++++++++++++++++----------------------------------
 src/payload.c        |  80 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 128 insertions(+), 89 deletions(-)

diff --git a/include/expression.h b/include/expression.h
index d502fc2a8611..b3e79c490b1a 100644
--- a/include/expression.h
+++ b/include/expression.h
@@ -10,6 +10,7 @@
 #include <utils.h>
 #include <list.h>
 #include <json.h>
+#include <libnftnl/udata.h>
 
 /**
  * enum expr_types
@@ -166,6 +167,9 @@ struct expr_ops {
 				       const struct expr *e2);
 	void			(*pctx_update)(struct proto_ctx *ctx,
 					       const struct expr *expr);
+	int			(*build_udata)(struct nftnl_udata_buf *udbuf,
+					       const struct expr *expr);
+	struct expr *		(*parse_udata)(const struct nftnl_udata *ud);
 };
 
 const struct expr_ops *expr_ops(const struct expr *e);
diff --git a/src/mnl.c b/src/mnl.c
index bcf633002f15..1d6d82e3332d 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -816,28 +816,18 @@ static void set_key_expression(struct netlink_ctx *ctx,
 				struct nftnl_udata_buf *udbuf,
 				unsigned int type)
 {
-	struct output_ctx octx = {};
-	char buf[64];
+	struct nftnl_udata *nest1, *nest2;
 
 	if (expr->flags & EXPR_F_CONSTANT ||
 	    set_flags & NFT_SET_ANONYMOUS)
 		return;
 
-	buf[0] = 0;
-	/* Set definition uses typeof to define datatype. */
-	octx.output_fp = fmemopen(buf, sizeof(buf), "w");
-	if (octx.output_fp) {
-		char *end;
-
-		expr_print(expr, &octx);
-		fclose(octx.output_fp);
-		end = strchr(buf, '&');
-		if (end)
-			* end = 0;
-
-		if (!nftnl_udata_put(udbuf, type, strlen(buf) + 1, buf))
-			memory_allocation_error();
-	}
+	nest1 = nftnl_udata_nest_start(udbuf, type);
+	nftnl_udata_put_u32(udbuf, NFTNL_UDATA_SET_TYPEOF_EXPR, expr->etype);
+	nest2 = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_TYPEOF_DATA);
+	expr_ops(expr)->build_udata(udbuf, expr);
+	nftnl_udata_nest_end(udbuf, nest2);
+	nftnl_udata_nest_end(udbuf, nest1);
 }
 
 /*
@@ -910,9 +900,9 @@ int mnl_nft_set_add(struct netlink_ctx *ctx, const struct cmd *cmd,
 				 set->automerge))
 		memory_allocation_error();
 
-	set_key_expression(ctx, set->key, set->flags, udbuf, NFTNL_UDATA_SET_TYPEOF_KEY);
+	set_key_expression(ctx, set->key, set->flags, udbuf, NFTNL_UDATA_SET_KEY_TYPEOF);
 	if (set->data)
-		set_key_expression(ctx, set->data, set->flags, udbuf, NFTNL_UDATA_SET_TYPEOF_DATA);
+		set_key_expression(ctx, set->data, set->flags, udbuf, NFTNL_UDATA_SET_DATA_TYPEOF);
 
 	nftnl_set_set_data(nls, NFTNL_SET_USERDATA, nftnl_udata_buf_data(udbuf),
 			   nftnl_udata_buf_len(udbuf));
diff --git a/src/netlink.c b/src/netlink.c
index 790c663ccd6d..3ceeba68dc94 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -27,6 +27,7 @@
 #include <libnftnl/udata.h>
 #include <libnftnl/ruleset.h>
 #include <libnftnl/common.h>
+#include <libnftnl/udata.h>
 #include <linux/netfilter/nfnetlink.h>
 #include <linux/netfilter/nf_tables.h>
 #include <linux/netfilter.h>
@@ -571,8 +572,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_TYPEOF_KEY:
-	case NFTNL_UDATA_SET_TYPEOF_DATA:
+	case NFTNL_UDATA_SET_KEY_TYPEOF:
+	case NFTNL_UDATA_SET_DATA_TYPEOF:
 		if (len < 3)
 			return -1;
 		break;
@@ -583,87 +584,50 @@ static int set_parse_udata_cb(const struct nftnl_udata *attr, void *data)
 	return 0;
 }
 
-static int parse_desc(struct nft_ctx *nft, const char *buf, struct list_head *cmds)
+static int set_key_parse_udata(const struct nftnl_udata *attr, void *data)
 {
-	static const struct input_descriptor indesc_udata = {
-		.type	= INDESC_BUFFER,
-		.name	= "<setudata>",
-	};
-	struct error_record *erec, *nexte;
-	LIST_HEAD(errors);
-	int ret;
-
-	parser_init(nft, nft->state, &errors, cmds, nft->top_scope);
-	nft->scanner = scanner_init(nft->state);
-	scanner_push_buffer(nft->scanner, &indesc_udata, buf);
-
-	ret = nft_parse(nft, nft->scanner, nft->state);
-
-	list_for_each_entry_safe(erec, nexte, &errors, list) {
-		list_del(&erec->list);
-		erec_destroy(erec);
-	}
+	const struct nftnl_udata **tb = data;
+	uint8_t type = nftnl_udata_type(attr);
+	uint8_t len = nftnl_udata_len(attr);
 
-	if (nft->scanner) {
-		scanner_destroy(nft);
-		nft->scanner = NULL;
+	switch (type) {
+	case NFTNL_UDATA_SET_TYPEOF_EXPR:
+		if (len != sizeof(uint32_t))
+			return -1;
+		break;
+	case NFTNL_UDATA_SET_TYPEOF_DATA:
+		break;
+	default:
+		return 0;
 	}
-
-	if (ret != 0 || nft->state->nerrs > 0)
-		return -1;
-
+	tb[type] = attr;
 	return 0;
 }
 
-static struct expr *set_make_key(const struct nftnl_udata *ud)
+static struct expr *set_make_key(const struct nftnl_udata *attr)
 {
-	struct cmd *cmd, *nextc;
-	struct nft_ctx *nft;
-	char cmdline[1024];
+	const struct nftnl_udata *ud[NFTNL_UDATA_SET_TYPEOF_MAX + 1] = {};
+	const struct expr_ops *ops;
+	enum expr_types etype;
 	struct expr *expr;
-	const char *buf;
-	LIST_HEAD(cmds);
-	uint8_t len;
-	int i;
-
-	if (!ud)
-		return NULL;
+	int err;
 
-	len = nftnl_udata_len(ud);
-	if (!len)
+	err = nftnl_udata_parse(nftnl_udata_get(attr), nftnl_udata_len(attr),
+				set_key_parse_udata, ud);
+	if (err < 0)
 		return NULL;
 
-	buf = nftnl_udata_get(ud);
-	if (!buf)
+	if (!ud[NFTNL_UDATA_SET_TYPEOF_EXPR] ||
+	    !ud[NFTNL_UDATA_SET_TYPEOF_DATA])
 		return NULL;
 
-	for (i = 0; i < len; i++) {
-		if (buf[i] > 'z')
-			return NULL;
-		if (buf[i] == ' ' || buf[i] == '6')
-			continue;
-		if (buf[i] < 'a') {
-			if (buf[i] == '\0' && i == len - 1)
-				continue;
+	etype = nftnl_udata_get_u32(ud[NFTNL_UDATA_SET_TYPEOF_EXPR]);
+	ops = expr_ops_by_type(etype);
 
-			return NULL;
-		}
-	}
-
-	snprintf(cmdline, sizeof(cmdline), "describe %s\n", buf);
-
-	nft = __nft_ctx_new();
-	parse_desc(nft, cmdline, &cmds);
-
-	expr = NULL;
-	list_for_each_entry_safe(cmd, nextc, &cmds, list) {
-		if (cmd->op == CMD_DESCRIBE && !expr)
-			expr = expr_get(cmd->expr);
-		list_del(&cmd->list);
-		cmd_free(cmd);
-	}
+	expr = ops->parse_udata(ud[NFTNL_UDATA_SET_TYPEOF_DATA]);
+	if (!expr)
+		return NULL;
 
-	__nft_ctx_free(nft);
 	return expr;
 }
 
@@ -710,8 +674,9 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
 		GET_U32_UDATA(automerge, NFTNL_UDATA_SET_MERGE_ELEMENTS);
 
 #undef GET_U32_UDATA
-		typeof_expr_key = set_make_key(ud[NFTNL_UDATA_SET_TYPEOF_KEY]);
-		typeof_expr_data = set_make_key(ud[NFTNL_UDATA_SET_TYPEOF_DATA]);
+		typeof_expr_key = set_make_key(ud[NFTNL_UDATA_SET_KEY_TYPEOF]);
+		if (ud[NFTNL_UDATA_SET_DATA_TYPEOF])
+			typeof_expr_data = set_make_key(ud[NFTNL_UDATA_SET_DATA_TYPEOF]);
 	}
 
 	key = nftnl_set_get_u32(nls, NFTNL_SET_KEY_TYPE);
diff --git a/src/payload.c b/src/payload.c
index 3576400bbfc8..664d95df67e8 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -105,6 +105,84 @@ static void payload_expr_pctx_update(struct proto_ctx *ctx,
 	proto_ctx_update(ctx, desc->base, &expr->location, desc);
 }
 
+#define NFTNL_SET_KEY_PAYLOAD_DESC 1
+#define NFTNL_SET_KEY_PAYLOAD_TYPE 2
+
+static unsigned int expr_payload_type(const struct proto_desc *desc,
+				      const struct proto_hdr_template *tmpl)
+{
+	unsigned int offset = (unsigned int)(tmpl - &desc->templates[0]);
+
+	return offset / sizeof(*tmpl);
+}
+
+static int payload_expr_build_udata(struct nftnl_udata_buf *udbuf,
+				    const struct expr *expr)
+{
+	const struct proto_hdr_template *tmpl = expr->payload.tmpl;
+	const struct proto_desc *desc = expr->payload.desc;
+	unsigned int type = expr_payload_type(desc, tmpl);
+
+	nftnl_udata_put_u32(udbuf, NFTNL_SET_KEY_PAYLOAD_DESC, desc->id);
+	nftnl_udata_put_u32(udbuf, NFTNL_SET_KEY_PAYLOAD_TYPE, type);
+
+	return 0;
+}
+
+static const struct proto_desc *find_proto_desc(const struct nftnl_udata *ud)
+{
+	return NULL;
+}
+
+#define NFTNL_UDATA_SET_KEY_PAYLOAD_DESC 0
+#define NFTNL_UDATA_SET_KEY_PAYLOAD_TYPE 1
+#define NFTNL_UDATA_SET_KEY_PAYLOAD_MAX 2
+
+static int payload_parse_udata(const struct nftnl_udata *attr, void *data)
+{
+	const struct nftnl_udata **ud = data;
+	uint8_t type = nftnl_udata_type(attr);
+	uint8_t len = nftnl_udata_len(attr);
+
+	switch (type) {
+	case NFTNL_UDATA_SET_KEY_PAYLOAD_DESC:
+	case NFTNL_UDATA_SET_KEY_PAYLOAD_TYPE:
+		if (len != sizeof(uint32_t))
+			return -1;
+		break;
+	default:
+		return 0;
+	}
+
+	ud[type] = attr;
+	return 0;
+}
+
+static struct expr *payload_expr_parse_udata(const struct nftnl_udata *attr)
+{
+	const struct nftnl_udata *ud[NFTNL_UDATA_SET_KEY_PAYLOAD_MAX + 1] = {};
+	const struct proto_desc *desc;
+	unsigned int type;
+	int err;
+
+	err = nftnl_udata_parse(nftnl_udata_get(attr), nftnl_udata_len(attr),
+				payload_parse_udata, ud);
+	if (err < 0)
+		return NULL;
+
+	if (!ud[NFTNL_UDATA_SET_KEY_PAYLOAD_DESC] ||
+	    !ud[NFTNL_UDATA_SET_KEY_PAYLOAD_TYPE])
+		return NULL;
+
+	desc = find_proto_desc(ud[NFTNL_SET_KEY_PAYLOAD_DESC]);
+	if (!desc)
+		return NULL;
+
+	type = nftnl_udata_get_u32(ud[NFTNL_SET_KEY_PAYLOAD_TYPE]);
+
+	return payload_expr_alloc(&internal_location, desc, type);
+}
+
 const struct expr_ops payload_expr_ops = {
 	.type		= EXPR_PAYLOAD,
 	.name		= "payload",
@@ -113,6 +191,8 @@ const struct expr_ops payload_expr_ops = {
 	.cmp		= payload_expr_cmp,
 	.clone		= payload_expr_clone,
 	.pctx_update	= payload_expr_pctx_update,
+	.build_udata	= payload_expr_build_udata,
+	.parse_udata	= payload_expr_parse_udata,
 };
 
 /*
-- 
2.11.0

Re: [PATCH nft 0/3] typeof incremental enhancements

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Hi Florian,
> 
> This patchset removes the need to self invoke the parser and the
> evaluation to fetch the datatype. Instead, the expression type and
> the expression description are stored into the userdata area.
> 
> This patch only supports for the payload expression, but it should be
> relatively easy to extend it to support for other existing expressions
> types.
> 
> This patch could be squashed into 06/11 src: add "typeof" print support
> of your patch series, which is actually not just adding support for
> printing but also for building the userdata.

I had considered that but found that storing netlink data
needs more space in the udata area compared to text and it needs more/extra
parsing for serialize/deserialize, so I abandoned this idea.

If you think its the way to go, then ok, I can rework it but
I will be unable to add the extra steps for other expression types
for some time I fear.

Re: [PATCH nft 0/3] typeof incremental enhancements

[Pablo Neira Ayuso]

On Mon, Dec 16, 2019 at 01:47:49PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Hi Florian,
> > 
> > This patchset removes the need to self invoke the parser and the
> > evaluation to fetch the datatype. Instead, the expression type and
> > the expression description are stored into the userdata area.
> > 
> > This patch only supports for the payload expression, but it should be
> > relatively easy to extend it to support for other existing expressions
> > types.
> > 
> > This patch could be squashed into 06/11 src: add "typeof" print support
> > of your patch series, which is actually not just adding support for
> > printing but also for building the userdata.
> 
> I had considered that but found that storing netlink data
> needs more space in the udata area compared to text and it needs more/extra
> parsing for serialize/deserialize, so I abandoned this idea.

Yes, it's scratching a bit extra of the userdata area.

The listing path should be easier, since it's just parsing the TLVs
instead of invoking the nft parsing and evaluation phases.

> If you think its the way to go, then ok, I can rework it but
> I will be unable to add the extra steps for other expression types
> for some time I fear.

If you send a v3 including this work, I'll finishing the remaining
expressions.

One more thing regarding your patchset is:

        integer,128

If the typeof works for all of the existing selectors, then I think
there is not need to expose this raw type, right?

Thanks.

Re: [PATCH nft 0/3] typeof incremental enhancements

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> The listing path should be easier, since it's just parsing the TLVs
> instead of invoking the nft parsing and evaluation phases.
> 
> > If you think its the way to go, then ok, I can rework it but
> > I will be unable to add the extra steps for other expression types
> > for some time I fear.
> 
> If you send a v3 including this work, I'll finishing the remaining
> expressions.

Ok, will respin.

> One more thing regarding your patchset is:
> 
>         integer,128
> 
> If the typeof works for all of the existing selectors, then I think
> there is not need to expose this raw type, right?

How would you handle the 'udate missing' case?

If its not a problem to display a non-restoreable ruleset
(e.g. unspecific 'type integer' shown as set keys) in that case
then the interger,width part can be omitted indeed.

Let me know.  For concatenations, we will be unable to show
a proper ruleset without the udata info anyway (concatentations
do not work at the moment for non-specific types anyway though).

Re: [PATCH nft 0/3] typeof incremental enhancements

[Pablo Neira Ayuso]

On Mon, Dec 16, 2019 at 03:03:36PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > The listing path should be easier, since it's just parsing the TLVs
> > instead of invoking the nft parsing and evaluation phases.
> > 
> > > If you think its the way to go, then ok, I can rework it but
> > > I will be unable to add the extra steps for other expression types
> > > for some time I fear.
> > 
> > If you send a v3 including this work, I'll finishing the remaining
> > expressions.
> 
> Ok, will respin.

Thanks!

> > One more thing regarding your patchset is:
> > 
> >         integer,128
> > 
> > If the typeof works for all of the existing selectors, then I think
> > there is not need to expose this raw type, right?
> 
> How would you handle the 'udate missing' case?
>
> If its not a problem to display a non-restoreable ruleset
> (e.g. unspecific 'type integer' shown as set keys) in that case
> then the interger,width part can be omitted indeed.
> 
> Let me know.  For concatenations, we will be unable to show
> a proper ruleset without the udata info anyway (concatentations
> do not work at the moment for non-specific types anyway though).

Indeed, what scenario are you considering that set udata might be
missing?

We could still print it in such a case, even if we cannot parse it if
you are willing to deal with. Just to provide some information to the
user.

Re: [PATCH nft 0/3] typeof incremental enhancements

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > If its not a problem to display a non-restoreable ruleset
> > (e.g. unspecific 'type integer' shown as set keys) in that case
> > then the interger,width part can be omitted indeed.
> > 
> > Let me know.  For concatenations, we will be unable to show
> > a proper ruleset without the udata info anyway (concatentations
> > do not work at the moment for non-specific types anyway though).
> 
> Indeed, what scenario are you considering that set udata might be
> missing?

Any non-nft client/direct netlink user.

> We could still print it in such a case, even if we cannot parse it if
> you are willing to deal with. Just to provide some information to the
> user.

If udata is missing, we only have the type available.

If its a type with unspecific length (string, integer) we can use
the key length to get the bit size.

But for concatenation case, it might be ambigiuos.

So, I would remove the "type integer, length" format again so in
such case we would print

type string
or
type integer.

Users won't see this non-restoreable ruleset listed as long as the udata
is there.

Re: [PATCH nft 0/3] typeof incremental enhancements

[Pablo Neira Ayuso]

On Mon, Dec 16, 2019 at 04:48:41PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > If its not a problem to display a non-restoreable ruleset
> > > (e.g. unspecific 'type integer' shown as set keys) in that case
> > > then the interger,width part can be omitted indeed.
> > > 
> > > Let me know.  For concatenations, we will be unable to show
> > > a proper ruleset without the udata info anyway (concatentations
> > > do not work at the moment for non-specific types anyway though).
> > 
> > Indeed, what scenario are you considering that set udata might be
> > missing?
> 
> Any non-nft client/direct netlink user.

Ah I see, as direct uapi users.

> > We could still print it in such a case, even if we cannot parse it if
> > you are willing to deal with. Just to provide some information to the
> > user.
> 
> If udata is missing, we only have the type available.
>
> If its a type with unspecific length (string, integer) we can use
> the key length to get the bit size.
> 
> But for concatenation case, it might be ambigiuos.
> 
> So, I would remove the "type integer, length" format again so in
> such case we would print
> 
> type string
> or
> type integer.
>
> Users won't see this non-restoreable ruleset listed as long as the udata
> is there.

That's good enough by now, I think, thanks.

Once we get more of those users, if they want some sort of interaction
with nft, then we probably revisit this. Those clients might not even
use nft for listing the ruleset at all.

Re: [PATCH nft 0/3] typeof incremental enhancements

[Pablo Neira Ayuso]

Hi Florian,

One more thought regarding userdata, in case this is provided by the
user, eg.

table x {
        set  y {
                typeof ip saddr
        }
}

I think 'nft list ruleset' should also print the same input, instead
of falling back to 'type ipv4_addr'.

Thanks.