Re: [PATCH nft v2 1/3] netlink_delinearize: add missing icmp id/sequence support

[Phil Sutter <phil@nwl.cc>]

Hi,

On Tue, Jun 15, 2021 at 06:01:49PM +0200, Florian Westphal wrote:
> Pablo reports following input and output:
> in: icmpv6 id 1
> out: icmpv6 type { echo-request, echo-reply } icmpv6 parameter-problem 65536/16
> 
> Reason is that icmp fields overlap, decoding of the correct name requires
> check of the icmpv6 type.  This only works for equality tests, for
> instance
> 
> in: icmpv6 type echo-request icmpv6 id 1
> will be listed as "icmpv6 id 1" (which is not correct either, since the
> input only matches on echo-request).
> 
> with this patch, output of 'icmpv6 id 1' is
> icmpv6 type { echo-request, echo-reply } icmpv6 id 1
> 
> The second problem, the removal of a single check (request OR reply),
> is resolved in the followup patch.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

Eric reported a testcase in which this patch seems to cause a segfault
(bisected). The test is as simple as:

| nft -f - <<EOF
| add table inet firewalld_check_rule_index
| add chain inet firewalld_check_rule_index foobar { type filter hook input priority 0 ; }
| add rule inet firewalld_check_rule_index foobar tcp dport 1234 accept
| add rule inet firewalld_check_rule_index foobar accept
| insert rule inet firewalld_check_rule_index foobar index 1 udp dport 4321 accept
| EOF

But a ruleset is in place at this time. Also, I can't reproduce it on my
own machine but only on Eric's VM for testing.

[...]
>  static void payload_match_postprocess(struct rule_pp_ctx *ctx,
>  				      struct expr *expr,
>  				      struct expr *payload)
> @@ -1883,6 +1932,19 @@ static void payload_match_postprocess(struct rule_pp_ctx *ctx,
>  		if (expr->right->etype == EXPR_VALUE) {
>  			payload_match_expand(ctx, expr, payload);
>  			break;
> +		} else if (expr->right->etype == EXPR_SET_REF) {
> +			struct set *set = expr->right->set;
> +
> +			if (set_is_anonymous(set->flags) &&
> +			    !list_empty(&set->init->expressions)) {

According to GDB, set->init is NULL here.

I am not familiar with recent changes in cache code, maybe there's the
actual culprit: Debug printf in cache_init_objects() states flags
variable is 0x4000005f, i.e. NFT_CACHE_SETELEM_BIT is not set.

I am not sure if caching is incomplete and we need that bit or if the
above code should expect sets with missing elements and therefore check
'set->init != NULL' before accessing expressions field.

Cheers, Phil