[PATCH nft] scanner: enable verdicts in rate scope too

[PATCH nft] scanner: enable verdicts in rate scope too

[Florian Westphal]

Blamed commit added first use of exclusive start conditions in the
nftables parser.

Inclusive start conditions extend the grammar:
 - token NEW_COND will start to match in scanner.l
 - all other known tokens remain valid
 - bison calls back into scanner.l (via scanner_pop_start_cond())
   when it has parsed a complete rule.

Exclusive start conditions work in the same way, but with a slight
difference: Once we enter SCANSTATE_RATE, ALL OTHER tokens that are
not SCANSTATE_RATE or tagged as <*>, will stop matching.

This was done to allow something like 1 mbytes/second get handled
via NUM MBYTES SLASH SECOND rather than STRING (which is no longer
in scope).

This is a problem: in nftables, there is no end-token for
the 'rate' keyword.  The scanstate is popped the same way as for
the inclusive start conditions.  But flex is greedy.  By the time
we call scanner_pop_start_cond(), next token has already been parsed
according to RATE rules.

This breaks following rule:
  nft add rule .. limit rate over 1 mbytes/second drop
  Error: syntax error, unexpected junk
  expected any of: end of file, newline, semicolon, [..] drop, continue ...

Problem is that flex parsed 'd'(rop) while in SCANSTATE_RATE.

There is no good solution for this problem (aside from altering the
grammar to be explicitly scoped, e.g. limit rate { over ... }

Another alternative might be to add some string-alike catchall rule to
<SCANSTATE_RATE> in scanner.l and use that to pop state + rescan via
yyless(0).  But it feels even more gross than this.

Technially we'd have to <*> a lot more keywords, e.g.

 rate over 1 mbytes/second ip saddr 1.2.3.4

is valid (or should be).  Its not allowed anymore either.
It makes a bit less sense to add expressions after a rate limiter, however.

Fixes: 9d105581b5f1 ("scanner: Introduce SCANSTATE_RATE")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/scanner.l | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/scanner.l b/src/scanner.l
index 1b4eb1cf13a4..bd66cf7bbfa1 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -348,12 +348,15 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 }
 "tproxy"		{ scanner_push_start_cond(yyscanner, SCANSTATE_STMT_TPROXY); return TPROXY; }
 
+<*>{
 "accept"		{ return ACCEPT; }
 "drop"			{ return DROP; }
 "continue"		{ return CONTINUE; }
 "jump"			{ return JUMP; }
 "goto"			{ return GOTO; }
 "return"		{ return RETURN; }
+}
+
 <SCANSTATE_EXPR_QUEUE,SCANSTATE_STMT_DUP,SCANSTATE_STMT_FWD,SCANSTATE_STMT_NAT,SCANSTATE_STMT_TPROXY,SCANSTATE_IP,SCANSTATE_IP6>"to"			{ return TO; } /* XXX: SCANSTATE_IP is a workaround */
 
 "inet"			{ return INET; }
-- 
2.53.0

Re: [PATCH nft] scanner: enable verdicts in rate scope too

[Phil Sutter]

Hi,

On Tue, May 05, 2026 at 12:37:30PM +0200, Florian Westphal wrote:
> Blamed commit added first use of exclusive start conditions in the
> nftables parser.
> 
> Inclusive start conditions extend the grammar:
>  - token NEW_COND will start to match in scanner.l
>  - all other known tokens remain valid
>  - bison calls back into scanner.l (via scanner_pop_start_cond())
>    when it has parsed a complete rule.
> 
> Exclusive start conditions work in the same way, but with a slight
> difference: Once we enter SCANSTATE_RATE, ALL OTHER tokens that are
> not SCANSTATE_RATE or tagged as <*>, will stop matching.
> 
> This was done to allow something like 1 mbytes/second get handled
> via NUM MBYTES SLASH SECOND rather than STRING (which is no longer
> in scope).
> 
> This is a problem: in nftables, there is no end-token for
> the 'rate' keyword.  The scanstate is popped the same way as for
> the inclusive start conditions.  But flex is greedy.  By the time
> we call scanner_pop_start_cond(), next token has already been parsed
> according to RATE rules.

Oh crap, sorry for the mess. Looks like I relied too much on 'make
check' instead of verifying Bison's lookahead doesn't break it.

> This breaks following rule:
>   nft add rule .. limit rate over 1 mbytes/second drop
>   Error: syntax error, unexpected junk
>   expected any of: end of file, newline, semicolon, [..] drop, continue ...
> 
> Problem is that flex parsed 'd'(rop) while in SCANSTATE_RATE.

