[PATCH nf-next 0/4] netfilter: add hook dump feature

[PATCH nf-next 0/4] netfilter: add hook dump feature

[Florian Westphal]

Enable dump of the registered netfilter hooks to userspace.
This allows userspace to peek at the active hooks for each family/hook
point.

Example:
    $ nft list hook ip type input
    family ip hook input {
            +0000000000 nft_do_chain_inet [nf_tables]       # nft table ip filter chain input
            +0000000010 nft_do_chain_inet [nf_tables]       # nft table ip firewalld chain filter_INPUT
            +0000000100 nf_nat_ipv4_local_in [nf_nat]
            +2147483647 ipv4_confirm [nf_conntrack]
    }

Implementation is done in nf_tables.
Alternative would be to add this as a separate/new nfnetlink family.

Let me know if thats the preferred route and I will respin.
I did this in nf_tables because it allows re-use of the existing
nft_hook_attributes and it seemed strange to add a new kernel module
for this.

Florian Westphal (4):
  netfilter: nf_tables: allow to dump all registered base hooks
  netfilter: nf_tables: include function and module name in hook dumps
  netfilter: annotate nf_tables base hook ops
  netfilter: nf_tables: include table and chain name when dumping hooks

 include/linux/netfilter.h                |  12 +-
 include/uapi/linux/netfilter/nf_tables.h |   7 +
 net/netfilter/core.c                     |   6 +
 net/netfilter/nf_queue.c                 |   4 +-
 net/netfilter/nf_tables_api.c            | 275 ++++++++++++++++++++++-
 5 files changed, 300 insertions(+), 4 deletions(-)

-- 
2.26.3

[PATCH nf-next 1/4] netfilter: nf_tables: allow to dump all registered base hooks

[Florian Westphal]

This dumps the active hook pipeline to userspace.

Userspace needs to pass the address family and the hook point.

For the netdev family/ingress hook, a device name must be
provided as well.

This allows 'nft' to display the priority of each hook.

Example:
family ip hook prerouting {
        -0000000300
        -0000000150
        -0000000100
}

... so this shows 3 active hooks with -300/-150/-100.

With manual work this is enough to figure out which hooks these are.

Followup patch will include the hook and module name (if any).

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/uapi/linux/netfilter/nf_tables.h |   1 +
 net/netfilter/nf_queue.c                 |   4 +-
 net/netfilter/nf_tables_api.c            | 216 +++++++++++++++++++++++
 3 files changed, 219 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 19715e2679d1..5810e41eff33 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -124,6 +124,7 @@ enum nf_tables_msg_types {
 	NFT_MSG_NEWFLOWTABLE,
 	NFT_MSG_GETFLOWTABLE,
 	NFT_MSG_DELFLOWTABLE,
+	NFT_MSG_GETNFHOOKS,
 	NFT_MSG_MAX,
 };
 
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index bbd1209694b8..224238b20825 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -259,7 +259,7 @@ static unsigned int nf_iterate(struct sk_buff *skb,
 	return NF_ACCEPT;
 }
 
