[nf-next PATCH v2 0/5] netfilter: nf_tables: Kill name length restrictions

[nf-next PATCH v2 0/5] netfilter: nf_tables: Kill name length restrictions

[Phil Sutter]

The following series removes the hard-coded restriction on name length
of tables, chains, sets and objects.

The first patch introduces nla_strdup() which aids in duplicating a
string contained in a netlink attribute. It is used to replace the call
to nla_strlcpy() when populating name fields.

I've tested the series manually by creating tables, chains, sets and
counter objects with long names and automated by running the py and
shell testsuites of nftables repo. Also, kmemleak did not find anything
nftables related.

Changes since v1:
- Introduce NFT_NAME_MAXLEN as an upper boundary to restrict overly long
  names but still allow to use e.g. domain names.
- Adjust commit messages accordingly.

Phil Sutter (5):
  networking: Introduce nla_strdup()
  netfilter: nf_tables: Unlimit table name length
  netfilter: nf_tables: Unlimit chain name length
  netfilter: nf_tables: Unlimit set name length
  netfilter: nf_tables: Unlimit object name length

 include/net/netfilter/nf_tables.h        |  10 +--
 include/net/netlink.h                    |   1 +
 include/uapi/linux/netfilter/nf_tables.h |   5 +-
 lib/nlattr.c                             |  24 ++++++
 net/netfilter/nf_tables_api.c            | 136 ++++++++++++++++++++++---------
 net/netfilter/nf_tables_trace.c          |  10 ++-
 net/netfilter/nft_dynset.c               |   2 +-
 net/netfilter/nft_lookup.c               |   2 +-
 net/netfilter/nft_objref.c               |   4 +-
 9 files changed, 139 insertions(+), 55 deletions(-)

-- 
2.13.1

[nf-next PATCH v2 1/5] networking: Introduce nla_strdup()

[Phil Sutter]

This is similar to strdup() for netlink string attributes.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/net/netlink.h |  1 +
 lib/nlattr.c          | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 01709172b3d38..5c1fc1d4b0969 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -247,6 +247,7 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
 int nla_policy_len(const struct nla_policy *, int);
 struct nlattr *nla_find(const struct nlattr *head, int len, int attrtype);
 size_t nla_strlcpy(char *dst, const struct nlattr *nla, size_t dstsize);
+char *nla_strdup(const struct nlattr *nla, gfp_t flags);
 int nla_memcpy(void *dest, const struct nlattr *src, int count);
 int nla_memcmp(const struct nlattr *nla, const void *data, size_t size);
 int nla_strcmp(const struct nlattr *nla, const char *str);
