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

[Phil Sutter <phil@nwl.cc>]

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