-static struct nf_hook_entries *nf_hook_entries_head(const struct net *net, u8 pf, u8 hooknum)
+static struct nf_hook_entries *nf_queue_hook_entries_head(const struct net *net, u8 pf, u8 hooknum)
 {
 	switch (pf) {
 #ifdef CONFIG_NETFILTER_FAMILY_BRIDGE
@@ -292,7 +292,7 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 	net = entry->state.net;
 	pf = entry->state.pf;
 
-	hooks = nf_hook_entries_head(net, pf, entry->state.hook);
+	hooks = nf_queue_hook_entries_head(net, pf, entry->state.hook);
 
 	i = entry->hook_index;
 	if (WARN_ON_ONCE(!hooks || i >= hooks->num_hook_entries)) {
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index d63d2d8f769c..2bfa80e93658 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -7911,6 +7911,216 @@ static int nf_tables_getgen(struct sk_buff *skb, const struct nfnl_info *info,
 	return err;
 }
 
+static const struct nf_hook_entries *
+nf_hook_entries_head(u8 pf, unsigned int hook, struct net *net, const char *dev)
+{
+	const struct nf_hook_entries *hook_head = NULL;
+	struct net_device *netdev;
+
+	switch (pf) {
+	case NFPROTO_IPV4:
+		if (hook >= ARRAY_SIZE(net->nf.hooks_ipv4))
+			return ERR_PTR(-EINVAL);
+		hook_head = rcu_dereference(net->nf.hooks_ipv4[hook]);
+		break;
+	case NFPROTO_IPV6:
+		hook_head = rcu_dereference(net->nf.hooks_ipv6[hook]);
+		if (hook >= ARRAY_SIZE(net->nf.hooks_ipv6))
+			return ERR_PTR(-EINVAL);
+		break;
+	case NFPROTO_ARP:
+#ifdef CONFIG_NETFILTER_FAMILY_ARP
+		if (hook >= ARRAY_SIZE(net->nf.hooks_arp))
+			return ERR_PTR(-EINVAL);
+		hook_head = rcu_dereference(net->nf.hooks_arp[hook]);
+#endif
+		break;
+	case NFPROTO_BRIDGE:
+#ifdef CONFIG_NETFILTER_FAMILY_BRIDGE
+		if (hook >= ARRAY_SIZE(net->nf.hooks_bridge))
+			return ERR_PTR(-EINVAL);
+		hook_head = rcu_dereference(net->nf.hooks_bridge[hook]);
+#endif
+		break;
+#if IS_ENABLED(CONFIG_DECNET)
+	case NFPROTO_DECNET:
+		if (hook >= ARRAY_SIZE(net->nf.hooks_decnet))
+			return ERR_PTR(-EINVAL);
+		hook_head = rcu_dereference(net->nf.hooks_decnet[hook]);
+		break;
+#endif
+#ifdef CONFIG_NETFILTER_INGRESS
+	case NFPROTO_NETDEV:
+		if (hook != NF_NETDEV_INGRESS)
+			return ERR_PTR(-EOPNOTSUPP);
+
+		if (!dev)
+			return ERR_PTR(-ENODEV);
+
+		netdev = dev_get_by_name_rcu(net, dev);
+		if (!netdev)
+			return ERR_PTR(-ENODEV);
+
+		return rcu_dereference(netdev->nf_hooks_ingress);
+#endif
+	default:
+		return ERR_PTR(-EPROTONOSUPPORT);
+	}
+
+	return hook_head;
+}
+
+struct nft_dump_hooks_data {
+	char devname[IFNAMSIZ];
+	unsigned long headv;
+	unsigned int seq;
+	u8 hook;
+};
+
+static int nf_tables_dump_one_hook(struct sk_buff *nlskb,
+				   const struct nft_dump_hooks_data *ctx,
+				   const struct nf_hook_ops *ops)
+{
+	unsigned int portid = NETLINK_CB(nlskb).portid;
+	struct net *net = sock_net(nlskb->sk);
+	struct nlmsghdr *nlh;
+	int ret;
+
+	nlh = nfnl_msg_put(nlskb, portid, ctx->seq, NFT_MSG_GETNFHOOKS,
+			   NLM_F_MULTI, ops->pf, NFNETLINK_V0, nft_base_seq(net));
+	if (!nlh)
+		goto nla_put_failure;
+
+	ret = nla_put_be32(nlskb, NFTA_HOOK_HOOKNUM, htonl(ops->hooknum));
+	if (ret)
+		goto nla_put_failure;
+
+	ret = nla_put_be32(nlskb, NFTA_HOOK_PRIORITY, htonl(ops->priority));
+	if (ret)
+		goto nla_put_failure;
+
+	nlmsg_end(nlskb, nlh);
+	return 0;
+nla_put_failure:
+	nlmsg_trim(nlskb, nlh);
+	return ret;
+}
+
+static int nf_tables_dump_basehooks(struct sk_buff *nlskb,
+				    struct netlink_callback *cb)
+{
+	struct nfgenmsg *nfmsg = nlmsg_data(cb->nlh);
+	struct nft_dump_hooks_data *ctx = cb->data;
+	int err, family = nfmsg->nfgen_family;
+	struct net *net = sock_net(nlskb->sk);
+	struct nf_hook_ops * const *ops;
+	const struct nf_hook_entries *e;
+	unsigned int i = cb->args[0];
+
+	rcu_read_lock();
+	cb->seq = ctx->seq;
+
+	e = nf_hook_entries_head(family, ctx->hook, net, ctx->devname);
+	if (!e)
+		goto done;
+
+	if (IS_ERR(e)) {
+		ctx->seq++;
+		goto done;
+	}
+
+	if ((unsigned long)e != ctx->headv || i >= e->num_hook_entries)
+		ctx->seq++;
+
+	ops = nf_hook_entries_get_hook_ops(e);
+
+	for (; i < e->num_hook_entries; i++) {
+		err = nf_tables_dump_one_hook(nlskb, ctx, ops[i]);
+		if (err)
+			break;
+	}
+
+done:
+	nl_dump_check_consistent(cb, nlmsg_hdr(nlskb));
+	rcu_read_unlock();
+	cb->args[0] = i;
+	return nlskb->len;
+}
+
+static int nf_tables_dump_basehooks_start(struct netlink_callback *cb)
+{
+	const struct nfgenmsg *nfmsg = nlmsg_data(cb->nlh);
+	const struct nlattr * const *nla = cb->data;
+	struct nft_dump_hooks_data *ctx = NULL;
+	struct net *net = sock_net(cb->skb->sk);
+	u8 family = nfmsg->nfgen_family;
+	char name[IFNAMSIZ] = "";
+	const void *head;
+	u32 hooknum;
+
+	hooknum = ntohl(nla_get_be32(nla[NFTA_HOOK_HOOKNUM]));
+	if (hooknum > 255)
+		return -EINVAL;
+
+	if (family == NFPROTO_NETDEV) {
+		if (!nla[NFTA_HOOK_DEV])
+			return -EINVAL;
+
+		nla_strscpy(name, nla[NFTA_HOOK_DEV], sizeof(name));
+	}
+
+	rcu_read_lock();
+	/* Not dereferenced; for consistency check only */
+	head = nf_hook_entries_head(family, hooknum, net, name);
+	rcu_read_unlock();
+
+	if (head && IS_ERR(head))
+		return PTR_ERR(head);
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->headv = (unsigned long)head;
+
+	ctx->hook = hooknum;
+	strscpy(ctx->devname, name, sizeof(ctx->devname));
+	cb->data = ctx;
+	cb->seq = 1;
+	return 0;
+}
+
+static int nf_tables_dump_basehooks_stop(struct netlink_callback *cb)
+{
+	kfree(cb->data);
+	return 0;
+}
+
+static int nft_get_basehooks(struct sk_buff *skb,
+			     const struct nfnl_info *info,
+			     const struct nlattr * const nla[])
+{
+	if (!nla[NFTA_HOOK_HOOKNUM])
+		return -EINVAL;
+
+	if (ntohl(nla_get_be32(nla[NFTA_HOOK_HOOKNUM])) > 255)
+		return -EINVAL;
+
+	if (info->nlh->nlmsg_flags & NLM_F_DUMP) {
+		struct netlink_dump_control c = {
+			.start = nf_tables_dump_basehooks_start,
+			.done = nf_tables_dump_basehooks_stop,
+			.dump = nf_tables_dump_basehooks,
+			.module = THIS_MODULE,
+			.data = (void *)nla,
+		};
+
+		return nft_netlink_dump_start_rcu(info->sk, skb, info->nlh, &c);
+	}
+
+	return -EOPNOTSUPP;
+}
+
 static const struct nfnl_callback nf_tables_cb[NFT_MSG_MAX] = {
 	[NFT_MSG_NEWTABLE] = {
 		.call		= nf_tables_newtable,
@@ -8048,6 +8258,12 @@ static const struct nfnl_callback nf_tables_cb[NFT_MSG_MAX] = {
 		.attr_count	= NFTA_FLOWTABLE_MAX,
 		.policy		= nft_flowtable_policy,
 	},
+	[NFT_MSG_GETNFHOOKS] = {
+		.call		= nft_get_basehooks,
+		.type		= NFNL_CB_RCU,
+		.attr_count	= NFTA_HOOK_MAX,
+		.policy		= nft_hook_policy,
+	},
 };
 
 static int nf_tables_validate(struct net *net)
-- 
2.26.3

[PATCH nf-next 2/4] netfilter: nf_tables: include function and module name in hook dumps

[Florian Westphal]

If KALLSYMS are available, include the hook function name and the
module name that registered the hook.

This avoids need to manually annotate all existing hooks.

Example output:
family ip hook prerouting {
        -0000000300 iptable_raw_hook [iptable_raw]
        -0000000150 iptable_mangle_hook [iptable_mangle]
        -0000000100 nf_nat_ipv4_pre_routing [nf_nat]
}

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/linux/netfilter.h                |  4 ++++
 include/uapi/linux/netfilter/nf_tables.h |  4 ++++
 net/netfilter/core.c                     |  6 ++++++
 net/netfilter/nf_tables_api.c            | 13 +++++++++++++
 4 files changed, 27 insertions(+)

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index f0f3a8354c3c..63f77794f5ed 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -195,6 +195,10 @@ int nf_hook_slow(struct sk_buff *skb, struct nf_hook_state *state,
 
 void nf_hook_slow_list(struct list_head *head, struct nf_hook_state *state,
 		       const struct nf_hook_entries *e);
+
+bool nf_get_hook_info(const struct nf_hook_ops *ops,
+		      char fn[KSYM_NAME_LEN], char **module_name);
+
 /**
  *	nf_hook - call a netfilter hook
  *
diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 5810e41eff33..ba6545a32e34 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -147,6 +147,8 @@ enum nft_list_attributes {
  * @NFTA_HOOK_PRIORITY: netfilter hook priority (NLA_U32)
  * @NFTA_HOOK_DEV: netdevice name (NLA_STRING)
  * @NFTA_HOOK_DEVS: list of netdevices (NLA_NESTED)
+ * @NFTA_HOOK_FUNCTION_NAME: hook function name (NLA_STRING)
+ * @NFTA_HOOK_MODULE_NAME: kernel module that registered this hook (NLA_STRING)
  */
 enum nft_hook_attributes {
 	NFTA_HOOK_UNSPEC,
@@ -154,6 +156,8 @@ enum nft_hook_attributes {
 	NFTA_HOOK_PRIORITY,
 	NFTA_HOOK_DEV,
 	NFTA_HOOK_DEVS,
+	NFTA_HOOK_FUNCTION_NAME,
+	NFTA_HOOK_MODULE_NAME,
 	__NFTA_HOOK_MAX
 };
 #define NFTA_HOOK_MAX		(__NFTA_HOOK_MAX - 1)
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 63d032191e62..d14715c568c8 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -749,6 +749,12 @@ static struct pernet_operations netfilter_net_ops = {
 	.exit = netfilter_net_exit,
 };
 
+bool nf_get_hook_info(const struct nf_hook_ops *ops, char fn[KSYM_NAME_LEN], char **modname)
+{
+	return kallsyms_lookup((unsigned long)ops->hook, NULL, NULL, modname, fn);
+}
+EXPORT_SYMBOL_GPL(nf_get_hook_info);
+
 int __init netfilter_init(void)
 {
 	int ret;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 2bfa80e93658..216f2921be0f 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -7983,6 +7983,7 @@ static int nf_tables_dump_one_hook(struct sk_buff *nlskb,
 {
 	unsigned int portid = NETLINK_CB(nlskb).portid;
 	struct net *net = sock_net(nlskb->sk);
+	char *module_name, fn[KSYM_NAME_LEN];
 	struct nlmsghdr *nlh;
 	int ret;
 
@@ -7991,6 +7992,18 @@ static int nf_tables_dump_one_hook(struct sk_buff *nlskb,
 	if (!nlh)
 		goto nla_put_failure;
 
+	if (nf_get_hook_info(ops, fn, &module_name)) {
+		ret = nla_put_string(nlskb, NFTA_HOOK_FUNCTION_NAME, fn);
+		if (ret)
+			goto nla_put_failure;
+
+		if (module_name) {
+			ret = nla_put_string(nlskb, NFTA_HOOK_MODULE_NAME, module_name);
+			if (ret)
+				goto nla_put_failure;
+		}
+	}
+
 	ret = nla_put_be32(nlskb, NFTA_HOOK_HOOKNUM, htonl(ops->hooknum));
 	if (ret)
 		goto nla_put_failure;
-- 
2.26.3

[PATCH nf-next 3/4] netfilter: annotate nf_tables base hook ops

[Florian Westphal]

This will allow a followup patch to treat the 'ops->priv' pointer
as nft_chain argument without having to first walk to table/chains
for a match.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/linux/netfilter.h     | 8 +++++++-
 net/netfilter/nf_tables_api.c | 4 +++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index 63f77794f5ed..6c327689ff82 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -77,12 +77,18 @@ struct nf_hook_state {
 typedef unsigned int nf_hookfn(void *priv,
 			       struct sk_buff *skb,
 			       const struct nf_hook_state *state);
+enum nf_hook_ops_type {
+	NF_HOOK_OP_UNDEFINED,
+	NF_HOOK_OP_NF_TABLES,
+};
+
 struct nf_hook_ops {
 	/* User fills in from here down. */
 	nf_hookfn		*hook;
 	struct net_device	*dev;
 	void			*priv;
-	u_int8_t		pf;
+	u8			pf;
+	enum nf_hook_ops_type	hook_ops_type:8;
 	unsigned int		hooknum;
 	/* Hooks are ordered in ascending priority. */
 	int			priority;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 216f2921be0f..935f46db16bb 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2172,8 +2172,10 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 genmask,
 	}
 
 	nft_trans_chain_policy(trans) = NFT_CHAIN_POLICY_UNSET;
-	if (nft_is_base_chain(chain))
+	if (nft_is_base_chain(chain)) {
+		basechain->ops.hook_ops_type = NF_HOOK_OP_NF_TABLES;
 		nft_trans_chain_policy(trans) = policy;
+	}
 
 	err = nft_chain_add(table, chain);
 	if (err < 0) {
-- 
2.26.3

[PATCH nf-next 4/4] netfilter: nf_tables: include table and chain name when dumping hooks

[Florian Westphal]

For ip(6)tables, the function names will show 'raw', 'mangle',
and so on, but for nf_tables the interpreter name is identical for all
base chains in the same family, so its not easy to line up the defined
chains with the hook function name.

To make it easier to see how the ruleset lines up with the defined
hooks, extend the hook dump to include the chain+table name.

Example list:
family ip hook input {
  -0000000150 iptable_mangle_hook [iptable_mangle]
  +0000000000 nft_do_chain_inet [nf_tables]  # nft table filter chain input
 [..]                                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/uapi/linux/netfilter/nf_tables.h |  2 ++
 net/netfilter/nf_tables_api.c            | 42 ++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index ba6545a32e34..4822a837250d 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -149,6 +149,7 @@ enum nft_list_attributes {
  * @NFTA_HOOK_DEVS: list of netdevices (NLA_NESTED)
  * @NFTA_HOOK_FUNCTION_NAME: hook function name (NLA_STRING)
  * @NFTA_HOOK_MODULE_NAME: kernel module that registered this hook (NLA_STRING)
+ * @NFTA_HOOK_NFT_CHAIN_INFO: nft chain and table name (NLA_NESTED)
  */
 enum nft_hook_attributes {
 	NFTA_HOOK_UNSPEC,
@@ -158,6 +159,7 @@ enum nft_hook_attributes {
 	NFTA_HOOK_DEVS,
 	NFTA_HOOK_FUNCTION_NAME,
 	NFTA_HOOK_MODULE_NAME,
+	NFTA_HOOK_NFT_CHAIN_INFO,
 	__NFTA_HOOK_MAX
 };
 #define NFTA_HOOK_MAX		(__NFTA_HOOK_MAX - 1)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 935f46db16bb..832531e457c6 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -7979,6 +7979,44 @@ struct nft_dump_hooks_data {
 	u8 hook;
 };
 
+static int nla_put_chain_hook_info(struct sk_buff *nlskb, const struct nft_dump_hooks_data *ctx,
+				   const struct nf_hook_ops *ops)
+{
+	struct net *net = sock_net(nlskb->sk);
+	struct nft_chain *chain;
+	struct nlattr *nest;
+	int ret = 0;
+
+	if (ops->hook_ops_type != NF_HOOK_OP_NF_TABLES)
+		return 0;
+
+	chain = ops->priv;
+
+	if (WARN_ON_ONCE(!chain))
+		return 0;
+
+	if (!nft_is_active(net, chain))
+		return 0;
+
+	nest = nla_nest_start(nlskb, NFTA_HOOK_NFT_CHAIN_INFO);
+	if (!nest)
+		return -EMSGSIZE;
+
+	ret = nla_put_string(nlskb, NFTA_CHAIN_TABLE, chain->table->name);
+	if (ret) {
+		nla_nest_cancel(nlskb, nest);
+		return -EMSGSIZE;
+	}
+
+	ret = nla_put_string(nlskb, NFTA_CHAIN_NAME, chain->name);
+	if (ret)
+		nla_nest_cancel(nlskb, nest);
+	else
+		nla_nest_end(nlskb, nest);
+
+	return ret;
+}
+
 static int nf_tables_dump_one_hook(struct sk_buff *nlskb,
 				   const struct nft_dump_hooks_data *ctx,
 				   const struct nf_hook_ops *ops)
@@ -8014,6 +8052,10 @@ static int nf_tables_dump_one_hook(struct sk_buff *nlskb,
 	if (ret)
 		goto nla_put_failure;
 
+	ret = nla_put_chain_hook_info(nlskb, ctx, ops);
+	if (ret)
+		goto nla_put_failure;
+
 	nlmsg_end(nlskb, nlh);
 	return 0;
 nla_put_failure:
-- 
2.26.3

Re: [PATCH nf-next 1/4] netfilter: nf_tables: allow to dump all registered base hooks

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3114 bytes --]

Hi Florian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on nf-next/master]

url:    https://github.com/0day-ci/linux/commits/Florian-Westphal/netfilter-add-hook-dump-feature/20210522-225655
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git master
config: x86_64-randconfig-r026-20210522 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project e84a9b9bb3051c35dea993cdad7b3d2575638f85)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/0d9a4c5b44ef1ad2d9ed5c4c8cea7eae29560706
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florian-Westphal/netfilter-add-hook-dump-feature/20210522-225655
        git checkout 0d9a4c5b44ef1ad2d9ed5c4c8cea7eae29560706
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/netfilter/nf_tables_api.c:7991:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
           if (!nlh)
               ^~~~
   net/netfilter/nf_tables_api.c:8006:9: note: uninitialized use occurs here
           return ret;
                  ^~~
   net/netfilter/nf_tables_api.c:7991:2: note: remove the 'if' if its condition is always false
           if (!nlh)
           ^~~~~~~~~
   net/netfilter/nf_tables_api.c:7987:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   1 warning generated.


vim +7991 net/netfilter/nf_tables_api.c

  7979	
  7980	static int nf_tables_dump_one_hook(struct sk_buff *nlskb,
  7981					   const struct nft_dump_hooks_data *ctx,
  7982					   const struct nf_hook_ops *ops)
  7983	{
  7984		unsigned int portid = NETLINK_CB(nlskb).portid;
  7985		struct net *net = sock_net(nlskb->sk);
  7986		struct nlmsghdr *nlh;
  7987		int ret;
  7988	
  7989		nlh = nfnl_msg_put(nlskb, portid, ctx->seq, NFT_MSG_GETNFHOOKS,
  7990				   NLM_F_MULTI, ops->pf, NFNETLINK_V0, nft_base_seq(net));
> 7991		if (!nlh)
  7992			goto nla_put_failure;
  7993	
  7994		ret = nla_put_be32(nlskb, NFTA_HOOK_HOOKNUM, htonl(ops->hooknum));
  7995		if (ret)
  7996			goto nla_put_failure;
  7997	
  7998		ret = nla_put_be32(nlskb, NFTA_HOOK_PRIORITY, htonl(ops->priority));
  7999		if (ret)
  8000			goto nla_put_failure;
  8001	
  8002		nlmsg_end(nlskb, nlh);
  8003		return 0;
  8004	nla_put_failure:
  8005		nlmsg_trim(nlskb, nlh);
  8006		return ret;
  8007	}
  8008	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35091 bytes --]

Re: [PATCH nf-next 1/4] netfilter: nf_tables: allow to dump all registered base hooks

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3195 bytes --]

Hi Florian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on nf-next/master]

url:    https://github.com/0day-ci/linux/commits/Florian-Westphal/netfilter-add-hook-dump-feature/20210522-225655
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git master
config: x86_64-randconfig-r026-20210522 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project e84a9b9bb3051c35dea993cdad7b3d2575638f85)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/0d9a4c5b44ef1ad2d9ed5c4c8cea7eae29560706
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florian-Westphal/netfilter-add-hook-dump-feature/20210522-225655
        git checkout 0d9a4c5b44ef1ad2d9ed5c4c8cea7eae29560706
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/netfilter/nf_tables_api.c:7991:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
           if (!nlh)
               ^~~~
   net/netfilter/nf_tables_api.c:8006:9: note: uninitialized use occurs here
           return ret;
                  ^~~
   net/netfilter/nf_tables_api.c:7991:2: note: remove the 'if' if its condition is always false
           if (!nlh)
           ^~~~~~~~~
   net/netfilter/nf_tables_api.c:7987:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   1 warning generated.


vim +7991 net/netfilter/nf_tables_api.c

  7979	
  7980	static int nf_tables_dump_one_hook(struct sk_buff *nlskb,
  7981					   const struct nft_dump_hooks_data *ctx,
  7982					   const struct nf_hook_ops *ops)
  7983	{
  7984		unsigned int portid = NETLINK_CB(nlskb).portid;
  7985		struct net *net = sock_net(nlskb->sk);
  7986		struct nlmsghdr *nlh;
  7987		int ret;
  7988	
  7989		nlh = nfnl_msg_put(nlskb, portid, ctx->seq, NFT_MSG_GETNFHOOKS,
  7990				   NLM_F_MULTI, ops->pf, NFNETLINK_V0, nft_base_seq(net));
> 7991		if (!nlh)
  7992			goto nla_put_failure;
  7993	
  7994		ret = nla_put_be32(nlskb, NFTA_HOOK_HOOKNUM, htonl(ops->hooknum));
  7995		if (ret)
  7996			goto nla_put_failure;
  7997	
  7998		ret = nla_put_be32(nlskb, NFTA_HOOK_PRIORITY, htonl(ops->priority));
  7999		if (ret)
  8000			goto nla_put_failure;
  8001	
  8002		nlmsg_end(nlskb, nlh);
  8003		return 0;
  8004	nla_put_failure:
  8005		nlmsg_trim(nlskb, nlh);
  8006		return ret;
  8007	}
  8008	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 35091 bytes --]

Re: [PATCH nf-next 4/4] netfilter: nf_tables: include table and chain name when dumping hooks

[Pablo Neira Ayuso]

On Fri, May 21, 2021 at 01:39:22PM +0200, Florian Westphal wrote:
> For ip(6)tables, the function names will show 'raw', 'mangle',
> and so on, but for nf_tables the interpreter name is identical for all
> base chains in the same family, so its not easy to line up the defined
> chains with the hook function name.
> 
> To make it easier to see how the ruleset lines up with the defined
> hooks, extend the hook dump to include the chain+table name.
> 
> Example list:
> family ip hook input {
>   -0000000150 iptable_mangle_hook [iptable_mangle]
>   +0000000000 nft_do_chain_inet [nf_tables]  # nft table filter chain input
>  [..]                                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  include/uapi/linux/netfilter/nf_tables.h |  2 ++
>  net/netfilter/nf_tables_api.c            | 42 ++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> index ba6545a32e34..4822a837250d 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -149,6 +149,7 @@ enum nft_list_attributes {
>   * @NFTA_HOOK_DEVS: list of netdevices (NLA_NESTED)
>   * @NFTA_HOOK_FUNCTION_NAME: hook function name (NLA_STRING)
>   * @NFTA_HOOK_MODULE_NAME: kernel module that registered this hook (NLA_STRING)
> + * @NFTA_HOOK_NFT_CHAIN_INFO: nft chain and table name (NLA_NESTED)

Probably NFTA_HOOK_CHAIN_INFO ?

Re: [PATCH nf-next 4/4] netfilter: nf_tables: include table and chain name when dumping hooks

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, May 21, 2021 at 01:39:22PM +0200, Florian Westphal wrote:
> >   * @NFTA_HOOK_FUNCTION_NAME: hook function name (NLA_STRING)
> >   * @NFTA_HOOK_MODULE_NAME: kernel module that registered this hook (NLA_STRING)
> > + * @NFTA_HOOK_NFT_CHAIN_INFO: nft chain and table name (NLA_NESTED)
> 
> Probably NFTA_HOOK_CHAIN_INFO ?

I added _NFT_ to avoid ambiguity in case this would be extended
to add xt-legacy chain info.  I can drop the _NFT_, let me know.

Re: [PATCH nf-next 4/4] netfilter: nf_tables: include table and chain name when dumping hooks

[Pablo Neira Ayuso]

On Sun, May 23, 2021 at 08:54:31PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Fri, May 21, 2021 at 01:39:22PM +0200, Florian Westphal wrote:
> > >   * @NFTA_HOOK_FUNCTION_NAME: hook function name (NLA_STRING)
> > >   * @NFTA_HOOK_MODULE_NAME: kernel module that registered this hook (NLA_STRING)
> > > + * @NFTA_HOOK_NFT_CHAIN_INFO: nft chain and table name (NLA_NESTED)
> > 
> > Probably NFTA_HOOK_CHAIN_INFO ?
> 
> I added _NFT_ to avoid ambiguity in case this would be extended
> to add xt-legacy chain info.  I can drop the _NFT_, let me know.

It's a NLA_NESTED, you might add a _TYPE field inside the nest to
describe what type of chain info is stored there, maybe?

Re: [PATCH nf-next 4/4] netfilter: nf_tables: include table and chain name when dumping hooks

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sun, May 23, 2021 at 08:54:31PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Fri, May 21, 2021 at 01:39:22PM +0200, Florian Westphal wrote:
> > > >   * @NFTA_HOOK_FUNCTION_NAME: hook function name (NLA_STRING)
> > > >   * @NFTA_HOOK_MODULE_NAME: kernel module that registered this hook (NLA_STRING)
> > > > + * @NFTA_HOOK_NFT_CHAIN_INFO: nft chain and table name (NLA_NESTED)
> > > 
> > > Probably NFTA_HOOK_CHAIN_INFO ?
> > 
> > I added _NFT_ to avoid ambiguity in case this would be extended
> > to add xt-legacy chain info.  I can drop the _NFT_, let me know.
> 
> It's a NLA_NESTED, you might add a _TYPE field inside the nest to
> describe what type of chain info is stored there, maybe?

It uses enum nft_chain_attributes, it somehow feels wrong to add a
'type' field for that.

I could add a new enum if you prefer.

At this point I don't think adding xt specific info is useful
because the chain function name already tell if its mangle, raw etc.

Re: [PATCH nf-next 4/4] netfilter: nf_tables: include table and chain name when dumping hooks

[Pablo Neira Ayuso]

On Sun, May 23, 2021 at 11:26:46PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Sun, May 23, 2021 at 08:54:31PM +0200, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > On Fri, May 21, 2021 at 01:39:22PM +0200, Florian Westphal wrote:
> > > > >   * @NFTA_HOOK_FUNCTION_NAME: hook function name (NLA_STRING)
> > > > >   * @NFTA_HOOK_MODULE_NAME: kernel module that registered this hook (NLA_STRING)
> > > > > + * @NFTA_HOOK_NFT_CHAIN_INFO: nft chain and table name (NLA_NESTED)
> > > > 
> > > > Probably NFTA_HOOK_CHAIN_INFO ?
> > > 
> > > I added _NFT_ to avoid ambiguity in case this would be extended
> > > to add xt-legacy chain info.  I can drop the _NFT_, let me know.
> > 
> > It's a NLA_NESTED, you might add a _TYPE field inside the nest to
> > describe what type of chain info is stored there, maybe?
> 
> It uses enum nft_chain_attributes, it somehow feels wrong to add a
> 'type' field for that.

Agreed. Probably another nest level.

NFTA_HOOK_CHAIN_INFO
    CHAIN_INFO_DESC
         nft_chain_attributes
    CHAIN_INFO_TYPE

> I could add a new enum if you prefer.
> 
> At this point I don't think adding xt specific info is useful
> because the chain function name already tell if its mangle, raw etc.

I'd prefer to not expose internal kernel functions names, but I
understand this approach is simple.

[PATCH nf-next 4/4] netfilter: nf_tables: include table and chain name when dumping hooks

[Florian Westphal]

For ip(6)tables, the function names will show 'raw', 'mangle',
and so on, but for nf_tables the interpreter name is identical for all
base chains in the same family, so its not easy to line up the defined
chains with the hook function name.

To make it easier to see how the ruleset lines up with the defined
hooks, extend the hook dump to include the chain+table name.

Example list:
family ip hook input {
  -0000000150 iptable_mangle_hook [iptable_mangle]
  +0000000000 nft_do_chain_inet [nf_tables]  # nft table filter chain input
 [..]                                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/uapi/linux/netfilter/nf_tables.h | 25 +++++++++++
 net/netfilter/nf_tables_api.c            | 53 ++++++++++++++++++++++++
 2 files changed, 78 insertions(+)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index ba6545a32e34..099fad689311 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -149,6 +149,7 @@ enum nft_list_attributes {
  * @NFTA_HOOK_DEVS: list of netdevices (NLA_NESTED)
  * @NFTA_HOOK_FUNCTION_NAME: hook function name (NLA_STRING)
  * @NFTA_HOOK_MODULE_NAME: kernel module that registered this hook (NLA_STRING)
+ * @NFTA_HOOK_CHAIN_INFO: basechain hook metadata (NLA_NESTED)
  */
 enum nft_hook_attributes {
 	NFTA_HOOK_UNSPEC,
@@ -158,10 +159,34 @@ enum nft_hook_attributes {
 	NFTA_HOOK_DEVS,
 	NFTA_HOOK_FUNCTION_NAME,
 	NFTA_HOOK_MODULE_NAME,
+	NFTA_HOOK_CHAIN_INFO,
 	__NFTA_HOOK_MAX
 };
 #define NFTA_HOOK_MAX		(__NFTA_HOOK_MAX - 1)
 
+/**
+ * enum nft_chain_info_attributes - chain description
+ *
+ * NFTA_CHAIN_INFO_DESC: chain and table name (enum nft_table_attributes) (NLA_NESTED)
+ * NFTA_CHAIN_INFO_TYPE: chain type (enum nf_chain_type) (NLA_U32)
+ */
+enum nft_chain_info_attributes {
+	NFTA_CHAIN_INFO_UNSPEC,
+	NFTA_CHAIN_INFO_DESC,
+	NFTA_CHAIN_INFO_TYPE,
+	__NFTA_CHAIN_INFO_MAX,
+};
+#define NFTA_CHAIN_INFO_MAX (__NFTA_CHAIN_INFO_MAX - 1)
+
+/**
+ * enum nft_chain_info_attributes - chain description
+ *
+ * @NF_CHAININFO_TYPE_NF_TABLES nf_tables base chain
+ */
+enum nf_chaininfo_type {
+	NF_CHAININFO_TYPE_NF_TABLES = 0x1,
+};
+
 /**
  * enum nft_table_flags - nf_tables table flags
  *
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index e7e80c8ee123..d58e4b39efe1 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -7979,6 +7979,55 @@ struct nft_dump_hooks_data {
 	u8 hook;
 };
 
+static int nla_put_chain_hook_info(struct sk_buff *nlskb, const struct nft_dump_hooks_data *ctx,
+				   const struct nf_hook_ops *ops)
+{
+	struct net *net = sock_net(nlskb->sk);
+	struct nlattr *nest, *nest2;
+	struct nft_chain *chain;
+	int ret = 0;
+
+	if (ops->hook_ops_type != NF_HOOK_OP_NF_TABLES)
+		return 0;
+
+	chain = ops->priv;
+
+	if (WARN_ON_ONCE(!chain))
+		return 0;
+
+	if (!nft_is_active(net, chain))
+		return 0;
+
+	nest = nla_nest_start(nlskb, NFTA_HOOK_CHAIN_INFO);
+	if (!nest)
+		return -EMSGSIZE;
+
+	ret = nla_put_be32(nlskb, NFTA_CHAIN_INFO_TYPE,
+			   htonl(NF_CHAININFO_TYPE_NF_TABLES));
+	if (ret)
+		goto cancel_nest;
+
+	nest2 = nla_nest_start(nlskb, NFTA_CHAIN_INFO_DESC);
+	if (!nest2)
+		goto cancel_nest;
+
+	ret = nla_put_string(nlskb, NFTA_CHAIN_TABLE, chain->table->name);
+	if (ret)
+		goto cancel_nest;
+
+	ret = nla_put_string(nlskb, NFTA_CHAIN_NAME, chain->name);
+	if (ret)
+		goto cancel_nest;
+
+	nla_nest_end(nlskb, nest2);
+	nla_nest_end(nlskb, nest);
+	return ret;
+
+cancel_nest:
+	nla_nest_cancel(nlskb, nest);
+	return -EMSGSIZE;
+}
+
 static int nf_tables_dump_one_hook(struct sk_buff *nlskb,
 				   const struct nft_dump_hooks_data *ctx,
 				   const struct nf_hook_ops *ops)
@@ -8014,6 +8063,10 @@ static int nf_tables_dump_one_hook(struct sk_buff *nlskb,
 	if (ret)
 		goto nla_put_failure;
 
+	ret = nla_put_chain_hook_info(nlskb, ctx, ops);
+	if (ret)
+		goto nla_put_failure;
+
 	nlmsg_end(nlskb, nlh);
 	return 0;
 nla_put_failure:
-- 
2.26.3