[PATCH nft v2 0/5] src: mnl: rework list hooks infra

[PATCH nft v2 0/5] src: mnl: rework list hooks infra

[Florian Westphal]

Turns out that not only was 'nft list hooks' mostly undocumented,
there was also confusion on what it should do.

First, clean this code up and make it strictly a tool to dump
the NFPROTO_X registered functions.

Then, remove the 'hook' function argument, this was still passed
from back in the day when one could ask to only dump e.g.
ipv4 prerouting.  This ability is of little value, so don't restore
this but instead just remove the leftover code.

Next, allow dumping of netdev:egress hooks.
Lastly, document this in more detail and make it clear that this
dumps the netfilter hooks registered for the protocol families,
and nothing else.

Once this gets applied I intend to make
'nft list hooks netdev'

dump device hooks for all interfaces, if any, instead of a
'no device provided' warning.

Florian Westphal (5):
  src: mnl: clean up hook listing code
  src: mnl: make family specification more strict when listing
  src: drop obsolete hook argument form hook dump functions
  src: add egress support for 'list hooks'
  doc: add documentation about list hooks feature

 Makefile.am                 |   1 +
 doc/additional-commands.txt | 116 ++++++++++++++++++++++++++++
 doc/nft.txt                 |  63 +--------------
 include/mnl.h               |   2 +-
 src/mnl.c                   | 150 ++++++++++++++----------------------
 src/rule.c                  |   6 +-
 6 files changed, 179 insertions(+), 159 deletions(-)
 create mode 100644 doc/additional-commands.txt

-- 
2.44.2

[PATCH nft v2 1/5] src: mnl: clean up hook listing code

[Florian Westphal]

mnl_nft_dump_nf_hooks() can call itself for the UNSPEC case, this
avoids the second switch/case to handle printing for inet/unspec.

As for the error handling, 'nft list hooks' should not print an error,
even if nothing is printed, UNLESS there was also a lowlevel (syscall)
error from the kernel.

We don't want to indicate failure just because e.g. kernel doesn't support
NFPROTO_ARP.

This also fixes a display bug, 'nft list hooks device foo' would show hooks
registered for that device as 'bridge' family instead of the expected
'netdev' family.

This was because UNSPEC handling did not query 'netdev' family and did
pass the device name to the lowlevel function.  Add it, and pass NULL
device name for those families that don't support device attachment.

The lowelevel function currently always queries NFPROTO_NETDEV to handle
the 'inet' ingress case.

This is dubious, as 'inet ingress' is a pseudo-alias to netdev family
(inet itself is a pseudo-family that ends up registering for both ipv4
 and ipv6 hooks).

This is resolved in next patch.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/mnl.c | 98 ++++++++++++++++++++-----------------------------------
 1 file changed, 35 insertions(+), 63 deletions(-)

diff --git a/src/mnl.c b/src/mnl.c
index ec7d2bd5defc..e4bbbcf6d536 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -2518,65 +2518,42 @@ static void print_hooks(struct netlink_ctx *ctx, int family, struct list_head *h
 	fprintf(fp, "}\n");
 }
 