Yes, this is the '<*>. { return JUNK; }' rule which catches invalid
input.

> There is no good solution for this problem (aside from altering the
> grammar to be explicitly scoped, e.g. limit rate { over ... }

Not an option, right?

> Another alternative might be to add some string-alike catchall rule to
> <SCANSTATE_RATE> in scanner.l and use that to pop state + rescan via
> yyless(0).  But it feels even more gross than this.

I tested this using the following in scanner.l:

-<*>.                   { return JUNK; }
+<SCANSTATE_RATE>.      { scanner_pop_start_cond(yyscanner, SCANSTATE_RATE); yyless(0); }
+.                      { return JUNK; }

It causes "start-condition stack underflow" with below sample rule, so I
also had to turn 'close_scope_rate' into a NOP in parser_bison.y. Is
this implicit scan state dropping upon reading junk OK? Should we follow
that path with exclusive start conditions?

> Technially we'd have to <*> a lot more keywords, e.g.
> 
>  rate over 1 mbytes/second ip saddr 1.2.3.4
> 
> is valid (or should be).  Its not allowed anymore either.
> It makes a bit less sense to add expressions after a rate limiter, however.

Explicitly marking "global" tokens as such could be regarded syntactic
sugar for further attempts at reducing tokens accepted in global scope.
If you think that's fine, I'll prepare a patch (and an excessive test
case, I guess).

Cheers, Phil

Re: [PATCH nft] scanner: enable verdicts in rate scope too

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> On Tue, May 05, 2026 at 12:37:30PM +0200, Florian Westphal wrote:
> > Blamed commit added first use of exclusive start conditions in the
> > nftables parser.
> > 
> > Inclusive start conditions extend the grammar:
> >  - token NEW_COND will start to match in scanner.l
> >  - all other known tokens remain valid
> >  - bison calls back into scanner.l (via scanner_pop_start_cond())
> >    when it has parsed a complete rule.
> > 
> > Exclusive start conditions work in the same way, but with a slight
> > difference: Once we enter SCANSTATE_RATE, ALL OTHER tokens that are
> > not SCANSTATE_RATE or tagged as <*>, will stop matching.
> > 
> > This was done to allow something like 1 mbytes/second get handled
> > via NUM MBYTES SLASH SECOND rather than STRING (which is no longer
> > in scope).
> > 
> > This is a problem: in nftables, there is no end-token for
> > the 'rate' keyword.  The scanstate is popped the same way as for
> > the inclusive start conditions.  But flex is greedy.  By the time
> > we call scanner_pop_start_cond(), next token has already been parsed
> > according to RATE rules.
> 
> Oh crap, sorry for the mess. Looks like I relied too much on 'make
> check' instead of verifying Bison's lookahead doesn't break it.
> 
> > This breaks following rule:
> >   nft add rule .. limit rate over 1 mbytes/second drop
> >   Error: syntax error, unexpected junk
> >   expected any of: end of file, newline, semicolon, [..] drop, continue ...
> > 
> > Problem is that flex parsed 'd'(rop) while in SCANSTATE_RATE.
> 
> Yes, this is the '<*>. { return JUNK; }' rule which catches invalid
> input.
> 
> > There is no good solution for this problem (aside from altering the
> > grammar to be explicitly scoped, e.g. limit rate { over ... }
> 
> Not an option, right?
> 
> > Another alternative might be to add some string-alike catchall rule to
> > <SCANSTATE_RATE> in scanner.l and use that to pop state + rescan via
> > yyless(0).  But it feels even more gross than this.
> 
> I tested this using the following in scanner.l:
> 
> -<*>.                   { return JUNK; }
> +<SCANSTATE_RATE>.      { scanner_pop_start_cond(yyscanner, SCANSTATE_RATE); yyless(0); }
> +.                      { return JUNK; }
> 
> It causes "start-condition stack underflow" with below sample rule, so I
> also had to turn 'close_scope_rate' into a NOP in parser_bison.y.

Yes, I forgot to mention this.  Yes, with above rule RATE state exits
from scanner.l only.

> Is
> this implicit scan state dropping upon reading junk OK? Should we follow
> that path with exclusive start conditions?

No idea.  I *think* enabling the relevant keywords via <*> is the safer
approach, yyless undo seems worse to me.

> Explicitly marking "global" tokens as such could be regarded syntactic
> sugar for further attempts at reducing tokens accepted in global scope.
> If you think that's fine, I'll prepare a patch (and an excessive test
> case, I guess).

I think its fine.  Thanks for extending test cases.