On Thu, Sep 11, 2025 at 10:14:11AM +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 10, 2025 at 12:48:46AM +0200, Phil Sutter wrote:
> > Hi Pablo,
> >
> > On Tue, Sep 09, 2025 at 11:34:00PM +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Sep 09, 2025 at 10:49:48PM +0200, Phil Sutter wrote:
> > > > Adjust the expression size to 1B so cmp expression value is correct.
> > > > Without this, the rule 'fib saddr . iif check exists' generates
> > > > following byte code on BE:
> > > >
> > > > | [ fib saddr . iif oif present => reg 1 ]
> > > > | [ cmp eq reg 1 0x00000001 ]
> > > >
> > > > Though with NFTA_FIB_F_PRESENT flag set, nft_fib.ko writes to the first
> > > > byte of reg 1 only (using nft_reg_store8()). With this patch in place,
> > > > byte code is correct:
> > > >
> > > > | [ fib saddr . iif oif present => reg 1 ]
> > > > | [ cmp eq reg 1 0x01000000 ]
> > >
> > > Is this a generic issue of boolean that is using 1 bit?
> > >
> > > const struct datatype boolean_type = {
> > > .type = TYPE_BOOLEAN,
> > > .name = "boolean",
> > > .desc = "boolean type",
> > > .size = 1,
> >
> > Maybe, yes: I compared fib existence checks to exthdr ones in order to
> > find the bug. With exthdr, we know in parser already that it is an
> > existence check (see exthdr_exists_expr rule in parser_bison.y). If so,
> > exthdr expression is allocated with type 1 which is (assumed to be) the
> > NEXTHDR field in all extension headers. This field has
> > inet_protocol_type, which is size 8b.
> >
> > Via expr_ctx::len, RHS will then be adjusted to 8b size (see 'expr->len =
> > masklen' in expr_evaluate_integer()).
> >
> > IIRC, LHS defines the RHS size in relationals. I am not sure if we may
> > sanely reverse this rule if RHS is a boolean_type.
>
> Probably this fix is more generic, see untested patch.
> diff --git a/src/datatype.c b/src/datatype.c
> index f347010f4a1a..2d39239316a6 100644
> --- a/src/datatype.c
> +++ b/src/datatype.c
> @@ -1581,7 +1581,7 @@ const struct datatype boolean_type = {
> .type = TYPE_BOOLEAN,
> .name = "boolean",
> .desc = "boolean type",
> - .size = 1,
> + .size = BITS_PER_BYTE,
> .parse = boolean_type_parse,
> .basetype = &integer_type,
> .sym_tbl = &boolean_tbl,
This does not make a difference. Since the fib expr::len remains at
value 32, the cmp payload will be 32 bits as well with the first three
bytes being zero on BE irrespective of RHS value.
Cheers, Phil
