[PATCH libnftnl 0/3] add description infrastructure

[PATCH libnftnl 0/3] add description infrastructure

[Pablo Neira Ayuso]

Hi Phil,

This is my proposal to address the snprintf data printing depending on
the arch. The idea is to add description objects that can be used to
build the userdata area as well as to parse the userdata to create the
description object.

This is revisiting 6e48df5329ea ("src: add "typeof" build/parse/print
support") in nftables which adds build and parse userdata callbacks to
expression in libnftables. My proposal is to move this to libnftnl.

This allows to consolidate codebase to address two different usecases:
- you can pass the description object to the snprintf function.
- it provides helpers to build and to parse the userdata area. The
  userdata TLV attributes do not need to be exposed through
  libnftnl/udata.h anymore, instead users can just rely on these helper
  functions.

The userdata area has been extended with new attributes, but it is still
incomplete since it does not allow to represent a concatenation.

Note that this will also allow us to deprecate NFTA_SET_DATA_TYPE at
some point (this netlink attribute represents a concatenation using 6
bits of the 32-bit integer, hence limiting concatenations to 5
components at this stage).

I'm afraid we'll also have to keep the existing userdata attributes
added by 6e48df5329ea in nftables for a little while (at least the
parser functions), so nftables does not break on updates since my
userdata TLV coming in this patchset for libnftnl is different than the
one available at 6e48df5329ea.

Please also note that nftables needs to be updated to use this
infrastructure.

My proposal follows a longer route but it will allow to addressing a
number of existing shortcomings in the set infrastrcture.

This is compile-tested only.

Pablo Neira Ayuso (3):
  desc: add expression description
  desc: add datatype description
  desc: add set description

 include/Makefile.am          |   1 +
 include/desc.h               |  50 +++
 include/expr_ops.h           |  11 +
 include/internal.h           |   1 +
 include/libnftnl/Makefile.am |   1 +
 include/libnftnl/desc.h      | 107 +++++++
 include/libnftnl/udata.h     |  18 +-
 src/Makefile.am              |   1 +
 src/desc.c                   | 598 +++++++++++++++++++++++++++++++++++
 src/expr/payload.c           |  81 +++++
 src/expr_ops.c               |  13 +
 11 files changed, 875 insertions(+), 7 deletions(-)
 create mode 100644 include/desc.h
 create mode 100644 include/libnftnl/desc.h
 create mode 100644 src/desc.c

-- 
2.30.2

[PATCH libnftnl 1/3] desc: add expression description

[Pablo Neira Ayuso]

Add a new object to describe an expression. This allows to describe the
set key when typeof is used to define the set.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/Makefile.am          |   1 +
 include/desc.h               |  19 +++++
 include/expr_ops.h           |  11 +++
 include/internal.h           |   1 +
 include/libnftnl/Makefile.am |   1 +
 include/libnftnl/desc.h      |  57 ++++++++++++++
 src/Makefile.am              |   1 +
 src/desc.c                   | 142 +++++++++++++++++++++++++++++++++++
 src/expr/payload.c           |  81 ++++++++++++++++++++
 src/expr_ops.c               |  13 ++++
 10 files changed, 327 insertions(+)
 create mode 100644 include/desc.h
 create mode 100644 include/libnftnl/desc.h
 create mode 100644 src/desc.c

diff --git a/include/Makefile.am b/include/Makefile.am
index 738f80708ca0..83a0e5209ed7 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -3,6 +3,7 @@ SUBDIRS = libnftnl linux
 noinst_HEADERS = internal.h	\
 		 linux_list.h	\
 		 data_reg.h	\
+		 desc.h		\
 		 expr_ops.h	\
 		 obj.h		\
 		 linux_list.h	\
diff --git a/include/desc.h b/include/desc.h
new file mode 100644
index 000000000000..d78af3528118
--- /dev/null
+++ b/include/desc.h
@@ -0,0 +1,19 @@
+#ifndef _LIBNFTNL_DESC_INTERNAL_H_
+#define _LIBNFTNL_DESC_INTERNAL_H_
+
+struct nftnl_expr_desc {
+	uint32_t	etype;
+	uint32_t	byteorder;
+	uint32_t	len;
+
+	struct expr_ops *ops;
+
+	union {
+		struct {
+			uint32_t desc_id;
+			uint32_t type;
+		} payload;
+	};
+};
+
+#endif
diff --git a/include/expr_ops.h b/include/expr_ops.h
index 7a6aa23f9bd1..77e010bf2a7d 100644
--- a/include/expr_ops.h
+++ b/include/expr_ops.h
@@ -2,14 +2,19 @@
 #define _EXPR_OPS_H_
 
 #include <stdint.h>
+#include <libnftnl/desc.h>
 #include "internal.h"
 
 struct nlattr;
 struct nlmsghdr;
 struct nftnl_expr;
+struct nftnl_expr_desc;
+struct nftnl_udata;
+struct nftnl_udata_buf;
 
 struct expr_ops {
 	const char *name;
+	enum nftnl_expr_desc_type type;
 	uint32_t alloc_len;
 	int	max_attr;
 	void	(*init)(const struct nftnl_expr *e);
@@ -19,9 +24,15 @@ struct expr_ops {
 	int 	(*parse)(struct nftnl_expr *e, struct nlattr *attr);
 	void	(*build)(struct nlmsghdr *nlh, const struct nftnl_expr *e);
 	int	(*snprintf)(char *buf, size_t len, uint32_t flags, const struct nftnl_expr *e);
+	struct {
+		int	(*set)(struct nftnl_expr_desc *uexpr, uint8_t type, const void *data, uint32_t data_len);
+		int	(*build)(struct nftnl_udata_buf *ud, const struct nftnl_expr_desc *uexpr);
+		int	(*parse)(const struct nftnl_udata *attr, struct nftnl_expr_desc *uexpr);
+	} desc;
 };
 
 struct expr_ops *nftnl_expr_ops_lookup(const char *name);
+struct expr_ops *nftnl_expr_ops_lookup_by_type(uint32_t type);
 
 #define nftnl_expr_data(ops) (void *)ops->data
 
diff --git a/include/internal.h b/include/internal.h
index 1f96731589c0..7b058aa2ced8 100644
--- a/include/internal.h
+++ b/include/internal.h
@@ -12,5 +12,6 @@
 #include "expr.h"
 #include "expr_ops.h"
 #include "rule.h"
+#include "desc.h"
 
 #endif /* _LIBNFTNL_INTERNAL_H_ */
diff --git a/include/libnftnl/Makefile.am b/include/libnftnl/Makefile.am
index d846a574f438..3d072ed8642b 100644
--- a/include/libnftnl/Makefile.am
+++ b/include/libnftnl/Makefile.am
@@ -1,4 +1,5 @@
 pkginclude_HEADERS = batch.h		\
+		     desc.h		\
 		     table.h		\
 		     trace.h		\
 		     chain.h		\
diff --git a/include/libnftnl/desc.h b/include/libnftnl/desc.h
new file mode 100644
index 000000000000..1202eb9ddf79
--- /dev/null
+++ b/include/libnftnl/desc.h
@@ -0,0 +1,57 @@
+#ifndef _LIBNFTNL_DESC_H_
+#define _LIBNFTNL_DESC_H_
+
+#include <stdio.h>
+#include <stdint.h>
+#include <stdbool.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* from include/expression.h in libnftables */
+enum nftnl_expr_desc_type {
+	NFTNL_EXPR_UNSPEC	= 0,
+	NFTNL_EXPR_PAYLOAD	= 7,
+	NFTNL_EXPR_EXTHDR,
+	NFTNL_EXPR_META,
+	NFTNL_EXPR_SOCKET,
+	NFTNL_EXPR_OSF,
+	NFTNL_EXPR_CT,
+	NFTNL_EXPR_RT		= 25,
+};
+
+#define NFTNL_DESC_EXPR_BASE		16
+
+enum nftnl_expr_desc_types {
+	NFTNL_DESC_EXPR_TYPE,
+	NFTNL_DESC_EXPR_BYTEORDER,
+	NFTNL_DESC_EXPR_LEN,
+	NFTNL_DESC_EXPR_DATA		= NFTNL_DESC_EXPR_BASE,
+	__NFTNL_DESC_EXPR_MAX
+};
+#define NFTNL_DESC_EXPR_MAX (__NFTNL_DESC_EXPR_MAX - 1)
+
+enum nftnl_expr_desc_payload_types {
+	NFTNL_DESC_PAYLOAD_DESC		= NFTNL_DESC_EXPR_BASE,
+	NFTNL_DESC_PAYLOAD_TYPE
+};
+
+struct nftnl_expr_desc;
+struct nftnl_udata_buf;
+struct nftnl_udata;
+
+struct nftnl_expr_desc *nftnl_expr_desc_alloc(void);
+void nftnl_expr_desc_free(struct nftnl_expr_desc *dexpr);
+int nftnl_expr_desc_set(struct nftnl_expr_desc *dexpr, uint8_t type,
+			const void *data, uint32_t data_len);
+int nftnl_expr_desc_build(struct nftnl_udata_buf *udbuf,
+			  const struct nftnl_expr_desc *dexpr);
+int nftnl_expr_desc_parse(const struct nftnl_udata *attr,
+			  struct nftnl_expr_desc *dexpr);
+
+#ifdef __cplusplus
+} /* extern "C" */
+#endif
+
+#endif /* _LIBNFTNL_DESC_H_ */
diff --git a/src/Makefile.am b/src/Makefile.am
index c3b0ab974bd2..06e1567ffd13 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -7,6 +7,7 @@ libnftnl_la_LDFLAGS = -Wl,--version-script=$(srcdir)/libnftnl.map	\
 
 libnftnl_la_SOURCES = utils.c		\
 		      batch.c		\
+		      desc.c		\
 		      flowtable.c	\
 		      common.c		\
 		      gen.c		\
diff --git a/src/desc.c b/src/desc.c
new file mode 100644
index 000000000000..d8566ba2ee38
--- /dev/null
+++ b/src/desc.c
@@ -0,0 +1,142 @@
+/*
+ * (C) 2022 by Pablo Neira Ayuso <pablo@netfilter.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published
+ * by the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include "internal.h"
+
+#include <time.h>
+#include <endian.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <netinet/in.h>
+
+#include <libmnl/libmnl.h>
+#include <linux/netfilter/nfnetlink.h>
+#include <linux/netfilter/nf_tables.h>
+
+#include <libnftnl/udata.h>
+#include <libnftnl/desc.h>
+
+EXPORT_SYMBOL(nftnl_expr_desc_alloc);
+struct nftnl_expr_desc *nftnl_expr_desc_alloc(void)
+{
+	return calloc(1, sizeof(struct nftnl_expr_desc));
+}
+
+EXPORT_SYMBOL(nftnl_expr_desc_free);
+void nftnl_expr_desc_free(struct nftnl_expr_desc *dexpr)
+{
+	free(dexpr);
+}
+
+EXPORT_SYMBOL(nftnl_expr_desc_set);
+int nftnl_expr_desc_set(struct nftnl_expr_desc *dexpr, uint8_t type,
+			const void *data, uint32_t data_len)
+{
+	int err = 0;
+
+	switch (type) {
+	case NFTNL_DESC_EXPR_TYPE:
+		memcpy(&dexpr->etype, data, data_len);
+		dexpr->ops = nftnl_expr_ops_lookup_by_type(dexpr->etype);
+		break;
+	case NFTNL_DESC_EXPR_BYTEORDER:
+		memcpy(&dexpr->byteorder, data, data_len);
+		break;
+	case NFTNL_DESC_EXPR_LEN:
+		memcpy(&dexpr->len, data, data_len);
+		break;
+	case NFTNL_DESC_EXPR_DATA:
+		if (dexpr->ops && dexpr->ops->desc.set)
+			err = dexpr->ops->desc.set(dexpr, type, data, data_len);
+		break;
+	default:
+		err = -1;
+		break;
+	}
+
+	return err;
+}
+
+#define NFTNL_UDATA_EXPR_TYPE		0
+#define NFTNL_UDATA_EXPR_BYTEORDER	1
+#define NFTNL_UDATA_EXPR_LEN		2
+#define NFTNL_UDATA_EXPR_DATA		3
+#define NFTNL_UDATA_EXPR_MAX		4
+
+EXPORT_SYMBOL(nftnl_expr_desc_build);
+int nftnl_expr_desc_build(struct nftnl_udata_buf *udbuf,
+			  const struct nftnl_expr_desc *dexpr)
+{
+	struct nftnl_udata *nest;
+	int err = 0;
+
+	nftnl_udata_put_u32(udbuf, NFTNL_UDATA_EXPR_TYPE, dexpr->etype);
+	nftnl_udata_put_u32(udbuf, NFTNL_UDATA_EXPR_BYTEORDER, dexpr->byteorder);
+	nftnl_udata_put_u32(udbuf, NFTNL_UDATA_EXPR_LEN, dexpr->len);
+	if (dexpr->ops && dexpr->ops->desc.build) {
+		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_EXPR_DATA);
+		err = dexpr->ops->desc.build(udbuf, dexpr);
+		nftnl_udata_nest_end(udbuf, nest);
+	}
+
+	return err;
+}
+
+static int nftnl_expr_desc_parse_nested(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_EXPR_TYPE:
+	case NFTNL_UDATA_EXPR_BYTEORDER:
+	case NFTNL_UDATA_EXPR_LEN:
+		if (len != sizeof(uint32_t))
+			return -1;
+		break;
+	case NFTNL_UDATA_EXPR_DATA:
+		break;
+	default:
+	        return 0;
+	}
+
+	ud[type] = attr;
+	return 0;
+}
+
+EXPORT_SYMBOL(nftnl_expr_desc_parse);
+int nftnl_expr_desc_parse(const struct nftnl_udata *attr,
+			  struct nftnl_expr_desc *dexpr)
+{
+	const struct nftnl_udata *ud[NFTNL_UDATA_EXPR_MAX + 1] = {};
+	int err;
+
+	err = nftnl_udata_parse(nftnl_udata_get(attr), nftnl_udata_len(attr),
+				nftnl_expr_desc_parse_nested, ud);
+	if (err < 0)
+		return -1;
+
+	err = 0;
+	if (ud[NFTNL_UDATA_EXPR_TYPE]) {
+		dexpr->etype = nftnl_udata_get_u32(ud[NFTNL_UDATA_EXPR_TYPE]);
+		dexpr->ops = nftnl_expr_ops_lookup_by_type(dexpr->etype);
+	}
+	if (ud[NFTNL_UDATA_EXPR_BYTEORDER])
+		dexpr->byteorder = nftnl_udata_get_u32(ud[NFTNL_UDATA_EXPR_BYTEORDER]);
+	if (ud[NFTNL_UDATA_EXPR_LEN])
+		dexpr->len = nftnl_udata_get_u32(ud[NFTNL_UDATA_EXPR_LEN]);
+	if (ud[NFTNL_UDATA_EXPR_DATA]) {
+		if (dexpr->ops && dexpr->ops->desc.parse)
+			err = dexpr->ops->desc.parse(ud[NFTNL_UDATA_EXPR_DATA], dexpr);
+	}
+
+	return err;
+}
diff --git a/src/expr/payload.c b/src/expr/payload.c
index 82747ec8994f..afa0f2451155 100644
--- a/src/expr/payload.c
+++ b/src/expr/payload.c
@@ -23,6 +23,7 @@
 
 #include <libnftnl/expr.h>
 #include <libnftnl/rule.h>
+#include <libnftnl/udata.h>
 
 struct nftnl_expr_payload {
 	enum nft_registers	sreg;
@@ -251,8 +252,83 @@ nftnl_expr_payload_snprintf(char *buf, size_t len,
 				payload->offset, payload->dreg);
 }
 
+static int nftnl_expr_payload_desc_set(struct nftnl_expr_desc *dexpr,
+				       uint8_t type, const void *data,
+				       uint32_t data_len)
+{
+	switch (type) {
+	case NFTNL_DESC_PAYLOAD_DESC:
+		memcpy(&dexpr->payload.desc_id, data, sizeof(dexpr->payload.desc_id));
+		break;
+	case NFTNL_DESC_PAYLOAD_TYPE:
+		memcpy(&dexpr->payload.type, data, sizeof(dexpr->payload.type));
+		break;
+	default:
+		return -1;
+	}
+
+	return 0;
+}
+
+#define NFTNL_UDATA_PAYLOAD_DESC 0
+#define NFTNL_UDATA_PAYLOAD_TYPE 1
+#define NFTNL_UDATA_PAYLOAD_MAX 2
+
+static int nftnl_expr_payload_desc_build(struct nftnl_udata_buf *udbuf,
+					 const struct nftnl_expr_desc *dexpr)
+{
+	nftnl_udata_put_u32(udbuf, NFTNL_UDATA_PAYLOAD_DESC,
+			    dexpr->payload.desc_id);
+	nftnl_udata_put_u32(udbuf, NFTNL_UDATA_PAYLOAD_TYPE,
+			    dexpr->payload.type);
+
+	return 0;
+}
+
+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_PAYLOAD_DESC:
+	case NFTNL_UDATA_PAYLOAD_TYPE:
+		if (len != sizeof(uint32_t))
+			return -1;
+		break;
+	default:
+		return 0;
+	}
+
+	ud[type] = attr;
+	return 0;
+}
+
+static int nftnl_expr_payload_desc_parse(const struct nftnl_udata *attr,
+					 struct nftnl_expr_desc *dexpr)
+{
+	const struct nftnl_udata *ud[NFTNL_UDATA_PAYLOAD_MAX + 1] = {};
+	int err;
+
+	err = nftnl_udata_parse(nftnl_udata_get(attr), nftnl_udata_len(attr),
+				payload_parse_udata, ud);
+	if (err < 0)
+		return -1;
+
+	if (!ud[NFTNL_UDATA_PAYLOAD_DESC] ||
+	    !ud[NFTNL_UDATA_PAYLOAD_TYPE])
+		return -1;
+
+	dexpr->payload.desc_id = nftnl_udata_get_u32(ud[NFTNL_UDATA_PAYLOAD_DESC]);
+	dexpr->payload.type = nftnl_udata_get_u32(ud[NFTNL_UDATA_PAYLOAD_TYPE]);
+
+	return 0;
+}
+
 struct expr_ops expr_ops_payload = {
 	.name		= "payload",
+	.type		= NFTNL_EXPR_PAYLOAD,
 	.alloc_len	= sizeof(struct nftnl_expr_payload),
 	.max_attr	= NFTA_PAYLOAD_MAX,
 	.set		= nftnl_expr_payload_set,
@@ -260,4 +336,9 @@ struct expr_ops expr_ops_payload = {
 	.parse		= nftnl_expr_payload_parse,
 	.build		= nftnl_expr_payload_build,
 	.snprintf	= nftnl_expr_payload_snprintf,
+	.desc		= {
+		.set	= nftnl_expr_payload_desc_set,
+		.build	= nftnl_expr_payload_desc_build,
+		.parse	= nftnl_expr_payload_desc_parse,
+	},
 };
diff --git a/src/expr_ops.c b/src/expr_ops.c
index 7248e4f98b0a..383414a42619 100644
--- a/src/expr_ops.c
+++ b/src/expr_ops.c
@@ -102,3 +102,16 @@ struct expr_ops *nftnl_expr_ops_lookup(const char *name)
 	}
 	return NULL;
 }
+
+struct expr_ops *nftnl_expr_ops_lookup_by_type(uint32_t type)
+{
+	int i = 0;
+
+	while (expr_ops[i] != NULL) {
+		if (expr_ops[i]->type == type)
+			return expr_ops[i];
+
+		i++;
+	}
+	return NULL;
+}
-- 
2.30.2

[PATCH libnftnl 2/3] desc: add datatype description

[Pablo Neira Ayuso]

Add a new object to describe a datatype. This allows to describe the
set key when type is used to define the set.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/desc.h          |  6 +++
 include/libnftnl/desc.h | 19 ++++++++
 src/desc.c              | 96 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 121 insertions(+)

diff --git a/include/desc.h b/include/desc.h
index d78af3528118..2f61a1e5963e 100644
--- a/include/desc.h
+++ b/include/desc.h
@@ -16,4 +16,10 @@ struct nftnl_expr_desc {
 	};
 };
 
+struct nftnl_dtype_desc {
+	uint32_t	dtype;
+	uint32_t	byteorder;
+	uint32_t	len;
+};
+
 #endif
diff --git a/include/libnftnl/desc.h b/include/libnftnl/desc.h
index 1202eb9ddf79..cb1cac91f934 100644
--- a/include/libnftnl/desc.h
+++ b/include/libnftnl/desc.h
@@ -50,6 +50,25 @@ int nftnl_expr_desc_build(struct nftnl_udata_buf *udbuf,
 int nftnl_expr_desc_parse(const struct nftnl_udata *attr,
 			  struct nftnl_expr_desc *dexpr);
 
+enum nftnl_dtype_desc_types {
+	NFTNL_DESC_DTYPE_TYPE,
+	NFTNL_DESC_DTYPE_BYTEORDER,
+	NFTNL_DESC_DTYPE_LEN,
+	__NFTNL_DESC_DTYPE_MAX
+};
+#define NFTNL_DESC_DTYPE_MAX (__NFTNL_DESC_DTYPE_MAX - 1)
+
+struct nftnl_dtype_desc;
+
+struct nftnl_dtype_desc *nftnl_dtype_desc_alloc(void);
+void nftnl_dtype_desc_free(struct nftnl_dtype_desc *dtype);
+int nftnl_dtype_desc_set(struct nftnl_dtype_desc *dtype, uint8_t type,
+			 const void *data, uint32_t data_len);
+int nftnl_dtype_desc_build(struct nftnl_udata_buf *udbuf,
+			   const struct nftnl_dtype_desc *dtype);
+int nftnl_dtype_desc_parse(const struct nftnl_udata *attr,
+			    struct nftnl_dtype_desc *dtype);
+
 #ifdef __cplusplus
 } /* extern "C" */
 #endif
diff --git a/src/desc.c b/src/desc.c
index d8566ba2ee38..f73e74c2c7d3 100644
--- a/src/desc.c
+++ b/src/desc.c
@@ -140,3 +140,99 @@ int nftnl_expr_desc_parse(const struct nftnl_udata *attr,
 
 	return err;
 }
+
+EXPORT_SYMBOL(nftnl_dtype_desc_alloc);
+struct nftnl_dtype_desc *nftnl_dtype_desc_alloc(void)
+{
+	return calloc(1, sizeof(struct nftnl_dtype_desc));
+}
+
+EXPORT_SYMBOL(nftnl_dtype_desc_free);
+void nftnl_dtype_desc_free(struct nftnl_dtype_desc *dtype)
+{
+	free(dtype);
+}
+
+EXPORT_SYMBOL(nftnl_dtype_desc_set);
+int nftnl_dtype_desc_set(struct nftnl_dtype_desc *dtype, uint8_t type,
+			 const void *data, uint32_t data_len)
+{
+	int err = 0;
+
+	switch (type) {
+	case NFTNL_DESC_DTYPE_TYPE:
+		memcpy(&dtype->dtype, data, data_len);
+		break;
+	case NFTNL_DESC_DTYPE_BYTEORDER:
+		memcpy(&dtype->byteorder, data, data_len);
+		break;
+	case NFTNL_DESC_DTYPE_LEN:
+		memcpy(&dtype->len, data, data_len);
+		break;
+	default:
+		err = -1;
+		break;
+	}
+
+	return err;
+}
+
+#define NFTNL_UDATA_DTYPE_TYPE		0
+#define NFTNL_UDATA_DTYPE_BYTEORDER	1
+#define NFTNL_UDATA_DTYPE_LEN		2
+#define NFTNL_UDATA_DTYPE_MAX		3
+
+EXPORT_SYMBOL(nftnl_dtype_desc_build);
+int nftnl_dtype_desc_build(struct nftnl_udata_buf *udbuf,
+			   const struct nftnl_dtype_desc *dtype)
+{
+	nftnl_udata_put_u32(udbuf, NFTNL_UDATA_DTYPE_TYPE, dtype->dtype);
+	nftnl_udata_put_u32(udbuf, NFTNL_UDATA_DTYPE_BYTEORDER, dtype->byteorder);
+	nftnl_udata_put_u32(udbuf, NFTNL_UDATA_DTYPE_LEN, dtype->len);
+
+	return 0;
+}
+
+static int nftnl_dtype_desc_parse_nested(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_DTYPE_TYPE:
+	case NFTNL_UDATA_DTYPE_BYTEORDER:
+	case NFTNL_UDATA_DTYPE_LEN:
+		if (len != sizeof(uint32_t))
+			return -1;
+		break;
+	default:
+	        return 0;
+	}
+
+	ud[type] = attr;
+	return 0;
+}
+
+EXPORT_SYMBOL(nftnl_dtype_desc_parse);
+int nftnl_dtype_desc_parse(const struct nftnl_udata *attr,
+			   struct nftnl_dtype_desc *dtype)
+{
+	const struct nftnl_udata *ud[NFTNL_UDATA_DTYPE_MAX + 1] = {};
+	int err;
+
+	err = nftnl_udata_parse(nftnl_udata_get(attr), nftnl_udata_len(attr),
+				nftnl_dtype_desc_parse_nested, ud);
+	if (err < 0)
+		return -1;
+
+	err = 0;
+	if (ud[NFTNL_UDATA_DTYPE_TYPE])
+		dtype->dtype = nftnl_udata_get_u32(ud[NFTNL_UDATA_DTYPE_TYPE]);
+	if (ud[NFTNL_UDATA_DTYPE_BYTEORDER])
+		dtype->byteorder = nftnl_udata_get_u32(ud[NFTNL_UDATA_DTYPE_BYTEORDER]);
+	if (ud[NFTNL_UDATA_DTYPE_LEN])
+		dtype->len = nftnl_udata_get_u32(ud[NFTNL_UDATA_DTYPE_LEN]);
+
+	return err;
+}
-- 
2.30.2

