[nf_tables 1/3] netfilter: nf_tables: store and dump sets mechanism options

[nf_tables 1/3] netfilter: nf_tables: store and dump sets mechanism options

[Arturo Borrero Gonzalez]

The sets mechanism options was not being stored anywhere.

We want to know in which cases the user explicitly set the mechanism
options. In that case, we also want to dump back the info.

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 include/net/netfilter/nf_tables.h |   12 +++++++++++
 net/netfilter/nf_tables_api.c     |   42 +++++++++++++++++++++++++++----------
 2 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index c4d8619..a9c6387 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -231,6 +231,14 @@ struct nft_set_ops {
 int nft_register_set(struct nft_set_ops *ops);
 void nft_unregister_set(struct nft_set_ops *ops);
 
+/* internal flags to know which attributes were originally set
+ * from userspace.
+ */
+enum nft_set_attr {
+	NFT_SET_ATTR_POLICY	= 0x1,
+	NFT_SET_ATTR_DESC_SIZE	= 0x2,
+};
+
 /**
  * 	struct nft_set - nf_tables set instance
  *
@@ -241,6 +249,8 @@ void nft_unregister_set(struct nft_set_ops *ops);
  * 	@dtype: data type (verdict or numeric type defined by userspace)
  * 	@size: maximum set size
  * 	@nelems: number of elements
+ *	@attr_flags: (enum nft_set_flags)
+ *	@policy: (enum nft_set_policies)
  * 	@ops: set ops
  * 	@flags: set flags
  * 	@klen: key length
@@ -255,6 +265,8 @@ struct nft_set {
 	u32				dtype;
 	u32				size;
 	u32				nelems;
+	u16				attr_flags;
+	u32				policy;
 	/* runtime data below here */
 	const struct nft_set_ops	*ops ____cacheline_aligned;
 	u16				flags;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 8237460..d1c3f3e 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2342,13 +2342,24 @@ static int nf_tables_fill_set(struct sk_buff *skb, const struct nft_ctx *ctx,
 			goto nla_put_failure;
 	}
 
-	desc = nla_nest_start(skb, NFTA_SET_DESC);
-	if (desc == NULL)
-		goto nla_put_failure;
-	if (set->size &&
-	    nla_put_be32(skb, NFTA_SET_DESC_SIZE, htonl(set->size)))
-		goto nla_put_failure;
-	nla_nest_end(skb, desc);
+	/* dump policy and desc info only if they were explicitly set */
+	if (set->attr_flags & (1 << NFT_SET_ATTR_POLICY)) {
+		if (nla_put_be32(skb, NFTA_SET_POLICY, htonl(set->policy)))
+			goto nla_put_failure;
+	}
+
+	if (set->attr_flags & (1 << NFT_SET_ATTR_DESC_SIZE)) {
+		desc = nla_nest_start(skb, NFTA_SET_DESC);
+		if (desc == NULL)
+			goto nla_put_failure;
+
+		if (set->size &&
+		    nla_put_be32(skb, NFTA_SET_DESC_SIZE, htonl(set->size))) {
+			goto nla_put_failure;
+		}
+
+		nla_nest_end(skb, desc);
+	}
 
 	return nlmsg_end(skb, nlh);
 
@@ -2519,7 +2530,8 @@ err:
 
 static int nf_tables_set_desc_parse(const struct nft_ctx *ctx,
 				    struct nft_set_desc *desc,
-				    const struct nlattr *nla)
+				    const struct nlattr *nla,
+				    u16 *attr_flags)
 {
 	struct nlattr *da[NFTA_SET_DESC_MAX + 1];
 	int err;
@@ -2528,8 +2540,10 @@ static int nf_tables_set_desc_parse(const struct nft_ctx *ctx,
 	if (err < 0)
 		return err;
 
-	if (da[NFTA_SET_DESC_SIZE] != NULL)
+	if (da[NFTA_SET_DESC_SIZE] != NULL) {
 		desc->size = ntohl(nla_get_be32(da[NFTA_SET_DESC_SIZE]));
+		*attr_flags |= (1 << NFT_SET_ATTR_DESC_SIZE);
+	}
 
 	return 0;
 }
@@ -2549,6 +2563,7 @@ static int nf_tables_newset(struct sock *nlsk, struct sk_buff *skb,
 	unsigned int size;
 	bool create;
 	u32 ktype, dtype, flags, policy;
+	u16 attr_flags = 0;
 	struct nft_set_desc desc;
 	int err;
 
@@ -2602,11 +2617,14 @@ static int nf_tables_newset(struct sock *nlsk, struct sk_buff *skb,
 		return -EINVAL;
 
 	policy = NFT_SET_POL_PERFORMANCE;
-	if (nla[NFTA_SET_POLICY] != NULL)
+	if (nla[NFTA_SET_POLICY] != NULL) {
 		policy = ntohl(nla_get_be32(nla[NFTA_SET_POLICY]));
+		attr_flags |= (1 << NFT_SET_ATTR_POLICY);
+	}
 
 	if (nla[NFTA_SET_DESC] != NULL) {
-		err = nf_tables_set_desc_parse(&ctx, &desc, nla[NFTA_SET_DESC]);
+		err = nf_tables_set_desc_parse(&ctx, &desc, nla[NFTA_SET_DESC],
+					       &attr_flags);
 		if (err < 0)
 			return err;
 	}
@@ -2667,6 +2685,8 @@ static int nf_tables_newset(struct sock *nlsk, struct sk_buff *skb,
 	set->dlen  = desc.dlen;
 	set->flags = flags;
 	set->size  = desc.size;
+	set->attr_flags	= attr_flags;
+	set->policy	= policy;
 
 	err = ops->init(set, &desc, nla);
 	if (err < 0)
-- 
1.7.10.4

[libnftnl 2/3] set: fix set nlmsg desc parsing

[Arturo Borrero Gonzalez]

In commit [ff62959](set: add support for set mechanism selection) the
support for parsing the nested attribute (NFTA_SET_DESC) was invalid.

This patch address the issue.

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 src/set.c |   22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/src/set.c b/src/set.c
index 3c38334..223ddec 100644
--- a/src/set.c
+++ b/src/set.c
@@ -340,7 +340,7 @@ static int nft_set_desc_parse_attr_cb(const struct nlattr *attr, void *data)
 static int nft_set_desc_parse(struct nft_set *s,
 			      const struct nlattr *attr)
 {
-	struct nlattr *tb[NFTA_SET_MAX+1] = {};
+	struct nlattr *tb[NFTA_SET_DESC_MAX + 1] = {};
 
 	if (mnl_attr_parse_nested(attr, nft_set_desc_parse_attr_cb, tb) < 0)
 		return -1;
@@ -353,24 +353,6 @@ static int nft_set_desc_parse(struct nft_set *s,
 	return 0;
 }
 
-static int nft_set_nlmsg_desc_parse(const struct nlattr *nest,
-				    struct nft_set *s)
-{
-	struct nlattr *attr;
-	int ret = 0;
-
-	mnl_attr_for_each_nested(attr, nest) {
-		if (mnl_attr_get_type(attr) != NFTA_SET_DESC)
-			return -1;
-
-		ret = nft_set_desc_parse(s, attr);
-		if (ret != 0)
-			break;
-	}
-
-	return ret;
-}
-
 int nft_set_nlmsg_parse(const struct nlmsghdr *nlh, struct nft_set *s)
 {
 	struct nlattr *tb[NFTA_SET_MAX+1] = {};
@@ -417,7 +399,7 @@ int nft_set_nlmsg_parse(const struct nlmsghdr *nlh, struct nft_set *s)
 		s->flags |= (1 << NFT_SET_ATTR_POLICY);
 	}
 	if (tb[NFTA_SET_DESC])
-		ret = nft_set_nlmsg_desc_parse(tb[NFTA_SET_DESC], s);
+		ret = nft_set_desc_parse(s, tb[NFTA_SET_DESC]);
 
 	s->family = nfg->nfgen_family;
 	s->flags |= (1 << NFT_SET_ATTR_FAMILY);
-- 
1.7.10.4

[nft 3/3] src: add set optimization options

[Arturo Borrero Gonzalez]

This patch adds options to choose set optimization mechanisms.

Two new statements are added to the set syntax, and they can be mixed:

 nft add set filter set1 { type ipv4_addr ; size 1024 ; }
 nft add set filter set1 { type ipv4_addr ; policy memory ; }
 nft add set filter set1 { type ipv4_addr ; policy performance ; }
 nft add set filter set1 { type ipv4_addr ; policy memory ; size 1024 ; }
 nft add set filter set1 { type ipv4_addr ; size 1024 ; policy memory ; }
 nft add set filter set1 { type ipv4_addr ; policy performance ; size 1024 ; }
 nft add set filter set1 { type ipv4_addr ; size 1024 ; policy performance ; }

Also valid for maps:

 nft add map filter map1 { type ipv4_addr : verdict ; policy performace ; }
 [...]


This is the output format, which can be imported later with `nft -f':

table filter {
	set set1 {
		type ipv4_addr
		policy memory
		size 1024
	}
}

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 include/rule.h |    8 ++++++++
 src/netlink.c  |   19 +++++++++++++++++++
 src/parser.y   |   31 +++++++++++++++++++++++++++++++
 src/rule.c     |   27 +++++++++++++++++++++++++++
 src/scanner.l  |    5 +++++
 5 files changed, 90 insertions(+)

diff --git a/include/rule.h b/include/rule.h
index 88aefc6..49a39ea 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -180,6 +180,9 @@ enum set_flags {
  * @datatype:	mapping data type
  * @datalen:	mapping data len
  * @init:	initializer
+ * @attr_flags:	set attr flags
+ * @policy:	set mechanism policy
+ * @desc:	set mechanism desc
  */
 struct set {
 	struct list_head	list;
@@ -192,6 +195,11 @@ struct set {
 	const struct datatype	*datatype;
 	unsigned int		datalen;
 	struct expr		*init;
+	uint32_t		attr_flags;
+	uint32_t		policy;
+	struct {
+		uint32_t	size;
+	} desc;
 };
 
 extern struct set *set_alloc(const struct location *loc);
diff --git a/src/netlink.c b/src/netlink.c
index 84d5db3..eca4858 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -1045,6 +1045,17 @@ static struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
 		set->datalen = data_len * BITS_PER_BYTE;
 	}
 
+	if (nft_set_attr_is_set(nls, NFT_SET_ATTR_POLICY)) {
+		set->policy = nft_set_attr_get_u32(nls, NFT_SET_ATTR_POLICY);
+		set->attr_flags |= (1 << NFT_SET_ATTR_POLICY);
+	}
+
+	if (nft_set_attr_is_set(nls, NFT_SET_ATTR_DESC_SIZE)) {
+		set->desc.size = nft_set_attr_get_u32(nls,
+						      NFT_SET_ATTR_DESC_SIZE);
+		set->attr_flags |= (1 << NFT_SET_ATTR_DESC_SIZE);
+	}
+
 	return set;
 }
 
@@ -1103,6 +1114,14 @@ static int netlink_add_set_batch(struct netlink_ctx *ctx,
 	}
 	set->handle.set_id = ++set_id;
 	nft_set_attr_set_u32(nls, NFT_SET_ATTR_ID, set->handle.set_id);
+
+	if (set->attr_flags & (1 << NFT_SET_ATTR_POLICY))
+		nft_set_attr_set_u32(nls, NFT_SET_ATTR_POLICY, set->policy);
+
+	if (set->attr_flags & (1 << NFT_SET_ATTR_DESC_SIZE))
+		nft_set_attr_set_u32(nls, NFT_SET_ATTR_DESC_SIZE,
+				     set->desc.size);
+
 	netlink_dump_set(nls);
 
 	err = mnl_nft_set_batch_add(nf_sock, nls, NLM_F_EXCL, ctx->seqnum);
diff --git a/src/parser.y b/src/parser.y
index c9b22f0..8c8f94a 100644
--- a/src/parser.y
+++ b/src/parser.y
@@ -20,6 +20,7 @@
 #include <linux/netfilter/nf_tables.h>
 #include <linux/netfilter/nf_conntrack_tuple_common.h>
 #include <libnftnl/common.h>
+#include <libnftnl/set.h>
 
 #include <rule.h>
 #include <statement.h>
@@ -201,6 +202,11 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %token INTERVAL			"interval"
 %token ELEMENTS			"elements"
 
+%token POLICY			"policy"
+%token MEMORY			"memory"
+%token PERFORMANCE		"performance"
+%token SIZE			"size"
+
 %token <val> NUM		"number"
 %token <string> STRING		"string"
 %token <string> QUOTED_STRING
@@ -401,6 +407,9 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 
 %type <val>			set_flag_list	set_flag
 
+%type <val>			set_policy_spec
+%type <val>			set_size_spec
+
 %type <set>			set_block_alloc set_block
 %destructor { set_free($$); }	set_block_alloc
 
@@ -953,6 +962,7 @@ set_block		:	/* empty */	{ $$ = $<set>-1; }
 						   state->msgs);
 					YYERROR;
 				}
+
 				$$ = $1;
 			}
 			|	set_block	FLAGS		set_flag_list	stmt_seperator
@@ -965,6 +975,7 @@ set_block		:	/* empty */	{ $$ = $<set>-1; }
 				$1->init = $4;
 				$$ = $1;
 			}
+			|	set_block	set_mechanism	stmt_seperator
 			;
 
 set_flag_list		:	set_flag_list	COMMA		set_flag
@@ -1018,6 +1029,26 @@ map_block		:	/* empty */	{ $$ = $<set>-1; }
 				$1->init = $4;
 				$$ = $1;
 			}
+			|	map_block	set_mechanism	stmt_seperator
+			;
+
+set_mechanism		:	set_policy_spec
+			{
+				$<set>0->attr_flags |= (1 << NFT_SET_ATTR_POLICY);
+				$<set>0->policy = $1;
+			}
+			|	set_size_spec
+			{
+				$<set>0->attr_flags |= (1 << NFT_SET_ATTR_DESC_SIZE);
+				$<set>0->desc.size = $1;
+			}
+			;
+
+set_policy_spec		:	POLICY	PERFORMANCE	{ $$ = NFT_SET_POL_PERFORMANCE; }
+			|	POLICY	MEMORY		{ $$ = NFT_SET_POL_MEMORY; }
+			;
+
+set_size_spec		:	SIZE	NUM	{ $$ = $2; }
 			;
 
 hook_spec		:	TYPE		STRING		HOOK		STRING		PRIORITY	NUM
diff --git a/src/rule.c b/src/rule.c
index 80deb1b..e2290d7 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -75,6 +75,7 @@ void set_free(struct set *set)
 	if (--set->refcnt > 0)
 		return;
 	handle_free(&set->handle);
+
 	xfree(set);
 }
 
@@ -90,6 +91,9 @@ struct set *set_clone(const struct set *set)
 	newset->datatype = set->datatype;
 	newset->datalen = set->datalen;
 	newset->init = expr_clone(set->init);
+	newset->attr_flags = set->attr_flags;
+	newset->policy = set->policy;
+	newset->desc.size = set->desc.size;
 
 	return newset;
 }
@@ -134,6 +138,18 @@ struct print_fmt_options {
 	const char	*stmt_separator;
 };
 
+static const char *set_policy2str(uint32_t policy)
+{
+	switch (policy) {
+	case NFT_SET_POL_PERFORMANCE:
+		return "performance";
+	case NFT_SET_POL_MEMORY:
+		return "memory";
+	default:
+		return "unknown";
+	}
+}
+
 static void do_set_print(const struct set *set, struct print_fmt_options *opts)
 {
 	const char *delim = "";
@@ -153,8 +169,19 @@ static void do_set_print(const struct set *set, struct print_fmt_options *opts)
 	printf("%s%stype %s", opts->tab, opts->tab, set->keytype->name);
 	if (set->flags & SET_F_MAP)
 		printf(" : %s", set->datatype->name);
+
 	printf("%s", opts->stmt_separator);
 
+	if (set->attr_flags & (1 << NFT_SET_ATTR_POLICY)) {
+		printf("%s%spolicy %s%s", opts->tab, opts->tab,
+		       set_policy2str(set->policy), opts->stmt_separator);
+	}
+
+	if (set->attr_flags & (1 << NFT_SET_ATTR_DESC_SIZE)) {
+		printf("%s%ssize %u%s", opts->tab, opts->tab,
+		       set->desc.size, opts->stmt_separator);
+	}
+
 	if (set->flags & (SET_F_CONSTANT | SET_F_INTERVAL)) {
 		printf("%s%sflags ", opts->tab, opts->tab);
 		if (set->flags & SET_F_CONSTANT) {
diff --git a/src/scanner.l b/src/scanner.l
index 8aab38f..6458d09 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -271,6 +271,11 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "interval"		{ return INTERVAL; }
 "elements"		{ return ELEMENTS; }
 
+"policy"		{ return POLICY; }
+"size"			{ return SIZE; }
+"performance"		{ return PERFORMANCE; }
+"memory"		{ return MEMORY; }
+
 "counter"		{ return COUNTER; }
 "packets"		{ return PACKETS; }
 "bytes"			{ return BYTES; }
-- 
1.7.10.4

Re: [nf_tables 1/3] netfilter: nf_tables: store and dump sets mechanism options

[Patrick McHardy]

On 18. September 2014 20:18:18 MESZ, Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote:
>The sets mechanism options was not being stored anywhere.
>
>We want to know in which cases the user explicitly set the mechanism
>options. In that case, we also want to dump back the info.
>

I don't think this is needed. Basically we always want to dump options for non-constant sets, since they couldn't have been chosen automatically, and don't need to dump them for constant sets, since they can always be determined automatically based on the contents.



>Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
>---
> include/net/netfilter/nf_tables.h |   12 +++++++++++
>net/netfilter/nf_tables_api.c     |   42
>+++++++++++++++++++++++++++----------
> 2 files changed, 43 insertions(+), 11 deletions(-)
>
>diff --git a/include/net/netfilter/nf_tables.h
>b/include/net/netfilter/nf_tables.h
>index c4d8619..a9c6387 100644
>--- a/include/net/netfilter/nf_tables.h
>+++ b/include/net/netfilter/nf_tables.h
>@@ -231,6 +231,14 @@ struct nft_set_ops {
> int nft_register_set(struct nft_set_ops *ops);
> void nft_unregister_set(struct nft_set_ops *ops);
> 
>+/* internal flags to know which attributes were originally set
>+ * from userspace.
>+ */
>+enum nft_set_attr {
>+	NFT_SET_ATTR_POLICY	= 0x1,
>+	NFT_SET_ATTR_DESC_SIZE	= 0x2,
>+};
>+
> /**
>  * 	struct nft_set - nf_tables set instance
>  *
>@@ -241,6 +249,8 @@ void nft_unregister_set(struct nft_set_ops *ops);
>  * 	@dtype: data type (verdict or numeric type defined by userspace)
>  * 	@size: maximum set size
>  * 	@nelems: number of elements
>+ *	@attr_flags: (enum nft_set_flags)
>+ *	@policy: (enum nft_set_policies)
>  * 	@ops: set ops
>  * 	@flags: set flags
>  * 	@klen: key length
>@@ -255,6 +265,8 @@ struct nft_set {
> 	u32				dtype;
> 	u32				size;
> 	u32				nelems;
>+	u16				attr_flags;
>+	u32				policy;
> 	/* runtime data below here */
> 	const struct nft_set_ops	*ops ____cacheline_aligned;
> 	u16				flags;
>diff --git a/net/netfilter/nf_tables_api.c
>b/net/netfilter/nf_tables_api.c
>index 8237460..d1c3f3e 100644
>--- a/net/netfilter/nf_tables_api.c
>+++ b/net/netfilter/nf_tables_api.c
>@@ -2342,13 +2342,24 @@ static int nf_tables_fill_set(struct sk_buff
>*skb, const struct nft_ctx *ctx,
> 			goto nla_put_failure;
> 	}
> 
>-	desc = nla_nest_start(skb, NFTA_SET_DESC);
>-	if (desc == NULL)
>-		goto nla_put_failure;
>-	if (set->size &&
>-	    nla_put_be32(skb, NFTA_SET_DESC_SIZE, htonl(set->size)))
>-		goto nla_put_failure;
>-	nla_nest_end(skb, desc);
>+	/* dump policy and desc info only if they were explicitly set */
>+	if (set->attr_flags & (1 << NFT_SET_ATTR_POLICY)) {
>+		if (nla_put_be32(skb, NFTA_SET_POLICY, htonl(set->policy)))
>+			goto nla_put_failure;
>+	}
>+
>+	if (set->attr_flags & (1 << NFT_SET_ATTR_DESC_SIZE)) {
>+		desc = nla_nest_start(skb, NFTA_SET_DESC);
>+		if (desc == NULL)
>+			goto nla_put_failure;
>+
>+		if (set->size &&
>+		    nla_put_be32(skb, NFTA_SET_DESC_SIZE, htonl(set->size))) {
>+			goto nla_put_failure;
>+		}
>+
>+		nla_nest_end(skb, desc);
>+	}
> 
> 	return nlmsg_end(skb, nlh);
> 
>@@ -2519,7 +2530,8 @@ err:
> 
> static int nf_tables_set_desc_parse(const struct nft_ctx *ctx,
> 				    struct nft_set_desc *desc,
>-				    const struct nlattr *nla)
>+				    const struct nlattr *nla,
>+				    u16 *attr_flags)
> {
> 	struct nlattr *da[NFTA_SET_DESC_MAX + 1];
> 	int err;
>@@ -2528,8 +2540,10 @@ static int nf_tables_set_desc_parse(const struct
>nft_ctx *ctx,
> 	if (err < 0)
> 		return err;
> 
>-	if (da[NFTA_SET_DESC_SIZE] != NULL)
>+	if (da[NFTA_SET_DESC_SIZE] != NULL) {
> 		desc->size = ntohl(nla_get_be32(da[NFTA_SET_DESC_SIZE]));
>+		*attr_flags |= (1 << NFT_SET_ATTR_DESC_SIZE);
>+	}
> 
> 	return 0;
> }
>@@ -2549,6 +2563,7 @@ static int nf_tables_newset(struct sock *nlsk,
>struct sk_buff *skb,
> 	unsigned int size;
> 	bool create;
> 	u32 ktype, dtype, flags, policy;
>+	u16 attr_flags = 0;
> 	struct nft_set_desc desc;
> 	int err;
> 
>@@ -2602,11 +2617,14 @@ static int nf_tables_newset(struct sock *nlsk,
>struct sk_buff *skb,
> 		return -EINVAL;
> 
> 	policy = NFT_SET_POL_PERFORMANCE;
>-	if (nla[NFTA_SET_POLICY] != NULL)
>+	if (nla[NFTA_SET_POLICY] != NULL) {
> 		policy = ntohl(nla_get_be32(nla[NFTA_SET_POLICY]));
>+		attr_flags |= (1 << NFT_SET_ATTR_POLICY);
>+	}
> 
> 	if (nla[NFTA_SET_DESC] != NULL) {
>-		err = nf_tables_set_desc_parse(&ctx, &desc, nla[NFTA_SET_DESC]);
>+		err = nf_tables_set_desc_parse(&ctx, &desc, nla[NFTA_SET_DESC],
>+					       &attr_flags);
> 		if (err < 0)
> 			return err;
> 	}
>@@ -2667,6 +2685,8 @@ static int nf_tables_newset(struct sock *nlsk,
>struct sk_buff *skb,
> 	set->dlen  = desc.dlen;
> 	set->flags = flags;
> 	set->size  = desc.size;
>+	set->attr_flags	= attr_flags;
>+	set->policy	= policy;
> 
> 	err = ops->init(set, &desc, nla);
> 	if (err < 0)

Re: [nft 3/3] src: add set optimization options

[Patrick McHardy]

On 18. September 2014 20:18:20 MESZ, Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote:
>This patch adds options to choose set optimization mechanisms.
>
>Two new statements are added to the set syntax, and they can be mixed:
>
> nft add set filter set1 { type ipv4_addr ; size 1024 ; }
> nft add set filter set1 { type ipv4_addr ; policy memory ; }
> nft add set filter set1 { type ipv4_addr ; policy performance ; }
>nft add set filter set1 { type ipv4_addr ; policy memory ; size 1024 ;
>}
>nft add set filter set1 { type ipv4_addr ; size 1024 ; policy memory ;
>}
>nft add set filter set1 { type ipv4_addr ; policy performance ; size
>1024 ; }
>nft add set filter set1 { type ipv4_addr ; size 1024 ; policy
>performance ; }
>
>Also valid for maps:
>
>nft add map filter map1 { type ipv4_addr : verdict ; policy performace
>; }
> [...]
>
>
>This is the output format, which can be imported later with `nft -f':
>
>table filter {
>	set set1 {
>		type ipv4_addr
>		policy memory
>		size 1024
>	}
>}

Conceptually this looks good, I'll have a look at the implementation after dinner.

What my patch did was only handle the case where limits can be determined automatically, IOW literal sets. Both is needed.

>
>Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
>---
> include/rule.h |    8 ++++++++
> src/netlink.c  |   19 +++++++++++++++++++
> src/parser.y   |   31 +++++++++++++++++++++++++++++++
> src/rule.c     |   27 +++++++++++++++++++++++++++
> src/scanner.l  |    5 +++++
> 5 files changed, 90 insertions(+)
>
>diff --git a/include/rule.h b/include/rule.h
>index 88aefc6..49a39ea 100644
>--- a/include/rule.h
>+++ b/include/rule.h
>@@ -180,6 +180,9 @@ enum set_flags {
>  * @datatype:	mapping data type
>  * @datalen:	mapping data len
>  * @init:	initializer
>+ * @attr_flags:	set attr flags
>+ * @policy:	set mechanism policy
>+ * @desc:	set mechanism desc
>  */
> struct set {
> 	struct list_head	list;
>@@ -192,6 +195,11 @@ struct set {
> 	const struct datatype	*datatype;
> 	unsigned int		datalen;
> 	struct expr		*init;
>+	uint32_t		attr_flags;
>+	uint32_t		policy;
>+	struct {
>+		uint32_t	size;
>+	} desc;
> };
> 
> extern struct set *set_alloc(const struct location *loc);
>diff --git a/src/netlink.c b/src/netlink.c
>index 84d5db3..eca4858 100644
>--- a/src/netlink.c
>+++ b/src/netlink.c
>@@ -1045,6 +1045,17 @@ static struct set
>*netlink_delinearize_set(struct netlink_ctx *ctx,
> 		set->datalen = data_len * BITS_PER_BYTE;
> 	}
> 
>+	if (nft_set_attr_is_set(nls, NFT_SET_ATTR_POLICY)) {
>+		set->policy = nft_set_attr_get_u32(nls, NFT_SET_ATTR_POLICY);
>+		set->attr_flags |= (1 << NFT_SET_ATTR_POLICY);
>+	}
>+
>+	if (nft_set_attr_is_set(nls, NFT_SET_ATTR_DESC_SIZE)) {
>+		set->desc.size = nft_set_attr_get_u32(nls,
>+						      NFT_SET_ATTR_DESC_SIZE);
>+		set->attr_flags |= (1 << NFT_SET_ATTR_DESC_SIZE);
>+	}
>+
> 	return set;
> }
> 
>@@ -1103,6 +1114,14 @@ static int netlink_add_set_batch(struct
>netlink_ctx *ctx,
> 	}
> 	set->handle.set_id = ++set_id;
> 	nft_set_attr_set_u32(nls, NFT_SET_ATTR_ID, set->handle.set_id);
>+
>+	if (set->attr_flags & (1 << NFT_SET_ATTR_POLICY))
>+		nft_set_attr_set_u32(nls, NFT_SET_ATTR_POLICY, set->policy);
>+
>+	if (set->attr_flags & (1 << NFT_SET_ATTR_DESC_SIZE))
>+		nft_set_attr_set_u32(nls, NFT_SET_ATTR_DESC_SIZE,
>+				     set->desc.size);
>+
> 	netlink_dump_set(nls);
> 
> 	err = mnl_nft_set_batch_add(nf_sock, nls, NLM_F_EXCL, ctx->seqnum);
>diff --git a/src/parser.y b/src/parser.y
>index c9b22f0..8c8f94a 100644
>--- a/src/parser.y
>+++ b/src/parser.y
>@@ -20,6 +20,7 @@
> #include <linux/netfilter/nf_tables.h>
> #include <linux/netfilter/nf_conntrack_tuple_common.h>
> #include <libnftnl/common.h>
>+#include <libnftnl/set.h>
> 
> #include <rule.h>
> #include <statement.h>
>@@ -201,6 +202,11 @@ static void location_update(struct location *loc,
>struct location *rhs, int n)
> %token INTERVAL			"interval"
> %token ELEMENTS			"elements"
> 
>+%token POLICY			"policy"
>+%token MEMORY			"memory"
>+%token PERFORMANCE		"performance"
>+%token SIZE			"size"
>+
> %token <val> NUM		"number"
> %token <string> STRING		"string"
> %token <string> QUOTED_STRING
>@@ -401,6 +407,9 @@ static void location_update(struct location *loc,
>struct location *rhs, int n)
> 
> %type <val>			set_flag_list	set_flag
> 
>+%type <val>			set_policy_spec
>+%type <val>			set_size_spec
>+
> %type <set>			set_block_alloc set_block
> %destructor { set_free($$); }	set_block_alloc
> 
>@@ -953,6 +962,7 @@ set_block		:	/* empty */	{ $$ = $<set>-1; }
> 						   state->msgs);
> 					YYERROR;
> 				}
>+
> 				$$ = $1;
> 			}
> 			|	set_block	FLAGS		set_flag_list	stmt_seperator
>@@ -965,6 +975,7 @@ set_block		:	/* empty */	{ $$ = $<set>-1; }
> 				$1->init = $4;
> 				$$ = $1;
> 			}
>+			|	set_block	set_mechanism	stmt_seperator
> 			;
> 
> set_flag_list		:	set_flag_list	COMMA		set_flag
>@@ -1018,6 +1029,26 @@ map_block		:	/* empty */	{ $$ = $<set>-1; }
> 				$1->init = $4;
> 				$$ = $1;
> 			}
>+			|	map_block	set_mechanism	stmt_seperator
>+			;
>+
>+set_mechanism		:	set_policy_spec
>+			{
>+				$<set>0->attr_flags |= (1 << NFT_SET_ATTR_POLICY);
>+				$<set>0->policy = $1;
>+			}
>+			|	set_size_spec
>+			{
>+				$<set>0->attr_flags |= (1 << NFT_SET_ATTR_DESC_SIZE);
>+				$<set>0->desc.size = $1;
>+			}
>+			;
>+
>+set_policy_spec		:	POLICY	PERFORMANCE	{ $$ = NFT_SET_POL_PERFORMANCE;
>}
>+			|	POLICY	MEMORY		{ $$ = NFT_SET_POL_MEMORY; }
>+			;
>+
>+set_size_spec		:	SIZE	NUM	{ $$ = $2; }
> 			;
> 
> hook_spec		:	TYPE		STRING		HOOK		STRING		PRIORITY	NUM
>diff --git a/src/rule.c b/src/rule.c
>index 80deb1b..e2290d7 100644
>--- a/src/rule.c
>+++ b/src/rule.c
>@@ -75,6 +75,7 @@ void set_free(struct set *set)
> 	if (--set->refcnt > 0)
> 		return;
> 	handle_free(&set->handle);
>+
> 	xfree(set);
> }
> 
>@@ -90,6 +91,9 @@ struct set *set_clone(const struct set *set)
> 	newset->datatype = set->datatype;
> 	newset->datalen = set->datalen;
> 	newset->init = expr_clone(set->init);
>+	newset->attr_flags = set->attr_flags;
>+	newset->policy = set->policy;
>+	newset->desc.size = set->desc.size;
> 
> 	return newset;
> }
>@@ -134,6 +138,18 @@ struct print_fmt_options {
> 	const char	*stmt_separator;
> };
> 
>+static const char *set_policy2str(uint32_t policy)
>+{
>+	switch (policy) {
>+	case NFT_SET_POL_PERFORMANCE:
>+		return "performance";
>+	case NFT_SET_POL_MEMORY:
>+		return "memory";
>+	default:
>+		return "unknown";
>+	}
>+}
>+
>static void do_set_print(const struct set *set, struct
>print_fmt_options *opts)
> {
> 	const char *delim = "";
>@@ -153,8 +169,19 @@ static void do_set_print(const struct set *set,
>struct print_fmt_options *opts)
> 	printf("%s%stype %s", opts->tab, opts->tab, set->keytype->name);
> 	if (set->flags & SET_F_MAP)
> 		printf(" : %s", set->datatype->name);
>+
> 	printf("%s", opts->stmt_separator);
> 
>+	if (set->attr_flags & (1 << NFT_SET_ATTR_POLICY)) {
>+		printf("%s%spolicy %s%s", opts->tab, opts->tab,
>+		       set_policy2str(set->policy), opts->stmt_separator);
>+	}
>+
>+	if (set->attr_flags & (1 << NFT_SET_ATTR_DESC_SIZE)) {
>+		printf("%s%ssize %u%s", opts->tab, opts->tab,
>+		       set->desc.size, opts->stmt_separator);
>+	}
>+
> 	if (set->flags & (SET_F_CONSTANT | SET_F_INTERVAL)) {
> 		printf("%s%sflags ", opts->tab, opts->tab);
> 		if (set->flags & SET_F_CONSTANT) {
>diff --git a/src/scanner.l b/src/scanner.l
>index 8aab38f..6458d09 100644
>--- a/src/scanner.l
>+++ b/src/scanner.l
>@@ -271,6 +271,11 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
> "interval"		{ return INTERVAL; }
> "elements"		{ return ELEMENTS; }
> 
>+"policy"		{ return POLICY; }
>+"size"			{ return SIZE; }
>+"performance"		{ return PERFORMANCE; }
>+"memory"		{ return MEMORY; }
>+
> "counter"		{ return COUNTER; }
> "packets"		{ return PACKETS; }
> "bytes"			{ return BYTES; }

Re: [libnftnl 2/3] set: fix set nlmsg desc parsing

[Pablo Neira Ayuso]

On Thu, Sep 18, 2014 at 08:18:19PM +0200, Arturo Borrero Gonzalez wrote:
> In commit [ff62959](set: add support for set mechanism selection) the
> support for parsing the nested attribute (NFTA_SET_DESC) was invalid.

Applied this little fix, thanks Arturo.

Re: [nf_tables 1/3] netfilter: nf_tables: store and dump sets mechanism options

[Arturo Borrero Gonzalez]

On 18 September 2014 20:34, Patrick McHardy <kaber@trash.net> wrote:
> On 18. September 2014 20:18:18 MESZ, Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote:
>>The sets mechanism options was not being stored anywhere.
>>
>>We want to know in which cases the user explicitly set the mechanism
>>options. In that case, we also want to dump back the info.
>>
>
> I don't think this is needed. Basically we always want to dump options for non-constant sets, since they couldn't have been chosen automatically, and don't need to dump them for constant sets, since they can always be determined automatically based on the contents.
>

Could you please elaborate?

If the set is non-constant, currently the options are by default
NFT_SET_POL_PERFORMANCE and size 0.
Do you want these options to be printed to the end user even when the
user did not originally use the 'policy xx' and 'size xx' statements?
Or in the other hand, to never print the options even when the user
gave an explicit config?

In the case of constant sets, what If I want to test the performance
of different policies? I need to know which policy is applied to a
given set, (in order to change it).

-- 
Arturo Borrero González
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [nft 3/3] src: add set optimization options

[Arturo Borrero Gonzalez]

On 18 September 2014 20:39, Patrick McHardy <kaber@trash.net> wrote:
> On 18. September 2014 20:18:20 MESZ, Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote:
>>This patch adds options to choose set optimization mechanisms.
>>
>>Two new statements are added to the set syntax, and they can be mixed:
>>
>> nft add set filter set1 { type ipv4_addr ; size 1024 ; }
>> nft add set filter set1 { type ipv4_addr ; policy memory ; }
>> nft add set filter set1 { type ipv4_addr ; policy performance ; }
>>nft add set filter set1 { type ipv4_addr ; policy memory ; size 1024 ;
>>}
>>nft add set filter set1 { type ipv4_addr ; size 1024 ; policy memory ;
>>}
>>nft add set filter set1 { type ipv4_addr ; policy performance ; size
>>1024 ; }
>>nft add set filter set1 { type ipv4_addr ; size 1024 ; policy
>>performance ; }
>>
>>Also valid for maps:
>>
>>nft add map filter map1 { type ipv4_addr : verdict ; policy performace
>>; }
>> [...]
>>
>>
>>This is the output format, which can be imported later with `nft -f':
>>
>>table filter {
>>       set set1 {
>>               type ipv4_addr
>>               policy memory
>>               size 1024
>>       }
>>}
>
> Conceptually this looks good, I'll have a look at the implementation after dinner.
>
> What my patch did was only handle the case where limits can be determined automatically, IOW literal sets. Both is needed.
>

Do you mean to give the size parameter a value when we know the set
has a concrete number of elements?
For example:
add rule tcp dport {1 , 2 , 3} counter --> then add a set with fixed size 3.

I realize now the patch includes some newlines included by mistake. So
a v2 is likely to be needed.
-- 
Arturo Borrero González
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [nf_tables 1/3] netfilter: nf_tables: store and dump sets mechanism options

[Patrick McHardy]

On 19. September 2014 08:51:48 MESZ, Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote:
>On 18 September 2014 20:34, Patrick McHardy <kaber@trash.net> wrote:
>> On 18. September 2014 20:18:18 MESZ, Arturo Borrero Gonzalez
><arturo.borrero.glez@gmail.com> wrote:
>>>The sets mechanism options was not being stored anywhere.
>>>
>>>We want to know in which cases the user explicitly set the mechanism
>>>options. In that case, we also want to dump back the info.
>>>
>>
>> I don't think this is needed. Basically we always want to dump
>options for non-constant sets, since they couldn't have been chosen
>automatically, and don't need to dump them for constant sets, since
>they can always be determined automatically based on the contents.
>>
>
>Could you please elaborate?
>
>If the set is non-constant, currently the options are by default
>NFT_SET_POL_PERFORMANCE and size 0.
>Do you want these options to be printed to the end user even when the
>user did not originally use the 'policy xx' and 'size xx' statements?
>Or in the other hand, to never print the options even when the user
>gave an explicit config?

No. When we have literal/constant sets we can automatically determine the options, and we don't need to return them from the kernel. We could, but its useless.

For named sets any options that are not the default must have been specified by the user. So we must always dump them unless they are the default.

>In the case of constant sets, what If I want to test the performance
>of different policies? I need to know which policy is applied to a
>given set, (in order to change it).

Well, the mechanism is supposed to pick the best implementation. For testing, you can unload modules, the kernel won't autoload if at least one set implementation is available.

Re: [nft 3/3] src: add set optimization options

[Patrick McHardy]

On 19. September 2014 09:04:17 MESZ, Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote:
>On 18 September 2014 20:39, Patrick McHardy <kaber@trash.net> wrote:
>> On 18. September 2014 20:18:20 MESZ, Arturo Borrero Gonzalez
><arturo.borrero.glez@gmail.com> wrote:
>>>This patch adds options to choose set optimization mechanisms.
>>>
>>>Two new statements are added to the set syntax, and they can be
>mixed:
>>>
>>> nft add set filter set1 { type ipv4_addr ; size 1024 ; }
>>> nft add set filter set1 { type ipv4_addr ; policy memory ; }
>>> nft add set filter set1 { type ipv4_addr ; policy performance ; }
>>>nft add set filter set1 { type ipv4_addr ; policy memory ; size 1024
>;
>>>}
>>>nft add set filter set1 { type ipv4_addr ; size 1024 ; policy memory
>;
>>>}
>>>nft add set filter set1 { type ipv4_addr ; policy performance ; size
>>>1024 ; }
>>>nft add set filter set1 { type ipv4_addr ; size 1024 ; policy
>>>performance ; }
>>>
>>>Also valid for maps:
>>>
>>>nft add map filter map1 { type ipv4_addr : verdict ; policy
>performace
>>>; }
>>> [...]
>>>
>>>
>>>This is the output format, which can be imported later with `nft -f':
>>>
>>>table filter {
>>>       set set1 {
>>>               type ipv4_addr
>>>               policy memory
>>>               size 1024
>>>       }
>>>}
>>
>> Conceptually this looks good, I'll have a look at the implementation
>after dinner.

Sorry, late nicht after dinner, but I'll get to it tommorrow :)

>> What my patch did was only handle the case where limits can be
>determined automatically, IOW literal sets. Both is needed.
>>
>
>Do you mean to give the size parameter a value when we know the set
>has a concrete number of elements?
>For example:
>add rule tcp dport {1 , 2 , 3} counter --> then add a set with fixed
>size 3.

Yes, this is what my patch is doing, and some further analysis of common prefixes etc for an unfinished feature.

>I realize now the patch includes some newlines included by mistake. So
>a v2 is likely to be needed.

Yeah. I've only read it on my mobile so far, but I also noticed some changes which aren't needed, so please hold off with a V2 until tommorrow.

Re: [nf_tables 1/3] netfilter: nf_tables: store and dump sets mechanism options

[Patrick McHardy]

On Thu, Sep 18, 2014 at 08:18:18PM +0200, Arturo Borrero Gonzalez wrote:
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -231,6 +231,14 @@ struct nft_set_ops {
>  int nft_register_set(struct nft_set_ops *ops);
>  void nft_unregister_set(struct nft_set_ops *ops);
>  
> +/* internal flags to know which attributes were originally set
> + * from userspace.
> + */
> +enum nft_set_attr {
> +	NFT_SET_ATTR_POLICY	= 0x1,
> +	NFT_SET_ATTR_DESC_SIZE	= 0x2,
> +};

We don't need the size, any value != 0 is set by userspace. I'm unsure
about the policy, we don't really *need* to dump if it is the default,
and otherwise its obvious that it originated from userspace.

> @@ -241,6 +249,8 @@ void nft_unregister_set(struct nft_set_ops *ops);
>   * 	@dtype: data type (verdict or numeric type defined by userspace)
>   * 	@size: maximum set size
>   * 	@nelems: number of elements
> + *	@attr_flags: (enum nft_set_flags)
> + *	@policy: (enum nft_set_policies)
>   * 	@ops: set ops
>   * 	@flags: set flags
>   * 	@klen: key length
> @@ -255,6 +265,8 @@ struct nft_set {
>  	u32				dtype;
>  	u32				size;
>  	u32				nelems;
> +	u16				attr_flags;
> +	u32				policy;

These are way to big and introduce holes in the structure.

>  	/* runtime data below here */
>  	const struct nft_set_ops	*ops ____cacheline_aligned;
>  	u16				flags;
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 8237460..d1c3f3e 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -2342,13 +2342,24 @@ static int nf_tables_fill_set(struct sk_buff *skb, const struct nft_ctx *ctx,
>  			goto nla_put_failure;
>  	}
>  
> -	desc = nla_nest_start(skb, NFTA_SET_DESC);
> -	if (desc == NULL)
> -		goto nla_put_failure;
> -	if (set->size &&
> -	    nla_put_be32(skb, NFTA_SET_DESC_SIZE, htonl(set->size)))
> -		goto nla_put_failure;
> -	nla_nest_end(skb, desc);
> +	/* dump policy and desc info only if they were explicitly set */
> +	if (set->attr_flags & (1 << NFT_SET_ATTR_POLICY)) {
> +		if (nla_put_be32(skb, NFTA_SET_POLICY, htonl(set->policy)))
> +			goto nla_put_failure;
> +	}
> +
> +	if (set->attr_flags & (1 << NFT_SET_ATTR_DESC_SIZE)) {
> +		desc = nla_nest_start(skb, NFTA_SET_DESC);
> +		if (desc == NULL)
> +			goto nla_put_failure;
> +
> +		if (set->size &&
> +		    nla_put_be32(skb, NFTA_SET_DESC_SIZE, htonl(set->size))) {
> +			goto nla_put_failure;
> +		}
> +
> +		nla_nest_end(skb, desc);
> +	}

As mentioned earlier, dumping the parameters is not necessary for sets
with NFT_SET_CONSTANT as they have been determined automatically. 

Re: [nft 3/3] src: add set optimization options

[Patrick McHardy]

On Thu, Sep 18, 2014 at 08:18:20PM +0200, Arturo Borrero Gonzalez wrote:
> This patch adds options to choose set optimization mechanisms.
> 
> Two new statements are added to the set syntax, and they can be mixed:
> 
>  nft add set filter set1 { type ipv4_addr ; size 1024 ; }
>  nft add set filter set1 { type ipv4_addr ; policy memory ; }
>  nft add set filter set1 { type ipv4_addr ; policy performance ; }
>  nft add set filter set1 { type ipv4_addr ; policy memory ; size 1024 ; }
>  nft add set filter set1 { type ipv4_addr ; size 1024 ; policy memory ; }
>  nft add set filter set1 { type ipv4_addr ; policy performance ; size 1024 ; }
>  nft add set filter set1 { type ipv4_addr ; size 1024 ; policy performance ; }
> 
> Also valid for maps:
> 
>  nft add map filter map1 { type ipv4_addr : verdict ; policy performace ; }
>  [...]

I'd suggest to also add a global option which is the default unless
overridden by a specific set instantiation. Doesn't have to be done
in this patch however.

> @@ -1103,6 +1114,14 @@ static int netlink_add_set_batch(struct netlink_ctx *ctx,
>  	}
>  	set->handle.set_id = ++set_id;
>  	nft_set_attr_set_u32(nls, NFT_SET_ATTR_ID, set->handle.set_id);
> +
> +	if (set->attr_flags & (1 << NFT_SET_ATTR_POLICY))
> +		nft_set_attr_set_u32(nls, NFT_SET_ATTR_POLICY, set->policy);
> +
> +	if (set->attr_flags & (1 << NFT_SET_ATTR_DESC_SIZE))
> +		nft_set_attr_set_u32(nls, NFT_SET_ATTR_DESC_SIZE,
> +				     set->desc.size);
> +

As in the kernel, the size definitely doesn't need an attribute, the policy
can be argued about.

> @@ -965,6 +975,7 @@ set_block		:	/* empty */	{ $$ = $<set>-1; }
>  				$1->init = $4;
>  				$$ = $1;
>  			}
> +			|	set_block	set_mechanism	stmt_seperator
>  			;
>  
>  set_flag_list		:	set_flag_list	COMMA		set_flag
> @@ -1018,6 +1029,26 @@ map_block		:	/* empty */	{ $$ = $<set>-1; }
>  				$1->init = $4;
>  				$$ = $1;
>  			}
> +			|	map_block	set_mechanism	stmt_seperator
> +			;
> +
> +set_mechanism		:	set_policy_spec
> +			{
> +				$<set>0->attr_flags |= (1 << NFT_SET_ATTR_POLICY);
> +				$<set>0->policy = $1;
> +			}
> +			|	set_size_spec
> +			{
> +				$<set>0->attr_flags |= (1 << NFT_SET_ATTR_DESC_SIZE);
> +				$<set>0->desc.size = $1;
> +			}
> +			;
> +
> +set_policy_spec		:	POLICY	PERFORMANCE	{ $$ = NFT_SET_POL_PERFORMANCE; }
> +			|	POLICY	MEMORY		{ $$ = NFT_SET_POL_MEMORY; }
> +			;
> +
> +set_size_spec		:	SIZE	NUM	{ $$ = $2; }

We usually have rules like this:

set_policy:		PERFORMANCE ...
			MEMORY

and the keyword POLICY one level above.

Re: [nf_tables 1/3] netfilter: nf_tables: store and dump sets mechanism options

[Arturo Borrero Gonzalez]

On 20 September 2014 13:39, Patrick McHardy <kaber@trash.net> wrote:
> On Thu, Sep 18, 2014 at 08:18:18PM +0200, Arturo Borrero Gonzalez wrote:
>> --- a/include/net/netfilter/nf_tables.h
>> +++ b/include/net/netfilter/nf_tables.h
>> @@ -231,6 +231,14 @@ struct nft_set_ops {
>>  int nft_register_set(struct nft_set_ops *ops);
>>  void nft_unregister_set(struct nft_set_ops *ops);
>>
>> +/* internal flags to know which attributes were originally set
>> + * from userspace.
>> + */
>> +enum nft_set_attr {
>> +     NFT_SET_ATTR_POLICY     = 0x1,
>> +     NFT_SET_ATTR_DESC_SIZE  = 0x2,
>> +};
>
> We don't need the size, any value != 0 is set by userspace. I'm unsure
> about the policy, we don't really *need* to dump if it is the default,
> and otherwise its obvious that it originated from userspace.
>
>> @@ -241,6 +249,8 @@ void nft_unregister_set(struct nft_set_ops *ops);
>>   *   @dtype: data type (verdict or numeric type defined by userspace)
>>   *   @size: maximum set size
>>   *   @nelems: number of elements
>> + *   @attr_flags: (enum nft_set_flags)
>> + *   @policy: (enum nft_set_policies)
>>   *   @ops: set ops
>>   *   @flags: set flags
>>   *   @klen: key length
>> @@ -255,6 +265,8 @@ struct nft_set {
>>       u32                             dtype;
>>       u32                             size;
>>       u32                             nelems;
>> +     u16                             attr_flags;
>> +     u32                             policy;
>
> These are way to big and introduce holes in the structure.
>
>>       /* runtime data below here */
>>       const struct nft_set_ops        *ops ____cacheline_aligned;
>>       u16                             flags;
>> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
>> index 8237460..d1c3f3e 100644
>> --- a/net/netfilter/nf_tables_api.c
>> +++ b/net/netfilter/nf_tables_api.c
>> @@ -2342,13 +2342,24 @@ static int nf_tables_fill_set(struct sk_buff *skb, const struct nft_ctx *ctx,
>>                       goto nla_put_failure;
>>       }
>>
>> -     desc = nla_nest_start(skb, NFTA_SET_DESC);
>> -     if (desc == NULL)
>> -             goto nla_put_failure;
>> -     if (set->size &&
>> -         nla_put_be32(skb, NFTA_SET_DESC_SIZE, htonl(set->size)))
>> -             goto nla_put_failure;
>> -     nla_nest_end(skb, desc);
>> +     /* dump policy and desc info only if they were explicitly set */
>> +     if (set->attr_flags & (1 << NFT_SET_ATTR_POLICY)) {
>> +             if (nla_put_be32(skb, NFTA_SET_POLICY, htonl(set->policy)))
>> +                     goto nla_put_failure;
>> +     }
>> +
>> +     if (set->attr_flags & (1 << NFT_SET_ATTR_DESC_SIZE)) {
>> +             desc = nla_nest_start(skb, NFTA_SET_DESC);
>> +             if (desc == NULL)
>> +                     goto nla_put_failure;
>> +
>> +             if (set->size &&
>> +                 nla_put_be32(skb, NFTA_SET_DESC_SIZE, htonl(set->size))) {
>> +                     goto nla_put_failure;
>> +             }
>> +
>> +             nla_nest_end(skb, desc);
>> +     }
>
> As mentioned earlier, dumping the parameters is not necessary for sets
> with NFT_SET_CONSTANT as they have been determined automatically.


Ok Patrick, so I understand we can drop this patch.
I can patch the fill_set() function to simply dump the policy if
policy != PERFORMANCE (the default).

How many bits should I use to store the policy?

regards.
-- 
Arturo Borrero González
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html