[PATCH nft 0/6] arbirary table/chain names

[PATCH nft 0/6] arbirary table/chain names

[Florian Westphal]

This series allows (almost) arbitrary chain names.

Unsolved problem:
nft has implict 'rule add' behaviour, e.g.

'nft add rule ip filter input foo ip saddr 1.2.3.4 drop' can be written like
'nft ip filter input foo ip saddr 1.2.3.4 drop' or even
'nft filter input foo ip saddr 1.2.3.4 drop'.

IOW, the scanner cannot switch to the exclusive rule scope
added in patch 5 to allow for arbitrary names.

Patch 6 resolves this by switching state from bison, but this
requires to add future tokens to a special whitelist.

It might be better to omit patch 6 and/or deprecate the
implicit rule add behaviour.  See patch 6 for details.

Florian Westphal (6):
  scanner: add support for scope nesting
  scanner: counter: move to own scope
  scanner: log: move to own scope
  scanner: support arbitary table names
  scanner: support arbitrary chain names
  src: allow arbitary chain name in implicit rule add case

 include/parser.h   |  12 ++++
 src/parser_bison.y |  97 ++++++++++++++++++-------
 src/scanner.l      | 173 +++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 241 insertions(+), 41 deletions(-)

-- 
2.26.2

[PATCH nft 1/6] scanner: add support for scope nesting

[Florian Westphal]

Adding a COUNTER scope introduces parsing errors.  Example:

add rule  ... counter ip saddr 1.2.3.4

This is supposed to be

    COUNTER	IP SADDR SYMBOL

but it will be parsed as

    COUNTER	IP STRING SYMBOL

... and rule fails with unknown saddr.
This is because IP state change gets popped right after it was pushed.

bison parser invokes scanner_pop_start_cond() helper via
'close_scope_counter' rule after it has processed the entire 'counter' rule.
But that happens *after* flex has executed the 'IP' rule.

IOW, the sequence of events is not the exepcted
"COUNTER close_scope_counter IP SADDR SYMBOL close_scope_ip", it is
"COUNTER IP close_scope_counter".

close_scope_counter pops the just-pushed SCANSTATE_IP and returns the
scanner to SCANSTATE_COUNTER, so next input token (saddr) gets parsed
as a string, which gets then rejected from bison.

To resolve this, defer the pop operation until the current state is done.
scanner_pop_start_cond() already gets the scope that it has been
completed as an argument, so we can compare it to the active state.

If those are not the same, just defer the pop operation until the
bison reports its done with the active flex scope.

This leads to following sequence of events:
  1. flex switches to SCANSTATE_COUNTER
  2. flex switches to SCANSTATE_IP
  3. bison calls scanner_pop_start_cond(SCANSTATE_COUNTER)
  4. flex remains in SCANSTATE_IP, bison continues
  5. bison calls scanner_pop_start_cond(SCANSTATE_IP) once the entire
     ip rule has completed: this pops both IP and COUNTER.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/parser.h |  2 ++
 src/scanner.l    | 17 +++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/include/parser.h b/include/parser.h
index 9fdebcd11dd2..0c229963d3be 100644
--- a/include/parser.h
+++ b/include/parser.h
@@ -23,6 +23,8 @@ struct parser_state {
 	struct scope			*scopes[SCOPE_NEST_MAX];
 	unsigned int			scope;
 
+	unsigned int			flex_state_pop;
+	unsigned int			startcond_type;
 	struct list_head		*cmds;
 };
 
diff --git a/src/scanner.l b/src/scanner.l
index a73ce1b819d8..01e1dca52fdd 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -1017,11 +1017,28 @@ void scanner_destroy(struct nft_ctx *nft)
 
 static void scanner_push_start_cond(void *scanner, enum startcond_type type)
 {
+	struct parser_state *state = yyget_extra(scanner);
+
+	state->startcond_type = type;
 	yy_push_state((int)type, scanner);
 }
 
 void scanner_pop_start_cond(void *scanner, enum startcond_type t)
 {
+	struct parser_state *state = yyget_extra(scanner);
+
+	if (state->startcond_type != t) {
+		state->flex_state_pop++;
+		return; /* Can't pop just yet! */
+	}
+
+	while (state->flex_state_pop) {
+		state->flex_state_pop--;
+		state->startcond_type = yy_top_state(scanner);
+		yy_pop_state(scanner);
+	}
+
+	state->startcond_type = yy_top_state(scanner);
 	yy_pop_state(scanner);
 	(void)yy_top_state(scanner); /* suppress gcc warning wrt. unused function */
 }
-- 
2.26.2

[PATCH nft 2/6] scanner: counter: move to own scope

[Florian Westphal]

move bytes/packets away from initial state.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/parser.h   |  1 +
 src/parser_bison.y | 31 ++++++++++++++++---------------
 src/scanner.l      |  7 ++++---
 3 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/include/parser.h b/include/parser.h