[PATCH libnftnl 3/3] desc: add set description

[Pablo Neira Ayuso]

Add a new object to describe a set. Add helpers to build and to parse
the userdata set description.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/desc.h           |  25 +++
 include/libnftnl/desc.h  |  31 ++++
 include/libnftnl/udata.h |  18 +-
 src/desc.c               | 360 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 427 insertions(+), 7 deletions(-)

diff --git a/include/desc.h b/include/desc.h
index 2f61a1e5963e..135f1a7b6317 100644
--- a/include/desc.h
+++ b/include/desc.h
@@ -1,6 +1,8 @@
 #ifndef _LIBNFTNL_DESC_INTERNAL_H_
 #define _LIBNFTNL_DESC_INTERNAL_H_
 
+#include <libnftnl/udata.h>
+
 struct nftnl_expr_desc {
 	uint32_t	etype;
 	uint32_t	byteorder;
@@ -22,4 +24,27 @@ struct nftnl_dtype_desc {
 	uint32_t	len;
 };
 
+#define NFTNL_SET_DESC_MAX        6
+
+struct nftnl_concat_desc {
+	union {
+		struct {
+			const struct nftnl_expr_desc	*expr[NFTNL_SET_DESC_MAX];
+			uint32_t			num_typeof;
+		};
+		struct {
+			const struct nftnl_dtype_desc	*dtype[NFTNL_SET_DESC_MAX];
+			uint32_t			num_type;
+		};
+	};
+};
+
+struct nftnl_set_desc {
+	enum nftnl_set_desc_type	type;
+	uint32_t			flags;
+	struct nftnl_concat_desc	key;
+	struct nftnl_concat_desc	data;
+	char				comment[NFTNL_UDATA_COMMENT_MAXLEN];
+};
+
 #endif
diff --git a/include/libnftnl/desc.h b/include/libnftnl/desc.h
index cb1cac91f934..aa14145d4b41 100644
--- a/include/libnftnl/desc.h
+++ b/include/libnftnl/desc.h
@@ -69,6 +69,37 @@ int nftnl_dtype_desc_build(struct nftnl_udata_buf *udbuf,
 int nftnl_dtype_desc_parse(const struct nftnl_udata *attr,
 			    struct nftnl_dtype_desc *dtype);
 
+enum nftnl_set_desc_type {
+	NFTNL_DESC_SET_UNSPEC	= 0,
+	NFTNL_DESC_SET_DATATYPE,
+	NFTNL_DESC_SET_TYPEOF,
+};
+
+enum nftnl_set_desc_types {
+	NFTNL_DESC_SET_TYPE,
+	NFTNL_DESC_SET_KEY,
+	NFTNL_DESC_SET_DATA,
+	NFTNL_DESC_SET_FLAGS,
+	NFTNL_DESC_SET_COMMENT,
+	__NFTNL_DESC_SET_MAX
+};
+#define NFTNL_DESC_SET_MAX (__NFTNL_DESC_SET_MAX - 1)
+
+struct nftnl_set_desc;
+
+struct nftnl_set_desc *nftnl_set_desc_alloc(void);
+void nftnl_set_desc_free(struct nftnl_set_desc *dset);
+int nftnl_set_desc_set(struct nftnl_set_desc *dset, enum nftnl_set_desc_types type,
+		       const void *data, uint32_t data_len);
+int nftnl_set_desc_add_expr(struct nftnl_set_desc *su, enum nftnl_set_desc_types type,
+			    const struct nftnl_expr_desc *dexpr);
+int nftnl_set_desc_add_datatype(struct nftnl_set_desc *dset, enum nftnl_set_desc_types type,
+				const struct nftnl_dtype_desc *dtype);
+int nftnl_set_desc_build_udata(struct nftnl_udata_buf *udbuf,
+			       const struct nftnl_set_desc *dset);
+int nftnl_set_desc_parse_udata(const char *udata, uint32_t len,
+			       struct nftnl_set_desc *dset);
+
 #ifdef __cplusplus
 } /* extern "C" */
 #endif
diff --git a/include/libnftnl/udata.h b/include/libnftnl/udata.h
index dbf3a60ff06e..3349ae7bf7b1 100644
--- a/include/libnftnl/udata.h
+++ b/include/libnftnl/udata.h
@@ -37,14 +37,18 @@ enum nftnl_udata_obj_types {
 #define NFTNL_UDATA_COMMENT_MAXLEN	128
 
 enum nftnl_udata_set_types {
-	NFTNL_UDATA_SET_KEYBYTEORDER,
-	NFTNL_UDATA_SET_DATABYTEORDER,
-	NFTNL_UDATA_SET_MERGE_ELEMENTS,
-	NFTNL_UDATA_SET_KEY_TYPEOF,
-	NFTNL_UDATA_SET_DATA_TYPEOF,
-	NFTNL_UDATA_SET_EXPR,
-	NFTNL_UDATA_SET_DATA_INTERVAL,
+	NFTNL_UDATA_SET_KEYBYTEORDER,	/* not used in newer versions */
+	NFTNL_UDATA_SET_DATABYTEORDER,	/* not used in newer versions */
+	NFTNL_UDATA_SET_MERGE_ELEMENTS,	/* not used in newer versions */
+	NFTNL_UDATA_SET_KEY_TYPEOF,	/* not used in newer versions */
+	NFTNL_UDATA_SET_DATA_TYPEOF,	/* not used in newer versions */
+	NFTNL_UDATA_SET_EXPR,		/* not used in newer versions */
+	NFTNL_UDATA_SET_DATA_INTERVAL,	/* not used in newer versions */
 	NFTNL_UDATA_SET_COMMENT,
+	NFTNL_UDATA_SET_TYPE,
+	NFTNL_UDATA_SET_KEY,
+	NFTNL_UDATA_SET_DATA,
+	NFTNL_UDATA_SET_FLAGS,
 	__NFTNL_UDATA_SET_MAX
 };
 #define NFTNL_UDATA_SET_MAX (__NFTNL_UDATA_SET_MAX - 1)
diff --git a/src/desc.c b/src/desc.c
index f73e74c2c7d3..c8b3195db850 100644
--- a/src/desc.c
+++ b/src/desc.c
@@ -236,3 +236,363 @@ int nftnl_dtype_desc_parse(const struct nftnl_udata *attr,
 
 	return err;
 }
+
+EXPORT_SYMBOL(nftnl_set_desc_alloc);
+struct nftnl_set_desc *nftnl_set_desc_alloc(void)
+{
+	return calloc(1, sizeof(struct nftnl_set_desc));
+}
+
+EXPORT_SYMBOL(nftnl_set_desc_free);
+void nftnl_set_desc_free(struct nftnl_set_desc *dset)
+{
+	uint32_t i;
+
+	if (dset->type == NFTNL_DESC_SET_TYPEOF) {
+		for (i = 0; i < dset->key.num_typeof; i++)
+			free((void *)dset->key.expr[i]);
+		for (i = 0; i < dset->data.num_typeof; i++)
+			free((void *)dset->data.expr[i]);
+	}
+
+	free(dset);
+}
+
+EXPORT_SYMBOL(nftnl_set_desc_set);
+int nftnl_set_desc_set(struct nftnl_set_desc *dset, enum nftnl_set_desc_types type,
+		       const void *data, uint32_t data_len)
+{
+	switch (type) {
+	case NFTNL_DESC_SET_TYPE:
+		memcpy(&dset->type, data, data_len);
+		break;
+	case NFTNL_DESC_SET_KEY:
+	case NFTNL_DESC_SET_DATA:
+		/* use nftnl_set_desc_add_expr() */
+		return -1;
+	case NFTNL_DESC_SET_FLAGS:
+		memcpy(&dset->flags, data, data_len);
+		break;
+	case NFTNL_DESC_SET_COMMENT:
+		memcpy(dset->comment, data, data_len);
+		break;
+	default:
+		return -1;
+	}
+
+	return 0;
+}
+
+EXPORT_SYMBOL(nftnl_set_desc_add_expr);
+int nftnl_set_desc_add_expr(struct nftnl_set_desc *dset, uint32_t type,
+			    const struct nftnl_expr_desc *dexpr)
+{
+	switch (type) {
+	case NFTNL_DESC_SET_KEY:
+		if (dset->key.num_typeof >= NFTNL_DESC_SET_MAX)
+			return -1;
+
+		dset->key.expr[dset->key.num_typeof++] = dexpr;
+		break;
+	case NFTNL_DESC_SET_DATA:
+		if (dset->key.num_type >= NFTNL_DESC_SET_MAX)
+			return -1;
+
+		dset->data.expr[dset->key.num_typeof++] = dexpr;
+		break;
+	default:
+		return -1;
+	}
+
+	return 0;
+}
+
+EXPORT_SYMBOL(nftnl_set_desc_add_datatype);
+int nftnl_set_desc_add_datatype(struct nftnl_set_desc *dset, uint32_t type,
+				 const struct nftnl_dtype_desc *dtype)
+{
+	switch (type) {
+	case NFTNL_DESC_SET_KEY:
+		if (dset->data.num_typeof >= NFTNL_DESC_SET_MAX)
+			return -1;
+
+		dset->key.dtype[dset->data.num_typeof++] = dtype;
+		break;
+	case NFTNL_DESC_SET_DATA:
+		if (dset->data.num_type >= NFTNL_DESC_SET_MAX)
+			return -1;
+
+		dset->data.dtype[dset->data.num_type++] = dtype;
+		break;
+	default:
+		return -1;
+	}
+
+	return 0;
+}
+
+static int __nftnl_udata_set_dtype_build(struct nftnl_udata_buf *udbuf,
+					 const struct nftnl_dtype_desc *dtype,
+					 uint8_t attr_type)
+{
+	struct nftnl_udata *nest;
+	int err;
+
+	nest = nftnl_udata_nest_start(udbuf, attr_type);
+	err = nftnl_dtype_desc_build(udbuf, dtype);
+	nftnl_udata_nest_end(udbuf, nest);
+
+	return err;
+}
+
+static int nftnl_set_desc_build_dtype(struct nftnl_udata_buf *udbuf,
+				      const struct nftnl_set_desc *dset)
+{
+	struct nftnl_udata *nest;
+	int i, err;
+
+	switch (dset->type) {
+	case NFTNL_DESC_SET_TYPEOF:
+		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_KEY);
+		for (i = 0; i < dset->key.num_type; i++) {
+			err = __nftnl_udata_set_dtype_build(udbuf, dset->key.dtype[i], i);
+			if (err < 0)
+				return err;
+		}
+		nftnl_udata_nest_end(udbuf, nest);
+		break;
+	case NFTNL_DESC_SET_DATATYPE:
+		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_DATA);
+		for (i = 0; i < dset->data.num_type; i++) {
+			err = __nftnl_udata_set_dtype_build(udbuf, dset->data.dtype[i], i);
+			if (err < 0)
+				return err;
+		}
+		nftnl_udata_nest_end(udbuf, nest);
+		break;
+	case NFTNL_DESC_SET_UNSPEC:
+		return -1;
+	}
+
+	return 0;
+}
+
+static int __nftnl_set_desc_build_typeof(struct nftnl_udata_buf *udbuf,
+					 const struct nftnl_expr_desc *dexpr,
+					 uint8_t attr_type)
+{
+	struct nftnl_udata *nest;
+	int err;
+
+	nest = nftnl_udata_nest_start(udbuf, attr_type);
+	err = nftnl_expr_desc_build(udbuf, dexpr);
+	nftnl_udata_nest_end(udbuf, nest);
+
+	return err;
+}
+
+static int nftnl_set_desc_build_typeof(struct nftnl_udata_buf *udbuf,
+				       const struct nftnl_set_desc *dset)
+{
+	struct nftnl_udata *nest;
+	int i;
+
+	switch (dset->type) {
+	case NFTNL_DESC_SET_TYPEOF:
+		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_KEY_TYPEOF);
+		for (i = 0; i < dset->key.num_typeof; i++)
+			__nftnl_set_desc_build_typeof(udbuf, dset->key.expr[i], i);
+
+		nftnl_udata_nest_end(udbuf, nest);
+		break;
+	case NFTNL_DESC_SET_DATATYPE:
+		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_DATA_TYPEOF);
+		for (i = 0; i < dset->key.num_typeof; i++)
+			__nftnl_set_desc_build_typeof(udbuf, dset->data.expr[i], i);
+
+		nftnl_udata_nest_end(udbuf, nest);
+		break;
+	case NFTNL_DESC_SET_UNSPEC:
+		return -1;
+	}
+
+	return 0;
+}
+
+EXPORT_SYMBOL(nftnl_set_desc_build_udata);
+int nftnl_set_desc_build_udata(struct nftnl_udata_buf *udbuf,
+			       const struct nftnl_set_desc *dset)
+{
+	if (!nftnl_udata_put_u32(udbuf, NFTNL_UDATA_SET_FLAGS, dset->flags))
+		return -1;
+
+	switch (dset->type) {
+	case NFTNL_DESC_SET_DATATYPE:
+		return nftnl_set_desc_build_dtype(udbuf, dset);
+	case NFTNL_DESC_SET_TYPEOF:
+		return nftnl_set_desc_build_typeof(udbuf, dset);
+	case NFTNL_DESC_SET_UNSPEC:
+		return -1;
+	}
+
+	if (!nftnl_udata_put_strz(udbuf, NFTNL_UDATA_SET_COMMENT, dset->comment))
+		return -1;
+
+	return -1;
+}
+
+static int parse_concat(const struct nftnl_udata *attr, void *data)
+{
+	const struct nftnl_udata **ud = data;
+	uint8_t type = nftnl_udata_type(attr);
+
+	if (type >= 10)
+		return -1;
+
+	ud[type] = attr;
+
+	return 0;
+}
+
+static int nftnl_set_desc_parse_datatype(const struct nftnl_udata *attr,
+					 struct nftnl_concat_desc *concat)
+{
+	const struct nftnl_udata *ud[NFTNL_DESC_SET_MAX + 1];
+	struct nftnl_dtype_desc *dtype;
+	int err, i;
+
+	err = nftnl_udata_parse(nftnl_udata_get(attr), nftnl_udata_len(attr),
+				parse_concat, ud);
+	if (err < 0)
+		return -1;
+
+	for (i = 0; i < array_size(ud); i++) {
+		if (!ud[i])
+			break;
+
+		dtype = nftnl_dtype_desc_alloc();
+		err = nftnl_dtype_desc_parse(ud[i], dtype);
+		if (err < 0) {
+			nftnl_dtype_desc_free(dtype);
+			break;
+		}
+		concat->dtype[concat->num_type++] = dtype;
+	}
+
+	return err;
+}
+
+static int nftnl_set_desc_parse_typeof(const struct nftnl_udata *attr,
+				       struct nftnl_concat_desc *concat)
+{
+	const struct nftnl_udata *ud[NFTNL_DESC_SET_MAX + 1];
+	struct nftnl_expr_desc *dexpr;
+	int err, i;
+
+	err = nftnl_udata_parse(nftnl_udata_get(attr), nftnl_udata_len(attr),
+				parse_concat, ud);
+	if (err < 0)
+		return -1;
+
+	for (i = 0; i < array_size(ud); i++) {
+		if (!ud[i])
+			break;
+
+		dexpr = nftnl_expr_desc_alloc();
+		err = nftnl_expr_desc_parse(ud[i], dexpr);
+		if (err < 0) {
+			nftnl_expr_desc_free(dexpr);
+			break;
+		}
+		concat->expr[concat->num_typeof++] = dexpr;
+	}
+
+	return err;
+}
+
+static int nftnl_set_desc_parse_concat(const struct nftnl_udata *attr,
+				       struct nftnl_concat_desc *concat,
+				       enum nftnl_set_desc_type type)
+{
+	int err;
+
+	switch (type) {
+	case NFTNL_DESC_SET_DATATYPE:
+		err = nftnl_set_desc_parse_datatype(attr, concat);
+		break;
+	case NFTNL_DESC_SET_TYPEOF:
+		err = nftnl_set_desc_parse_typeof(attr, concat);
+		break;
+	default:
+		err = -1;
+		break;
+	}
+
+	return err;
+}
+
+static int parse_set_desc(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_TYPE:
+	case NFTNL_UDATA_SET_FLAGS:
+		if (len != sizeof(uint32_t))
+			return -1;
+		break;
+	case NFTNL_UDATA_SET_KEY:
+	case NFTNL_UDATA_SET_DATA:
+	case NFTNL_UDATA_SET_COMMENT:
+		break;
+	default:
+		return 0;
+	}
+
+	ud[type] = attr;
+	return 0;
+}
+
+EXPORT_SYMBOL(nftnl_set_desc_parse_udata);
+int nftnl_set_desc_parse_udata(const char *udata, uint32_t len,
+			       struct nftnl_set_desc *dset)
+{
+	const struct nftnl_udata *ud[NFTNL_UDATA_SET_MAX + 1];
+	int err;
+
+	err = nftnl_udata_parse(udata, len, parse_set_desc, ud);
+	if (err < 0)
+		return -1;
+
+	if (ud[NFTNL_UDATA_SET_TYPE])
+		dset->type = nftnl_udata_get_u32(ud[NFTNL_UDATA_SET_TYPE]);
+
+	if (ud[NFTNL_UDATA_SET_KEY]) {
+		err = nftnl_set_desc_parse_concat(ud[NFTNL_UDATA_SET_KEY],
+						  &dset->key, dset->type);
+		if (err < 0)
+			return err;
+	}
+
+	if (ud[NFTNL_UDATA_SET_DATA]) {
+		err = nftnl_set_desc_parse_concat(ud[NFTNL_UDATA_SET_DATA],
+						  &dset->data, dset->type);
+		if (err < 0)
+			return err;
+	}
+
+	if (ud[NFTNL_UDATA_SET_FLAGS]) {
+		dset->flags = nftnl_udata_get_u32(ud[NFTNL_UDATA_SET_FLAGS]);
+		if (err < 0)
+			return err;
+	}
+
+	if (ud[NFTNL_UDATA_SET_COMMENT]) {
+		memcpy(dset->comment, nftnl_udata_get(ud[NFTNL_UDATA_SET_COMMENT]),
+		       nftnl_udata_len(ud[NFTNL_UDATA_SET_COMMENT]));
+	}
+
+	return err;
+}
-- 
2.30.2

