[nft PATCH RFC] scanner: nat: Move to own scope

[nft PATCH RFC] scanner: nat: Move to own scope

[Phil Sutter]

Unify nat, masquerade and redirect statements, they widely share their
syntax.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
This seemingly valid change breaks the parser with this rule:

| snat ip prefix to ip saddr map { 10.141.11.0/24 : 192.168.2.0/24 }

Problem is that 'prefix' is not in SC_IP and close_scope_ip called from
parser_bison.y:5067 is not sufficient. I assumed explicit scope closing
would eliminate this lookahead problem. Did I find a proof against the
concept or is there a bug in my patch?

Thanks, Phil
---
 include/parser.h   |  1 +
 src/parser_bison.y | 13 +++++++------
 src/scanner.l      | 19 +++++++++++--------
 3 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/include/parser.h b/include/parser.h
index e8635b4c0feb7..5bb45fc4380e4 100644
--- a/include/parser.h
+++ b/include/parser.h
@@ -52,6 +52,7 @@ enum startcond_type {
 	PARSER_SC_EXPR_SOCKET,
 
 	PARSER_SC_STMT_LOG,
+	PARSER_SC_STMT_NAT,
 };
 
 struct mnl_socket;
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 83f0250a87449..2634b90c559b9 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -924,6 +924,7 @@ close_scope_vlan	: { scanner_pop_start_cond(nft->scanner, PARSER_SC_VLAN); };
 close_scope_ipsec	: { scanner_pop_start_cond(nft->scanner, PARSER_SC_EXPR_IPSEC); };
 close_scope_list	: { scanner_pop_start_cond(nft->scanner, PARSER_SC_CMD_LIST); };
 close_scope_limit	: { scanner_pop_start_cond(nft->scanner, PARSER_SC_LIMIT); };
+close_scope_nat		: { scanner_pop_start_cond(nft->scanner, PARSER_SC_STMT_NAT); };
 close_scope_numgen	: { scanner_pop_start_cond(nft->scanner, PARSER_SC_EXPR_NUMGEN); };
 close_scope_quota	: { scanner_pop_start_cond(nft->scanner, PARSER_SC_QUOTA); };
 close_scope_queue	: { scanner_pop_start_cond(nft->scanner, PARSER_SC_EXPR_QUEUE); };
@@ -2793,12 +2794,12 @@ stmt			:	verdict_stmt
 			|	meta_stmt
 			|	log_stmt	close_scope_log
 			|	reject_stmt
-			|	nat_stmt
+			|	nat_stmt	close_scope_nat
 			|	tproxy_stmt
 			|	queue_stmt
 			|	ct_stmt
-			|	masq_stmt
-			|	redir_stmt
+			|	masq_stmt	close_scope_nat
+			|	redir_stmt	close_scope_nat
 			|	dup_stmt
 			|	fwd_stmt
 			|	set_stmt
@@ -4708,8 +4709,8 @@ keyword_expr		:	ETHER   close_scope_eth { $$ = symbol_value(&@$, "ether"); }
 			|	IP6	close_scope_ip6 { $$ = symbol_value(&@$, "ip6"); }
 			|	VLAN	close_scope_vlan { $$ = symbol_value(&@$, "vlan"); }
 			|	ARP	close_scope_arp { $$ = symbol_value(&@$, "arp"); }
-			|	DNAT			{ $$ = symbol_value(&@$, "dnat"); }
-			|	SNAT			{ $$ = symbol_value(&@$, "snat"); }
+			|	DNAT	close_scope_nat	{ $$ = symbol_value(&@$, "dnat"); }
+			|	SNAT	close_scope_nat	{ $$ = symbol_value(&@$, "snat"); }
 			|	ECN			{ $$ = symbol_value(&@$, "ecn"); }
 			|	RESET			{ $$ = symbol_value(&@$, "reset"); }
 			|	ORIGINAL		{ $$ = symbol_value(&@$, "original"); }