diff --git a/lib/nlattr.c b/lib/nlattr.c
index a7e0b16078dff..af8716fb8a3bf 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -272,6 +272,30 @@ size_t nla_strlcpy(char *dst, const struct nlattr *nla, size_t dstsize)
 EXPORT_SYMBOL(nla_strlcpy);
 
 /**
+ * nla_strdup - Copy string attribute payload into a newly allocated buffer
+ * @nla: attribute to copy the string from
+ * @flags: the type of memory to allocate (see kmalloc).
+ *
+ * Returns a pointer to the allocated buffer or NULL on error.
+ */
+char *nla_strdup(const struct nlattr *nla, gfp_t flags)
+{
+	size_t srclen = nla_len(nla);
+	char *src = nla_data(nla), *dst;
+
+	if (srclen > 0 && src[srclen - 1] == '\0')
+		srclen--;
+
+	dst = kmalloc(srclen + 1, flags);
+	if (dst != NULL) {
+		memcpy(dst, src, srclen);
+		dst[srclen] = '\0';
+	}
+	return dst;
+}
+EXPORT_SYMBOL(nla_strdup);
+
+/**
  * nla_memcpy - Copy a netlink attribute into another memory area
  * @dest: where to copy to memcpy
  * @src: netlink attribute to copy from
-- 
2.13.1

[nf-next PATCH v2 2/5] netfilter: nf_tables: Unlimit table name length

[Phil Sutter]

Allocate all table names dynamically to allow for arbitrary lengths but
introduce NFT_NAME_MAXLEN as an upper sanity boundary. It's value was
chosen to allow using a domain name as per RFC 1035.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/net/netfilter/nf_tables.h        |  2 +-
 include/uapi/linux/netfilter/nf_tables.h |  2 +-
 net/netfilter/nf_tables_api.c            | 61 ++++++++++++++++++++++----------
 net/netfilter/nf_tables_trace.c          |  4 ++-
 4 files changed, 47 insertions(+), 22 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index bd5be0d691d51..05ecf78ec0787 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -957,7 +957,7 @@ struct nft_table {
 	u32				use;
 	u16				flags:14,
 					genmask:2;
-	char				name[NFT_TABLE_MAXNAMELEN];
+	char				*name;
 };
 
 enum nft_af_flags {
diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 683f6f88fcace..35d1d9a21508e 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -1,7 +1,7 @@
 #ifndef _LINUX_NF_TABLES_H
 #define _LINUX_NF_TABLES_H
 
-#define NFT_TABLE_MAXNAMELEN	32
+#define NFT_NAME_MAXLEN		256
 #define NFT_CHAIN_MAXNAMELEN	32
 #define NFT_SET_MAXNAMELEN	32
 #define NFT_OBJ_MAXNAMELEN	32
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 7843efa33c598..cf12f63606aaf 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -428,7 +428,7 @@ nf_tables_chain_type_lookup(const struct nft_af_info *afi,
 
 static const struct nla_policy nft_table_policy[NFTA_TABLE_MAX + 1] = {
 	[NFTA_TABLE_NAME]	= { .type = NLA_STRING,
-				    .len = NFT_TABLE_MAXNAMELEN - 1 },
+				    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_TABLE_FLAGS]	= { .type = NLA_U32 },
 };
 
@@ -726,7 +726,10 @@ static int nf_tables_newtable(struct net *net, struct sock *nlsk,
 	if (table == NULL)
 		goto err2;
 
-	nla_strlcpy(table->name, name, NFT_TABLE_MAXNAMELEN);
+	table->name = nla_strdup(name, GFP_KERNEL);
+	if (table->name == NULL)
+		goto err3;
+
 	INIT_LIST_HEAD(&table->chains);
 	INIT_LIST_HEAD(&table->sets);
 	INIT_LIST_HEAD(&table->objects);
@@ -735,10 +738,12 @@ static int nf_tables_newtable(struct net *net, struct sock *nlsk,
 	nft_ctx_init(&ctx, net, skb, nlh, afi, table, NULL, nla);
 	err = nft_trans_table_add(&ctx, NFT_MSG_NEWTABLE);
 	if (err < 0)
-		goto err3;
+		goto err4;
 
 	list_add_tail_rcu(&table->list, &afi->tables);
 	return 0;
+err4:
+	kfree(table->name);
 err3:
 	kfree(table);
 err2:
@@ -865,6 +870,7 @@ static void nf_tables_table_destroy(struct nft_ctx *ctx)
 {
 	BUG_ON(ctx->table->use > 0);
 
+	kfree(ctx->table->name);
 	kfree(ctx->table);
 	module_put(ctx->afi->owner);
 }
@@ -935,7 +941,7 @@ static struct nft_chain *nf_tables_chain_lookup(const struct nft_table *table,
 
 static const struct nla_policy nft_chain_policy[NFTA_CHAIN_MAX + 1] = {
 	[NFTA_CHAIN_TABLE]	= { .type = NLA_STRING,
-				    .len = NFT_TABLE_MAXNAMELEN - 1 },
+				    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_CHAIN_HANDLE]	= { .type = NLA_U64 },
 	[NFTA_CHAIN_NAME]	= { .type = NLA_STRING,
 				    .len = NFT_CHAIN_MAXNAMELEN - 1 },
@@ -1873,7 +1879,7 @@ static struct nft_rule *nf_tables_rule_lookup(const struct nft_chain *chain,
 
 static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
 	[NFTA_RULE_TABLE]	= { .type = NLA_STRING,
-				    .len = NFT_TABLE_MAXNAMELEN - 1 },
+				    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_RULE_CHAIN]	= { .type = NLA_STRING,
 				    .len = NFT_CHAIN_MAXNAMELEN - 1 },
 	[NFTA_RULE_HANDLE]	= { .type = NLA_U64 },
@@ -1977,7 +1983,7 @@ static void nf_tables_rule_notify(const struct nft_ctx *ctx,
 }
 
 struct nft_rule_dump_ctx {
-	char table[NFT_TABLE_MAXNAMELEN];
+	char *table;
 	char chain[NFT_CHAIN_MAXNAMELEN];
 };
 
@@ -2002,7 +2008,7 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
 			continue;
 
 		list_for_each_entry_rcu(table, &afi->tables, list) {
-			if (ctx && ctx->table[0] &&
+			if (ctx && ctx->table &&
 			    strcmp(ctx->table, table->name) != 0)
 				continue;
 
@@ -2042,7 +2048,12 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
 
 static int nf_tables_dump_rules_done(struct netlink_callback *cb)
 {
-	kfree(cb->data);
+	struct nft_rule_dump_ctx *ctx = cb->data;
+
+	if (ctx) {
+		kfree(ctx->table);
+		kfree(ctx);
+	}
 	return 0;
 }
 
@@ -2074,9 +2085,14 @@ static int nf_tables_getrule(struct net *net, struct sock *nlsk,
 			if (!ctx)
 				return -ENOMEM;
 
-			if (nla[NFTA_RULE_TABLE])
-				nla_strlcpy(ctx->table, nla[NFTA_RULE_TABLE],
-					    sizeof(ctx->table));
+			if (nla[NFTA_RULE_TABLE]) {
+				ctx->table = nla_strdup(nla[NFTA_RULE_TABLE],
+							GFP_KERNEL);
+				if (!ctx->table) {
+					kfree(ctx);
+					return -ENOMEM;
+				}
+			}
 			if (nla[NFTA_RULE_CHAIN])
 				nla_strlcpy(ctx->chain, nla[NFTA_RULE_CHAIN],
 					    sizeof(ctx->chain));
@@ -2508,7 +2524,7 @@ nft_select_set_ops(const struct nft_ctx *ctx,
 
 static const struct nla_policy nft_set_policy[NFTA_SET_MAX + 1] = {
 	[NFTA_SET_TABLE]		= { .type = NLA_STRING,
-					    .len = NFT_TABLE_MAXNAMELEN - 1 },
+					    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_SET_NAME]			= { .type = NLA_STRING,
 					    .len = NFT_SET_MAXNAMELEN - 1 },
 	[NFTA_SET_FLAGS]		= { .type = NLA_U32 },
@@ -3275,7 +3291,7 @@ static const struct nla_policy nft_set_elem_policy[NFTA_SET_ELEM_MAX + 1] = {
 
 static const struct nla_policy nft_set_elem_list_policy[NFTA_SET_ELEM_LIST_MAX + 1] = {
 	[NFTA_SET_ELEM_LIST_TABLE]	= { .type = NLA_STRING,
-					    .len = NFT_TABLE_MAXNAMELEN - 1 },
+					    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_SET_ELEM_LIST_SET]	= { .type = NLA_STRING,
 					    .len = NFT_SET_MAXNAMELEN - 1 },
 	[NFTA_SET_ELEM_LIST_ELEMENTS]	= { .type = NLA_NESTED },
@@ -4219,7 +4235,7 @@ EXPORT_SYMBOL_GPL(nf_tables_obj_lookup);
 
 static const struct nla_policy nft_obj_policy[NFTA_OBJ_MAX + 1] = {
 	[NFTA_OBJ_TABLE]	= { .type = NLA_STRING,
-				    .len = NFT_TABLE_MAXNAMELEN - 1 },
+				    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_OBJ_NAME]		= { .type = NLA_STRING,
 				    .len = NFT_OBJ_MAXNAMELEN - 1 },
 	[NFTA_OBJ_TYPE]		= { .type = NLA_U32 },
@@ -4415,7 +4431,7 @@ static int nf_tables_fill_obj_info(struct sk_buff *skb, struct net *net,
 }
 
 struct nft_obj_filter {
-	char		table[NFT_OBJ_MAXNAMELEN];
+	char		*table;
 	u32		type;
 };
 
@@ -4480,7 +4496,10 @@ static int nf_tables_dump_obj(struct sk_buff *skb, struct netlink_callback *cb)
 
 static int nf_tables_dump_obj_done(struct netlink_callback *cb)
 {
-	kfree(cb->data);
+	struct nft_obj_filter *filter = cb->data;
+
+	kfree(filter->table);
+	kfree(filter);
 
 	return 0;
 }
@@ -4494,9 +4513,13 @@ nft_obj_filter_alloc(const struct nlattr * const nla[])
 	if (!filter)
 		return ERR_PTR(-ENOMEM);
 
-	if (nla[NFTA_OBJ_TABLE])
-		nla_strlcpy(filter->table, nla[NFTA_OBJ_TABLE],
-			    NFT_TABLE_MAXNAMELEN);
+	if (nla[NFTA_OBJ_TABLE]) {
+		filter->table = nla_strdup(nla[NFTA_OBJ_TABLE], GFP_KERNEL);
+		if (!filter->table) {
+			kfree(filter);
+			return ERR_PTR(-ENOMEM);
+		}
+	}
 	if (nla[NFTA_OBJ_TYPE])
 		filter->type = ntohl(nla_get_be32(nla[NFTA_OBJ_TYPE]));
 
diff --git a/net/netfilter/nf_tables_trace.c b/net/netfilter/nf_tables_trace.c
index e1b15e7a5793f..e95098c1faaf0 100644
--- a/net/netfilter/nf_tables_trace.c
+++ b/net/netfilter/nf_tables_trace.c
@@ -175,7 +175,6 @@ void nft_trace_notify(struct nft_traceinfo *info)
 		return;
 
 	size = nlmsg_total_size(sizeof(struct nfgenmsg)) +
-		nla_total_size(NFT_TABLE_MAXNAMELEN) +
 		nla_total_size(NFT_CHAIN_MAXNAMELEN) +
 		nla_total_size_64bit(sizeof(__be64)) +	/* rule handle */
 		nla_total_size(sizeof(__be32)) +	/* trace type */
