Re: [PATCH nft 2/3] netlink_linearize: incorrect netlink bytecode with binary operation and flags

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Thu, Jul 29, 2021 at 09:48:21AM +0800, Tom Yan wrote:
> I'm not sure it's just me or you that are missing something here.
> 
> On Wed, 28 Jul 2021 at 05:05, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > On Wed, Jul 28, 2021 at 02:36:18AM +0800, Tom Yan wrote:
> > > Hmm, that means `tcp flags & (fin | syn | rst | ack) syn` is now
> > > equivalent to 'tcp flags & (fin | syn | rst | ack) == syn'.
> >
> > Yes, those two are equivalent.
> >
> > > Does that mean `tcp flags syn` (was supposed to be and) is now
> > > equivalent to `tcp flags == syn`
> >
> > tcp flag syn
> >
> > is a shortcut to match on the syn bit regarless other bit values, it's
> > a property of the bitmask datatypes.
> 
> Don't you think the syntax will be inconsistent then? As a user, it
> looks pretty irrational to me: with a mask, just `syn` checks the
> exact value of the flags (combined); without a mask, it checks and
> checks only whether the specific bit is on.
> 
> At least to me I would then expect `tcp flags syn` should be
> equivalent / is a shortcut to `tcp flags & (fin | syn | rst | psh |
> ack | urg | ecn | cwr) syn` hence `tcp flags & (fin | syn | rst | psh
> | ack | urg | ecn | cwr) == syn` hence `tcp flags == syn`.

As I said, think of a different use-case:

        ct state new,established

people are _not_ expecting to match on both flags to be set on (that
will actually never happen).

Should ct state and tcp flags use the same syntax (comma-separated
values) but intepret things in a different way? I don't think so.

You can use:

        nft describe ct state

to check for the real datatype behind this selector: it's a bitmask.
For this datatype the implicit operation is not ==.

> > tcp flags == syn
> >
> > is an exact match, it checks that the syn bit is set on.
> >
> > > instead of `tcp flags & syn == syn` / `tcp flags & syn != 0`?
> >
> > these two above are equivalent, I just sent a patch to fix the
> > tcp flags & syn == syn case.
> >
> > > Suppose `tcp flags & syn != 0` should then be translated to `tcp flags
> > > syn / syn` instead, please note that while nft translates `tcp flags &
> > > syn == syn` to `tcp flags syn / syn`, it does not accept the
> > > translation as input (when the mask is not a comma-separated list):
> > >
> > > # nft --debug=netlink add rule meh tcp_flags 'tcp flags syn / syn'
> > > Error: syntax error, unexpected newline, expecting comma
> > > add rule meh tcp_flags tcp flags syn / syn
> > >                                           ^
> >
> > The most simple way to express this is: tcp flags == syn.
> 
> That does not sound right to me at all. Doesn't `syn / syn` means
> "with the mask (the second/"denominator" `syn`) applied on the flags,
> we get the value (the first/"nominator" `syn`), which means `tcp flags
> & syn == syn` instead of `tcp flags == syn` (which in turn means all
> bits but syn are cleared).

tcp flags syn / syn makes no sense, it's actually: tcp flags syn.

The goal is to provide a compact syntax for most common operations, so
users do not need to be mingling with explicit binary expressions
(which is considered to be a "more advanced" operation).