Re: [PATCH libnftnl 3/3] desc: add set description

[Phil Sutter]

Hi,

On Thu, Jan 20, 2022 at 01:04:02AM +0100, Pablo Neira Ayuso wrote:
[...]
> diff --git a/src/desc.c b/src/desc.c
> index f73e74c2c7d3..c8b3195db850 100644
> --- a/src/desc.c
> +++ b/src/desc.c
[...]
> +static int nftnl_set_desc_build_dtype(struct nftnl_udata_buf *udbuf,
> +				      const struct nftnl_set_desc *dset)
> +{
> +	struct nftnl_udata *nest;
> +	int i, err;
> +
> +	switch (dset->type) {
> +	case NFTNL_DESC_SET_TYPEOF:
> +		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_KEY);
> +		for (i = 0; i < dset->key.num_type; i++) {
> +			err = __nftnl_udata_set_dtype_build(udbuf, dset->key.dtype[i], i);
> +			if (err < 0)
> +				return err;
> +		}
> +		nftnl_udata_nest_end(udbuf, nest);
> +		break;
> +	case NFTNL_DESC_SET_DATATYPE:
> +		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_DATA);
> +		for (i = 0; i < dset->data.num_type; i++) {
> +			err = __nftnl_udata_set_dtype_build(udbuf, dset->data.dtype[i], i);
> +			if (err < 0)
> +				return err;
> +		}
> +		nftnl_udata_nest_end(udbuf, nest);
> +		break;
> +	case NFTNL_DESC_SET_UNSPEC:
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int __nftnl_set_desc_build_typeof(struct nftnl_udata_buf *udbuf,
> +					 const struct nftnl_expr_desc *dexpr,
> +					 uint8_t attr_type)
> +{
> +	struct nftnl_udata *nest;
> +	int err;
> +
> +	nest = nftnl_udata_nest_start(udbuf, attr_type);
> +	err = nftnl_expr_desc_build(udbuf, dexpr);
> +	nftnl_udata_nest_end(udbuf, nest);
> +
> +	return err;
> +}
> +
> +static int nftnl_set_desc_build_typeof(struct nftnl_udata_buf *udbuf,
> +				       const struct nftnl_set_desc *dset)
> +{
> +	struct nftnl_udata *nest;
> +	int i;
> +
> +	switch (dset->type) {
> +	case NFTNL_DESC_SET_TYPEOF:
> +		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_KEY_TYPEOF);
> +		for (i = 0; i < dset->key.num_typeof; i++)
> +			__nftnl_set_desc_build_typeof(udbuf, dset->key.expr[i], i);
> +
> +		nftnl_udata_nest_end(udbuf, nest);
> +		break;
> +	case NFTNL_DESC_SET_DATATYPE:
> +		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_DATA_TYPEOF);
> +		for (i = 0; i < dset->key.num_typeof; i++)
> +			__nftnl_set_desc_build_typeof(udbuf, dset->data.expr[i], i);
> +
> +		nftnl_udata_nest_end(udbuf, nest);
> +		break;
> +	case NFTNL_DESC_SET_UNSPEC:
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +EXPORT_SYMBOL(nftnl_set_desc_build_udata);
> +int nftnl_set_desc_build_udata(struct nftnl_udata_buf *udbuf,
> +			       const struct nftnl_set_desc *dset)
> +{
> +	if (!nftnl_udata_put_u32(udbuf, NFTNL_UDATA_SET_FLAGS, dset->flags))
> +		return -1;
> +
> +	switch (dset->type) {
> +	case NFTNL_DESC_SET_DATATYPE:
> +		return nftnl_set_desc_build_dtype(udbuf, dset);
> +	case NFTNL_DESC_SET_TYPEOF:
> +		return nftnl_set_desc_build_typeof(udbuf, dset);
> +	case NFTNL_DESC_SET_UNSPEC:
> +		return -1;
> +	}
> +
> +	if (!nftnl_udata_put_strz(udbuf, NFTNL_UDATA_SET_COMMENT, dset->comment))
> +		return -1;
> +
> +	return -1;
> +}