index 0c229963d3be..59eff16eac20 100644
--- a/include/parser.h
+++ b/include/parser.h
@@ -32,6 +32,7 @@ enum startcond_type {
 	PARSER_SC_BEGIN,
 	PARSER_SC_ARP,
 	PARSER_SC_CT,
+	PARSER_SC_COUNTER,
 	PARSER_SC_ETH,
 	PARSER_SC_IP,
 	PARSER_SC_IP6,
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 08a2599e5374..805a38ab22ed 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -863,6 +863,7 @@ opt_newline		:	NEWLINE
 
 close_scope_arp		: { scanner_pop_start_cond(nft->scanner, PARSER_SC_ARP); };
 close_scope_ct		: { scanner_pop_start_cond(nft->scanner, PARSER_SC_CT); };
+close_scope_counter	: { scanner_pop_start_cond(nft->scanner, PARSER_SC_COUNTER); };
 close_scope_eth		: { scanner_pop_start_cond(nft->scanner, PARSER_SC_ETH); };
 close_scope_fib		: { scanner_pop_start_cond(nft->scanner, PARSER_SC_EXPR_FIB); };
 close_scope_hash	: { scanner_pop_start_cond(nft->scanner, PARSER_SC_EXPR_HASH); };
@@ -1023,7 +1024,7 @@ add_cmd			:	TABLE		table_spec
 				handle_merge(&$3->handle, &$2);
 				$$ = cmd_alloc(CMD_ADD, CMD_OBJ_FLOWTABLE, &$2, &@$, $5);
 			}
-			|	COUNTER		obj_spec
+			|	COUNTER		obj_spec	close_scope_counter
 			{
 				struct obj *obj;
 
@@ -1032,11 +1033,11 @@ add_cmd			:	TABLE		table_spec
 				handle_merge(&obj->handle, &$2);
 				$$ = cmd_alloc(CMD_ADD, CMD_OBJ_COUNTER, &$2, &@$, obj);
 			}
-			|	COUNTER		obj_spec	counter_obj	counter_config
+			|	COUNTER		obj_spec	counter_obj	counter_config	close_scope_counter
 			{
 				$$ = cmd_alloc(CMD_ADD, CMD_OBJ_COUNTER, &$2, &@$, $3);
 			}
-			|	COUNTER		obj_spec	counter_obj	'{' counter_block '}'
+			|	COUNTER		obj_spec	counter_obj	'{' counter_block '}'	close_scope_counter
 			{
 				$$ = cmd_alloc(CMD_ADD, CMD_OBJ_COUNTER, &$2, &@$, $3);
 			}
@@ -1140,7 +1141,7 @@ create_cmd		:	TABLE		table_spec
 				handle_merge(&$3->handle, &$2);
 				$$ = cmd_alloc(CMD_CREATE, CMD_OBJ_FLOWTABLE, &$2, &@$, $5);
 			}
-			|	COUNTER		obj_spec
+			|	COUNTER		obj_spec	close_scope_counter
 			{
 				struct obj *obj;
 
@@ -1149,7 +1150,7 @@ create_cmd		:	TABLE		table_spec
 				handle_merge(&obj->handle, &$2);
 				$$ = cmd_alloc(CMD_CREATE, CMD_OBJ_COUNTER, &$2, &@$, obj);
 			}
-			|	COUNTER		obj_spec	counter_obj	counter_config
+			|	COUNTER		obj_spec	counter_obj	counter_config	close_scope_counter
 			{
 				$$ = cmd_alloc(CMD_CREATE, CMD_OBJ_COUNTER, &$2, &@$, $3);
 			}
@@ -1244,7 +1245,7 @@ delete_cmd		:	TABLE		table_or_id_spec
 				handle_merge(&$3->handle, &$2);
 				$$ = cmd_alloc(CMD_DELETE, CMD_OBJ_FLOWTABLE, &$2, &@$, $5);
 			}
-			|	COUNTER		obj_or_id_spec
+			|	COUNTER		obj_or_id_spec	close_scope_counter
 			{
 				$$ = cmd_alloc(CMD_DELETE, CMD_OBJ_COUNTER, &$2, &@$, NULL);
 			}
@@ -1312,7 +1313,7 @@ list_cmd		:	TABLE		table_spec
 			{
 				$$ = cmd_alloc(CMD_LIST, CMD_OBJ_COUNTERS, &$3, &@$, NULL);
 			}
-			|	COUNTER		obj_spec
+			|	COUNTER		obj_spec	close_scope_counter
 			{
 				$$ = cmd_alloc(CMD_LIST, CMD_OBJ_COUNTER, &$2, &@$, NULL);
 			}
@@ -1418,7 +1419,7 @@ reset_cmd		:	COUNTERS	ruleset_spec
 			{
 				$$ = cmd_alloc(CMD_RESET, CMD_OBJ_COUNTERS, &$3, &@$, NULL);
 			}
-			|       COUNTER         obj_spec
+			|       COUNTER         obj_spec	close_scope_counter
 			{
 				$$ = cmd_alloc(CMD_RESET, CMD_OBJ_COUNTER, &$2,&@$, NULL);
 			}
@@ -1621,7 +1622,7 @@ table_block		:	/* empty */	{ $$ = $<table>-1; }
 			}
 			|	table_block	COUNTER		obj_identifier
 					obj_block_alloc	'{'	counter_block	'}'
-					stmt_separator
+					stmt_separator	close_scope_counter
 			{
 				$4->location = @3;
 				$4->type = NFT_OBJECT_COUNTER;
@@ -1881,7 +1882,7 @@ map_block_alloc		:	/* empty */
 			}
 			;
 
-map_block_obj_type	:	COUNTER	{ $$ = NFT_OBJECT_COUNTER; }
+map_block_obj_type	:	COUNTER	close_scope_counter { $$ = NFT_OBJECT_COUNTER; }
 			|	QUOTA	close_scope_quota { $$ = NFT_OBJECT_QUOTA; }
 			|	LIMIT	close_scope_limit { $$ = NFT_OBJECT_LIMIT; }
 			|	SECMARK close_scope_secmark { $$ = NFT_OBJECT_SECMARK; }
