Re: [nft PATCH v4 08/32] netlink: send bit-length of bitwise binops to kernel

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

Hi,

I'm sorry for taking time to get back to this, I have questions.

On Mon, Apr 04, 2022 at 01:13:46PM +0100, Jeremy Sowden wrote:
> Some bitwise operations are generated when munging paylod expressions.
> During delinearization, we attempt to eliminate these operations.
> However, this is done before deducing the byte-order or the correct
> length in bits of the operands, which means that we don't always handle
> multi-byte host-endian operations correctly.  Therefore, pass the
> bit-length of these expressions to the kernel in order to have it
> available during delinearization.
> 
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> ---
>  src/netlink_delinearize.c | 14 ++++++++++++--
>  src/netlink_linearize.c   |  2 ++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
> index a1b00dee209a..733977bc526d 100644
> --- a/src/netlink_delinearize.c
> +++ b/src/netlink_delinearize.c
> @@ -451,20 +451,28 @@ static struct expr *netlink_parse_bitwise_bool(struct netlink_parse_ctx *ctx,
>  					       const struct nftnl_expr *nle,
>  					       enum nft_registers sreg,
>  					       struct expr *left)
> -
>  {
>  	struct nft_data_delinearize nld;
>  	struct expr *expr, *mask, *xor, *or;
> +	unsigned int nbits;
>  	mpz_t m, x, o;
>  
>  	expr = left;
>  
> +	nbits = nftnl_expr_get_u32(nle, NFTNL_EXPR_BITWISE_NBITS);
> +	if (nbits > 0)
> +		expr->len = nbits;

So NFTNL_EXPR_BITWISE_NBITS is signalling that this is an implicit
bitwise that has been generated to operate with a payload header bitfield?

Could you provide an example expression tree to show how this
simplifies delinearization?

> +
>  	nld.value = nftnl_expr_get(nle, NFTNL_EXPR_BITWISE_MASK, &nld.len);
>  	mask = netlink_alloc_value(loc, &nld);
> +	if (nbits > 0)
> +		mpz_switch_byteorder(mask->value, div_round_up(nbits, BITS_PER_BYTE));

What is the byteorder expected for the mask before this switch
operation?

>  	mpz_init_set(m, mask->value);
>  
>  	nld.value = nftnl_expr_get(nle, NFTNL_EXPR_BITWISE_XOR, &nld.len);
> -	xor  = netlink_alloc_value(loc, &nld);
> +	xor = netlink_alloc_value(loc, &nld);
> +	if (nbits > 0)
> +		mpz_switch_byteorder(xor->value, div_round_up(nbits, BITS_PER_BYTE));
>  	mpz_init_set(x, xor->value);
>  
>  	mpz_init_set_ui(o, 0);
> @@ -500,6 +508,8 @@ static struct expr *netlink_parse_bitwise_bool(struct netlink_parse_ctx *ctx,
>  
>  		or = netlink_alloc_value(loc, &nld);
>  		mpz_set(or->value, o);
> +		if (nbits > 0)
> +			mpz_switch_byteorder(or->value, div_round_up(nbits, BITS_PER_BYTE));
>  		expr = binop_expr_alloc(loc, OP_OR, expr, or);
>  		expr->len = left->len;
>  	}
> diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
> index c8bbcb7452b0..4793f3853bee 100644
> --- a/src/netlink_linearize.c
> +++ b/src/netlink_linearize.c
> @@ -677,6 +677,8 @@ static void netlink_gen_bitwise(struct netlink_linearize_ctx *ctx,
>  	netlink_put_register(nle, NFTNL_EXPR_BITWISE_DREG, dreg);
>  	nftnl_expr_set_u32(nle, NFTNL_EXPR_BITWISE_OP, NFT_BITWISE_BOOL);
>  	nftnl_expr_set_u32(nle, NFTNL_EXPR_BITWISE_LEN, len);
> +	if (expr->byteorder == BYTEORDER_HOST_ENDIAN)
> +		nftnl_expr_set_u32(nle, NFTNL_EXPR_BITWISE_NBITS, expr->len);

Why is this only required for host endian expressions?

>  	netlink_gen_raw_data(mask, expr->byteorder, len, &nld);
>  	nftnl_expr_set(nle, NFTNL_EXPR_BITWISE_MASK, nld.value, nld.len);
> -- 
> 2.35.1
>