-#define HOOK_FAMILY_MAX	5
-
-static uint8_t hook_family[HOOK_FAMILY_MAX] = {
-	NFPROTO_IPV4,
-	NFPROTO_IPV6,
-	NFPROTO_BRIDGE,
-	NFPROTO_ARP,
-};
-
 static int mnl_nft_dump_nf(struct netlink_ctx *ctx, int family, int hook,
-			   const char *devname, struct list_head *hook_list,
-			   int *ret)
+			   const char *devname, struct list_head *hook_list)
 {
 	int i, err;
 
 	/* show ingress in first place in hook listing. */
 	err = __mnl_nft_dump_nf_hooks(ctx, family, NFPROTO_NETDEV, NF_NETDEV_INGRESS, devname, hook_list);
-	if (err < 0)
-		*ret = err;
 
 	for (i = 0; i <= NF_INET_POST_ROUTING; i++) {
-		err = __mnl_nft_dump_nf_hooks(ctx, family, family, i, devname, hook_list);
-		if (err < 0)
-			*ret = err;
+		int tmp;
+
+		tmp = __mnl_nft_dump_nf_hooks(ctx, family, family, i, devname, hook_list);
+		if (tmp == 0)
+			err = 0;
 	}
 
 	return err;
 }
 
 static int mnl_nft_dump_nf_arp(struct netlink_ctx *ctx, int family, int hook,
-			       const char *devname, struct list_head *hook_list,
-			       int *ret)
+			       const char *devname, struct list_head *hook_list)
 {
-	int err;
-
-	/* show ingress in first place in hook listing. */
-	err = __mnl_nft_dump_nf_hooks(ctx, family, NFPROTO_NETDEV, NF_NETDEV_INGRESS, devname, hook_list);
-	if (err < 0)
-		*ret = err;
+	int err1, err2;
 
-	err = __mnl_nft_dump_nf_hooks(ctx, family, family, NF_ARP_IN, devname, hook_list);
-	if (err < 0)
-		*ret = err;
-	err = __mnl_nft_dump_nf_hooks(ctx, family, family, NF_ARP_OUT, devname, hook_list);
-	if (err < 0)
-		*ret = err;
+	err1 = __mnl_nft_dump_nf_hooks(ctx, family, family, NF_ARP_IN, devname, hook_list);
+	err2 = __mnl_nft_dump_nf_hooks(ctx, family, family, NF_ARP_OUT, devname, hook_list);
 
-	return err;
+	return err1 ? err2 : err1;
 }
 
 static int mnl_nft_dump_nf_netdev(struct netlink_ctx *ctx, int family, int hook,
-				  const char *devname, struct list_head *hook_list,
-				  int *ret)
+				  const char *devname, struct list_head *hook_list)
 {
 	int err;
 
 	err = __mnl_nft_dump_nf_hooks(ctx, family, NFPROTO_NETDEV, NF_NETDEV_INGRESS, devname, hook_list);
-	if (err < 0)
-		*ret = err;
 
 	return err;
 }
@@ -2592,51 +2569,46 @@ static void release_hook_list(struct list_head *hook_list)
 int mnl_nft_dump_nf_hooks(struct netlink_ctx *ctx, int family, int hook, const char *devname)
 {
 	LIST_HEAD(hook_list);
-	unsigned int i;
-	int ret;
+	int ret = -1, tmp;
 
 	errno = 0;
-	ret = 0;
 
 	switch (family) {
 	case NFPROTO_UNSPEC:
-		mnl_nft_dump_nf(ctx, NFPROTO_IPV4, hook, devname, &hook_list, &ret);
-		mnl_nft_dump_nf(ctx, NFPROTO_IPV6, hook, devname, &hook_list, &ret);
-		mnl_nft_dump_nf(ctx, NFPROTO_BRIDGE, hook, devname, &hook_list, &ret);
-		break;
+		ret = mnl_nft_dump_nf_hooks(ctx, NFPROTO_ARP, hook, NULL);
+		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_INET, hook, devname);
+		if (tmp == 0)
+			ret = 0;
+		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_BRIDGE, hook, NULL);
+		if (tmp == 0)
+			ret = 0;
+		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_NETDEV, hook, devname);
+		if (tmp == 0)
+			ret = 0;
+
+		return ret;
 	case NFPROTO_INET:
-		mnl_nft_dump_nf(ctx, NFPROTO_IPV4, hook, devname, &hook_list, &ret);
-		mnl_nft_dump_nf(ctx, NFPROTO_IPV6, hook, devname, &hook_list, &ret);
-		break;
+		ret = mnl_nft_dump_nf_hooks(ctx, NFPROTO_IPV4, hook, devname);
+		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_IPV6, hook, devname);
+		if (tmp == 0)
+			ret = 0;
+
+		return ret;
 	case NFPROTO_IPV4:
 	case NFPROTO_IPV6:
 	case NFPROTO_BRIDGE:
-		mnl_nft_dump_nf(ctx, family, hook, devname, &hook_list, &ret);
+		ret = mnl_nft_dump_nf(ctx, family, hook, devname, &hook_list);
 		break;
 	case NFPROTO_ARP:
