Hi Phil,
On Wed, Oct 03, 2018 at 09:00:51PM +0200, Phil Sutter wrote:
> On Wed, Oct 03, 2018 at 05:28:24PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Aug 24, 2018 at 01:26:57PM +0200, Phil Sutter wrote:
> > > Of all possible TCP flags, 'ecn' is special since it is recognized by
> > > lex as a keyword (there is a a field in IPv4 and IPv6 headers with the
> > > same name). Therefore it is listed in keyword_expr, but that was
> > > sufficient for RHS only. The following statement reproduces the issue:
> > >
> > > | tcp flags & (syn | ecn) == (syn | ecn)
> > >
> > > The solution is to limit binop expressions to accept an RHS expression
> > > on RHS ("real" LHS expressions don't make much sense there anyway),
> > > which then allows keyword_expr to occur there. In order to maintain the
> > > recursive behaviour if braces are present, allow primary_rhs_expr to
> > > consist of a basic_rhs_expr enclosed in braces. This in turn requires
> > > for braced RHS part in relational_expr to be dropped, otherwise bison
> > > complains about shift/reduce conflict.
> >
> > Sorry, I think I misunderstood this email.
> >
> > The following is:
> >
> > nft add rule x y tcp flags & (syn | ecn) == (syn | ecn)
> >
> > Same thing with:
> >
> > nft add rule x y 'tcp flags and (fin | syn | rst | psh | ack | urg | ecn | cwr) eq (fin | syn | rst | psh | ack | urg | ecn | cwr)
> >
> > So, what is what we don't support anymore after your patch?
>
> Yes, this was a misunderstanding. My patch doesn't limit functionality
> in any way (or at least it shouldn't :) - the original problem was that
> 'ecn' is recognized by scanner.l as a keyword, not a generic string
> (like the other flag names). The existing code handles this fine for
> RHS, e.g.:
>
> | tcp flags == ecn
>
> But on LHS, it wasn't possible to use 'ecn'. Simple example:
>
> | tcp flags & ecn == ecn
>
> The problem here is that 'and_expr' in parser_bison.y allows only
> 'shift_expr' after the '&' sign, while the 'ecn' keyword is contained in
> 'keyword_expr' which in turn is contained by the '*_rhs_expr's only.
Make senses.
> What my patch essentially does is change any of the binop expressions to
> accept a *_rhs_expr on their RHS (i.e., after the binop-specific
> symbol). This effectively makes the parser more strict: the rhs-variants
> don't contain all expressions the non-rhs-variants do. But in this case
> it should be correct, e.g. you wouldn't want to allow something like:
>
> | tcp flags & tcp dport == 0
Restricting the parser for things that don't make sense is a good idea
indeed, it helps reduce chances to hit more shift/reduce conflicts in
the future. The parser is slightly loose, taking probably more that it
can be processed at the evaluation and compilation step.
> My patch though caused a shift/reduce conflict. I could solve it by
> changing where the recursion (in bison) appears if braces are contained
> in the input. So I didn't change how braces may be specified in input,
> but "merely" what the parser resolves input containing braces into.
Thanks for explaining.