@@ -2011,7 +2012,7 @@ flowtable_block		:	/* empty */	{ $$ = $<flowtable>-1; }
 			{
 				$$->dev_expr = $4;
 			}
-			|	flowtable_block COUNTER
+			|	flowtable_block COUNTER	close_scope_counter
 			{
 				$$->flags |= NFT_FLOWTABLE_COUNTER;
 			}
@@ -2782,11 +2783,11 @@ connlimit_stmt		:	CT	COUNT	NUM	close_scope_ct
 counter_stmt		:	counter_stmt_alloc
 			|	counter_stmt_alloc	counter_args
 
-counter_stmt_alloc	:	COUNTER
+counter_stmt_alloc	:	COUNTER	close_scope_counter
 			{
 				$$ = counter_stmt_alloc(&@$);
 			}
-			|	COUNTER		NAME	stmt_expr
+			|	COUNTER		NAME	stmt_expr	close_scope_counter
 			{
 				$$ = objref_stmt_alloc(&@$);
 				$$->objref.type = NFT_OBJECT_COUNTER;
@@ -4133,11 +4134,11 @@ set_elem_stmt_list	:	set_elem_stmt
 			}
 			;
 
-set_elem_stmt		:	COUNTER
+set_elem_stmt		:	COUNTER	close_scope_counter
 			{
 				$$ = counter_stmt_alloc(&@$);
 			}