-		mnl_nft_dump_nf_arp(ctx, family, hook, devname, &hook_list, &ret);
+		ret = mnl_nft_dump_nf_arp(ctx, family, hook, devname, &hook_list);
 		break;
 	case NFPROTO_NETDEV:
-		mnl_nft_dump_nf_netdev(ctx, family, hook, devname, &hook_list, &ret);
-		break;
-	}
-
-	switch (family) {
-	case NFPROTO_UNSPEC:
-		for (i = 0; i < HOOK_FAMILY_MAX; i++)
-			print_hooks(ctx, hook_family[i], &hook_list);
-		break;
-	case NFPROTO_INET:
-		print_hooks(ctx, NFPROTO_IPV4, &hook_list);
-		print_hooks(ctx, NFPROTO_IPV6, &hook_list);
-		break;
-	default:
-		print_hooks(ctx, family, &hook_list);
+		ret = mnl_nft_dump_nf_netdev(ctx, family, hook, devname, &hook_list);
 		break;
 	}
 
+	print_hooks(ctx, family, &hook_list);
 	release_hook_list(&hook_list);
-	ret = 0;
 
 	return ret;
 }
-- 
2.44.2

[PATCH nft v2 2/5] src: mnl: make family specification more strict when listing

[Florian Westphal]

make "nft list hooks <family>" more strict.

nft list hooks: query/list all NFPROTO_XXX values, i.e.
arp, bridge, ipv4, ipv6.

If a device is also given, then do include the netdev family for
the given device as well.

"nft list hooks arp" will only dump the hooks registered
for NFPROTO_ARP (or nothing at all if none are active).

"bridge", "ip", "ip6" will list the pre/in/forward/output/postrouting
hooks for these families, if any.

"inet" serves as an alias for "ip" and "ip6".

Link: https://lore.kernel.org/netfilter-devel/20240729153211.GA26048@breakpoint.cc/
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/mnl.c | 53 ++++++++++++++++++++++++-----------------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/src/mnl.c b/src/mnl.c
index e4bbbcf6d536..88475ef4c25e 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -2391,25 +2391,6 @@ static int dump_nf_hooks(const struct nlmsghdr *nlh, void *_data)
 
 	hook->family = nfg->nfgen_family;
 
-	/* Netdev hooks potentially interfer with this family datapath. */
-	if (hook->family == NFPROTO_NETDEV) {
-		switch (data->family) {
-		case NFPROTO_IPV4:
-		case NFPROTO_IPV6:
-		case NFPROTO_INET:
-		case NFPROTO_BRIDGE:
-			hook->family = data->family;
-			hook->num = NF_INET_INGRESS;
-			break;
-		case NFPROTO_ARP:
-			if (hook->chain_family == NFPROTO_NETDEV) {
-				hook->family = data->family;
-				hook->num = __NF_ARP_INGRESS;
-			}
-			break;
-		}
-	}
-
 	basehook_list_add_tail(hook, data->hook_list);
 
 	return MNL_CB_OK;
@@ -2523,9 +2504,6 @@ static int mnl_nft_dump_nf(struct netlink_ctx *ctx, int family, int hook,
 {
 	int i, err;
 
-	/* show ingress in first place in hook listing. */
-	err = __mnl_nft_dump_nf_hooks(ctx, family, NFPROTO_NETDEV, NF_NETDEV_INGRESS, devname, hook_list);
-
 	for (i = 0; i <= NF_INET_POST_ROUTING; i++) {
 		int tmp;
 
@@ -2566,6 +2544,12 @@ static void release_hook_list(struct list_head *hook_list)
 		basehook_free(hook);
 }
 
+static void warn_if_device(struct nft_ctx *nft, const char *devname)
+{
+	if (devname)
+		nft_print(&nft->output, "# device keyword (%s) unexpected for this family\n", devname);
+}
+
 int mnl_nft_dump_nf_hooks(struct netlink_ctx *ctx, int family, int hook, const char *devname)
 {
 	LIST_HEAD(hook_list);
@@ -2576,30 +2560,41 @@ int mnl_nft_dump_nf_hooks(struct netlink_ctx *ctx, int family, int hook, const c
 	switch (family) {
 	case NFPROTO_UNSPEC:
 		ret = mnl_nft_dump_nf_hooks(ctx, NFPROTO_ARP, hook, NULL);
-		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_INET, hook, devname);
+		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_INET, hook, NULL);
 		if (tmp == 0)
 			ret = 0;
 		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_BRIDGE, hook, NULL);
 		if (tmp == 0)
 			ret = 0;
-		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_NETDEV, hook, devname);
-		if (tmp == 0)
-			ret = 0;
+
+		if (devname) {
+			tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_NETDEV, hook, devname);
+			if (tmp == 0)
+				ret = 0;
+		}
 
 		return ret;
 	case NFPROTO_INET:
