[nft PATCH] fib: Fix for existence check on Big Endian

[nft PATCH] fib: Fix for existence check on Big Endian

[Phil Sutter]

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 ]

Fixes: f686a17eafa0b ("fib: Support existence check")
Cc: Yi Chen <yiche@redhat.com>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/evaluate.c | 1 +
 src/fib.c      | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 8cecbe09de01c..6a1aa4963bceb 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3002,6 +3002,7 @@ static int expr_evaluate_fib(struct eval_ctx *ctx, struct expr **exprp)
 	if (expr->flags & EXPR_F_BOOLEAN) {
 		expr->fib.flags |= NFTA_FIB_F_PRESENT;
 		datatype_set(expr, &boolean_type);
+		expr->len = BITS_PER_BYTE;
 	}
 	return expr_evaluate_primary(ctx, exprp);
 }
diff --git a/src/fib.c b/src/fib.c
index 5383613292a5e..4db7cd2bbc9c3 100644
--- a/src/fib.c
+++ b/src/fib.c
@@ -198,8 +198,10 @@ struct expr *fib_expr_alloc(const struct location *loc,
 		BUG("Unknown result %d\n", result);
 	}
 
-	if (flags & NFTA_FIB_F_PRESENT)
+	if (flags & NFTA_FIB_F_PRESENT) {
 		type = &boolean_type;
+		len = BITS_PER_BYTE;
+	}
 
 	expr = expr_alloc(loc, EXPR_FIB, type,
 			  BYTEORDER_HOST_ENDIAN, len);
-- 
2.51.0

Re: [nft PATCH] fib: Fix for existence check on Big Endian

[Pablo Neira Ayuso]

Hi Phil,

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,

> Fixes: f686a17eafa0b ("fib: Support existence check")
> Cc: Yi Chen <yiche@redhat.com>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/evaluate.c | 1 +
>  src/fib.c      | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 8cecbe09de01c..6a1aa4963bceb 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -3002,6 +3002,7 @@ static int expr_evaluate_fib(struct eval_ctx *ctx, struct expr **exprp)
>  	if (expr->flags & EXPR_F_BOOLEAN) {
>  		expr->fib.flags |= NFTA_FIB_F_PRESENT;
>  		datatype_set(expr, &boolean_type);
> +		expr->len = BITS_PER_BYTE;
>  	}
>  	return expr_evaluate_primary(ctx, exprp);
>  }
> diff --git a/src/fib.c b/src/fib.c
> index 5383613292a5e..4db7cd2bbc9c3 100644
> --- a/src/fib.c
> +++ b/src/fib.c
> @@ -198,8 +198,10 @@ struct expr *fib_expr_alloc(const struct location *loc,
>  		BUG("Unknown result %d\n", result);
>  	}
>  
> -	if (flags & NFTA_FIB_F_PRESENT)
> +	if (flags & NFTA_FIB_F_PRESENT) {
>  		type = &boolean_type;
> +		len = BITS_PER_BYTE;
> +	}
>  
>  	expr = expr_alloc(loc, EXPR_FIB, type,
>  			  BYTEORDER_HOST_ENDIAN, len);
> -- 
> 2.51.0
> 

Re: [nft PATCH] fib: Fix for existence check on Big Endian

[Phil Sutter]

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.

Cheers, Phil

Re: [nft PATCH] fib: Fix for existence check on Big Endian

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1737 bytes --]

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.

[-- Attachment #2: y.patch --]
[-- Type: text/x-diff, Size: 390 bytes --]

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,

Re: [nft PATCH] fib: Fix for existence check on Big Endian

[Phil Sutter]

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

Re: [nft PATCH] fib: Fix for existence check on Big Endian

[Pablo Neira Ayuso]

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 ]
> 
> Fixes: f686a17eafa0b ("fib: Support existence check")
> Cc: Yi Chen <yiche@redhat.com>
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org>

Thanks.

Re: [nft PATCH] fib: Fix for existence check on Big Endian

[Phil Sutter]

On Thu, Sep 11, 2025 at 04:20:51PM +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 ]
> > 
> > Fixes: f686a17eafa0b ("fib: Support existence check")
> > Cc: Yi Chen <yiche@redhat.com>
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> 
> Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org>

Patch applied, thanks!