This is odd: Depending on dset->type, nftnl_set_desc_build_udata() calls
either nftnl_set_desc_build_dtype() or nftnl_set_desc_build_typeof().
Yet both check dset->type again. This looks like a mix-up of set/map key
and data definitions and typeof vs. "regular" definition styles.

Cheers, Phil

Re: [PATCH libnftnl 0/3] add description infrastructure

[Phil Sutter]

Hi Pablo,

On Thu, Jan 20, 2022 at 01:03:59AM +0100, Pablo Neira Ayuso wrote:
> This is my proposal to address the snprintf data printing depending on
> the arch. The idea is to add description objects that can be used to
> build the userdata area as well as to parse the userdata to create the
> description object.

I tried to integrate this into nftables, but failed to understand how
this all is supposed to come together: In nftables, concat is treated
like any other expression. Your series seems to require special
treatment? At least there are separate "desc" data structures for each.
It seems like one can't just replace build_udata callbacks to populate
an nftnl_expr_desc object?

Cheers, Phil

Re: [PATCH libnftnl 3/3] desc: add set description

[Pablo Neira Ayuso]

On Thu, Mar 10, 2022 at 12:31:15PM +0100, Phil Sutter wrote:
> Hi,
> 
> On Thu, Jan 20, 2022 at 01:04:02AM +0100, Pablo Neira Ayuso wrote:
> [...]
> > diff --git a/src/desc.c b/src/desc.c
> > index f73e74c2c7d3..c8b3195db850 100644
> > --- a/src/desc.c
> > +++ b/src/desc.c
> [...]
> > +static int nftnl_set_desc_build_dtype(struct nftnl_udata_buf *udbuf,
> > +				      const struct nftnl_set_desc *dset)
> > +{
> > +	struct nftnl_udata *nest;
> > +	int i, err;
> > +
> > +	switch (dset->type) {
> > +	case NFTNL_DESC_SET_TYPEOF:
> > +		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_KEY);
> > +		for (i = 0; i < dset->key.num_type; i++) {
> > +			err = __nftnl_udata_set_dtype_build(udbuf, dset->key.dtype[i], i);
> > +			if (err < 0)
> > +				return err;
> > +		}
> > +		nftnl_udata_nest_end(udbuf, nest);
> > +		break;
> > +	case NFTNL_DESC_SET_DATATYPE:
> > +		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_DATA);
> > +		for (i = 0; i < dset->data.num_type; i++) {
> > +			err = __nftnl_udata_set_dtype_build(udbuf, dset->data.dtype[i], i);
> > +			if (err < 0)
> > +				return err;
> > +		}
> > +		nftnl_udata_nest_end(udbuf, nest);
> > +		break;
> > +	case NFTNL_DESC_SET_UNSPEC:
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int __nftnl_set_desc_build_typeof(struct nftnl_udata_buf *udbuf,
> > +					 const struct nftnl_expr_desc *dexpr,
> > +					 uint8_t attr_type)
> > +{
> > +	struct nftnl_udata *nest;
> > +	int err;
> > +
> > +	nest = nftnl_udata_nest_start(udbuf, attr_type);
> > +	err = nftnl_expr_desc_build(udbuf, dexpr);
> > +	nftnl_udata_nest_end(udbuf, nest);
> > +
> > +	return err;
> > +}
> > +
> > +static int nftnl_set_desc_build_typeof(struct nftnl_udata_buf *udbuf,
> > +				       const struct nftnl_set_desc *dset)
> > +{
> > +	struct nftnl_udata *nest;
> > +	int i;
> > +
> > +	switch (dset->type) {
> > +	case NFTNL_DESC_SET_TYPEOF:
> > +		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_KEY_TYPEOF);
> > +		for (i = 0; i < dset->key.num_typeof; i++)
> > +			__nftnl_set_desc_build_typeof(udbuf, dset->key.expr[i], i);
> > +
> > +		nftnl_udata_nest_end(udbuf, nest);
> > +		break;
> > +	case NFTNL_DESC_SET_DATATYPE:
> > +		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_DATA_TYPEOF);
> > +		for (i = 0; i < dset->key.num_typeof; i++)
> > +			__nftnl_set_desc_build_typeof(udbuf, dset->data.expr[i], i);
> > +
> > +		nftnl_udata_nest_end(udbuf, nest);
> > +		break;
> > +	case NFTNL_DESC_SET_UNSPEC:
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +EXPORT_SYMBOL(nftnl_set_desc_build_udata);
> > +int nftnl_set_desc_build_udata(struct nftnl_udata_buf *udbuf,
> > +			       const struct nftnl_set_desc *dset)
> > +{
> > +	if (!nftnl_udata_put_u32(udbuf, NFTNL_UDATA_SET_FLAGS, dset->flags))
> > +		return -1;
> > +
> > +	switch (dset->type) {
> > +	case NFTNL_DESC_SET_DATATYPE:
> > +		return nftnl_set_desc_build_dtype(udbuf, dset);
> > +	case NFTNL_DESC_SET_TYPEOF:
> > +		return nftnl_set_desc_build_typeof(udbuf, dset);
> > +	case NFTNL_DESC_SET_UNSPEC:
> > +		return -1;
> > +	}
> > +
> > +	if (!nftnl_udata_put_strz(udbuf, NFTNL_UDATA_SET_COMMENT, dset->comment))
> > +		return -1;
> > +
> > +	return -1;
> > +}
> 
> This is odd: Depending on dset->type, nftnl_set_desc_build_udata() calls
> either nftnl_set_desc_build_dtype() or nftnl_set_desc_build_typeof().