@@ -4798,7 +4799,7 @@ primary_rhs_expr	:	symbol_expr		{ $$ = $1; }
 							 BYTEORDER_HOST_ENDIAN,
 							 sizeof(data) * BITS_PER_BYTE, &data);
 			}
-			|	REDIRECT
+			|	REDIRECT	close_scope_nat
 			{
 				uint8_t data = ICMP_REDIRECT;
 				$$ = constant_expr_alloc(&@$, &icmp_type_type,
diff --git a/src/scanner.l b/src/scanner.l
index 6cc7778dd85e1..f1e0162b0ae5e 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -218,6 +218,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 %s SCANSTATE_EXPR_SOCKET
 
 %s SCANSTATE_STMT_LOG
+%s SCANSTATE_STMT_NAT
 
 %%
 
@@ -366,7 +367,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "quotas"		{ return QUOTAS; }
 
 "log"			{ scanner_push_start_cond(yyscanner, SCANSTATE_STMT_LOG); return LOG; }
-"prefix"		{ return PREFIX; }
+<SCANSTATE_STMT_LOG,SCANSTATE_STMT_NAT>"prefix"		{ return PREFIX; }
 "group"			{ return GROUP; }
 <SCANSTATE_STMT_LOG>{
 	"snaplen"		{ return SNAPLEN; }
@@ -403,13 +404,16 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "with"			{ return WITH; }
 "icmpx"			{ return ICMPX; }
 
-"snat"			{ return SNAT; }
-"dnat"			{ return DNAT; }
-"masquerade"		{ return MASQUERADE; }
-"redirect"		{ return REDIRECT; }
+"snat"			{ scanner_push_start_cond(yyscanner, SCANSTATE_STMT_NAT); return SNAT; }
+"dnat"			{ scanner_push_start_cond(yyscanner, SCANSTATE_STMT_NAT); return DNAT; }
+"masquerade"		{ scanner_push_start_cond(yyscanner, SCANSTATE_STMT_NAT); return MASQUERADE; }
+"redirect"		{ scanner_push_start_cond(yyscanner, SCANSTATE_STMT_NAT); return REDIRECT; }
 "random"		{ return RANDOM; }
-"fully-random"		{ return FULLY_RANDOM; }
-"persistent"		{ return PERSISTENT; }
+<SCANSTATE_STMT_NAT>{
+	"fully-random"		{ return FULLY_RANDOM; }
+	"persistent"		{ return PERSISTENT; }
+	"port"			{ return PORT; }
+}
 
 "ll"			{ return LL_HDR; }
 "nh"			{ return NETWORK_HDR; }
@@ -523,7 +527,6 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "udplite"		{ return UDPLITE; }
 "sport"			{ return SPORT; }
 "dport"			{ return DPORT; }
-"port"			{ return PORT; }
 
 "tcp"			{ return TCP; }
 "ackseq"		{ return ACKSEQ; }
-- 
2.32.0

Re: [nft PATCH RFC] scanner: nat: Move to own scope

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Unify nat, masquerade and redirect statements, they widely share their
> syntax.
> This seemingly valid change breaks the parser with this rule:
> 
> | snat ip prefix to ip saddr map { 10.141.11.0/24 : 192.168.2.0/24 }

Yes.

> Problem is that 'prefix' is not in SC_IP and close_scope_ip called from
> parser_bison.y:5067 is not sufficient. I assumed explicit scope closing
> would eliminate this lookahead problem. Did I find a proof against the
> concept or is there a bug in my patch?

You have to keep 'prefix' in the global scope.
What should work as well is to permit 'prefix' from SCANSTATE_IP(6).

The problem is that the parser can't close the new 'IP' scope until
it has enough tokens available to match a complete bison rule.

So, it is in IP scope, sees 'prefix' (which will be STRING as the
PREFIX scan rule is off) and that ends up in a parser error due to lack
of a 'IP STRING' rule.

Re: [nft PATCH RFC] scanner: nat: Move to own scope

[Phil Sutter]

On Mon, Aug 09, 2021 at 05:18:33PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Unify nat, masquerade and redirect statements, they widely share their
> > syntax.
> > This seemingly valid change breaks the parser with this rule:
> > 
> > | snat ip prefix to ip saddr map { 10.141.11.0/24 : 192.168.2.0/24 }
> 
> Yes.
> 
> > Problem is that 'prefix' is not in SC_IP and close_scope_ip called from
> > parser_bison.y:5067 is not sufficient. I assumed explicit scope closing
> > would eliminate this lookahead problem. Did I find a proof against the
> > concept or is there a bug in my patch?
> 
> You have to keep 'prefix' in the global scope.
> What should work as well is to permit 'prefix' from SCANSTATE_IP(6).
> 
> The problem is that the parser can't close the new 'IP' scope until
> it has enough tokens available to match a complete bison rule.
> 
> So, it is in IP scope, sees 'prefix' (which will be STRING as the
> PREFIX scan rule is off) and that ends up in a parser error due to lack
> of a 'IP STRING' rule.

OK, thanks. So does this mean we won't ever be able to move keywords
opening a statement or expression out of INIT scope or is my case a
special one?

To clarify, what I have in mind is a sample rule 'ip id 1 tcp dport 1'
where 'tcp' must either be in INIT scope or part of SC_IP. 

Cheers, Phil

Re: [nft PATCH RFC] scanner: nat: Move to own scope

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> > So, it is in IP scope, sees 'prefix' (which will be STRING as the
> > PREFIX scan rule is off) and that ends up in a parser error due to lack
> > of a 'IP STRING' rule.
> 
> OK, thanks. So does this mean we won't ever be able to move keywords
> opening a statement or expression out of INIT scope or is my case a
> special one?

We can move them out of INIT scope, but only if they remain within
the same scope.

For example, you could create a 'rule scope' or 'expression scope'
that contains all tokens (ip, tcp, etc.) that start a new expression.

I did not do this because there are too many cases where expressions are
permitted outside of rule scope, e.g. in set definitions (key, elements
...)

> To clarify, what I have in mind is a sample rule 'ip id 1 tcp dport 1'
> where 'tcp' must either be in INIT scope or part of SC_IP. 

IP and TCP need to be in the same scope (e.g. INIT).

In the given example I suspect that TCP doesn't have to be in SC_IP
scope since 'ip id 1' is a full expression and scope closure happens
before next token gets scanned.

Re: [nft PATCH RFC] scanner: nat: Move to own scope

[Phil Sutter]

On Mon, Aug 09, 2021 at 08:45:59PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > > So, it is in IP scope, sees 'prefix' (which will be STRING as the
> > > PREFIX scan rule is off) and that ends up in a parser error due to lack
> > > of a 'IP STRING' rule.
> > 
> > OK, thanks. So does this mean we won't ever be able to move keywords
> > opening a statement or expression out of INIT scope or is my case a
> > special one?
> 
> We can move them out of INIT scope, but only if they remain within
> the same scope.
> 
> For example, you could create a 'rule scope' or 'expression scope'
> that contains all tokens (ip, tcp, etc.) that start a new expression.

Ah, OK!

> I did not do this because there are too many cases where expressions are
> permitted outside of rule scope, e.g. in set definitions (key, elements
> ...)
> 
> > To clarify, what I have in mind is a sample rule 'ip id 1 tcp dport 1'
> > where 'tcp' must either be in INIT scope or part of SC_IP. 
> 
> IP and TCP need to be in the same scope (e.g. INIT).
> 
> In the given example I suspect that TCP doesn't have to be in SC_IP
> scope since 'ip id 1' is a full expression and scope closure happens
> before next token gets scanned.

Now I got it: The problem with nat_stmt is that 'snat ip' may either be
'SNAT nf_key_proto' or 'SNAT stmt_expr' and bison carries on until it's
clear. I missed the fact that stmt_expr is allowed in nat_stmt_args.

Thanks for the clarification! Maybe one day I'll finally understand how
bison really works. %)

Cheers, Phil