@@ -194,6 +193,9 @@ void nft_trace_notify(struct nft_traceinfo *info)
 		nla_total_size(sizeof(u32)) +		/* nfproto */
 		nla_total_size(sizeof(u32));		/* policy */
 
+	if (info->chain)
+		size += nla_total_size(strlen(info->chain->table->name));
+
 	skb = nlmsg_new(size, GFP_ATOMIC);
 	if (!skb)
 		return;
-- 
2.13.1

[nf-next PATCH v2 3/5] netfilter: nf_tables: Unlimit chain name length

[Phil Sutter]

Same conversion as for table names, use NFT_NAME_MAXLEN as upper
boundary as well.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/net/netfilter/nf_tables.h        |  4 ++--
 include/uapi/linux/netfilter/nf_tables.h |  1 -
 net/netfilter/nf_tables_api.c            | 40 +++++++++++++++++++++++---------
 net/netfilter/nf_tables_trace.c          |  8 ++++---
 4 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 05ecf78ec0787..be1610162ee02 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -859,7 +859,7 @@ struct nft_chain {
 	u16				level;
 	u8				flags:6,
 					genmask:2;
-	char				name[NFT_CHAIN_MAXNAMELEN];
+	char				*name;
 };
 
 enum nft_chain_type {
@@ -1272,7 +1272,7 @@ struct nft_trans_set {
 
 struct nft_trans_chain {
 	bool				update;
-	char				name[NFT_CHAIN_MAXNAMELEN];
+	char				*name;
 	struct nft_stats __percpu	*stats;
 	u8				policy;
 };
diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 35d1d9a21508e..29e57df26530f 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -2,7 +2,6 @@
 #define _LINUX_NF_TABLES_H
 
 #define NFT_NAME_MAXLEN		256
-#define NFT_CHAIN_MAXNAMELEN	32
 #define NFT_SET_MAXNAMELEN	32
 #define NFT_OBJ_MAXNAMELEN	32
 #define NFT_USERDATA_MAXLEN	256
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index cf12f63606aaf..002898e557a92 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -944,7 +944,7 @@ static const struct nla_policy nft_chain_policy[NFTA_CHAIN_MAX + 1] = {
 				    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_CHAIN_HANDLE]	= { .type = NLA_U64 },
 	[NFTA_CHAIN_NAME]	= { .type = NLA_STRING,
-				    .len = NFT_CHAIN_MAXNAMELEN - 1 },
+				    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_CHAIN_HOOK]	= { .type = NLA_NESTED },
 	[NFTA_CHAIN_POLICY]	= { .type = NLA_U32 },
 	[NFTA_CHAIN_TYPE]	= { .type = NLA_STRING },
@@ -1248,8 +1248,10 @@ static void nf_tables_chain_destroy(struct nft_chain *chain)
 		free_percpu(basechain->stats);
 		if (basechain->ops[0].dev != NULL)
 			dev_put(basechain->ops[0].dev);
+		kfree(chain->name);
 		kfree(basechain);
 	} else {
+		kfree(chain->name);
 		kfree(chain);
 	}
 }
