[PATCH xtables-nft 0/3] xt-monitor fixes

[PATCH xtables-nft 0/3] xt-monitor fixes

[Florian Westphal]

rule tracing via xt-monitor has various bugs:
 1. It prints unrelated rules because the function supposed
    to print the traced rule does a dump instead of a handle
    lookup.  This prints all rules in the chain instead of just one.
 2. Print the table family, not whatever family user provided on command line.
 3. The packet shoud be printed first, instead of after the first
    trace event.
 4. also make sure to flush stdout after each event so stdout redirect
    to files/pipes etc. works.

After this the output is much more similar to nft monitor, just in
xt rule format.

Florian Westphal (3):
  xtables-monitor: fix rule printing
  xtables-monitor: fix packet family protocol
  xtables-monitor: print packet first

 iptables/xtables-monitor.c | 70 ++++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 30 deletions(-)

-- 
2.28.0

[PATCH xtables-nft 1/3] xtables-monitor: fix rule printing

[Florian Westphal]

trace_print_rule does a rule dump.  This prints unrelated rules
in the same chain.  Instead the function should only request the
specific handle.

Furthermore, flush output buffer afterwards so this plays nice when
output isn't a terminal.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 iptables/xtables-monitor.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/iptables/xtables-monitor.c b/iptables/xtables-monitor.c
index 4008cc00d469..364e600e1b38 100644
--- a/iptables/xtables-monitor.c
+++ b/iptables/xtables-monitor.c
@@ -227,12 +227,12 @@ static void trace_print_rule(const struct nftnl_trace *nlt, struct cb_arg *args)
 		exit(EXIT_FAILURE);
 	}
 
-	nlh = nftnl_chain_nlmsg_build_hdr(buf, NFT_MSG_GETRULE, family, NLM_F_DUMP, 0);
+	nlh = nftnl_chain_nlmsg_build_hdr(buf, NFT_MSG_GETRULE, family, 0, 0);
 
         nftnl_rule_set_u32(r, NFTNL_RULE_FAMILY, family);
 	nftnl_rule_set_str(r, NFTNL_RULE_CHAIN, chain);
 	nftnl_rule_set_str(r, NFTNL_RULE_TABLE, table);
-	nftnl_rule_set_u64(r, NFTNL_RULE_POSITION, handle);
+	nftnl_rule_set_u64(r, NFTNL_RULE_HANDLE, handle);
 	nftnl_rule_nlmsg_build_payload(nlh, r);
 	nftnl_rule_free(r);
 
@@ -248,24 +248,21 @@ static void trace_print_rule(const struct nftnl_trace *nlt, struct cb_arg *args)
 	}
 
 	portid = mnl_socket_get_portid(nl);
-        if (mnl_socket_sendto(nl, nlh, nlh->nlmsg_len) < 0) {
-                perror("mnl_socket_send");
-                exit(EXIT_FAILURE);
-        }
+	if (mnl_socket_sendto(nl, nlh, nlh->nlmsg_len) < 0) {
+		perror("mnl_socket_send");
+		exit(EXIT_FAILURE);
+	}
 
 	ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
-        while (ret > 0) {
+	if (ret > 0) {
 		args->is_event = false;
-                ret = mnl_cb_run(buf, ret, 0, portid, rule_cb, args);
-                if (ret <= 0)
-                        break;
-                ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
-        }
-        if (ret == -1) {
-                perror("error");
-                exit(EXIT_FAILURE);
-        }
-        mnl_socket_close(nl);
+		ret = mnl_cb_run(buf, ret, 0, portid, rule_cb, args);
+	}
+	if (ret == -1) {
+		perror("error");
+		exit(EXIT_FAILURE);
+	}
+	mnl_socket_close(nl);
 }
 
 static void trace_print_packet(const struct nftnl_trace *nlt, struct cb_arg *args)
@@ -531,6 +528,7 @@ static int trace_cb(const struct nlmsghdr *nlh, struct cb_arg *arg)
 err_free:
 	nftnl_trace_free(nlt);
 err:
+	fflush(stdout);
 	return MNL_CB_OK;
 }
 
-- 
2.28.0

Re: [PATCH xtables-nft 1/3] xtables-monitor: fix rule printing

[Phil Sutter]