-		ret = mnl_nft_dump_nf_hooks(ctx, NFPROTO_IPV4, hook, devname);
-		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_IPV6, hook, devname);
+		ret = 0;
+		if (devname)
+			ret = __mnl_nft_dump_nf_hooks(ctx, family, NFPROTO_NETDEV,
+						      NF_NETDEV_INGRESS, devname, &hook_list);
+		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_IPV4, hook, NULL);
+		if (tmp == 0)
+			ret = 0;
+		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_IPV6, hook, NULL);
 		if (tmp == 0)
 			ret = 0;
 
-		return ret;
+		break;
 	case NFPROTO_IPV4:
 	case NFPROTO_IPV6:
 	case NFPROTO_BRIDGE:
+		warn_if_device(ctx->nft, devname);
 		ret = mnl_nft_dump_nf(ctx, family, hook, devname, &hook_list);
 		break;
 	case NFPROTO_ARP:
+		warn_if_device(ctx->nft, devname);
 		ret = mnl_nft_dump_nf_arp(ctx, family, hook, devname, &hook_list);
 		break;
 	case NFPROTO_NETDEV:
-- 
2.44.2

[PATCH nft v2 3/5] src: drop obsolete hook argument form hook dump functions

[Florian Westphal]

since commit b98fee20bfe2 ("mnl: revisit hook listing"), handle.chain is
never set in this path, so 'hook' is always set to -1, so the hook arg
can be dropped.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/mnl.h |  2 +-
 src/mnl.c     | 26 +++++++++++++-------------
 src/rule.c    |  6 +-----
 3 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/include/mnl.h b/include/mnl.h
index cd5a2053b166..c9502f328f1c 100644
--- a/include/mnl.h
+++ b/include/mnl.h
@@ -90,7 +90,7 @@ int mnl_nft_flowtable_add(struct netlink_ctx *ctx, struct cmd *cmd,
 			  unsigned int flags);
 int mnl_nft_flowtable_del(struct netlink_ctx *ctx, struct cmd *cmd);
 