That's indeed incorrect, nftnl_set_desc_build_typeof() should be:

static int nftnl_set_desc_build_typeof(struct nftnl_udata_buf *udbuf,
				       const struct nftnl_set_desc *dset)
{
	struct nftnl_udata *nest;
	int i;

	nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_KEY_TYPEOF);
	for (i = 0; i < dset->key.num_typeof; i++)
		__nftnl_set_desc_build_typeof(udbuf, dset->key.expr[i], i);

	nftnl_udata_nest_end(udbuf, nest);

	return 0;
}

The idea is: A set can either use datatype or typeof to define the
elements that it stores. Then, a concatenation is possible.

> Yet both check dset->type again. This looks like a mix-up of set/map key
> and data definitions and typeof vs. "regular" definition styles.

This patchset was a sketch PoC, I should have label it more explicit
as such.

Re: [PATCH libnftnl 0/3] add description infrastructure

[Pablo Neira Ayuso]

On Thu, Mar 10, 2022 at 12:35:57PM +0100, Phil Sutter wrote:
> Hi Pablo,
> 
> On Thu, Jan 20, 2022 at 01:03:59AM +0100, Pablo Neira Ayuso wrote:
> > This is my proposal to address the snprintf data printing depending on
> > the arch. The idea is to add description objects that can be used to
> > build the userdata area as well as to parse the userdata to create the
> > description object.
> 
> I tried to integrate this into nftables, but failed to understand how
> this all is supposed to come together: In nftables, concat is treated
> like any other expression. Your series seems to require special
> treatment?