On Sat, Dec 12, 2020 at 04:15:32PM +0100, Florian Westphal wrote:
> trace_print_rule does a rule dump.  This prints unrelated rules
> in the same chain.  Instead the function should only request the
> specific handle.
> 
> Furthermore, flush output buffer afterwards so this plays nice when
> output isn't a terminal.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  iptables/xtables-monitor.c | 32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/iptables/xtables-monitor.c b/iptables/xtables-monitor.c
> index 4008cc00d469..364e600e1b38 100644
> --- a/iptables/xtables-monitor.c
> +++ b/iptables/xtables-monitor.c
> @@ -227,12 +227,12 @@ static void trace_print_rule(const struct nftnl_trace *nlt, struct cb_arg *args)
>  		exit(EXIT_FAILURE);
>  	}
>  
> -	nlh = nftnl_chain_nlmsg_build_hdr(buf, NFT_MSG_GETRULE, family, NLM_F_DUMP, 0);
> +	nlh = nftnl_chain_nlmsg_build_hdr(buf, NFT_MSG_GETRULE, family, 0, 0);
>  
>          nftnl_rule_set_u32(r, NFTNL_RULE_FAMILY, family);
>  	nftnl_rule_set_str(r, NFTNL_RULE_CHAIN, chain);
>  	nftnl_rule_set_str(r, NFTNL_RULE_TABLE, table);
> -	nftnl_rule_set_u64(r, NFTNL_RULE_POSITION, handle);
> +	nftnl_rule_set_u64(r, NFTNL_RULE_HANDLE, handle);
>  	nftnl_rule_nlmsg_build_payload(nlh, r);
>  	nftnl_rule_free(r);
>  
> @@ -248,24 +248,21 @@ static void trace_print_rule(const struct nftnl_trace *nlt, struct cb_arg *args)
>  	}
>  
>  	portid = mnl_socket_get_portid(nl);
> -        if (mnl_socket_sendto(nl, nlh, nlh->nlmsg_len) < 0) {
> -                perror("mnl_socket_send");
> -                exit(EXIT_FAILURE);
> -        }
> +	if (mnl_socket_sendto(nl, nlh, nlh->nlmsg_len) < 0) {
> +		perror("mnl_socket_send");
> +		exit(EXIT_FAILURE);
> +	}

Just in case someone else wonders as well: This does a whitespace
cleanup, replacing spaces by tabs. Later changes contain the same
cleanup, too.

Cheers, Phil

[PATCH xtables-nft 2/3] xtables-monitor: fix packet family protocol

[Florian Westphal]