-int mnl_nft_dump_nf_hooks(struct netlink_ctx *ctx, int family, int hook,
+int mnl_nft_dump_nf_hooks(struct netlink_ctx *ctx, int family,
 			  const char *devname);
 
 int mnl_nft_event_listener(struct mnl_socket *nf_sock, unsigned int debug_mask,
diff --git a/src/mnl.c b/src/mnl.c
index 88475ef4c25e..1b424e427124 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -2499,7 +2499,7 @@ static void print_hooks(struct netlink_ctx *ctx, int family, struct list_head *h
 	fprintf(fp, "}\n");
 }
 
-static int mnl_nft_dump_nf(struct netlink_ctx *ctx, int family, int hook,
+static int mnl_nft_dump_nf(struct netlink_ctx *ctx, int family,
 			   const char *devname, struct list_head *hook_list)
 {
 	int i, err;
@@ -2515,7 +2515,7 @@ static int mnl_nft_dump_nf(struct netlink_ctx *ctx, int family, int hook,
 	return err;
 }
 
-static int mnl_nft_dump_nf_arp(struct netlink_ctx *ctx, int family, int hook,
+static int mnl_nft_dump_nf_arp(struct netlink_ctx *ctx, int family,
 			       const char *devname, struct list_head *hook_list)
 {
 	int err1, err2;
@@ -2526,7 +2526,7 @@ static int mnl_nft_dump_nf_arp(struct netlink_ctx *ctx, int family, int hook,
 	return err1 ? err2 : err1;
 }
 
-static int mnl_nft_dump_nf_netdev(struct netlink_ctx *ctx, int family, int hook,
+static int mnl_nft_dump_nf_netdev(struct netlink_ctx *ctx, int family,
 				  const char *devname, struct list_head *hook_list)
 {
 	int err;
@@ -2550,7 +2550,7 @@ static void warn_if_device(struct nft_ctx *nft, const char *devname)
 		nft_print(&nft->output, "# device keyword (%s) unexpected for this family\n", devname);
 }
 
-int mnl_nft_dump_nf_hooks(struct netlink_ctx *ctx, int family, int hook, const char *devname)
+int mnl_nft_dump_nf_hooks(struct netlink_ctx *ctx, int family, const char *devname)
 {
 	LIST_HEAD(hook_list);
 	int ret = -1, tmp;
@@ -2559,16 +2559,16 @@ int mnl_nft_dump_nf_hooks(struct netlink_ctx *ctx, int family, int hook, const c
 
 	switch (family) {
 	case NFPROTO_UNSPEC:
-		ret = mnl_nft_dump_nf_hooks(ctx, NFPROTO_ARP, hook, NULL);
-		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_INET, hook, NULL);
+		ret = mnl_nft_dump_nf_hooks(ctx, NFPROTO_ARP, NULL);
+		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_INET, NULL);
 		if (tmp == 0)
 			ret = 0;
-		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_BRIDGE, hook, NULL);
+		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_BRIDGE, NULL);
 		if (tmp == 0)
 			ret = 0;
 
 		if (devname) {
-			tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_NETDEV, hook, devname);
+			tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_NETDEV, devname);
 			if (tmp == 0)
 				ret = 0;
 		}
@@ -2579,10 +2579,10 @@ int mnl_nft_dump_nf_hooks(struct netlink_ctx *ctx, int family, int hook, const c
 		if (devname)
 			ret = __mnl_nft_dump_nf_hooks(ctx, family, NFPROTO_NETDEV,
 						      NF_NETDEV_INGRESS, devname, &hook_list);
-		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_IPV4, hook, NULL);
+		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_IPV4, NULL);
 		if (tmp == 0)
 			ret = 0;
-		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_IPV6, hook, NULL);
+		tmp = mnl_nft_dump_nf_hooks(ctx, NFPROTO_IPV6, NULL);
 		if (tmp == 0)
 			ret = 0;
 
@@ -2591,14 +2591,14 @@ int mnl_nft_dump_nf_hooks(struct netlink_ctx *ctx, int family, int hook, const c
 	case NFPROTO_IPV6:
 	case NFPROTO_BRIDGE:
 		warn_if_device(ctx->nft, devname);
-		ret = mnl_nft_dump_nf(ctx, family, hook, devname, &hook_list);
+		ret = mnl_nft_dump_nf(ctx, family, devname, &hook_list);
 		break;
 	case NFPROTO_ARP:
 		warn_if_device(ctx->nft, devname);
-		ret = mnl_nft_dump_nf_arp(ctx, family, hook, devname, &hook_list);
+		ret = mnl_nft_dump_nf_arp(ctx, family, devname, &hook_list);
 		break;
 	case NFPROTO_NETDEV:
-		ret = mnl_nft_dump_nf_netdev(ctx, family, hook, devname, &hook_list);
+		ret = mnl_nft_dump_nf_netdev(ctx, family, devname, &hook_list);
 		break;
 	}
 
diff --git a/src/rule.c b/src/rule.c
index 545f9b2b5463..0f92ef532ece 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -2341,12 +2341,8 @@ static int do_list_set(struct netlink_ctx *ctx, struct cmd *cmd,
 static int do_list_hooks(struct netlink_ctx *ctx, struct cmd *cmd)
 {
 	const char *devname = cmd->handle.obj.name;
-	int hooknum = -1;
 
-	if (cmd->handle.chain.name)
-		hooknum = cmd->handle.chain_id;
-
-	return mnl_nft_dump_nf_hooks(ctx, cmd->handle.family, hooknum, devname);
+	return mnl_nft_dump_nf_hooks(ctx, cmd->handle.family, devname);
 }
 
 static int do_command_list(struct netlink_ctx *ctx, struct cmd *cmd)
-- 
2.44.2

[PATCH nft v2 4/5] src: add egress support for 'list hooks'

[Florian Westphal]

This was missing:  Also include the egress hooks when listing
the netdev family (or unspec).

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/mnl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/mnl.c b/src/mnl.c
index 1b424e427124..3cacb47e7242 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -2529,11 +2529,12 @@ static int mnl_nft_dump_nf_arp(struct netlink_ctx *ctx, int family,
 static int mnl_nft_dump_nf_netdev(struct netlink_ctx *ctx, int family,
 				  const char *devname, struct list_head *hook_list)
 {
-	int err;
+	int err1, err2;
 
-	err = __mnl_nft_dump_nf_hooks(ctx, family, NFPROTO_NETDEV, NF_NETDEV_INGRESS, devname, hook_list);
+	err1 = __mnl_nft_dump_nf_hooks(ctx, family, NFPROTO_NETDEV, NF_NETDEV_INGRESS, devname, hook_list);
+	err2 = __mnl_nft_dump_nf_hooks(ctx, family, NFPROTO_NETDEV, NF_NETDEV_EGRESS, devname, hook_list);
 
-	return err;
+	return err1 ? err2 : err1;
 }
 
 static void release_hook_list(struct list_head *hook_list)
-- 
2.44.2

[PATCH nft v2 5/5] doc: add documentation about list hooks feature

[Florian Westphal]

Add a brief segment about 'nft list hooks' and a summary
of the output format.

As nft.txt is quite large, split the additonal commands
into their own file.

The existing listing section is removed; list subcommand is
already mentioned in the relevant statement sections.

Reported-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Makefile.am                 |   1 +
 doc/additional-commands.txt | 116 ++++++++++++++++++++++++++++++++++++
 doc/nft.txt                 |  63 +-------------------
 3 files changed, 118 insertions(+), 62 deletions(-)
 create mode 100644 doc/additional-commands.txt

diff --git a/Makefile.am b/Makefile.am
index d5ae1f66755a..fb64105dda88 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -321,6 +321,7 @@ A2X_OPTS_MANPAGE = \
 ASCIIDOC_MAIN = doc/nft.txt
 
 ASCIIDOC_INCLUDES = \
+	doc/additional-commands.txt \
 	doc/data-types.txt \
 	doc/payload-expression.txt \
 	doc/primary-expression.txt \
diff --git a/doc/additional-commands.txt b/doc/additional-commands.txt
new file mode 100644
index 000000000000..9ad338f8c7d1
--- /dev/null
+++ b/doc/additional-commands.txt
@@ -0,0 +1,116 @@
+LIST HOOKS
+~~~~~~~~~~
+
+This shows the list of functions that have been registered for the
+given protocol family, including functions that have been
+registered implicitly by kernel modules such as nf_conntrack. +
+
+[verse]
+____
+*list hooks* ['family']
+*list hooks netdev device* 'DEVICE_NAME'
+____
+
+*list hooks* is enough to display everything that is active
+on the system, however, it does currently omit hooks that are
+tied to a specific network device (netdev family). To obtain
+those, the network device needs to be queried by name.
+Example Usage:
+
+.List all active netfilter hooks in either the ip or ip6 stack
+--------------------------------------------------------------
+% nft list hooks inet
+family ip {
+        hook prerouting {
+                -0000000400 ipv4_conntrack_defrag [nf_defrag_ipv4]
+                -0000000200 ipv4_conntrack_in [nf_conntrack]
+                -0000000100 nf_nat_ipv4_pre_routing [nf_nat]
+        }
+        hook input {
+                 0000000000 chain inet filter input [nf_tables]
+                +0000000100 nf_nat_ipv4_local_in [nf_nat]
+[..]
+--------------------------------------------------------------
+
+The above shows a host that has nat, conntrack and ipv4 packet
+defragmentation enabled.
+For each hook location for the queried family a list of active hooks
+using the format +
+
+*priority* *identifier* [*module_name*]
+
+will be shown.
+
+The *priority* value dictates the order in which the hooks are called.
+The list is sorted, the lowest number is run first.
+
+The priority value of hooks registered by the kernel cannot be changed.
+For basechains registered by nftables, this value corresponds to the
+*priority* value specified in the base chain definition.
+
+After the numerical value, information about the hook is shown.
+For basechains defined in nftables this includes the table family,
+the table name and the basechains name.
+For hooks coming from kernel modules, the function name is used
+instead.
+
+If a *module name* is given, the hook was registered by the kernel
+module with this name.  You can use 'modinfo *module name*' to
+obtain more information about the module.
+
+This functionality requires a kernel built with the option +
+CONFIG_NETFILTER_NETLINK_HOOK
+enabled, either as a module or builtin. The module is named
+*nfnetlink_hook*.
+
+MONITOR
+~~~~~~~
+The monitor command allows you to listen to Netlink events produced by the
+nf_tables subsystem. These are either related to creation and deletion of
+objects or to packets for which *meta nftrace* was enabled. When they
+occur, nft will print to stdout the monitored events in either JSON or
+native nft format. +
+
+[verse]
+____
+*monitor* [*new* | *destroy*] 'MONITOR_OBJECT'
+*monitor* *trace*
+
+'MONITOR_OBJECT' := *tables* | *chains* | *sets* | *rules* | *elements* | *ruleset*
+____
+
+To filter events related to a concrete object, use one of the keywords in
+'MONITOR_OBJECT'.
+
+To filter events related to a concrete action, use keyword *new* or *destroy*.
+
+The second form of invocation takes no further options and exclusively prints
+events generated for packets with *nftrace* enabled.
+
+Hit ^C to finish the monitor operation.
+
+.Listen to all events, report in native nft format
+--------------------------------------------------
+% nft monitor
+--------------------------------------------------
+
+.Listen to deleted rules, report in JSON format
+-----------------------------------------------
+% nft -j monitor destroy rules
+-----------------------------------------------
+
+.Listen to both new and destroyed chains, in native nft format
+-----------------------------------------------------------------
+% nft monitor chains
+-------------------------------
+
+.Listen to ruleset events such as table, chain, rule, set, counters and quotas, in native nft format
+----------------------------------------------------------------------------------------------------
+% nft monitor ruleset
+---------------------
+
+.Trace incoming packets from host 10.0.0.1
+------------------------------------------
+% nft add rule filter input ip saddr 10.0.0.1 meta nftrace set 1
+% nft monitor trace
+------------------------------------------
diff --git a/doc/nft.txt b/doc/nft.txt
index 3f4593a29831..7e8c8695522d 100644
--- a/doc/nft.txt
+++ b/doc/nft.txt
@@ -766,17 +766,6 @@ and subtraction can be used to set relative priority, e.g. filter + 5 equals to
 *destroy*:: Delete the specified flowtable, it does not fail if it does not exist.
 *list*:: List all flowtables.
 
-LISTING
--------
-[verse]
-*list { secmarks | synproxys | flow tables | meters | hooks }* ['family']
-*list { secmarks | synproxys | flow tables | meters | hooks } table* ['family'] 'table'
-*list ct { timeout | expectation | helper | helpers } table* ['family'] 'table'
-
-Inspect configured objects.
-*list hooks* shows the full hook pipeline, including those registered by
-kernel modules, such as nf_conntrack.
-
 STATEFUL OBJECTS
 ----------------
 [verse]
@@ -908,57 +897,7 @@ ADDITIONAL COMMANDS
 -------------------
 These are some additional commands included in nft.
 
-MONITOR
-~~~~~~~~
-The monitor command allows you to listen to Netlink events produced by the
-nf_tables subsystem. These are either related to creation and deletion of
-objects or to packets for which *meta nftrace* was enabled. When they
-occur, nft will print to stdout the monitored events in either JSON or
-native nft format. +
-
-[verse]
-____
-*monitor* [*new* | *destroy*] 'MONITOR_OBJECT'
-*monitor* *trace*
-
-'MONITOR_OBJECT' := *tables* | *chains* | *sets* | *rules* | *elements* | *ruleset*
-____
-
-To filter events related to a concrete object, use one of the keywords in
-'MONITOR_OBJECT'.
-
-To filter events related to a concrete action, use keyword *new* or *destroy*.
-
-The second form of invocation takes no further options and exclusively prints
-events generated for packets with *nftrace* enabled.
-
-Hit ^C to finish the monitor operation.
-
-.Listen to all events, report in native nft format
---------------------------------------------------
-% nft monitor
---------------------------------------------------
-
-.Listen to deleted rules, report in JSON format
------------------------------------------------
-% nft -j monitor destroy rules
------------------------------------------------
-
-.Listen to both new and destroyed chains, in native nft format
------------------------------------------------------------------
-% nft monitor chains
--------------------------------
-
-.Listen to ruleset events such as table, chain, rule, set, counters and quotas, in native nft format
-----------------------------------------------------------------------------------------------------
-% nft monitor ruleset
----------------------
-
-.Trace incoming packets from host 10.0.0.1
-------------------------------------------
-% nft add rule filter input ip saddr 10.0.0.1 meta nftrace set 1
-% nft monitor trace
-------------------------------------------
+include::additional-commands.txt[]
 
 ERROR REPORTING
 ---------------
-- 
2.44.2

Re: [PATCH nft v2 0/5] src: mnl: rework list hooks infra

[Pablo Neira Ayuso]

Hi Florian,

A few suggestions:

- nft list hooks could probably take 'ip' family as default for
  consistency with other commands? There is 'nft list ruleset' which
  is special because it is family agnostic. Otherwise print all
  hooks with 'nft list hooks'? leaving netdev out of the picture
  unless 'device' is specified. I guess this approach you follow
  is to be conservative with the existing behaviour.

- Maybe plain reject 'device' for arp and bridge families?

  nft list hooks arp device enp0s25
  # device keyword (enp0s25) unexpected for this family

  instead of displaying a warning? I guess you are being conservative
  again here, that is fine.

- I understand you don't like the inet/ingress hack. It is there to
  address the shortcomming of not allowing sets to be shared accross
  families, and it does not even address it fully. I admit it is not
  the best approach, I'd like to explore better ones to address the
  need for set sharing.

  Going back to your approach: this is a bit low level, it exposes
  internal implementation details, such as the inet syntactic sugar
  (this is all hidden behind after the {register,unregister}_hook API).
  I understand that my attempt to describe the pipeline was
  incomplete, but I still wonder if, from user POV, it might makes
  sense at least to show the inet/ingress hook when listing the inet
  family to give an idea of what hook is registered according to
  priority. I would still show the inet/ingress in netdev family too
  (yes, it would be redundant).

Documentation looks good.

Thanks!

On Wed, Jul 31, 2024 at 06:51:00PM +0200, Florian Westphal wrote:
> Turns out that not only was 'nft list hooks' mostly undocumented,
> there was also confusion on what it should do.
> 
> First, clean this code up and make it strictly a tool to dump
> the NFPROTO_X registered functions.
> 
> Then, remove the 'hook' function argument, this was still passed
> from back in the day when one could ask to only dump e.g.
> ipv4 prerouting.  This ability is of little value, so don't restore
> this but instead just remove the leftover code.
> 
> Next, allow dumping of netdev:egress hooks.
> Lastly, document this in more detail and make it clear that this
> dumps the netfilter hooks registered for the protocol families,
> and nothing else.
> 
> Once this gets applied I intend to make
> 'nft list hooks netdev'
> 
> dump device hooks for all interfaces, if any, instead of a
> 'no device provided' warning.
> 
> Florian Westphal (5):
>   src: mnl: clean up hook listing code
>   src: mnl: make family specification more strict when listing
>   src: drop obsolete hook argument form hook dump functions
>   src: add egress support for 'list hooks'
>   doc: add documentation about list hooks feature
> 
>  Makefile.am                 |   1 +
>  doc/additional-commands.txt | 116 ++++++++++++++++++++++++++++
>  doc/nft.txt                 |  63 +--------------
>  include/mnl.h               |   2 +-
>  src/mnl.c                   | 150 ++++++++++++++----------------------
>  src/rule.c                  |   6 +-
>  6 files changed, 179 insertions(+), 159 deletions(-)
>  create mode 100644 doc/additional-commands.txt
> 
> -- 
> 2.44.2
> 
>