* [nft PATCH RFC] scanner: nat: Move to own scope
@ 2021-08-09 14:01 Phil Sutter
2021-08-09 15:18 ` Florian Westphal
0 siblings, 1 reply; 5+ messages in thread
From: Phil Sutter @ 2021-08-09 14:01 UTC (permalink / raw)
To: Florian Westphal; +Cc: Pablo Neira Ayuso, netfilter-devel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [nft PATCH RFC] scanner: nat: Move to own scope
2021-08-09 14:01 [nft PATCH RFC] scanner: nat: Move to own scope Phil Sutter
@ 2021-08-09 15:18 ` Florian Westphal
2021-08-09 16:25 ` Phil Sutter
0 siblings, 1 reply; 5+ messages in thread
From: Florian Westphal @ 2021-08-09 15:18 UTC (permalink / raw)
To: Phil Sutter; +Cc: Florian Westphal, Pablo Neira Ayuso, netfilter-devel
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [nft PATCH RFC] scanner: nat: Move to own scope
2021-08-09 15:18 ` Florian Westphal
@ 2021-08-09 16:25 ` Phil Sutter
2021-08-09 18:45 ` Florian Westphal
0 siblings, 1 reply; 5+ messages in thread
From: Phil Sutter @ 2021-08-09 16:25 UTC (permalink / raw)
To: Florian Westphal; +Cc: Pablo Neira Ayuso, netfilter-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [nft PATCH RFC] scanner: nat: Move to own scope
2021-08-09 16:25 ` Phil Sutter
@ 2021-08-09 18:45 ` Florian Westphal
2021-08-10 11:19 ` Phil Sutter
0 siblings, 1 reply; 5+ messages in thread
From: Florian Westphal @ 2021-08-09 18:45 UTC (permalink / raw)
To: Phil Sutter, Florian Westphal, Pablo Neira Ayuso, netfilter-devel
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [nft PATCH RFC] scanner: nat: Move to own scope
2021-08-09 18:45 ` Florian Westphal
@ 2021-08-10 11:19 ` Phil Sutter
0 siblings, 0 replies; 5+ messages in thread
From: Phil Sutter @ 2021-08-10 11:19 UTC (permalink / raw)
To: Florian Westphal; +Cc: Pablo Neira Ayuso, netfilter-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-08-10 11:19 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-09 14:01 [nft PATCH RFC] scanner: nat: Move to own scope Phil Sutter
2021-08-09 15:18 ` Florian Westphal
2021-08-09 16:25 ` Phil Sutter
2021-08-09 18:45 ` Florian Westphal
2021-08-10 11:19 ` Phil Sutter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.