This prints the family passed on the command line (which might be 0).
Print the table family instead.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 iptables/xtables-monitor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/iptables/xtables-monitor.c b/iptables/xtables-monitor.c
index 364e600e1b38..8850a12032d2 100644
--- a/iptables/xtables-monitor.c
+++ b/iptables/xtables-monitor.c
@@ -273,14 +273,14 @@ static void trace_print_packet(const struct nftnl_trace *nlt, struct cb_arg *arg
 	uint32_t mark;
 	char name[IFNAMSIZ];
 
-	printf("PACKET: %d %08x ", args->nfproto, nftnl_trace_get_u32(nlt, NFTNL_TRACE_ID));
+	family = nftnl_trace_get_u32(nlt, NFTNL_TRACE_FAMILY);
+	printf("PACKET: %d %08x ", family, nftnl_trace_get_u32(nlt, NFTNL_TRACE_ID));
 
 	if (nftnl_trace_is_set(nlt, NFTNL_TRACE_IIF))
 		printf("IN=%s ", if_indextoname(nftnl_trace_get_u32(nlt, NFTNL_TRACE_IIF), name));
 	if (nftnl_trace_is_set(nlt, NFTNL_TRACE_OIF))
 		printf("OUT=%s ", if_indextoname(nftnl_trace_get_u32(nlt, NFTNL_TRACE_OIF), name));
 
-	family = nftnl_trace_get_u32(nlt, NFTNL_TRACE_FAMILY);
 	nfproto = family;
 	if (nftnl_trace_is_set(nlt, NFTNL_TRACE_NFPROTO)) {
 		nfproto = nftnl_trace_get_u32(nlt, NFTNL_TRACE_NFPROTO);
-- 
2.28.0

[PATCH xtables-nft 3/3] xtables-monitor: print packet first

[Florian Westphal]

The trace mode should first print the packet that was received and
then the rule/verdict.

Furthermore, the monitor did sometimes print an extra newline.

After this patch, output is more consistent with nft monitor.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 iptables/xtables-monitor.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/iptables/xtables-monitor.c b/iptables/xtables-monitor.c
index 8850a12032d2..45a0d6bf1343 100644
--- a/iptables/xtables-monitor.c
+++ b/iptables/xtables-monitor.c
@@ -106,6 +106,7 @@ static int rule_cb(const struct nlmsghdr *nlh, void *data)
 		printf("-0 ");
 		break;
 	default:
+		puts("");
 		goto err_free;
 	}
 
@@ -433,9 +434,18 @@ static void trace_print_packet(const struct nftnl_trace *nlt, struct cb_arg *arg
 	mark = nftnl_trace_get_u32(nlt, NFTNL_TRACE_MARK);
 	if (mark)
 		printf("MARK=0x%x ", mark);
+	puts("");
+}
+
+static void trace_print_hdr(const struct nftnl_trace *nlt)
+{
+	printf(" TRACE: %d %08x %s:%s", nftnl_trace_get_u32(nlt, NFTNL_TABLE_FAMILY),
+					nftnl_trace_get_u32(nlt, NFTNL_TRACE_ID),
+					nftnl_trace_get_str(nlt, NFTNL_TRACE_TABLE),
+					nftnl_trace_get_str(nlt, NFTNL_TRACE_CHAIN));
 }
 
-static void print_verdict(struct nftnl_trace *nlt, uint32_t verdict)
+static void print_verdict(const struct nftnl_trace *nlt, uint32_t verdict)
 {
 	const char *chain;
 
@@ -496,35 +506,37 @@ static int trace_cb(const struct nlmsghdr *nlh, struct cb_arg *arg)
 	    arg->nfproto != nftnl_trace_get_u32(nlt, NFTNL_TABLE_FAMILY))
 		goto err_free;
 
-	printf(" TRACE: %d %08x %s:%s", nftnl_trace_get_u32(nlt, NFTNL_TABLE_FAMILY),
-					nftnl_trace_get_u32(nlt, NFTNL_TRACE_ID),
-					nftnl_trace_get_str(nlt, NFTNL_TRACE_TABLE),
-					nftnl_trace_get_str(nlt, NFTNL_TRACE_CHAIN));
-
 	switch (nftnl_trace_get_u32(nlt, NFTNL_TRACE_TYPE)) {
 	case NFT_TRACETYPE_RULE:
 		verdict = nftnl_trace_get_u32(nlt, NFTNL_TRACE_VERDICT);
-		printf(":rule:0x%llx:", (unsigned long long)nftnl_trace_get_u64(nlt, NFTNL_TRACE_RULE_HANDLE));
-		print_verdict(nlt, verdict);
 
-		if (nftnl_trace_is_set(nlt, NFTNL_TRACE_RULE_HANDLE))
-			trace_print_rule(nlt, arg);
 		if (nftnl_trace_is_set(nlt, NFTNL_TRACE_LL_HEADER) ||
 		    nftnl_trace_is_set(nlt, NFTNL_TRACE_NETWORK_HEADER))
 			trace_print_packet(nlt, arg);
+
+		if (nftnl_trace_is_set(nlt, NFTNL_TRACE_RULE_HANDLE)) {
+			trace_print_hdr(nlt);
+			printf(":rule:0x%llx:", (unsigned long long)nftnl_trace_get_u64(nlt, NFTNL_TRACE_RULE_HANDLE));
+			print_verdict(nlt, verdict);
+			printf(" ");
+			trace_print_rule(nlt, arg);
+		}
 		break;
 	case NFT_TRACETYPE_POLICY:
+		trace_print_hdr(nlt);
 		printf(":policy:");
 		verdict = nftnl_trace_get_u32(nlt, NFTNL_TRACE_POLICY);
 
 		print_verdict(nlt, verdict);
+		puts("");
 		break;
 	case NFT_TRACETYPE_RETURN:
+		trace_print_hdr(nlt);
 		printf(":return:");
 		trace_print_return(nlt);
+		puts("");
 		break;
 	}
-	puts("");
 err_free:
 	nftnl_trace_free(nlt);
 err:
-- 
2.28.0

Re: [PATCH xtables-nft 3/3] xtables-monitor: print packet first

[Phil Sutter]

On Sat, Dec 12, 2020 at 04:15:34PM +0100, Florian Westphal wrote:
> The trace mode should first print the packet that was received and
> then the rule/verdict.
> 
> Furthermore, the monitor did sometimes print an extra newline.
> 
> After this patch, output is more consistent with nft monitor.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  iptables/xtables-monitor.c | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/iptables/xtables-monitor.c b/iptables/xtables-monitor.c
> index 8850a12032d2..45a0d6bf1343 100644
> --- a/iptables/xtables-monitor.c
> +++ b/iptables/xtables-monitor.c
> @@ -106,6 +106,7 @@ static int rule_cb(const struct nlmsghdr *nlh, void *data)
>  		printf("-0 ");
>  		break;
>  	default:
> +		puts("");
>  		goto err_free;
>  	}
>  
> @@ -433,9 +434,18 @@ static void trace_print_packet(const struct nftnl_trace *nlt, struct cb_arg *arg
>  	mark = nftnl_trace_get_u32(nlt, NFTNL_TRACE_MARK);
>  	if (mark)
>  		printf("MARK=0x%x ", mark);
> +	puts("");
> +}
> +
> +static void trace_print_hdr(const struct nftnl_trace *nlt)
> +{
> +	printf(" TRACE: %d %08x %s:%s", nftnl_trace_get_u32(nlt, NFTNL_TABLE_FAMILY),
> +					nftnl_trace_get_u32(nlt, NFTNL_TRACE_ID),
> +					nftnl_trace_get_str(nlt, NFTNL_TRACE_TABLE),
> +					nftnl_trace_get_str(nlt, NFTNL_TRACE_CHAIN));
>  }
>  
> -static void print_verdict(struct nftnl_trace *nlt, uint32_t verdict)
> +static void print_verdict(const struct nftnl_trace *nlt, uint32_t verdict)
>  {
>  	const char *chain;
>  
> @@ -496,35 +506,37 @@ static int trace_cb(const struct nlmsghdr *nlh, struct cb_arg *arg)
>  	    arg->nfproto != nftnl_trace_get_u32(nlt, NFTNL_TABLE_FAMILY))
>  		goto err_free;
>  
> -	printf(" TRACE: %d %08x %s:%s", nftnl_trace_get_u32(nlt, NFTNL_TABLE_FAMILY),
> -					nftnl_trace_get_u32(nlt, NFTNL_TRACE_ID),
> -					nftnl_trace_get_str(nlt, NFTNL_TRACE_TABLE),
> -					nftnl_trace_get_str(nlt, NFTNL_TRACE_CHAIN));
> -
>  	switch (nftnl_trace_get_u32(nlt, NFTNL_TRACE_TYPE)) {
>  	case NFT_TRACETYPE_RULE:
>  		verdict = nftnl_trace_get_u32(nlt, NFTNL_TRACE_VERDICT);
> -		printf(":rule:0x%llx:", (unsigned long long)nftnl_trace_get_u64(nlt, NFTNL_TRACE_RULE_HANDLE));

Quite long long line here. ;)
How about using PRIx64 in the format string to avoid the cast?