@@ -1474,8 +1476,13 @@ static int nf_tables_newchain(struct net *net, struct sock *nlsk,
 			nft_trans_chain_policy(trans) = -1;
 
 		if (nla[NFTA_CHAIN_HANDLE] && name) {
-			nla_strlcpy(nft_trans_chain_name(trans), name,
-				    NFT_CHAIN_MAXNAMELEN);
+			nft_trans_chain_name(trans) =
+				nla_strdup(name, GFP_KERNEL);
+			if (!nft_trans_chain_name(trans)) {
+				kfree(trans);
+				free_percpu(stats);
+				return -ENOMEM;
+			}
 		}
 		list_add_tail(&trans->list, &net->nft.commit_list);
 		return 0;
@@ -1549,7 +1556,11 @@ static int nf_tables_newchain(struct net *net, struct sock *nlsk,
 	INIT_LIST_HEAD(&chain->rules);
 	chain->handle = nf_tables_alloc_handle(table);
 	chain->table = table;
-	nla_strlcpy(chain->name, name, NFT_CHAIN_MAXNAMELEN);
+	chain->name = nla_strdup(name, GFP_KERNEL);
+	if (!chain->name) {
+		err = -ENOMEM;
+		goto err1;
+	}
 
 	err = nf_tables_register_hooks(net, table, chain, afi->nops);
 	if (err < 0)
@@ -1881,7 +1892,7 @@ static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
 	[NFTA_RULE_TABLE]	= { .type = NLA_STRING,
 				    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_RULE_CHAIN]	= { .type = NLA_STRING,
-				    .len = NFT_CHAIN_MAXNAMELEN - 1 },
+				    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_RULE_HANDLE]	= { .type = NLA_U64 },
 	[NFTA_RULE_EXPRESSIONS]	= { .type = NLA_NESTED },
 	[NFTA_RULE_COMPAT]	= { .type = NLA_NESTED },
@@ -1984,7 +1995,7 @@ static void nf_tables_rule_notify(const struct nft_ctx *ctx,
 
 struct nft_rule_dump_ctx {
 	char *table;
-	char chain[NFT_CHAIN_MAXNAMELEN];
+	char *chain;
 };
 
 static int nf_tables_dump_rules(struct sk_buff *skb,
@@ -2052,6 +2063,7 @@ static int nf_tables_dump_rules_done(struct netlink_callback *cb)
 
 	if (ctx) {
 		kfree(ctx->table);
+		kfree(ctx->chain);
 		kfree(ctx);
 	}
 	return 0;
@@ -2093,9 +2105,15 @@ static int nf_tables_getrule(struct net *net, struct sock *nlsk,
 					return -ENOMEM;
 				}
 			}
-			if (nla[NFTA_RULE_CHAIN])
-				nla_strlcpy(ctx->chain, nla[NFTA_RULE_CHAIN],
-					    sizeof(ctx->chain));
+			if (nla[NFTA_RULE_CHAIN]) {
+				ctx->chain = nla_strdup(nla[NFTA_RULE_CHAIN],
+							GFP_KERNEL);
+				if (!ctx->chain) {
+					kfree(ctx->table);
+					kfree(ctx);
+					return -ENOMEM;
+				}
+			}
 			c.data = ctx;
 		}
 
@@ -4865,7 +4883,7 @@ static void nft_chain_commit_update(struct nft_trans *trans)
 {
 	struct nft_base_chain *basechain;
 
-	if (nft_trans_chain_name(trans)[0])
+	if (nft_trans_chain_name(trans))
 		strcpy(trans->ctx.chain->name, nft_trans_chain_name(trans));
 
 	if (!nft_is_base_chain(trans->ctx.chain))
@@ -5464,7 +5482,7 @@ EXPORT_SYMBOL_GPL(nft_validate_register_store);
 static const struct nla_policy nft_verdict_policy[NFTA_VERDICT_MAX + 1] = {
 	[NFTA_VERDICT_CODE]	= { .type = NLA_U32 },
 	[NFTA_VERDICT_CHAIN]	= { .type = NLA_STRING,
-				    .len = NFT_CHAIN_MAXNAMELEN - 1 },
+				    .len = NFT_NAME_MAXLEN - 1 },
 };
 
 static int nft_verdict_init(const struct nft_ctx *ctx, struct nft_data *data,
diff --git a/net/netfilter/nf_tables_trace.c b/net/netfilter/nf_tables_trace.c
index e95098c1faaf0..37e664baeb18c 100644
--- a/net/netfilter/nf_tables_trace.c
+++ b/net/netfilter/nf_tables_trace.c
@@ -175,12 +175,10 @@ void nft_trace_notify(struct nft_traceinfo *info)
 		return;
 
 	size = nlmsg_total_size(sizeof(struct nfgenmsg)) +
-		nla_total_size(NFT_CHAIN_MAXNAMELEN) +
 		nla_total_size_64bit(sizeof(__be64)) +	/* rule handle */
 		nla_total_size(sizeof(__be32)) +	/* trace type */
 		nla_total_size(0) +			/* VERDICT, nested */
 			nla_total_size(sizeof(u32)) +	/* verdict code */
-			nla_total_size(NFT_CHAIN_MAXNAMELEN) + /* jump target */
 		nla_total_size(sizeof(u32)) +		/* id */
 		nla_total_size(NFT_TRACETYPE_LL_HSIZE) +
 		nla_total_size(NFT_TRACETYPE_NETWORK_HSIZE) +
@@ -194,7 +192,11 @@ void nft_trace_notify(struct nft_traceinfo *info)
 		nla_total_size(sizeof(u32));		/* policy */
 
 	if (info->chain)
-		size += nla_total_size(strlen(info->chain->table->name));
+		size += nla_total_size(strlen(info->chain->table->name)) +
+			nla_total_size(strlen(info->chain->name));
+
+	if (info->verdict->chain)
+		size += nla_total_size(strlen(info->verdict->chain->name)); /* jump target */
 
 	skb = nlmsg_new(size, GFP_ATOMIC);
 	if (!skb)
-- 
2.13.1

[nf-next PATCH v2 4/5] netfilter: nf_tables: Unlimit set name length

[Phil Sutter]

Same conversion as for table names, use NFT_NAME_MAXLEN as upper
boundary as well.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/net/netfilter/nf_tables.h        |  2 +-
 include/uapi/linux/netfilter/nf_tables.h |  1 -
 net/netfilter/nf_tables_api.c            | 22 ++++++++++++++++------
 net/netfilter/nft_dynset.c               |  2 +-
 net/netfilter/nft_lookup.c               |  2 +-
 net/netfilter/nft_objref.c               |  2 +-
 6 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index be1610162ee02..66ba62fa7d90e 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -396,7 +396,7 @@ void nft_unregister_set(struct nft_set_type *type);
 struct nft_set {
 	struct list_head		list;
 	struct list_head		bindings;
-	char				name[NFT_SET_MAXNAMELEN];
+	char				*name;
 	u32				ktype;
 	u32				dtype;
 	u32				objtype;
diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 29e57df26530f..73543b4f507eb 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -2,7 +2,6 @@
 #define _LINUX_NF_TABLES_H
 
 #define NFT_NAME_MAXLEN		256
-#define NFT_SET_MAXNAMELEN	32
 #define NFT_OBJ_MAXNAMELEN	32
 #define NFT_USERDATA_MAXLEN	256
 
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 002898e557a92..f48f8a46de89b 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2544,7 +2544,7 @@ static const struct nla_policy nft_set_policy[NFTA_SET_MAX + 1] = {
 	[NFTA_SET_TABLE]		= { .type = NLA_STRING,
 					    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_SET_NAME]			= { .type = NLA_STRING,
-					    .len = NFT_SET_MAXNAMELEN - 1 },
+					    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_SET_FLAGS]		= { .type = NLA_U32 },
 	[NFTA_SET_KEY_TYPE]		= { .type = NLA_U32 },
 	[NFTA_SET_KEY_LEN]		= { .type = NLA_U32 },
@@ -2655,7 +2655,7 @@ static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set,
 	unsigned long *inuse;
 	unsigned int n = 0, min = 0;
 
-	p = strnchr(name, NFT_SET_MAXNAMELEN, '%');
+	p = strchr(name, '%');
 	if (p != NULL) {
 		if (p[1] != 'd' || strchr(p + 2, '%'))
 			return -EINVAL;
@@ -2686,7 +2686,10 @@ static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set,
 		free_page((unsigned long)inuse);
 	}
 
-	snprintf(set->name, sizeof(set->name), name, min + n);
+	set->name = kasprintf(GFP_KERNEL, name, min + n);
+	if (!set->name)
+		return -ENOMEM;
+
 	list_for_each_entry(i, &ctx->table->sets, list) {
 		if (!nft_is_active_next(ctx->net, i))
 			continue;
@@ -2963,7 +2966,7 @@ static int nf_tables_newset(struct net *net, struct sock *nlsk,
 	struct nft_table *table;
 	struct nft_set *set;
 	struct nft_ctx ctx;
-	char name[NFT_SET_MAXNAMELEN];
+	char *name;
 	unsigned int size;
 	bool create;
 	u64 timeout;
@@ -3109,8 +3112,14 @@ static int nf_tables_newset(struct net *net, struct sock *nlsk,
 		goto err1;
 	}
 
-	nla_strlcpy(name, nla[NFTA_SET_NAME], sizeof(set->name));
+	name = nla_strdup(nla[NFTA_SET_NAME], GFP_KERNEL);
+	if (!name) {
+		err = -ENOMEM;
+		goto err2;
+	}
+
 	err = nf_tables_set_alloc_name(&ctx, set, name);
+	kfree(name);
 	if (err < 0)
 		goto err2;
 
@@ -3160,6 +3169,7 @@ static void nft_set_destroy(struct nft_set *set)
 {
 	set->ops->destroy(set);
 	module_put(set->ops->type->owner);
+	kfree(set->name);
 	kvfree(set);
 }
 
@@ -3311,7 +3321,7 @@ static const struct nla_policy nft_set_elem_list_policy[NFTA_SET_ELEM_LIST_MAX +
 	[NFTA_SET_ELEM_LIST_TABLE]	= { .type = NLA_STRING,
 					    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_SET_ELEM_LIST_SET]	= { .type = NLA_STRING,
-					    .len = NFT_SET_MAXNAMELEN - 1 },
+					    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_SET_ELEM_LIST_ELEMENTS]	= { .type = NLA_NESTED },
 	[NFTA_SET_ELEM_LIST_SET_ID]	= { .type = NLA_U32 },
 };
diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
index 66221ad891a9f..3528a03035825 100644
--- a/net/netfilter/nft_dynset.c
+++ b/net/netfilter/nft_dynset.c
@@ -98,7 +98,7 @@ static void nft_dynset_eval(const struct nft_expr *expr,
 
 static const struct nla_policy nft_dynset_policy[NFTA_DYNSET_MAX + 1] = {
 	[NFTA_DYNSET_SET_NAME]	= { .type = NLA_STRING,
-				    .len = NFT_SET_MAXNAMELEN - 1 },
+				    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_DYNSET_SET_ID]	= { .type = NLA_U32 },
 	[NFTA_DYNSET_OP]	= { .type = NLA_U32 },
 	[NFTA_DYNSET_SREG_KEY]	= { .type = NLA_U32 },
diff --git a/net/netfilter/nft_lookup.c b/net/netfilter/nft_lookup.c
index 475570e89ede7..000a6b729cb18 100644
--- a/net/netfilter/nft_lookup.c
+++ b/net/netfilter/nft_lookup.c
@@ -50,7 +50,7 @@ static void nft_lookup_eval(const struct nft_expr *expr,
 
 static const struct nla_policy nft_lookup_policy[NFTA_LOOKUP_MAX + 1] = {
 	[NFTA_LOOKUP_SET]	= { .type = NLA_STRING,
-				    .len = NFT_SET_MAXNAMELEN - 1 },
+				    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_LOOKUP_SET_ID]	= { .type = NLA_U32 },
 	[NFTA_LOOKUP_SREG]	= { .type = NLA_U32 },
 	[NFTA_LOOKUP_DREG]	= { .type = NLA_U32 },
diff --git a/net/netfilter/nft_objref.c b/net/netfilter/nft_objref.c
index 1dd428fbaaa3f..20d2289e8587b 100644
--- a/net/netfilter/nft_objref.c
+++ b/net/netfilter/nft_objref.c
@@ -192,7 +192,7 @@ static const struct nla_policy nft_objref_policy[NFTA_OBJREF_MAX + 1] = {
 	[NFTA_OBJREF_IMM_TYPE]	= { .type = NLA_U32 },
 	[NFTA_OBJREF_SET_SREG]	= { .type = NLA_U32 },
 	[NFTA_OBJREF_SET_NAME]	= { .type = NLA_STRING,
-				    .len = NFT_SET_MAXNAMELEN - 1 },
+				    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_OBJREF_SET_ID]	= { .type = NLA_U32 },
 };
 
-- 
2.13.1

[nf-next PATCH v2 5/5] netfilter: nf_tables: Unlimit object name length

[Phil Sutter]

Same conversion as for table names, use NFT_NAME_MAXLEN as upper
boundary as well.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/net/netfilter/nf_tables.h        |  2 +-
 include/uapi/linux/netfilter/nf_tables.h |  1 -
 net/netfilter/nf_tables_api.c            | 13 ++++++++++---
 net/netfilter/nft_objref.c               |  2 +-
 4 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 66ba62fa7d90e..f9795fe394f31 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1016,7 +1016,7 @@ int nft_verdict_dump(struct sk_buff *skb, int type,
  */
 struct nft_object {
 	struct list_head		list;
-	char				name[NFT_OBJ_MAXNAMELEN];
+	char				*name;
 	struct nft_table		*table;
 	u32				genmask:2,
 					use:30;
diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 73543b4f507eb..58263e23f3c5a 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -2,7 +2,6 @@
 #define _LINUX_NF_TABLES_H
 
 #define NFT_NAME_MAXLEN		256
-#define NFT_OBJ_MAXNAMELEN	32
 #define NFT_USERDATA_MAXLEN	256
 
 /**
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index f48f8a46de89b..0527d677716d1 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -4265,7 +4265,7 @@ static const struct nla_policy nft_obj_policy[NFTA_OBJ_MAX + 1] = {
 	[NFTA_OBJ_TABLE]	= { .type = NLA_STRING,
 				    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_OBJ_NAME]		= { .type = NLA_STRING,
-				    .len = NFT_OBJ_MAXNAMELEN - 1 },
+				    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_OBJ_TYPE]		= { .type = NLA_U32 },
 	[NFTA_OBJ_DATA]		= { .type = NLA_NESTED },
 };
@@ -4407,15 +4407,21 @@ static int nf_tables_newobj(struct net *net, struct sock *nlsk,
 		goto err1;
 	}
 	obj->table = table;
-	nla_strlcpy(obj->name, nla[NFTA_OBJ_NAME], NFT_OBJ_MAXNAMELEN);
+	obj->name = nla_strdup(nla[NFTA_OBJ_NAME], GFP_KERNEL);
+	if (!obj->name) {
+		err = -ENOMEM;
+		goto err2;
+	}
 
 	err = nft_trans_obj_add(&ctx, NFT_MSG_NEWOBJ, obj);
 	if (err < 0)
-		goto err2;
+		goto err3;
 
 	list_add_tail_rcu(&obj->list, &table->objects);
 	table->use++;
 	return 0;
+err3:
+	kfree(obj->name);
 err2:
 	if (obj->type->destroy)
 		obj->type->destroy(obj);
@@ -4631,6 +4637,7 @@ static void nft_obj_destroy(struct nft_object *obj)
 		obj->type->destroy(obj);
 
 	module_put(obj->type->owner);
+	kfree(obj->name);
 	kfree(obj);
 }
 
diff --git a/net/netfilter/nft_objref.c b/net/netfilter/nft_objref.c
index 20d2289e8587b..8a331c70918f2 100644
--- a/net/netfilter/nft_objref.c
+++ b/net/netfilter/nft_objref.c
@@ -188,7 +188,7 @@ nft_objref_select_ops(const struct nft_ctx *ctx,
 
 static const struct nla_policy nft_objref_policy[NFTA_OBJREF_MAX + 1] = {
 	[NFTA_OBJREF_IMM_NAME]	= { .type = NLA_STRING,
-				    .len = NFT_OBJ_MAXNAMELEN - 1 },
+				    .len = NFT_NAME_MAXLEN - 1 },
 	[NFTA_OBJREF_IMM_TYPE]	= { .type = NLA_U32 },
 	[NFTA_OBJREF_SET_SREG]	= { .type = NLA_U32 },
 	[NFTA_OBJREF_SET_NAME]	= { .type = NLA_STRING,
-- 
2.13.1

Re: [nf-next PATCH v2 2/5] netfilter: nf_tables: Unlimit table name length

[Pablo Neira Ayuso]

Hi Phil,

I think you can rename patch title to "allow table names up to 256
chars". This is not unlimited anymore since v2.

A couple more comments below.

On Mon, Jul 24, 2017 at 08:56:48PM +0200, Phil Sutter wrote:
> Allocate all table names dynamically to allow for arbitrary lengths but
> introduce NFT_NAME_MAXLEN as an upper sanity boundary. It's value was
> chosen to allow using a domain name as per RFC 1035.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  include/net/netfilter/nf_tables.h        |  2 +-
>  include/uapi/linux/netfilter/nf_tables.h |  2 +-
>  net/netfilter/nf_tables_api.c            | 61 ++++++++++++++++++++++----------
>  net/netfilter/nf_tables_trace.c          |  4 ++-
>  4 files changed, 47 insertions(+), 22 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index bd5be0d691d51..05ecf78ec0787 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -957,7 +957,7 @@ struct nft_table {
>  	u32				use;
>  	u16				flags:14,
>  					genmask:2;
> -	char				name[NFT_TABLE_MAXNAMELEN];
> +	char				*name;
>  };
>  
>  enum nft_af_flags {
> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> index 683f6f88fcace..35d1d9a21508e 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -1,7 +1,7 @@
>  #ifndef _LINUX_NF_TABLES_H
>  #define _LINUX_NF_TABLES_H
>  
> -#define NFT_TABLE_MAXNAMELEN	32
> +#define NFT_NAME_MAXLEN		256

I understand NFT_*_MAXNAMELEN per object is probably too much, but
given this is uapi, I think we have to keep it around, ie.

#define NFT_NAME_MAXLEN		256
#define NFT_TABLE_MAXNAMELEN    NFT_NAME_MAXLEN

And so on.

>  #define NFT_CHAIN_MAXNAMELEN	32
>  #define NFT_SET_MAXNAMELEN	32
>  #define NFT_OBJ_MAXNAMELEN	32
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 7843efa33c598..cf12f63606aaf 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -428,7 +428,7 @@ nf_tables_chain_type_lookup(const struct nft_af_info *afi,
>  
>  static const struct nla_policy nft_table_policy[NFTA_TABLE_MAX + 1] = {
>  	[NFTA_TABLE_NAME]	= { .type = NLA_STRING,
> -				    .len = NFT_TABLE_MAXNAMELEN - 1 },
> +				    .len = NFT_NAME_MAXLEN - 1 },
>  	[NFTA_TABLE_FLAGS]	= { .type = NLA_U32 },
>  };
>  
> @@ -726,7 +726,10 @@ static int nf_tables_newtable(struct net *net, struct sock *nlsk,
>  	if (table == NULL)
>  		goto err2;
>  
> -	nla_strlcpy(table->name, name, NFT_TABLE_MAXNAMELEN);
> +	table->name = nla_strdup(name, GFP_KERNEL);
> +	if (table->name == NULL)
> +		goto err3;
> +
>  	INIT_LIST_HEAD(&table->chains);
>  	INIT_LIST_HEAD(&table->sets);
>  	INIT_LIST_HEAD(&table->objects);
> @@ -735,10 +738,12 @@ static int nf_tables_newtable(struct net *net, struct sock *nlsk,
>  	nft_ctx_init(&ctx, net, skb, nlh, afi, table, NULL, nla);
>  	err = nft_trans_table_add(&ctx, NFT_MSG_NEWTABLE);
>  	if (err < 0)
> -		goto err3;
> +		goto err4;
>  
>  	list_add_tail_rcu(&table->list, &afi->tables);
>  	return 0;
> +err4:
> +	kfree(table->name);
>  err3:
>  	kfree(table);
>  err2:
> @@ -865,6 +870,7 @@ static void nf_tables_table_destroy(struct nft_ctx *ctx)
>  {
>  	BUG_ON(ctx->table->use > 0);
>  
> +	kfree(ctx->table->name);
>  	kfree(ctx->table);
>  	module_put(ctx->afi->owner);
>  }
> @@ -935,7 +941,7 @@ static struct nft_chain *nf_tables_chain_lookup(const struct nft_table *table,
>  
>  static const struct nla_policy nft_chain_policy[NFTA_CHAIN_MAX + 1] = {
>  	[NFTA_CHAIN_TABLE]	= { .type = NLA_STRING,
> -				    .len = NFT_TABLE_MAXNAMELEN - 1 },
> +				    .len = NFT_NAME_MAXLEN - 1 },
>  	[NFTA_CHAIN_HANDLE]	= { .type = NLA_U64 },
>  	[NFTA_CHAIN_NAME]	= { .type = NLA_STRING,
>  				    .len = NFT_CHAIN_MAXNAMELEN - 1 },
> @@ -1873,7 +1879,7 @@ static struct nft_rule *nf_tables_rule_lookup(const struct nft_chain *chain,
>  
>  static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
>  	[NFTA_RULE_TABLE]	= { .type = NLA_STRING,
> -				    .len = NFT_TABLE_MAXNAMELEN - 1 },
> +				    .len = NFT_NAME_MAXLEN - 1 },
>  	[NFTA_RULE_CHAIN]	= { .type = NLA_STRING,
>  				    .len = NFT_CHAIN_MAXNAMELEN - 1 },
>  	[NFTA_RULE_HANDLE]	= { .type = NLA_U64 },
> @@ -1977,7 +1983,7 @@ static void nf_tables_rule_notify(const struct nft_ctx *ctx,
>  }
>  
>  struct nft_rule_dump_ctx {
> -	char table[NFT_TABLE_MAXNAMELEN];
> +	char *table;
>  	char chain[NFT_CHAIN_MAXNAMELEN];
>  };
>  
> @@ -2002,7 +2008,7 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
>  			continue;
>  
>  		list_for_each_entry_rcu(table, &afi->tables, list) {
> -			if (ctx && ctx->table[0] &&
> +			if (ctx && ctx->table &&
>  			    strcmp(ctx->table, table->name) != 0)
>  				continue;
>  
> @@ -2042,7 +2048,12 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
>  
>  static int nf_tables_dump_rules_done(struct netlink_callback *cb)
>  {
> -	kfree(cb->data);
> +	struct nft_rule_dump_ctx *ctx = cb->data;
> +
> +	if (ctx) {
> +		kfree(ctx->table);
> +		kfree(ctx);
> +	}
>  	return 0;
>  }
>  
> @@ -2074,9 +2085,14 @@ static int nf_tables_getrule(struct net *net, struct sock *nlsk,
>  			if (!ctx)
>  				return -ENOMEM;
>  
> -			if (nla[NFTA_RULE_TABLE])
> -				nla_strlcpy(ctx->table, nla[NFTA_RULE_TABLE],
> -					    sizeof(ctx->table));
> +			if (nla[NFTA_RULE_TABLE]) {
> +				ctx->table = nla_strdup(nla[NFTA_RULE_TABLE],
> +							GFP_KERNEL);
> +				if (!ctx->table) {
> +					kfree(ctx);
> +					return -ENOMEM;
> +				}
> +			}
>  			if (nla[NFTA_RULE_CHAIN])
>  				nla_strlcpy(ctx->chain, nla[NFTA_RULE_CHAIN],
>  					    sizeof(ctx->chain));
> @@ -2508,7 +2524,7 @@ nft_select_set_ops(const struct nft_ctx *ctx,
>  
>  static const struct nla_policy nft_set_policy[NFTA_SET_MAX + 1] = {
>  	[NFTA_SET_TABLE]		= { .type = NLA_STRING,
> -					    .len = NFT_TABLE_MAXNAMELEN - 1 },
> +					    .len = NFT_NAME_MAXLEN - 1 },
>  	[NFTA_SET_NAME]			= { .type = NLA_STRING,
>  					    .len = NFT_SET_MAXNAMELEN - 1 },
>  	[NFTA_SET_FLAGS]		= { .type = NLA_U32 },
> @@ -3275,7 +3291,7 @@ static const struct nla_policy nft_set_elem_policy[NFTA_SET_ELEM_MAX + 1] = {
>  
>  static const struct nla_policy nft_set_elem_list_policy[NFTA_SET_ELEM_LIST_MAX + 1] = {
>  	[NFTA_SET_ELEM_LIST_TABLE]	= { .type = NLA_STRING,
> -					    .len = NFT_TABLE_MAXNAMELEN - 1 },
> +					    .len = NFT_NAME_MAXLEN - 1 },
>  	[NFTA_SET_ELEM_LIST_SET]	= { .type = NLA_STRING,
>  					    .len = NFT_SET_MAXNAMELEN - 1 },
>  	[NFTA_SET_ELEM_LIST_ELEMENTS]	= { .type = NLA_NESTED },
> @@ -4219,7 +4235,7 @@ EXPORT_SYMBOL_GPL(nf_tables_obj_lookup);
>  
>  static const struct nla_policy nft_obj_policy[NFTA_OBJ_MAX + 1] = {
>  	[NFTA_OBJ_TABLE]	= { .type = NLA_STRING,
> -				    .len = NFT_TABLE_MAXNAMELEN - 1 },
> +				    .len = NFT_NAME_MAXLEN - 1 },
>  	[NFTA_OBJ_NAME]		= { .type = NLA_STRING,
>  				    .len = NFT_OBJ_MAXNAMELEN - 1 },
>  	[NFTA_OBJ_TYPE]		= { .type = NLA_U32 },
> @@ -4415,7 +4431,7 @@ static int nf_tables_fill_obj_info(struct sk_buff *skb, struct net *net,
>  }
>  
>  struct nft_obj_filter {
> -	char		table[NFT_OBJ_MAXNAMELEN];
> +	char		*table;
>  	u32		type;
>  };
>  
> @@ -4480,7 +4496,10 @@ static int nf_tables_dump_obj(struct sk_buff *skb, struct netlink_callback *cb)
>  
>  static int nf_tables_dump_obj_done(struct netlink_callback *cb)
>  {
> -	kfree(cb->data);
> +	struct nft_obj_filter *filter = cb->data;
> +
> +	kfree(filter->table);
> +	kfree(filter);
>  
>  	return 0;
>  }
> @@ -4494,9 +4513,13 @@ nft_obj_filter_alloc(const struct nlattr * const nla[])
>  	if (!filter)
>  		return ERR_PTR(-ENOMEM);
>  
> -	if (nla[NFTA_OBJ_TABLE])
> -		nla_strlcpy(filter->table, nla[NFTA_OBJ_TABLE],
> -			    NFT_TABLE_MAXNAMELEN);
> +	if (nla[NFTA_OBJ_TABLE]) {
> +		filter->table = nla_strdup(nla[NFTA_OBJ_TABLE], GFP_KERNEL);
> +		if (!filter->table) {
> +			kfree(filter);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +	}
>  	if (nla[NFTA_OBJ_TYPE])
>  		filter->type = ntohl(nla_get_be32(nla[NFTA_OBJ_TYPE]));
>  
> diff --git a/net/netfilter/nf_tables_trace.c b/net/netfilter/nf_tables_trace.c
> index e1b15e7a5793f..e95098c1faaf0 100644
> --- a/net/netfilter/nf_tables_trace.c
> +++ b/net/netfilter/nf_tables_trace.c
> @@ -175,7 +175,6 @@ void nft_trace_notify(struct nft_traceinfo *info)
>  		return;
>  
>  	size = nlmsg_total_size(sizeof(struct nfgenmsg)) +
> -		nla_total_size(NFT_TABLE_MAXNAMELEN) +
>  		nla_total_size(NFT_CHAIN_MAXNAMELEN) +
>  		nla_total_size_64bit(sizeof(__be64)) +	/* rule handle */
>  		nla_total_size(sizeof(__be32)) +	/* trace type */
> @@ -194,6 +193,9 @@ void nft_trace_notify(struct nft_traceinfo *info)
>  		nla_total_size(sizeof(u32)) +		/* nfproto */
>  		nla_total_size(sizeof(u32));		/* policy */
>  
> +	if (info->chain)
> +		size += nla_total_size(strlen(info->chain->table->name));

Do we need a branch here? I think info->chain is always set in traces,
right?

Thanks!

Re: [nf-next PATCH v2 2/5] netfilter: nf_tables: Unlimit table name length

[Phil Sutter]

Hi Pablo,

On Tue, Jul 25, 2017 at 06:10:02PM +0200, Pablo Neira Ayuso wrote:
> I think you can rename patch title to "allow table names up to 256
> chars". This is not unlimited anymore since v2.

OK.

> A couple more comments below.
> 
> On Mon, Jul 24, 2017 at 08:56:48PM +0200, Phil Sutter wrote:
[...]
> > --- a/include/uapi/linux/netfilter/nf_tables.h
> > +++ b/include/uapi/linux/netfilter/nf_tables.h
> > @@ -1,7 +1,7 @@
> >  #ifndef _LINUX_NF_TABLES_H
> >  #define _LINUX_NF_TABLES_H
> >  
> > -#define NFT_TABLE_MAXNAMELEN	32
> > +#define NFT_NAME_MAXLEN		256
> 
> I understand NFT_*_MAXNAMELEN per object is probably too much, but
> given this is uapi, I think we have to keep it around, ie.
> 
> #define NFT_NAME_MAXLEN		256
> #define NFT_TABLE_MAXNAMELEN    NFT_NAME_MAXLEN
> 
> And so on.

OK, I will change it. But using NFT_NAME_MAXLEN throughout kernel code
is OK? Or should I revert the policy struct changes?

> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 7843efa33c598..cf12f63606aaf 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -428,7 +428,7 @@ nf_tables_chain_type_lookup(const struct nft_af_info *afi,
> >  
> >  static const struct nla_policy nft_table_policy[NFTA_TABLE_MAX + 1] = {
> >  	[NFTA_TABLE_NAME]	= { .type = NLA_STRING,
> > -				    .len = NFT_TABLE_MAXNAMELEN - 1 },
> > +				    .len = NFT_NAME_MAXLEN - 1 },
> >  	[NFTA_TABLE_FLAGS]	= { .type = NLA_U32 },
> >  };

This one, e.g.: Keeping the old NFT_TABLE_MAXNAMELEN around makes that
change needless in theory.

[...]
> > diff --git a/net/netfilter/nf_tables_trace.c b/net/netfilter/nf_tables_trace.c
> > index e1b15e7a5793f..e95098c1faaf0 100644
> > --- a/net/netfilter/nf_tables_trace.c
> > +++ b/net/netfilter/nf_tables_trace.c
> > @@ -175,7 +175,6 @@ void nft_trace_notify(struct nft_traceinfo *info)
> >  		return;
> >  
> >  	size = nlmsg_total_size(sizeof(struct nfgenmsg)) +
> > -		nla_total_size(NFT_TABLE_MAXNAMELEN) +
> >  		nla_total_size(NFT_CHAIN_MAXNAMELEN) +
> >  		nla_total_size_64bit(sizeof(__be64)) +	/* rule handle */
> >  		nla_total_size(sizeof(__be32)) +	/* trace type */
> > @@ -194,6 +193,9 @@ void nft_trace_notify(struct nft_traceinfo *info)
> >  		nla_total_size(sizeof(u32)) +		/* nfproto */
> >  		nla_total_size(sizeof(u32));		/* policy */
> >  
> > +	if (info->chain)
> > +		size += nla_total_size(strlen(info->chain->table->name));
> 
> Do we need a branch here? I think info->chain is always set in traces,
> right?

I wasn't sure, so I stuck to how nft_trace_notify() handles it later:

| if (info->chain) {
|         if (nla_put_string(skb, NFTA_TRACE_CHAIN,
|                            info->chain->name))
|                 goto nla_put_failure;
|         if (nla_put_string(skb, NFTA_TRACE_TABLE,
|                            info->chain->table->name))
|                 goto nla_put_failure;
| }

This made me believe there is a case where info->chain is not set.
Though looking at nft_do_chain() which is the only caller of
nft_trace_packet(), it seems like there is indeed always a chain (it is
dereferenced right at the top).

So probably nft_trace_notify() can unconditionally put NFTA_TRACE_CHAIN
and NFTA_TRACE_TABLE attributes. Maybe Florian knows more?

Another questionable part (in the 'Unlimit chain name length' patch) is
the existence check for info->verdict->chain: The relevant attribute is
created only if info->type is either NFT_TRACETYPE_RETURN or
NFT_TRACETYPE_RULE *and* info->verdict->code is either NFT_JUMP or
NFT_GOTO. Hard to tell whether this can be assumed to always exist.

Thanks, Phil

Re: [nf-next PATCH v2 2/5] netfilter: nf_tables: Unlimit table name length

[Pablo Neira Ayuso]

Hi Phil,

On Tue, Jul 25, 2017 at 09:16:28PM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Tue, Jul 25, 2017 at 06:10:02PM +0200, Pablo Neira Ayuso wrote:
> > I think you can rename patch title to "allow table names up to 256
> > chars". This is not unlimited anymore since v2.
> 
> OK.
> 
> > A couple more comments below.
> > 
> > On Mon, Jul 24, 2017 at 08:56:48PM +0200, Phil Sutter wrote:
> [...]
> > > --- a/include/uapi/linux/netfilter/nf_tables.h
> > > +++ b/include/uapi/linux/netfilter/nf_tables.h
> > > @@ -1,7 +1,7 @@
> > >  #ifndef _LINUX_NF_TABLES_H
> > >  #define _LINUX_NF_TABLES_H
> > >  
> > > -#define NFT_TABLE_MAXNAMELEN	32
> > > +#define NFT_NAME_MAXLEN		256
> > 
> > I understand NFT_*_MAXNAMELEN per object is probably too much, but
> > given this is uapi, I think we have to keep it around, ie.
> > 
> > #define NFT_NAME_MAXLEN		256
> > #define NFT_TABLE_MAXNAMELEN    NFT_NAME_MAXLEN
> > 
> > And so on.
> 
> OK, I will change it. But using NFT_NAME_MAXLEN throughout kernel code
> is OK? Or should I revert the policy struct changes?
>
> > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > > index 7843efa33c598..cf12f63606aaf 100644
> > > --- a/net/netfilter/nf_tables_api.c
> > > +++ b/net/netfilter/nf_tables_api.c
> > > @@ -428,7 +428,7 @@ nf_tables_chain_type_lookup(const struct nft_af_info *afi,
> > >  
> > >  static const struct nla_policy nft_table_policy[NFTA_TABLE_MAX + 1] = {
> > >  	[NFTA_TABLE_NAME]	= { .type = NLA_STRING,
> > > -				    .len = NFT_TABLE_MAXNAMELEN - 1 },
> > > +				    .len = NFT_NAME_MAXLEN - 1 },
> > >  	[NFTA_TABLE_FLAGS]	= { .type = NLA_U32 },
> > >  };
> 
> This one, e.g.: Keeping the old NFT_TABLE_MAXNAMELEN around makes that
> change needless in theory.

Probably good to keep using the old definitions, so we save one line
update in the patch. And we avoid people having the temptation to
remove this from uapi.

> [...]
> > > diff --git a/net/netfilter/nf_tables_trace.c b/net/netfilter/nf_tables_trace.c
> > > index e1b15e7a5793f..e95098c1faaf0 100644
> > > --- a/net/netfilter/nf_tables_trace.c
> > > +++ b/net/netfilter/nf_tables_trace.c
> > > @@ -175,7 +175,6 @@ void nft_trace_notify(struct nft_traceinfo *info)
> > >  		return;
> > >  
> > >  	size = nlmsg_total_size(sizeof(struct nfgenmsg)) +
> > > -		nla_total_size(NFT_TABLE_MAXNAMELEN) +
> > >  		nla_total_size(NFT_CHAIN_MAXNAMELEN) +
> > >  		nla_total_size_64bit(sizeof(__be64)) +	/* rule handle */
> > >  		nla_total_size(sizeof(__be32)) +	/* trace type */
> > > @@ -194,6 +193,9 @@ void nft_trace_notify(struct nft_traceinfo *info)
> > >  		nla_total_size(sizeof(u32)) +		/* nfproto */
> > >  		nla_total_size(sizeof(u32));		/* policy */
> > >  
> > > +	if (info->chain)
> > > +		size += nla_total_size(strlen(info->chain->table->name));
> > 
> > Do we need a branch here? I think info->chain is always set in traces,
> > right?
> 
> I wasn't sure, so I stuck to how nft_trace_notify() handles it later:
> 
> | if (info->chain) {
> |         if (nla_put_string(skb, NFTA_TRACE_CHAIN,
> |                            info->chain->name))
> |                 goto nla_put_failure;
> |         if (nla_put_string(skb, NFTA_TRACE_TABLE,
> |                            info->chain->table->name))
> |                 goto nla_put_failure;
> | }
> 
> This made me believe there is a case where info->chain is not set.
> Though looking at nft_do_chain() which is the only caller of
> nft_trace_packet(), it seems like there is indeed always a chain (it is
> dereferenced right at the top).
> 
> So probably nft_trace_notify() can unconditionally put NFTA_TRACE_CHAIN
> and NFTA_TRACE_TABLE attributes. Maybe Florian knows more?

info->chain is always set from __nft_trace_packet(), and all calls to
nft_trace_packet() pass a non-null the chain object. So my impression
is that you can send an unfront patch in this series to remove the
branch. Nonetheless, please make another review of what I'm telling
above to confirm this.

> Another questionable part (in the 'Unlimit chain name length' patch) is
> the existence check for info->verdict->chain: The relevant attribute is
> created only if info->type is either NFT_TRACETYPE_RETURN or
> NFT_TRACETYPE_RULE *and* info->verdict->code is either NFT_JUMP or
> NFT_GOTO. Hard to tell whether this can be assumed to always exist.

See struct nft_regs, it's a union, so info->verdict is a valid pointer
under very specific circunstances. I think patch 3/5 is not correct.

Thanks!