[nft PATCH 0/4] xt: Rewrite unsupported compat expression dumping

[nft PATCH 0/4] xt: Rewrite unsupported compat expression dumping

[Phil Sutter]

Alternative approach to my previous dump and restore support of xt
compat expressions:

If translation is not available or not successful, fall back to a
format which allows to be parsed easily.

When parsing, reject these expressions explicitly with a meaningful
error message.

Phil Sutter (4):
  xt: Delay libxtables access until translation
  xt: Purify enum nft_xt_type
  xt: Rewrite unsupported compat expression dumping
  xt: Fall back to generic printing from translation

 doc/libnftables-json.adoc |  18 +++-
 doc/statements.txt        |  17 ++++
 include/json.h            |   2 +
 include/parser.h          |   1 +
 include/statement.h       |  11 +-
 src/json.c                |  19 ++--
 src/parser_bison.y        |  18 ++++
 src/parser_json.c         |   5 +
 src/scanner.l             |   3 +
 src/statement.c           |   1 +
 src/xt.c                  | 207 +++++++++++++++-----------------------
 11 files changed, 163 insertions(+), 139 deletions(-)

-- 
2.38.0

[nft PATCH 1/4] xt: Delay libxtables access until translation

[Phil Sutter]

There is no point in spending efforts setting up the xt match/target
when it is not printed afterwards. So just store the statement data from
libnftnl in struct xt_stmt and perform the extension lookup from
xt_stmt_xlate() instead.

This means some data structures are only temporarily allocated for the
sake of passing to libxtables callbacks, no need to drag them around.
Also no need to clone the looked up extension, it is needed only to call
the functions it provides.

While being at it, select numeric output in xt_xlate_*_params -
otherwise there will be reverse DNS lookups which should not happen by
default.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/statement.h |   9 +--
 src/xt.c            | 192 ++++++++++++++++++--------------------------
 2 files changed, 80 insertions(+), 121 deletions(-)

diff --git a/include/statement.h b/include/statement.h
index 2a2d300106181..8651fc78892c9 100644
--- a/include/statement.h
+++ b/include/statement.h
@@ -264,12 +264,11 @@ struct xtables_target;
 struct xt_stmt {
 	const char			*name;
 	enum nft_xt_type		type;
+	uint32_t			rev;
+	uint32_t			family;
+	size_t				infolen;
+	void				*info;
 	uint32_t			proto;
-	union {
-		struct xtables_match	*match;
-		struct xtables_target	*target;
-	};
-	void				*entry;
 };
 
 extern struct stmt *xt_stmt_alloc(const struct location *loc);
diff --git a/src/xt.c b/src/xt.c
index a54173522c229..7880fa1bc6966 100644
--- a/src/xt.c
+++ b/src/xt.c
@@ -28,51 +28,94 @@
 
 #ifdef HAVE_LIBXTABLES
 #include <xtables.h>