-			|	COUNTER	PACKETS	NUM	BYTES	NUM
+			|	COUNTER	PACKETS	NUM	BYTES	NUM	close_scope_counter
 			{
 				$$ = counter_stmt_alloc(&@$);
 				$$->counter.packets = $3;
diff --git a/src/scanner.l b/src/scanner.l
index 01e1dca52fdd..783436504326 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -198,6 +198,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 %option stack
 %s SCANSTATE_ARP
 %s SCANSTATE_CT
+%s SCANSTATE_COUNTER
 %s SCANSTATE_ETH
 %s SCANSTATE_IP
 %s SCANSTATE_IP6
@@ -343,10 +344,10 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 
 "flowtables"		{ return FLOWTABLES; }
 
-"counter"		{ return COUNTER; }
+"counter"		{ scanner_push_start_cond(yyscanner, SCANSTATE_COUNTER); return COUNTER; }
 "name"			{ return NAME; }
-"packets"		{ return PACKETS; }
-"bytes"			{ return BYTES; }
+<SCANSTATE_COUNTER,SCANSTATE_CT,SCANSTATE_LIMIT>"packets"		{ return PACKETS; }
+<SCANSTATE_COUNTER,SCANSTATE_CT,SCANSTATE_LIMIT,SCANSTATE_QUOTA>"bytes"	{ return BYTES; }
 
 "counters"		{ return COUNTERS; }
 "quotas"		{ return QUOTAS; }
-- 
2.26.2

[PATCH nft 3/6] scanner: log: move to own scope

[Florian Westphal]

GROUP and PREFIX are used by igmp and nat, so they can't be moved out of
INITIAL scope yet.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/parser.h   |  2 ++
 src/parser_bison.y |  4 +++-
 src/scanner.l      | 12 ++++++++----
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/include/parser.h b/include/parser.h
index 59eff16eac20..d890ab223c52 100644
--- a/include/parser.h
+++ b/include/parser.h
@@ -47,6 +47,8 @@ enum startcond_type {
 	PARSER_SC_EXPR_QUEUE,
 	PARSER_SC_EXPR_RT,
 	PARSER_SC_EXPR_SOCKET,
+
+	PARSER_SC_STMT_LOG,
 };
 
 struct mnl_socket;
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 805a38ab22ed..98fe4431c4f4 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -879,6 +879,8 @@ close_scope_rt		: { scanner_pop_start_cond(nft->scanner, PARSER_SC_EXPR_RT); };
 close_scope_secmark	: { scanner_pop_start_cond(nft->scanner, PARSER_SC_SECMARK); };
 close_scope_socket	: { scanner_pop_start_cond(nft->scanner, PARSER_SC_EXPR_SOCKET); }
 
+close_scope_log		: { scanner_pop_start_cond(nft->scanner, PARSER_SC_STMT_LOG); }
+
 common_block		:	INCLUDE		QUOTED_STRING	stmt_separator
 			{
 				if (scanner_include_file(nft, scanner, $2, &@$) < 0) {
@@ -2695,7 +2697,7 @@ stmt			:	verdict_stmt
 			|	payload_stmt
 			|	stateful_stmt
 			|	meta_stmt
-			|	log_stmt
+			|	log_stmt	close_scope_log
 			|	reject_stmt
 			|	nat_stmt
 			|	tproxy_stmt
diff --git a/src/scanner.l b/src/scanner.l
index 783436504326..0082b3eeca29 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -214,6 +214,8 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 %s SCANSTATE_EXPR_RT
 %s SCANSTATE_EXPR_SOCKET
 
+%s SCANSTATE_STMT_LOG
+
 %%
 
 "=="			{ return EQ; }
@@ -354,12 +356,14 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "limits"		{ return LIMITS; }
 "synproxys"		{ return SYNPROXYS; }
 
-"log"			{ return LOG; }
+"log"			{ scanner_push_start_cond(yyscanner, SCANSTATE_STMT_LOG); return LOG; }
 "prefix"		{ return PREFIX; }
 "group"			{ return GROUP; }
-"snaplen"		{ return SNAPLEN; }
-"queue-threshold"	{ return QUEUE_THRESHOLD; }
-"level"			{ return LEVEL; }
+<SCANSTATE_STMT_LOG>{
+	"snaplen"		{ return SNAPLEN; }
+	"queue-threshold"	{ return QUEUE_THRESHOLD; }
+	"level"			{ return LEVEL; }
+}
 
 "queue"			{ scanner_push_start_cond(yyscanner, SCANSTATE_EXPR_QUEUE); return QUEUE;}
 <SCANSTATE_EXPR_QUEUE>{
-- 
2.26.2

[PATCH nft 4/6] scanner: support arbitary table names

[Florian Westphal]

Add exclusive start condition that only recognizes strings, then
switch to it from table keyword.

This prevents

table foo {

... from breaking when a foo expression keyword would be added to nft
in the future.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/parser.h |  3 ++
 src/scanner.l    | 72 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/include/parser.h b/include/parser.h
index d890ab223c52..0843aa1adb6a 100644
--- a/include/parser.h
+++ b/include/parser.h
@@ -25,6 +25,7 @@ struct parser_state {
 
 	unsigned int			flex_state_pop;
 	unsigned int			startcond_type;
+	unsigned int			saw_family:1;
 	struct list_head		*cmds;
 };
 
@@ -49,6 +50,8 @@ enum startcond_type {
 	PARSER_SC_EXPR_SOCKET,
 
 	PARSER_SC_STMT_LOG,
+
+	PARSER_SC_STRING_TABLE,
 };
 
 struct mnl_socket;
diff --git a/src/scanner.l b/src/scanner.l
index 0082b3eeca29..bf6f290db3db 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -98,6 +98,8 @@ static void reset_pos(struct parser_state *state, struct location *loc)
 	state->indesc->column		= 1;
 }
 
+static int scanner_handle_tablename(void *scanner, const char *token);
+
 static void scanner_push_start_cond(void *scanner, enum startcond_type type);
 
 #define YY_USER_ACTION {					\
@@ -216,6 +218,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 
 %s SCANSTATE_STMT_LOG
 
+%x SCANSTATE_STRING_TABLE
 %%
 
 "=="			{ return EQ; }
@@ -272,7 +275,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "hook"			{ return HOOK; }
 "device"		{ return DEVICE; }
 "devices"		{ return DEVICES; }
-"table"			{ return TABLE; }
+"table"			{ scanner_push_start_cond(yyscanner, SCANSTATE_STRING_TABLE); return TABLE; }
 "tables"		{ return TABLES; }
 "chain"			{ return CHAIN; }
 "chains"		{ return CHAINS; }
@@ -712,6 +715,34 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 
 .			{ return JUNK; }
 
+<SCANSTATE_STRING_TABLE>{string}		{
+				int token = scanner_handle_tablename(yyscanner, yytext);
+
+				if (token != STRING)
+					return token;
+
+				yylval->string = xstrdup(yytext);
+				return STRING;
+			}
+
+<SCANSTATE_STRING_TABLE>{
+\\{newline}		{
+				reset_pos(yyget_extra(yyscanner), yylloc);
+			}
+
+{newline}		{
+				reset_pos(yyget_extra(yyscanner), yylloc);
+				return NEWLINE;
+			}
+
+{tab}+
+{space}+
+{comment}
+"$"			{ return '$'; }
+
+.			{ return JUNK; }
+}
+
 %%
 
 static void scanner_push_indesc(struct parser_state *state,
@@ -1033,6 +1064,9 @@ void scanner_pop_start_cond(void *scanner, enum startcond_type t)
 	struct parser_state *state = yyget_extra(scanner);
 
 	if (state->startcond_type != t) {
+		if (state->startcond_type == SCANSTATE_STRING_TABLE)
+			return;
+
 		state->flex_state_pop++;
 		return; /* Can't pop just yet! */
 	}
@@ -1047,3 +1081,39 @@ void scanner_pop_start_cond(void *scanner, enum startcond_type t)
 	yy_pop_state(scanner);
 	(void)yy_top_state(scanner); /* suppress gcc warning wrt. unused function */
 }
+
+static int scanner_handle_tablename(void *scanner, const char *token)
+{
+	struct parser_state *state = yyget_extra(scanner);
+	int ret = STRING;
+
+	if (state->startcond_type != SCANSTATE_STRING_TABLE)
+		return STRING;
+
+	if (state->saw_family) {
+		state->saw_family = 0;
+		scanner_pop_start_cond(scanner, SCANSTATE_STRING_TABLE);
+		return STRING;
+	}
+
+	if (strcmp(token, "ip") == 0) {
+		ret = IP;
+	} else if (strcmp(token, "ip6") == 0) {
+		ret = IP6;
+	} else if (strcmp(token, "inet") == 0) {
+		ret = INET;
+	} else if (strcmp(token, "bridge") == 0) {
+		ret = BRIDGE;
+	} else if (strcmp(token, "arp") == 0) {
+		ret = ARP;
+	} else if (strcmp(token, "netdev") == 0) {
+		ret = NETDEV;
+	}
+
+	if (ret != STRING)
+		state->saw_family = 1;
+	else
+		scanner_pop_start_cond(scanner, SCANSTATE_STRING_TABLE);
+
+	return ret;
+}
-- 
2.26.2

[PATCH nft 5/6] scanner: support arbitrary chain names

[Florian Westphal]

Previous change removed constraints on table naming, this extends it
to handle chain names as well.

The new exclusive scope also needs to permit '{' to handle bounded
chains, e.g.

 .... jump { ip ...

and "handle", to deal with: 'delete chain test-ip handle 2' -- in both
cases flex needs to leave the chain scope immediately.

The parser_state struct is used to record if we saw a family or table
name:

chain tname cname
chain ip tname cname
chain cname {
chain tname handle 1
chain ip tname handle 1

... are all valid.  We can't exit after the second name, first name
could have been the family name, not the table.

This change allows nft to parse:
  table ip dup {
        chain fwd {
        }
 }
 table inet dup {
        chain fwd {
        }
 }

The point here is not to allow aribitrary names (for instance
'table ip handle' won't work), its to make sure a new/future expression
keyword won't break an existing ruleset where the 'new keyword' happened
to be the name of a chain/table.

Remaining issue:

   |       RULE            rule_position   rule
   {
       $$ = cmd_alloc(CMD_ADD, CMD_OBJ_RULE, &$2, &@$, $3);
   }
   |       /* empty */     rule_position   rule

so, implicit 'add rule', e.g.  "nft ip tname cname ip saddr ..."
will not work when either tname or cname would be added as
token/expression in a future release.

This can't be fully resolved, there is no context (from scanner) to know that
'ip tname cname' need to be parsed as 'IP STRING STRING'.

With the long form (nft add rule ip tname cname ...), the RULE token
allows to switch scanner start condition.

Switching state from bison doesn't work reliably as the switch rule
will be evaluated too late, e.g. 'nft add dup ...'.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/parser.h   |  3 ++
 src/parser_bison.y |  5 +--
 src/scanner.l      | 81 ++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 74 insertions(+), 15 deletions(-)

diff --git a/include/parser.h b/include/parser.h
index 0843aa1adb6a..d6cf20729421 100644
--- a/include/parser.h
+++ b/include/parser.h
@@ -26,6 +26,8 @@ struct parser_state {
 	unsigned int			flex_state_pop;
 	unsigned int			startcond_type;
 	unsigned int			saw_family:1;
+	unsigned int			saw_table:1;
+	unsigned int			saw_chain:1;
 	struct list_head		*cmds;
 };
 
@@ -52,6 +54,7 @@ enum startcond_type {
 	PARSER_SC_STMT_LOG,
 
 	PARSER_SC_STRING_TABLE,
+	PARSER_SC_STRING_CHAIN,
 };
 
 struct mnl_socket;
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 98fe4431c4f4..bbac85fd35ce 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -864,6 +864,7 @@ opt_newline		:	NEWLINE
 close_scope_arp		: { scanner_pop_start_cond(nft->scanner, PARSER_SC_ARP); };
 close_scope_ct		: { scanner_pop_start_cond(nft->scanner, PARSER_SC_CT); };
 close_scope_counter	: { scanner_pop_start_cond(nft->scanner, PARSER_SC_COUNTER); };
+close_scope_chain	: { scanner_pop_start_cond(nft->scanner, PARSER_SC_STRING_CHAIN); }
 close_scope_eth		: { scanner_pop_start_cond(nft->scanner, PARSER_SC_ETH); };
 close_scope_fib		: { scanner_pop_start_cond(nft->scanner, PARSER_SC_EXPR_FIB); };
 close_scope_hash	: { scanner_pop_start_cond(nft->scanner, PARSER_SC_EXPR_HASH); };
@@ -2462,7 +2463,7 @@ tableid_spec 		: 	family_spec 	HANDLE NUM
 			}
 			;
 
-chain_spec		:	table_spec	identifier
+chain_spec		:	table_spec	identifier	close_scope_chain
 			{
 				$$		= $1;
 				$$.chain.name	= $2;
@@ -2470,7 +2471,7 @@ chain_spec		:	table_spec	identifier
 			}
 			;
 
-chainid_spec 		: 	table_spec 	HANDLE NUM
+chainid_spec 		: 	table_spec 	HANDLE NUM	close_scope_chain
 			{
 				$$ 			= $1;
 				$$.handle.location 	= @3;
diff --git a/src/scanner.l b/src/scanner.l
index bf6f290db3db..a156accaa944 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -99,6 +99,7 @@ static void reset_pos(struct parser_state *state, struct location *loc)
 }
 
 static int scanner_handle_tablename(void *scanner, const char *token);
+static int scanner_handle_chainname(void *scanner, const char *token);
 
 static void scanner_push_start_cond(void *scanner, enum startcond_type type);
 
@@ -219,6 +220,8 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 %s SCANSTATE_STMT_LOG
 
 %x SCANSTATE_STRING_TABLE
+%x SCANSTATE_STRING_CHAIN
+%x SCANSTATE_STRING_JUMP
 %%
 
 "=="			{ return EQ; }
@@ -277,9 +280,9 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "devices"		{ return DEVICES; }
 "table"			{ scanner_push_start_cond(yyscanner, SCANSTATE_STRING_TABLE); return TABLE; }
 "tables"		{ return TABLES; }
-"chain"			{ return CHAIN; }
+"chain"			{ scanner_push_start_cond(yyscanner, SCANSTATE_STRING_CHAIN); return CHAIN; }
 "chains"		{ return CHAINS; }
-"rule"			{ return RULE; }
+"rule"			{ scanner_push_start_cond(yyscanner, SCANSTATE_STRING_CHAIN); return RULE; }
 "rules"			{ return RULES; }
 "sets"			{ return SETS; }
 "set"			{ return SET; }
@@ -301,8 +304,8 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "accept"		{ return ACCEPT; }
 "drop"			{ return DROP; }
 "continue"		{ return CONTINUE; }
-"jump"			{ return JUMP; }
-"goto"			{ return GOTO; }
+"jump"			{ scanner_push_start_cond(yyscanner, SCANSTATE_STRING_JUMP); return JUMP; }
+"goto"			{ scanner_push_start_cond(yyscanner, SCANSTATE_STRING_JUMP); return GOTO; }
 "return"		{ return RETURN; }
 "to"			{ return TO; }
 
@@ -721,11 +724,44 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 				if (token != STRING)
 					return token;
 
+				scanner_pop_start_cond(yyscanner, SCANSTATE_STRING_TABLE);
 				yylval->string = xstrdup(yytext);
 				return STRING;
 			}
 
-<SCANSTATE_STRING_TABLE>{
+<SCANSTATE_STRING_CHAIN>{
+	"handle"	{
+			scanner_pop_start_cond(yyscanner, SCANSTATE_STRING_CHAIN);
+			return HANDLE;
+	}
+	{string}	{
+				int token = scanner_handle_chainname(yyscanner, yytext);
+
+				if (token != STRING)
+					return token;
+
+				yylval->string = xstrdup(yytext);
+				return STRING;
+	}
+	"{"		{
+			scanner_pop_start_cond(yyscanner, SCANSTATE_STRING_CHAIN);
+			return '{';
+	}
+}
+
+<SCANSTATE_STRING_JUMP>{string}	{
+				yylval->string = xstrdup(yytext);
+				scanner_pop_start_cond(yyscanner, SCANSTATE_STRING_JUMP);
+				return STRING;
+			}
+<SCANSTATE_STRING_JUMP>"{"		{
+				/* chain binding, e.g. jump { ... */
+				scanner_pop_start_cond(yyscanner, SCANSTATE_STRING_JUMP);
+				return '{';
+			}
+
+
+<SCANSTATE_STRING_TABLE,SCANSTATE_STRING_CHAIN,SCANSTATE_STRING_JUMP>{
 \\{newline}		{
 				reset_pos(yyget_extra(yyscanner), yylloc);
 			}
@@ -1057,16 +1093,27 @@ static void scanner_push_start_cond(void *scanner, enum startcond_type type)
 
 	state->startcond_type = type;
 	yy_push_state((int)type, scanner);
+
+	if (type == SCANSTATE_STRING_CHAIN)
+		state->saw_chain = 1;
 }
 
 void scanner_pop_start_cond(void *scanner, enum startcond_type t)
 {
 	struct parser_state *state = yyget_extra(scanner);
 
-	if (state->startcond_type != t) {
-		if (state->startcond_type == SCANSTATE_STRING_TABLE)
+	if (t == SCANSTATE_STRING_CHAIN) {
+		if (!state->saw_chain) /* implicit chain scope: never pushed the state */
 			return;
 
+		if (state->startcond_type == t)
+			state->saw_chain = 0;
+	}
+
+	if (state->startcond_type != t) {
+		if (state->startcond_type == SCANSTATE_STRING_TABLE ||
+		    state->startcond_type == SCANSTATE_STRING_CHAIN)
+			return;
 		state->flex_state_pop++;
 		return; /* Can't pop just yet! */
 	}
@@ -1087,12 +1134,8 @@ static int scanner_handle_tablename(void *scanner, const char *token)
 	struct parser_state *state = yyget_extra(scanner);
 	int ret = STRING;
 
-	if (state->startcond_type != SCANSTATE_STRING_TABLE)
-		return STRING;
-
 	if (state->saw_family) {
 		state->saw_family = 0;
-		scanner_pop_start_cond(scanner, SCANSTATE_STRING_TABLE);
 		return STRING;
 	}
 
@@ -1112,8 +1155,20 @@ static int scanner_handle_tablename(void *scanner, const char *token)
 
 	if (ret != STRING)
 		state->saw_family = 1;
-	else
-		scanner_pop_start_cond(scanner, SCANSTATE_STRING_TABLE);
+
+	return ret;
+}
+
+static int scanner_handle_chainname(void *scanner, const char *token)
+{
+	struct parser_state *state = yyget_extra(scanner);
+	bool saw_table = state->saw_table;
+	int ret;
+
+	ret = scanner_handle_tablename(scanner, token);
+
+	if (ret == STRING && saw_table)
+		scanner_pop_start_cond(scanner, SCANSTATE_STRING_CHAIN);
 
 	return ret;
 }
-- 
2.26.2

[PATCH nft 6/6] src: allow arbitary chain name in implicit rule add case

[Florian Westphal]

Allow switch of the flex state from bison parser.
Note that this switch will happen too late to cover all cases:

nft add ip dup fwd ip saddr ...  # adds a rule to chain fwd in table dup
nft add dup fwd ... # syntax error  (flex parses dup as expression keyword)

to solve this, bison must carry a list of keywords that are allowed to
be used as table names.

This adds FWD as an example.  When new keywords are added, this can
then be extended as needed.

Another alternative is to deprecate implicit rule add altogether
so users would have to move to 'nft add rule ...'.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/parser.h   |  1 +
 src/parser_bison.y | 57 ++++++++++++++++++++++++++++++++++++++--------
 src/scanner.l      |  4 +---
 3 files changed, 50 insertions(+), 12 deletions(-)

diff --git a/include/parser.h b/include/parser.h
index d6cf20729421..35117acc977f 100644
--- a/include/parser.h
+++ b/include/parser.h
@@ -77,5 +77,6 @@ extern void scanner_push_buffer(void *scanner,
 				const char *buffer);
 
 extern void scanner_pop_start_cond(void *scanner, enum startcond_type sc);
+extern void scanner_push_start_cond(void *scanner, enum startcond_type sc);
 
 #endif /* NFTABLES_PARSER_H */
diff --git a/src/parser_bison.y b/src/parser_bison.y
index bbac85fd35ce..a910d813e637 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -568,8 +568,8 @@ int nft_lex(void *, void *, void *);
 %token IN			"in"
 %token OUT			"out"
 
-%type <string>			identifier type_identifier string comment_spec
-%destructor { xfree($$); }	identifier type_identifier string comment_spec
+%type <string>			identifier type_identifier string comment_spec	implicit_table_name
+%destructor { xfree($$); }	identifier type_identifier string comment_spec	implicit_table_name
 
 %type <val>			time_spec quota_used
 
@@ -582,13 +582,13 @@ int nft_lex(void *, void *, void *);
 %type <cmd>			base_cmd add_cmd replace_cmd create_cmd insert_cmd delete_cmd get_cmd list_cmd reset_cmd flush_cmd rename_cmd export_cmd monitor_cmd describe_cmd import_cmd
 %destructor { cmd_free($$); }	base_cmd add_cmd replace_cmd create_cmd insert_cmd delete_cmd get_cmd list_cmd reset_cmd flush_cmd rename_cmd export_cmd monitor_cmd describe_cmd import_cmd
 
-%type <handle>			table_spec tableid_spec table_or_id_spec
-%destructor { handle_free(&$$); } table_spec tableid_spec table_or_id_spec
-%type <handle>			chain_spec chainid_spec chain_or_id_spec
-%destructor { handle_free(&$$); } chain_spec chainid_spec chain_or_id_spec
+%type <handle>			table_spec tableid_spec table_or_id_spec implicit_table_spec
+%destructor { handle_free(&$$); } table_spec tableid_spec table_or_id_spec implicit_table_spec
+%type <handle>			chain_spec chainid_spec chain_or_id_spec implicit_chain_spec
+%destructor { handle_free(&$$); } chain_spec chainid_spec chain_or_id_spec implicit_chain_spec
 
-%type <handle>			flowtable_spec chain_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec index_spec
-%destructor { handle_free(&$$); } flowtable_spec chain_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec index_spec
+%type <handle>			flowtable_spec chain_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec index_spec	implicit_rule_position
+%destructor { handle_free(&$$); } flowtable_spec chain_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec index_spec	implicit_rule_position
 %type <handle>			set_spec setid_spec set_or_id_spec
 %destructor { handle_free(&$$); } set_spec setid_spec set_or_id_spec
 %type <handle>			obj_spec objid_spec obj_or_id_spec
@@ -882,6 +882,7 @@ close_scope_socket	: { scanner_pop_start_cond(nft->scanner, PARSER_SC_EXPR_SOCKE
 
 close_scope_log		: { scanner_pop_start_cond(nft->scanner, PARSER_SC_STMT_LOG); }
 
+open_scope_chain	: { scanner_push_start_cond(nft->scanner, PARSER_SC_STRING_CHAIN); };
 common_block		:	INCLUDE		QUOTED_STRING	stmt_separator
 			{
 				if (scanner_include_file(nft, scanner, $2, &@$) < 0) {
@@ -998,7 +999,7 @@ add_cmd			:	TABLE		table_spec
 			{
 				$$ = cmd_alloc(CMD_ADD, CMD_OBJ_RULE, &$2, &@$, $3);
 			}
-			|	/* empty */	rule_position	rule
+			|	/* empty */	implicit_rule_position	rule
 			{
 				$$ = cmd_alloc(CMD_ADD, CMD_OBJ_RULE, &$1, &@$, $2);
 			}
@@ -2607,6 +2608,44 @@ rule_position		:	chain_spec
 			}
 			;
 
+implicit_table_name	:	FWD { $$ = xstrdup("fwd"); }
+			|	DUP { $$ = xstrdup("dup"); }
+			;
+
+implicit_table_spec	:	family_spec implicit_table_name
+			{
+				memset(&$$, 0, sizeof($$));
+				$$.family = $1;
+				$$.table.location = @2;
+				$$.table.name	= $2;
+			}
+			;
+
+implicit_chain_spec	:	open_scope_chain implicit_table_spec	identifier	close_scope_chain
+			{
+				$$		= $2;
+				$$.chain.name	= $3;
+				$$.chain.location = @3;
+			}
+			;
+
+implicit_rule_position	:	open_scope_chain rule_position { $$ = $2; }
+			|	implicit_chain_spec { $$ = $1; }
+			|	implicit_chain_spec position_spec { handle_merge(&$1, &$2); $$ = $1; }
+			|	implicit_chain_spec handle_spec {
+				$2.position.location = $2.handle.location;
+				$2.position.id = $2.handle.id;
+				$2.handle.id = 0;
+				handle_merge(&$1, &$2);
+				$$ = $1;
+			}
+			|	implicit_chain_spec index_spec
+			{
+				handle_merge(&$1, &$2);
+				$$ = $1;
+			}
+			;
+
 ruleid_spec		:	chain_spec	handle_spec
 			{
 				handle_merge(&$1, &$2);
diff --git a/src/scanner.l b/src/scanner.l
index a156accaa944..a4747b39b314 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -101,8 +101,6 @@ static void reset_pos(struct parser_state *state, struct location *loc)
 static int scanner_handle_tablename(void *scanner, const char *token);
 static int scanner_handle_chainname(void *scanner, const char *token);
 
-static void scanner_push_start_cond(void *scanner, enum startcond_type type);
-
 #define YY_USER_ACTION {					\
 	update_pos(yyget_extra(yyscanner), yylloc, yyleng);	\
 	update_offset(yyget_extra(yyscanner), yylloc, yyleng);	\
@@ -1087,7 +1085,7 @@ void scanner_destroy(struct nft_ctx *nft)
 	yylex_destroy(nft->scanner);
 }
 
-static void scanner_push_start_cond(void *scanner, enum startcond_type type)
+void scanner_push_start_cond(void *scanner, enum startcond_type type)
 {
 	struct parser_state *state = yyget_extra(scanner);
 
-- 
2.26.2

Re: [PATCH nft 6/6] src: allow arbitary chain name in implicit rule add case

[Phil Sutter]

Hi,

On Wed, Mar 17, 2021 at 12:40:39AM +0100, Florian Westphal wrote:
[...]
> Another alternative is to deprecate implicit rule add altogether
> so users would have to move to 'nft add rule ...'.

Isn't this required for nested syntax? I didn't check, but does your
arbitrary table/chain name support work also when restoring a ruleset in
that nested syntax? Another interesting aspect might be arbitrary set
names - 'set' is also a valid keyword used in rules, this fact killed my
approach with start conditions. ;)

Cheers, Phil

Re: [PATCH nft 6/6] src: allow arbitary chain name in implicit rule add case

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> > Another alternative is to deprecate implicit rule add altogether
> > so users would have to move to 'nft add rule ...'.
> 
> Isn't this required for nested syntax? I didn't check, but does your
> arbitrary table/chain name support work also when restoring a ruleset in
> that nested syntax?

Whats 'nested syntax'?

You mean "table bla { chain foo {"?

> Another interesting aspect might be arbitrary set
> names - 'set' is also a valid keyword used in rules, this fact killed my
> approach with start conditions. ;)

Right, arbitrary set names are needed as well, I forgot about them.

It should be possible by using two "set" rules in flex.

One in the INITIAL scope (to handle set bla {), and one in
'rule' or 'expression scope'.

The former would switch to an exclusive start condition (expect
STRING, close condition on '{', just like CHAIN is handled here.

The latter would not change state and just return SET token.

Re: [PATCH nft 6/6] src: allow arbitary chain name in implicit rule add case

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Allow switch of the flex state from bison parser.
> Note that this switch will happen too late to cover all cases:
> 
> nft add ip dup fwd ip saddr ...  # adds a rule to chain fwd in table dup
> nft add dup fwd ... # syntax error  (flex parses dup as expression keyword)
> 
> to solve this, bison must carry a list of keywords that are allowed to
> be used as table names.
> 
> This adds FWD as an example.  When new keywords are added, this can
> then be extended as needed.
> 
> Another alternative is to deprecate implicit rule add altogether
> so users would have to move to 'nft add rule ...'.

... and another alternative is to not allow arbitrary table/chain/set
names after all.

We could just say that all future tokens that could break existing
table/chain/set name need to be added to the 'identifier' in
parser_bison.y.

Provided new expressions with args use start conditionals the list
of tokens would probably stay short.

Given the 'set' complication Phil mentioned that might be the best
way forward.

Re: [PATCH nft 6/6] src: allow arbitary chain name in implicit rule add case

[Phil Sutter]

On Thu, Mar 18, 2021 at 01:37:24PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > > Another alternative is to deprecate implicit rule add altogether
> > > so users would have to move to 'nft add rule ...'.
> > 
> > Isn't this required for nested syntax? I didn't check, but does your
> > arbitrary table/chain name support work also when restoring a ruleset in
> > that nested syntax?
> 
> Whats 'nested syntax'?
> 
> You mean "table bla { chain foo {"?

Yes, exactly.

> > Another interesting aspect might be arbitrary set
> > names - 'set' is also a valid keyword used in rules, this fact killed my
> > approach with start conditions. ;)
> 
> Right, arbitrary set names are needed as well, I forgot about them.
> 
> It should be possible by using two "set" rules in flex.
> 
> One in the INITIAL scope (to handle set bla {), and one in
> 'rule' or 'expression scope'.
> 
> The former would switch to an exclusive start condition (expect
> STRING, close condition on '{', just like CHAIN is handled here.
> 
> The latter would not change state and just return SET token.

Yes, that might work.

Thanks, Phil

Re: [PATCH nft 6/6] src: allow arbitary chain name in implicit rule add case

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > Allow switch of the flex state from bison parser.
> > Note that this switch will happen too late to cover all cases:
> > 
> > nft add ip dup fwd ip saddr ...  # adds a rule to chain fwd in table dup
> > nft add dup fwd ... # syntax error  (flex parses dup as expression keyword)
> > 
> > to solve this, bison must carry a list of keywords that are allowed to
> > be used as table names.
> > 
> > This adds FWD as an example.  When new keywords are added, this can
> > then be extended as needed.
> > 
> > Another alternative is to deprecate implicit rule add altogether
> > so users would have to move to 'nft add rule ...'.
> 
> ... and another alternative is to not allow arbitrary table/chain/set
> names after all.
> 
> We could just say that all future tokens that could break existing
> table/chain/set name need to be added to the 'identifier' in
> parser_bison.y.
> 
> Provided new expressions with args use start conditionals the list
> of tokens would probably stay short.
> 
> Given the 'set' complication Phil mentioned that might be the best
> way forward.

I've pushed the first 3 patches and marked the last 3 as deferred --
lets first try conservative approach first before attempting to support
arbitrary names.