The idea is that you build the nftnl description object either from
the set typeof expression or the set datatype (depending on how the
user has defined the set).

> At least there are separate "desc" data structures for each.
> It seems like one can't just replace build_udata callbacks to populate
> an nftnl_expr_desc object?

You can use the description object in two ways:

- build_udata is called when setting the libnftnl set udata
  field, to build it.

- you pass the description object to snprintf.

The existing code to build the userdata TLV that resides in nftables
should go away and use this new infrastructure, I'm basically moving
to libnftnl the existing nftables code to build the set userdata area.

Re: [PATCH libnftnl 3/3] desc: add set description

[Phil Sutter]

On Fri, Mar 11, 2022 at 12:24:33AM +0100, Pablo Neira Ayuso wrote:
> On Thu, Mar 10, 2022 at 12:31:15PM +0100, Phil Sutter wrote:
> > Hi,
> > 
> > On Thu, Jan 20, 2022 at 01:04:02AM +0100, Pablo Neira Ayuso wrote:
> > [...]
> > > diff --git a/src/desc.c b/src/desc.c
> > > index f73e74c2c7d3..c8b3195db850 100644
> > > --- a/src/desc.c
> > > +++ b/src/desc.c
> > [...]
> > > +static int nftnl_set_desc_build_dtype(struct nftnl_udata_buf *udbuf,
> > > +				      const struct nftnl_set_desc *dset)
> > > +{
> > > +	struct nftnl_udata *nest;
> > > +	int i, err;
> > > +
> > > +	switch (dset->type) {
> > > +	case NFTNL_DESC_SET_TYPEOF:
> > > +		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_KEY);
> > > +		for (i = 0; i < dset->key.num_type; i++) {
> > > +			err = __nftnl_udata_set_dtype_build(udbuf, dset->key.dtype[i], i);
> > > +			if (err < 0)
> > > +				return err;
> > > +		}
> > > +		nftnl_udata_nest_end(udbuf, nest);
> > > +		break;
> > > +	case NFTNL_DESC_SET_DATATYPE:
> > > +		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_DATA);
> > > +		for (i = 0; i < dset->data.num_type; i++) {
> > > +			err = __nftnl_udata_set_dtype_build(udbuf, dset->data.dtype[i], i);
> > > +			if (err < 0)
> > > +				return err;
> > > +		}
> > > +		nftnl_udata_nest_end(udbuf, nest);
> > > +		break;
> > > +	case NFTNL_DESC_SET_UNSPEC:
> > > +		return -1;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int __nftnl_set_desc_build_typeof(struct nftnl_udata_buf *udbuf,
> > > +					 const struct nftnl_expr_desc *dexpr,
> > > +					 uint8_t attr_type)
> > > +{
> > > +	struct nftnl_udata *nest;
> > > +	int err;
> > > +
> > > +	nest = nftnl_udata_nest_start(udbuf, attr_type);
> > > +	err = nftnl_expr_desc_build(udbuf, dexpr);
> > > +	nftnl_udata_nest_end(udbuf, nest);
> > > +
> > > +	return err;
> > > +}
> > > +
> > > +static int nftnl_set_desc_build_typeof(struct nftnl_udata_buf *udbuf,
> > > +				       const struct nftnl_set_desc *dset)
> > > +{
> > > +	struct nftnl_udata *nest;
> > > +	int i;
> > > +
> > > +	switch (dset->type) {
> > > +	case NFTNL_DESC_SET_TYPEOF:
> > > +		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_KEY_TYPEOF);
> > > +		for (i = 0; i < dset->key.num_typeof; i++)
> > > +			__nftnl_set_desc_build_typeof(udbuf, dset->key.expr[i], i);
> > > +
> > > +		nftnl_udata_nest_end(udbuf, nest);
> > > +		break;
> > > +	case NFTNL_DESC_SET_DATATYPE:
> > > +		nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_DATA_TYPEOF);
> > > +		for (i = 0; i < dset->key.num_typeof; i++)
> > > +			__nftnl_set_desc_build_typeof(udbuf, dset->data.expr[i], i);
> > > +
> > > +		nftnl_udata_nest_end(udbuf, nest);
> > > +		break;
> > > +	case NFTNL_DESC_SET_UNSPEC:
> > > +		return -1;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +EXPORT_SYMBOL(nftnl_set_desc_build_udata);
> > > +int nftnl_set_desc_build_udata(struct nftnl_udata_buf *udbuf,
> > > +			       const struct nftnl_set_desc *dset)
> > > +{
> > > +	if (!nftnl_udata_put_u32(udbuf, NFTNL_UDATA_SET_FLAGS, dset->flags))
> > > +		return -1;
> > > +
> > > +	switch (dset->type) {
> > > +	case NFTNL_DESC_SET_DATATYPE:
> > > +		return nftnl_set_desc_build_dtype(udbuf, dset);
> > > +	case NFTNL_DESC_SET_TYPEOF:
> > > +		return nftnl_set_desc_build_typeof(udbuf, dset);
> > > +	case NFTNL_DESC_SET_UNSPEC:
> > > +		return -1;
> > > +	}
> > > +
> > > +	if (!nftnl_udata_put_strz(udbuf, NFTNL_UDATA_SET_COMMENT, dset->comment))
> > > +		return -1;
> > > +
> > > +	return -1;
> > > +}
> > 
> > This is odd: Depending on dset->type, nftnl_set_desc_build_udata() calls
> > either nftnl_set_desc_build_dtype() or nftnl_set_desc_build_typeof().
> 
> That's indeed incorrect, nftnl_set_desc_build_typeof() should be:
> 
> static int nftnl_set_desc_build_typeof(struct nftnl_udata_buf *udbuf,
> 				       const struct nftnl_set_desc *dset)
> {
> 	struct nftnl_udata *nest;
> 	int i;
> 
> 	nest = nftnl_udata_nest_start(udbuf, NFTNL_UDATA_SET_KEY_TYPEOF);
> 	for (i = 0; i < dset->key.num_typeof; i++)
> 		__nftnl_set_desc_build_typeof(udbuf, dset->key.expr[i], i);
> 
> 	nftnl_udata_nest_end(udbuf, nest);
> 
> 	return 0;
> }