+
+static void *xt_entry_alloc(const struct xt_stmt *xt, uint32_t af);
 #endif
 
 void xt_stmt_xlate(const struct stmt *stmt, struct output_ctx *octx)
 {
 #ifdef HAVE_LIBXTABLES
 	struct xt_xlate *xl = xt_xlate_alloc(10240);
+	struct xtables_target *tg;
+	struct xt_entry_target *t;
+	struct xtables_match *mt;
+	struct xt_entry_match *m;
+	size_t size;
+	void *entry;
+
+	xtables_set_nfproto(stmt->xt.family);
+	entry = xt_entry_alloc(&stmt->xt, stmt->xt.family);
 
 	switch (stmt->xt.type) {
 	case NFT_XT_MATCH:
-		if (stmt->xt.match->xlate) {
+		mt = xtables_find_match(stmt->xt.name, XTF_TRY_LOAD, NULL);
+		if (!mt) {
+			fprintf(stderr, "XT match %s not found\n",
+				stmt->xt.name);
+			return;
+		}
+		size = XT_ALIGN(sizeof(*m)) + stmt->xt.infolen;
+
+		m = xzalloc(size);
+		memcpy(&m->data, stmt->xt.info, stmt->xt.infolen);
+
+		m->u.match_size = size;
+		m->u.user.revision = stmt->xt.rev;
+
+		if (mt->xlate) {
 			struct xt_xlate_mt_params params = {
-				.ip		= stmt->xt.entry,
-				.match		= stmt->xt.match->m,
-				.numeric        = 0,
+				.ip		= entry,
+				.match		= m,
+				.numeric        = 1,
 			};
 
-			stmt->xt.match->xlate(xl, &params);
+			mt->xlate(xl, &params);
 			nft_print(octx, "%s", xt_xlate_get(xl));
-		} else if (stmt->xt.match->print) {
+		} else if (mt->print) {
 			printf("#");
-			stmt->xt.match->print(&stmt->xt.entry,
-					      stmt->xt.match->m, 0);
+			mt->print(&entry, m, 0);
 		}
+		xfree(m);
 		break;
 	case NFT_XT_WATCHER:
 	case NFT_XT_TARGET:
-		if (stmt->xt.target->xlate) {
+		tg = xtables_find_target(stmt->xt.name, XTF_TRY_LOAD);
+		if (!tg) {
+			fprintf(stderr, "XT target %s not found\n",
+				stmt->xt.name);
+			return;
+		}
+		size = XT_ALIGN(sizeof(*t)) + stmt->xt.infolen;
+
+		t = xzalloc(size);
+		memcpy(&t->data, stmt->xt.info, stmt->xt.infolen);
+
+		t->u.target_size = size;
+		t->u.user.revision = stmt->xt.rev;
+
+		strcpy(t->u.user.name, tg->name);
+
+		if (tg->xlate) {
 			struct xt_xlate_tg_params params = {
-				.ip		= stmt->xt.entry,
-				.target		= stmt->xt.target->t,
-				.numeric        = 0,
+				.ip		= entry,
+				.target		= t,
+				.numeric        = 1,
 			};
 
-			stmt->xt.target->xlate(xl, &params);
+			tg->xlate(xl, &params);
 			nft_print(octx, "%s", xt_xlate_get(xl));
-		} else if (stmt->xt.target->print) {
+		} else if (tg->print) {
 			printf("#");
-			stmt->xt.target->print(NULL, stmt->xt.target->t, 0);
+			tg->print(NULL, t, 0);
 		}
+		xfree(t);
 		break;
 	default:
 		break;
 	}
 
 	xt_xlate_free(xl);
+	xfree(entry);
 #else
 	nft_print(octx, "# xt_%s", stmt->xt.name);
 #endif
@@ -80,33 +123,12 @@ void xt_stmt_xlate(const struct stmt *stmt, struct output_ctx *octx)
 
 void xt_stmt_destroy(struct stmt *stmt)
 {
-#ifdef HAVE_LIBXTABLES
-	switch (stmt->xt.type) {
-	case NFT_XT_MATCH:
-		if (!stmt->xt.match)
-			break;
-		if (stmt->xt.match->m)
-			xfree(stmt->xt.match->m);
-		xfree(stmt->xt.match);
-		break;
-	case NFT_XT_WATCHER:
-	case NFT_XT_TARGET:
-		if (!stmt->xt.target)
-			break;
-		if (stmt->xt.target->t)
-			xfree(stmt->xt.target->t);
-		xfree(stmt->xt.target);
-		break;
-	default:
-		break;
-	}
-#endif
-	xfree(stmt->xt.entry);
 	xfree(stmt->xt.name);
+	xfree(stmt->xt.info);
 }
 
 #ifdef HAVE_LIBXTABLES
-static void *xt_entry_alloc(struct xt_stmt *xt, uint32_t af)
+static void *xt_entry_alloc(const struct xt_stmt *xt, uint32_t af)
 {
 	union nft_entry {
 		struct ipt_entry ipt;
@@ -173,24 +195,6 @@ static uint32_t xt_proto(const struct proto_ctx *pctx)
 
 	return 0;
 }
-
-static struct xtables_target *xt_target_clone(struct xtables_target *t)
-{
-	struct xtables_target *clone;
-
-	clone = xzalloc(sizeof(struct xtables_target));
-	memcpy(clone, t, sizeof(struct xtables_target));
-	return clone;
-}
-
-static struct xtables_match *xt_match_clone(struct xtables_match *m)
-{
-	struct xtables_match *clone;
-
-	clone = xzalloc(sizeof(struct xtables_match));
-	memcpy(clone, m, sizeof(struct xtables_match));
-	return clone;
-}
 #endif
 
 /*
@@ -201,43 +205,22 @@ void netlink_parse_match(struct netlink_parse_ctx *ctx,
 			 const struct location *loc,
 			 const struct nftnl_expr *nle)
 {
-	struct stmt *stmt;
-	const char *name;
-#ifdef HAVE_LIBXTABLES
-	struct xtables_match *mt;
 	const char *mtinfo;
-	struct xt_entry_match *m;
+	struct stmt *stmt;
 	uint32_t mt_len;
 
-	xtables_set_nfproto(ctx->table->handle.family);
-
-	name = nftnl_expr_get_str(nle, NFTNL_EXPR_MT_NAME);
-
-	mt = xtables_find_match(name, XTF_TRY_LOAD, NULL);
-	if (!mt) {
-		fprintf(stderr, "XT match %s not found\n", name);
-		return;
-	}
 	mtinfo = nftnl_expr_get(nle, NFTNL_EXPR_MT_INFO, &mt_len);
 
-	m = xzalloc(sizeof(struct xt_entry_match) + mt_len);
-	memcpy(&m->data, mtinfo, mt_len);
-
-	m->u.match_size = mt_len + XT_ALIGN(sizeof(struct xt_entry_match));
-	m->u.user.revision = nftnl_expr_get_u32(nle, NFTNL_EXPR_MT_REV);
-
 	stmt = xt_stmt_alloc(loc);
-	stmt->xt.name = strdup(name);
+	stmt->xt.name = strdup(nftnl_expr_get_str(nle, NFTNL_EXPR_MT_NAME));
 	stmt->xt.type = NFT_XT_MATCH;
-	stmt->xt.match = xt_match_clone(mt);
-	stmt->xt.match->m = m;
-#else
-	name = nftnl_expr_get_str(nle, NFTNL_EXPR_MT_NAME);
+	stmt->xt.rev = nftnl_expr_get_u32(nle, NFTNL_EXPR_MT_REV);
+	stmt->xt.family = ctx->table->handle.family;
+
+	stmt->xt.infolen = mt_len;
+	stmt->xt.info = xmalloc(mt_len);
+	memcpy(stmt->xt.info, mtinfo, mt_len);
 
-	stmt = xt_stmt_alloc(loc);
-	stmt->xt.name = strdup(name);
-	stmt->xt.type = NFT_XT_MATCH;
-#endif
 	ctx->table->has_xt_stmts = true;
 	rule_stmt_append(ctx->rule, stmt);
 }
@@ -246,44 +229,22 @@ void netlink_parse_target(struct netlink_parse_ctx *ctx,
 			  const struct location *loc,
 			  const struct nftnl_expr *nle)
 {
-	struct stmt *stmt;
-	const char *name;
-#ifdef HAVE_LIBXTABLES
-	struct xtables_target *tg;
 	const void *tginfo;
-	struct xt_entry_target *t;
-	size_t size;
+	struct stmt *stmt;
 	uint32_t tg_len;
 
-	xtables_set_nfproto(ctx->table->handle.family);
-
-	name = nftnl_expr_get_str(nle, NFTNL_EXPR_TG_NAME);
-	tg = xtables_find_target(name, XTF_TRY_LOAD);
-	if (!tg) {
-		fprintf(stderr, "XT target %s not found\n", name);
-		return;
-	}
 	tginfo = nftnl_expr_get(nle, NFTNL_EXPR_TG_INFO, &tg_len);
 
-	size = XT_ALIGN(sizeof(struct xt_entry_target)) + tg_len;
-	t = xzalloc(size);
-	memcpy(&t->data, tginfo, tg_len);
-	t->u.target_size = size;
-	t->u.user.revision = nftnl_expr_get_u32(nle, NFTNL_EXPR_TG_REV);
-	strcpy(t->u.user.name, tg->name);
-
 	stmt = xt_stmt_alloc(loc);
-	stmt->xt.name = strdup(name);
+	stmt->xt.name = strdup(nftnl_expr_get_str(nle, NFTNL_EXPR_TG_NAME));
 	stmt->xt.type = NFT_XT_TARGET;
-	stmt->xt.target = xt_target_clone(tg);
-	stmt->xt.target->t = t;
-#else
-	name = nftnl_expr_get_str(nle, NFTNL_EXPR_TG_NAME);
+	stmt->xt.rev = nftnl_expr_get_u32(nle, NFTNL_EXPR_TG_REV);
+	stmt->xt.family = ctx->table->handle.family;
+
+	stmt->xt.infolen = tg_len;
+	stmt->xt.info = xmalloc(tg_len);
+	memcpy(stmt->xt.info, tginfo, tg_len);
 
-	stmt = xt_stmt_alloc(loc);
-	stmt->xt.name = strdup(name);
-	stmt->xt.type = NFT_XT_TARGET;
-#endif
 	ctx->table->has_xt_stmts = true;
 	rule_stmt_append(ctx->rule, stmt);
 }
@@ -311,7 +272,6 @@ void stmt_xt_postprocess(struct rule_pp_ctx *rctx, struct stmt *stmt,
 		stmt->xt.type = NFT_XT_WATCHER;
 
 	stmt->xt.proto = xt_proto(&rctx->pctx);
-	stmt->xt.entry = xt_entry_alloc(&stmt->xt, rctx->pctx.family);
 }
 
 static int nft_xt_compatible_revision(const char *name, uint8_t rev, int opt)
-- 
2.38.0

Re: [nft PATCH 1/4] xt: Delay libxtables access until translation

[Pablo Neira Ayuso]

Hi Phil,

On Thu, Nov 24, 2022 at 05:56:38PM +0100, Phil Sutter wrote:
> There is no point in spending efforts setting up the xt match/target
> when it is not printed afterwards. So just store the statement data from
> libnftnl in struct xt_stmt and perform the extension lookup from
> xt_stmt_xlate() instead.

There is nft -i and nft monitor which keep a ruleset cache. Both are
sort of incomplete: nft -i resorts to cleaning up the cache based on
the generation number and nft monitor still needs to be updated to
keep track of incremental ruleset updates via netlink events. Sooner
or later these two will get better support for incremental ruleset
updates.

I mean, in those two cases, every call to print the translation will
trigger the allocation of the xt structures, fill them and then call
.xlate. I agree it is a bit more work, I guess this won't case any
noticeable penalty, but it might be work that needs to be done over
and over again when ruleset uses xt match / target.

Re: [nft PATCH 1/4] xt: Delay libxtables access until translation

[Phil Sutter]

Hi Pablo,


On Thu, Dec 08, 2022 at 10:21:02PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Nov 24, 2022 at 05:56:38PM +0100, Phil Sutter wrote:
> > There is no point in spending efforts setting up the xt match/target
> > when it is not printed afterwards. So just store the statement data from
> > libnftnl in struct xt_stmt and perform the extension lookup from
> > xt_stmt_xlate() instead.
> 
> There is nft -i and nft monitor which keep a ruleset cache. Both are
> sort of incomplete: nft -i resorts to cleaning up the cache based on
> the generation number and nft monitor still needs to be updated to
> keep track of incremental ruleset updates via netlink events. Sooner
> or later these two will get better support for incremental ruleset
> updates.
> 
> I mean, in those two cases, every call to print the translation will
> trigger the allocation of the xt structures, fill them and then call
> .xlate. I agree it is a bit more work, I guess this won't case any
> noticeable penalty, but it might be work that needs to be done over
> and over again when ruleset uses xt match / target.

So you're saying the overhead when printing a rule might be more
significant than when fetching it. I doubt this simply because the same
rule is usually printed at most once and there are multiple other
commands requiring a rule cache.

IMO we may also just leave the code as-is and wait for someone to
complain about bad performance with rulesets containing many compat
expressions. Depending on the actual report, we may also follow a hybrid
approach and do the match/target lookup only when needed and cache it
for later use.

My patch made most sense with an nft in mind which does not need xtables
support for saving/restoring compat expressions. Users depending on this
for whatever reason will execute the xlate code path in any case now.

Cheers, Phil

Re: [nft PATCH 1/4] xt: Delay libxtables access until translation

[Pablo Neira Ayuso]

On Fri, Dec 09, 2022 at 01:14:50AM +0100, Phil Sutter wrote:
> Hi Pablo,
> 
> 
> On Thu, Dec 08, 2022 at 10:21:02PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Nov 24, 2022 at 05:56:38PM +0100, Phil Sutter wrote:
> > > There is no point in spending efforts setting up the xt match/target
> > > when it is not printed afterwards. So just store the statement data from
> > > libnftnl in struct xt_stmt and perform the extension lookup from
> > > xt_stmt_xlate() instead.
> > 
> > There is nft -i and nft monitor which keep a ruleset cache. Both are
> > sort of incomplete: nft -i resorts to cleaning up the cache based on
> > the generation number and nft monitor still needs to be updated to
> > keep track of incremental ruleset updates via netlink events. Sooner
> > or later these two will get better support for incremental ruleset
> > updates.
> > 
> > I mean, in those two cases, every call to print the translation will
> > trigger the allocation of the xt structures, fill them and then call
> > .xlate. I agree it is a bit more work, I guess this won't case any
> > noticeable penalty, but it might be work that needs to be done over
> > and over again when ruleset uses xt match / target.
> 
> So you're saying the overhead when printing a rule might be more
> significant than when fetching it.

I'm saying that there might be scenarios where, once the xt
match/target is set up, we might call .xlate to print it not just once
(considering nft -i use case), but see below.

> I doubt this simply because the same rule is usually printed at most
> once and there are multiple other commands requiring a rule cache.
>
> IMO we may also just leave the code as-is and wait for someone to
> complain about bad performance with rulesets containing many compat
> expressions.
>
> Depending on the actual report, we may also follow a hybrid approach
> and do the match/target lookup only when needed and cache it for
> later use.
>
> My patch made most sense with an nft in mind which does not need xtables
> support for saving/restoring compat expressions. Users depending on this
> for whatever reason will execute the xlate code path in any case now.

OK, you are refering to commands that do not need ruleset listing, in
that case setting up the xt match/target structure makes no sense.

Your original patch description says:

> Also no need to clone the looked up extension, it is needed only to call
> the functions it provides.

I agree, removing this extra clone is good.

I think this patch is fine, the caching of the xt match/target setup
should be easy to do, struct stmt is const on that path, but it could
be overrided in this case.

[nft PATCH 2/4] xt: Purify enum nft_xt_type

[Phil Sutter]

Remove NFT_XT_MAX from the enum, it is not a valid xt type.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/statement.h | 2 +-
 src/xt.c            | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/statement.h b/include/statement.h
index 8651fc78892c9..e648fb137b740 100644
--- a/include/statement.h
+++ b/include/statement.h
@@ -255,8 +255,8 @@ enum nft_xt_type {
 	NFT_XT_MATCH = 0,
 	NFT_XT_TARGET,
 	NFT_XT_WATCHER,
-	NFT_XT_MAX
 };
+#define NFT_XT_MAX	(NFT_XT_WATCHER + 1)
 
 struct xtables_match;
 struct xtables_target;
diff --git a/src/xt.c b/src/xt.c
index 7880fa1bc6966..300416a1e8d92 100644
--- a/src/xt.c
+++ b/src/xt.c
@@ -110,8 +110,6 @@ void xt_stmt_xlate(const struct stmt *stmt, struct output_ctx *octx)
 		}
 		xfree(t);
 		break;
-	default:
-		break;
 	}
 
 	xt_xlate_free(xl);
-- 
2.38.0

[nft PATCH 3/4] xt: Rewrite unsupported compat expression dumping

[Phil Sutter]

Choose a format which provides more information and is easily parseable.
Then teach parsers about it and make it explicitly reject the ruleset
giving a meaningful explanation. Also update the man pages with some
more details.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 doc/libnftables-json.adoc | 18 +++++++++++++++---
 doc/statements.txt        | 17 +++++++++++++++++
 include/json.h            |  2 ++
 include/parser.h          |  1 +
 src/json.c                | 19 +++++++++++++------
 src/parser_bison.y        | 18 ++++++++++++++++++
 src/parser_json.c         |  5 +++++
 src/scanner.l             |  3 +++
 src/statement.c           |  1 +
 src/xt.c                  |  8 +++++++-
 10 files changed, 82 insertions(+), 10 deletions(-)

diff --git a/doc/libnftables-json.adoc b/doc/libnftables-json.adoc
index bb59945fc510d..d985149a0af35 100644
--- a/doc/libnftables-json.adoc
+++ b/doc/libnftables-json.adoc
@@ -1059,10 +1059,22 @@ Assign connection tracking expectation.
 
 === XT
 [verse]
-*{ "xt": null }*
+____
+*{ "xt": {
+	"type":* 'TYPENAME'*,
+	"name":* 'STRING'
+*}}*
+
+'TYPENAME' := *match* | *target* | *watcher*
+____
+
+This represents an xt statement from xtables compat interface. It is a
+fallback if translation is not available or not complete.
+
+Seeing this means the ruleset (or parts of it) were created by *iptables-nft*
+and one should use that to manage it.
 
-This represents an xt statement from xtables compat interface. Sadly, at this
-point, it is not possible to provide any further information about its content.
+*BEWARE:* nftables won't restore these statements.
 
 == EXPRESSIONS
 Expressions are the building blocks of (most) statements. In their most basic
diff --git a/doc/statements.txt b/doc/statements.txt
index 8076c21cded41..6c0baf92dab27 100644
--- a/doc/statements.txt
+++ b/doc/statements.txt
@@ -783,3 +783,20 @@ ____
 # jump to different chains depending on layer 4 protocol type:
 nft add rule ip filter input ip protocol vmap { tcp : jump tcp-chain, udp : jump udp-chain , icmp : jump icmp-chain }
 ------------------------
+
+XT STATEMENT
+~~~~~~~~~~~~
+This represents an xt statement from xtables compat interface. It is a
+fallback if translation is not available or not complete.
+
+[verse]
+____
+*xt* 'TYPE' 'NAME'
+
+'TYPE' := *match* | *target* | *watcher*
+____
+
+Seeing this means the ruleset (or parts of it) were created by *iptables-nft*
+and one should use that to manage it.
+
+*BEWARE:* nftables won't restore these statements.
diff --git a/include/json.h b/include/json.h
index b0d78eb84987e..f691678d4d726 100644
--- a/include/json.h
+++ b/include/json.h
@@ -92,6 +92,7 @@ json_t *connlimit_stmt_json(const struct stmt *stmt, struct output_ctx *octx);
 json_t *tproxy_stmt_json(const struct stmt *stmt, struct output_ctx *octx);
 json_t *synproxy_stmt_json(const struct stmt *stmt, struct output_ctx *octx);
 json_t *optstrip_stmt_json(const struct stmt *stmt, struct output_ctx *octx);
+json_t *xt_stmt_json(const struct stmt *stmt, struct output_ctx *octx);
 
 int do_command_list_json(struct netlink_ctx *ctx, struct cmd *cmd);
 
@@ -194,6 +195,7 @@ STMT_PRINT_STUB(connlimit)
 STMT_PRINT_STUB(tproxy)
 STMT_PRINT_STUB(synproxy)
 STMT_PRINT_STUB(optstrip)
+STMT_PRINT_STUB(xt)
 
 #undef STMT_PRINT_STUB
 #undef EXPR_PRINT_STUB
diff --git a/include/parser.h b/include/parser.h
index f55da0fd47bf2..977fbb94fbd62 100644
--- a/include/parser.h
+++ b/include/parser.h
@@ -50,6 +50,7 @@ enum startcond_type {
 	PARSER_SC_TCP,
 	PARSER_SC_TYPE,
 	PARSER_SC_VLAN,
+	PARSER_SC_XT,
 	PARSER_SC_CMD_EXPORT,
 	PARSER_SC_CMD_IMPORT,
 	PARSER_SC_CMD_LIST,
diff --git a/src/json.c b/src/json.c
index 6662f8087736a..89ff8a344c2e4 100644
--- a/src/json.c
+++ b/src/json.c
@@ -82,12 +82,6 @@ static json_t *stmt_print_json(const struct stmt *stmt, struct output_ctx *octx)
 	char buf[1024];
 	FILE *fp;
 
-	/* XXX: Can't be supported at this point:
-	 * xt_stmt_xlate() ignores output_fp.
-	 */
-	if (stmt->ops->type == STMT_XT)
-		return json_pack("{s:n}", "xt");
-
 	if (stmt->ops->json)
 		return stmt->ops->json(stmt, octx);
 
@@ -1624,6 +1618,19 @@ json_t *optstrip_stmt_json(const struct stmt *stmt, struct output_ctx *octx)
 			 expr_print_json(stmt->optstrip.expr, octx));
 }
 
+json_t *xt_stmt_json(const struct stmt *stmt, struct output_ctx *octx)
+{
+	static const char *xt_typename[NFT_XT_MAX] = {
+		[NFT_XT_MATCH]          = "match",
+		[NFT_XT_TARGET]         = "target",
+		[NFT_XT_WATCHER]        = "watcher",
+	};
+
+	return json_pack("{s:{s:s, s:s}}", "xt",
+			 "type", xt_typename[stmt->xt.type],
+			 "name", stmt->xt.name);
+}
+
 static json_t *table_print_json_full(struct netlink_ctx *ctx,
 				     struct table *table)
 {
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 760c23cf33223..d7cf8bc5fb1ee 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -626,6 +626,8 @@ int nft_lex(void *, void *, void *);
 %token IN			"in"
 %token OUT			"out"
 
+%token XT		"xt"
+
 %type <limit_rate>		limit_rate_pkts
 %type <limit_rate>		limit_rate_bytes
 
@@ -900,6 +902,9 @@ int nft_lex(void *, void *, void *);
 %type <stmt>			optstrip_stmt
 %destructor { stmt_free($$); }	optstrip_stmt
 
+%type <stmt>			xt_stmt
+%destructor { stmt_free($$); }	xt_stmt
+
 %type <expr>			boolean_expr
 %destructor { expr_free($$); }	boolean_expr
 %type <val8>			boolean_keys
@@ -991,6 +996,7 @@ close_scope_udplite	: { scanner_pop_start_cond(nft->scanner, PARSER_SC_EXPR_UDPL
 
 close_scope_log		: { scanner_pop_start_cond(nft->scanner, PARSER_SC_STMT_LOG); }
 close_scope_synproxy	: { scanner_pop_start_cond(nft->scanner, PARSER_SC_STMT_SYNPROXY); }
+close_scope_xt		: { scanner_pop_start_cond(nft->scanner, PARSER_SC_XT); }
 
 common_block		:	INCLUDE		QUOTED_STRING	stmt_separator
 			{
@@ -2879,6 +2885,18 @@ stmt			:	verdict_stmt
 			|	synproxy_stmt	close_scope_synproxy
 			|	chain_stmt
 			|	optstrip_stmt
+			|	xt_stmt		close_scope_xt
+			;
+
+xt_stmt			:	XT	STRING	STRING
+			{
+				$$ = NULL;
+				xfree($2);
+				xfree($3);
+				erec_queue(error(&@$, "unsupported xtables compat expression, use iptables-nft with this ruleset"),
+					   state->msgs);
+				YYERROR;
+			}
 			;
 
 chain_stmt_type		:	JUMP	{ $$ = NFT_JUMP; }
diff --git a/src/parser_json.c b/src/parser_json.c
index 76c268f857202..057b4f4e3ff2c 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -2764,6 +2764,11 @@ static struct stmt *json_parse_stmt(struct json_ctx *ctx, json_t *root)
 		return verdict_stmt_alloc(int_loc, expr);
 	}
 
+	if (!strcmp(type, "xt")) {
+		json_error(ctx, "unsupported xtables compat expression, use iptables-nft with this ruleset");
+		return NULL;
+	}
+
 	for (i = 0; i < array_size(stmt_parser_tbl); i++) {
 		if (!strcmp(type, stmt_parser_tbl[i].key))
 			return stmt_parser_tbl[i].cb(ctx, stmt_parser_tbl[i].key, tmp);
diff --git a/src/scanner.l b/src/scanner.l
index 1371cd044b65a..4edd729c80dab 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -214,6 +214,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 %s SCANSTATE_TCP
 %s SCANSTATE_TYPE
 %s SCANSTATE_VLAN
+%s SCANSTATE_XT
 %s SCANSTATE_CMD_EXPORT
 %s SCANSTATE_CMD_IMPORT
 %s SCANSTATE_CMD_LIST
@@ -799,6 +800,8 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 
 "secmark"		{ scanner_push_start_cond(yyscanner, SCANSTATE_SECMARK); return SECMARK; }
 
+"xt"			{ scanner_push_start_cond(yyscanner, SCANSTATE_XT); return XT; }
+
 {addrstring}		{
 				yylval->string = xstrdup(yytext);
 				return STRING;
diff --git a/src/statement.c b/src/statement.c
index 327d00f99200a..eafc51c484de9 100644
--- a/src/statement.c
+++ b/src/statement.c
@@ -997,6 +997,7 @@ static const struct stmt_ops xt_stmt_ops = {
 	.name		= "xt",
 	.print		= xt_stmt_print,
 	.destroy	= xt_stmt_destroy,
+	.json		= xt_stmt_json,
 };
 
 struct stmt *xt_stmt_alloc(const struct location *loc)
diff --git a/src/xt.c b/src/xt.c
index 300416a1e8d92..12b52aa33bc30 100644
--- a/src/xt.c
+++ b/src/xt.c
@@ -115,7 +115,13 @@ void xt_stmt_xlate(const struct stmt *stmt, struct output_ctx *octx)
 	xt_xlate_free(xl);
 	xfree(entry);
 #else
-	nft_print(octx, "# xt_%s", stmt->xt.name);
+	static const char *typename[NFT_XT_MAX] = {
+		[NFT_XT_MATCH]		= "match",
+		[NFT_XT_TARGET]		= "target",
+		[NFT_XT_WATCHER]	= "watcher",
+	};
+
+	nft_print(octx, "xt %s %s", typename[stmt->xt.type], stmt->xt.name);
 #endif
 }
 
-- 
2.38.0

Re: [nft PATCH 3/4] xt: Rewrite unsupported compat expression dumping

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> diff --git a/src/scanner.l b/src/scanner.l
> index 1371cd044b65a..4edd729c80dab 100644
> --- a/src/scanner.l
> +++ b/src/scanner.l
> @@ -214,6 +214,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
>  %s SCANSTATE_TCP
>  %s SCANSTATE_TYPE
>  %s SCANSTATE_VLAN
> +%s SCANSTATE_XT
>  %s SCANSTATE_CMD_EXPORT
>  %s SCANSTATE_CMD_IMPORT
>  %s SCANSTATE_CMD_LIST
> @@ -799,6 +800,8 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
>  
>  "secmark"		{ scanner_push_start_cond(yyscanner, SCANSTATE_SECMARK); return SECMARK; }
>  
> +"xt"			{ scanner_push_start_cond(yyscanner, SCANSTATE_XT); return XT; }

Why is there a new scanner state?  It has no tokens, so it doesn't do
anything.

Perhaps a leftover?  Or did you plan to make match/target/watcher scoped
tokens?

Re: [nft PATCH 3/4] xt: Rewrite unsupported compat expression dumping

[Phil Sutter]

On Thu, Nov 24, 2022 at 06:04:54PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > diff --git a/src/scanner.l b/src/scanner.l
> > index 1371cd044b65a..4edd729c80dab 100644
> > --- a/src/scanner.l
> > +++ b/src/scanner.l
> > @@ -214,6 +214,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
> >  %s SCANSTATE_TCP
> >  %s SCANSTATE_TYPE
> >  %s SCANSTATE_VLAN
> > +%s SCANSTATE_XT
> >  %s SCANSTATE_CMD_EXPORT
> >  %s SCANSTATE_CMD_IMPORT
> >  %s SCANSTATE_CMD_LIST
> > @@ -799,6 +800,8 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
> >  
> >  "secmark"		{ scanner_push_start_cond(yyscanner, SCANSTATE_SECMARK); return SECMARK; }
> >  
> > +"xt"			{ scanner_push_start_cond(yyscanner, SCANSTATE_XT); return XT; }
> 
> Why is there a new scanner state?  It has no tokens, so it doesn't do
> anything.
> 
> Perhaps a leftover?  Or did you plan to make match/target/watcher scoped
> tokens?

We want to accept two arbitrary strings after "xt" and just complain
about the whole thing. Without the scope, something like "xt target
exthdr" will make the parser complain about the unexpected keyword.

Cheers, Phil

Re: [nft PATCH 3/4] xt: Rewrite unsupported compat expression dumping

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> On Thu, Nov 24, 2022 at 06:04:54PM +0100, Florian Westphal wrote:
> > Phil Sutter <phil@nwl.cc> wrote:
> > > diff --git a/src/scanner.l b/src/scanner.l
> > > index 1371cd044b65a..4edd729c80dab 100644
> > > --- a/src/scanner.l
> > > +++ b/src/scanner.l
> > > @@ -214,6 +214,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
> > >  %s SCANSTATE_TCP
> > >  %s SCANSTATE_TYPE
> > >  %s SCANSTATE_VLAN
> > > +%s SCANSTATE_XT
> > >  %s SCANSTATE_CMD_EXPORT
> > >  %s SCANSTATE_CMD_IMPORT
> > >  %s SCANSTATE_CMD_LIST
> > > @@ -799,6 +800,8 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
> > >  
> > >  "secmark"		{ scanner_push_start_cond(yyscanner, SCANSTATE_SECMARK); return SECMARK; }
> > >  
> > > +"xt"			{ scanner_push_start_cond(yyscanner, SCANSTATE_XT); return XT; }
> > 
> > Why is there a new scanner state?  It has no tokens, so it doesn't do
> > anything.
> > 
> > Perhaps a leftover?  Or did you plan to make match/target/watcher scoped
> > tokens?
> 
> We want to accept two arbitrary strings after "xt" and just complain
> about the whole thing. Without the scope, something like "xt target
> exthdr" will make the parser complain about the unexpected keyword.

I see.  FWIW I'm fine with the series.

Re: [nft PATCH 3/4] xt: Rewrite unsupported compat expression dumping

[Pablo Neira Ayuso]

On Thu, Nov 24, 2022 at 05:56:40PM +0100, Phil Sutter wrote:
> Choose a format which provides more information and is easily parseable.
> Then teach parsers about it and make it explicitly reject the ruleset
> giving a meaningful explanation. Also update the man pages with some
> more details.

There is a bugzilla ticket related to xt and json support, you can
probably add a Closes: tag link.

Re: [nft PATCH 3/4] xt: Rewrite unsupported compat expression dumping

[Phil Sutter]

On Fri, Dec 09, 2022 at 05:07:18PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Nov 24, 2022 at 05:56:40PM +0100, Phil Sutter wrote:
> > Choose a format which provides more information and is easily parseable.
> > Then teach parsers about it and make it explicitly reject the ruleset
> > giving a meaningful explanation. Also update the man pages with some
> > more details.
> 
> There is a bugzilla ticket related to xt and json support, you can
> probably add a Closes: tag link.

This should be nfbz#1621, but it's about translating xt to native in
JSON format. All my patch does is extend the xt JSON format a bit. AIUI,
we would have to extend libxtables to provide translations into JSON.

So even with perfect two-ways translation available, JSON interface is
unusable if iptables-nft is in use.

Cheers, Phil

Re: [nft PATCH 3/4] xt: Rewrite unsupported compat expression dumping

[Pablo Neira Ayuso]

On Fri, Dec 09, 2022 at 05:45:48PM +0100, Phil Sutter wrote:
> On Fri, Dec 09, 2022 at 05:07:18PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Nov 24, 2022 at 05:56:40PM +0100, Phil Sutter wrote:
> > > Choose a format which provides more information and is easily parseable.
> > > Then teach parsers about it and make it explicitly reject the ruleset
> > > giving a meaningful explanation. Also update the man pages with some
> > > more details.
> > 
> > There is a bugzilla ticket related to xt and json support, you can
> > probably add a Closes: tag link.
> 
> This should be nfbz#1621, but it's about translating xt to native in
> JSON format. All my patch does is extend the xt JSON format a bit. AIUI,
> we would have to extend libxtables to provide translations into JSON.
> 
> So even with perfect two-ways translation available, JSON interface is
> unusable if iptables-nft is in use.

The output of nftables without translation cannot be restored either,
this is also going to provide a hint to the user, without allowing it
to restore the JSON file.

[nft PATCH 4/4] xt: Fall back to generic printing from translation

[Phil Sutter]

If translation is not available or fails, print the generic format
instead of calling the print callback (which does not respect
output_fp) or silently failing.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/xt.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/src/xt.c b/src/xt.c
index 12b52aa33bc30..b75c94e856ca7 100644
--- a/src/xt.c
+++ b/src/xt.c
@@ -34,6 +34,12 @@ static void *xt_entry_alloc(const struct xt_stmt *xt, uint32_t af);
 
 void xt_stmt_xlate(const struct stmt *stmt, struct output_ctx *octx)
 {
+	static const char *typename[NFT_XT_MAX] = {
+		[NFT_XT_MATCH]		= "match",
+		[NFT_XT_TARGET]		= "target",
+		[NFT_XT_WATCHER]	= "watcher",
+	};
+	int rc = 0;
 #ifdef HAVE_LIBXTABLES
 	struct xt_xlate *xl = xt_xlate_alloc(10240);
 	struct xtables_target *tg;
@@ -69,11 +75,7 @@ void xt_stmt_xlate(const struct stmt *stmt, struct output_ctx *octx)
 				.numeric        = 1,
 			};
 
-			mt->xlate(xl, &params);
-			nft_print(octx, "%s", xt_xlate_get(xl));
-		} else if (mt->print) {
-			printf("#");
-			mt->print(&entry, m, 0);
+			rc = mt->xlate(xl, &params);
 		}
 		xfree(m);
 		break;
@@ -102,27 +104,20 @@ void xt_stmt_xlate(const struct stmt *stmt, struct output_ctx *octx)
 				.numeric        = 1,
 			};
 
-			tg->xlate(xl, &params);
-			nft_print(octx, "%s", xt_xlate_get(xl));
-		} else if (tg->print) {
-			printf("#");
-			tg->print(NULL, t, 0);
+			rc = tg->xlate(xl, &params);
 		}
 		xfree(t);
 		break;
 	}
 
+	if (rc == 1)
+		nft_print(octx, "%s", xt_xlate_get(xl));
 	xt_xlate_free(xl);
 	xfree(entry);
-#else
-	static const char *typename[NFT_XT_MAX] = {
-		[NFT_XT_MATCH]		= "match",
-		[NFT_XT_TARGET]		= "target",
-		[NFT_XT_WATCHER]	= "watcher",
-	};
-
-	nft_print(octx, "xt %s %s", typename[stmt->xt.type], stmt->xt.name);
 #endif
+	if (!rc)
+		nft_print(octx, "xt %s %s",
+			  typename[stmt->xt.type], stmt->xt.name);
 }
 
 void xt_stmt_destroy(struct stmt *stmt)
-- 
2.38.0

Re: [nft PATCH 0/4] xt: Rewrite unsupported compat expression dumping

[Phil Sutter]

On Thu, Nov 24, 2022 at 05:56:37PM +0100, Phil Sutter wrote:
> Alternative approach to my previous dump and restore support of xt
> compat expressions:
> 
> If translation is not available or not successful, fall back to a
> format which allows to be parsed easily.
> 
> When parsing, reject these expressions explicitly with a meaningful
> error message.
> 
> Phil Sutter (4):
>   xt: Delay libxtables access until translation
>   xt: Purify enum nft_xt_type
>   xt: Rewrite unsupported compat expression dumping
>   xt: Fall back to generic printing from translation

Series applied.