> -		print_verdict(nlt, verdict);
>  
> -		if (nftnl_trace_is_set(nlt, NFTNL_TRACE_RULE_HANDLE))
> -			trace_print_rule(nlt, arg);
>  		if (nftnl_trace_is_set(nlt, NFTNL_TRACE_LL_HEADER) ||
>  		    nftnl_trace_is_set(nlt, NFTNL_TRACE_NETWORK_HEADER))
>  			trace_print_packet(nlt, arg);
> +
> +		if (nftnl_trace_is_set(nlt, NFTNL_TRACE_RULE_HANDLE)) {
> +			trace_print_hdr(nlt);
> +			printf(":rule:0x%llx:", (unsigned long long)nftnl_trace_get_u64(nlt, NFTNL_TRACE_RULE_HANDLE));

Same here.

Cheers, Phil

Re: [PATCH xtables-nft 3/3] xtables-monitor: print packet first

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> >  	switch (nftnl_trace_get_u32(nlt, NFTNL_TRACE_TYPE)) {
> >  	case NFT_TRACETYPE_RULE:
> >  		verdict = nftnl_trace_get_u32(nlt, NFTNL_TRACE_VERDICT);
> > -		printf(":rule:0x%llx:", (unsigned long long)nftnl_trace_get_u64(nlt, NFTNL_TRACE_RULE_HANDLE));
> 
> Quite long long line here. ;)
> How about using PRIx64 in the format string to avoid the cast?

Its just being moved from here...

> > -		print_verdict(nlt, verdict);
> >  
> > -		if (nftnl_trace_is_set(nlt, NFTNL_TRACE_RULE_HANDLE))
> > -			trace_print_rule(nlt, arg);
> >  		if (nftnl_trace_is_set(nlt, NFTNL_TRACE_LL_HEADER) ||
> >  		    nftnl_trace_is_set(nlt, NFTNL_TRACE_NETWORK_HEADER))
> >  			trace_print_packet(nlt, arg);
> > +
> > +		if (nftnl_trace_is_set(nlt, NFTNL_TRACE_RULE_HANDLE)) {
> > +			trace_print_hdr(nlt);
> > +			printf(":rule:0x%llx:", (unsigned long long)nftnl_trace_get_u64(nlt, NFTNL_TRACE_RULE_HANDLE));

To this location.  But sure, I can change it.