Ah, so only the NFTNL_DESC_SET_TYPEOF case content.

> The idea is: A set can either use datatype or typeof to define the
> elements that it stores. Then, a concatenation is possible.
> 
> > Yet both check dset->type again. This looks like a mix-up of set/map key
> > and data definitions and typeof vs. "regular" definition styles.
> 
> This patchset was a sketch PoC, I should have label it more explicit
> as such.

Sure, I was aware of that. Figuring out how to make use of it is a bit
of forensics though. The API is not identical to the one internally in
nftables it replaces and since I don't know how it is supposed to
function but only see how it may function based on the existing code, it
is not easy to figure out which pieces are missing or where to adjust
either this series or nftables code.

Cheers, Phil

Re: [PATCH libnftnl 0/3] add description infrastructure

[Phil Sutter]

Hi Pablo,

On Thu, Jan 20, 2022 at 01:03:59AM +0100, Pablo Neira Ayuso wrote:
> This is my proposal to address the snprintf data printing depending on
> the arch. The idea is to add description objects that can be used to
> build the userdata area as well as to parse the userdata to create the
> description object.
> 
> This is revisiting 6e48df5329ea ("src: add "typeof" build/parse/print
> support") in nftables which adds build and parse userdata callbacks to
> expression in libnftables. My proposal is to move this to libnftnl.

Looking at your PoC again, I assume it was meant for use by applications
to create and populate an nftnl_set_desc object and serialize it into
nftnl_set's userdata using nftnl_set_desc_build_udata(). Since the
information is needed within libnftnl though, the whole API does not
make sense anymore and nftnl_set_desc must be serialized by libnftnl
itself. This in turn means one may just integrate the data structure
into nftnl_set's 'desc' field directly and extend nftnl_set_set_data()
to allow populating the new fields, plus
nftnl_set_desc_add_{expr,datatype}() I guess.

Am I on the right track there?

Maybe it's quicker for me to add the missing bits to my stuff instead of
adjusting it to your series after making it work for the intended
purpose. Especially since I'm not quite sure what goal we're trying to
achieve.

Cheers, Phil

Re: [PATCH libnftnl 0/3] add description infrastructure

[Pablo Neira Ayuso]

On Wed, Apr 06, 2022 at 01:06:13PM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Thu, Jan 20, 2022 at 01:03:59AM +0100, Pablo Neira Ayuso wrote:
> > This is my proposal to address the snprintf data printing depending on
> > the arch. The idea is to add description objects that can be used to
> > build the userdata area as well as to parse the userdata to create the
> > description object.
> > 
> > This is revisiting 6e48df5329ea ("src: add "typeof" build/parse/print
> > support") in nftables which adds build and parse userdata callbacks to
> > expression in libnftables. My proposal is to move this to libnftnl.
> 
> Looking at your PoC again, I assume it was meant for use by applications
> to create and populate an nftnl_set_desc object and serialize it into
> nftnl_set's userdata using nftnl_set_desc_build_udata(). Since the
> information is needed within libnftnl though, the whole API does not
> make sense anymore and nftnl_set_desc must be serialized by libnftnl
> itself. This in turn means one may just integrate the data structure
> into nftnl_set's 'desc' field directly and extend nftnl_set_set_data()
> to allow populating the new fields, plus
> nftnl_set_desc_add_{expr,datatype}() I guess.
> 
> Am I on the right track there?
> 
> Maybe it's quicker for me to add the missing bits to my stuff instead of
> adjusting it to your series after making it work for the intended
> purpose. Especially since I'm not quite sure what goal we're trying to
> achieve.

Goal is to consolidate code. Move the existing code in nftables to
libnftnl so there is a desc object that can be use to build the
userdata and to all assist the libnftnl print functions.

This will take a bit of work.