nftables >= 0.9.8: atomic update (nft -f ...) of a set not possible any more

nftables >= 0.9.8: atomic update (nft -f ...) of a set not possible any more

[etkaar]

Dear colleagues,

given is following perfectly working ruleset (nft list ruleset), which drops almost all of the IPv4 traffic, but grants access to port 22 (SSH) for two IPv4 addresses provided by the set named 'whitelist_ipv4_tcp':

+++
table inet filter {
	set whitelist_ipv4_tcp {
		type inet_service . ipv4_addr
		flags interval
		elements = { 22 . 111.222.333.444,
			     22 . 555.666.777.888 }
	}

	set whitelist_ipv4_udp {
		type inet_service . ipv4_addr
		flags interval
	}

	set blacklist_ipv4 {
		type ipv4_addr
		flags interval
	}

	chain default_input {
		type filter hook input priority filter + 1; policy drop;
		ip saddr @blacklist_ipv4 drop
		ct state established,related accept
		ct state invalid drop
		iifname "lo" accept
		ip protocol icmp icmp type { echo-reply, destination-unreachable, echo-request, router-advertisement, router-solicitation, time-exceeded, parameter-problem } accept
		meta nfproto ipv4 tcp dport . ip saddr @whitelist_ipv4_tcp accept
		meta nfproto ipv4 udp dport . ip saddr @whitelist_ipv4_udp accept
	}

	chain default_forward {
		type filter hook forward priority filter; policy drop;
	}

	chain default_output {
		type filter hook output priority filter; policy accept;
		oifname "lo" accept
	}
}

+++

Now comes the strange thing: In case the ruleset is atomically updated using following file (nft -f ...) the output of 'nft list ruleset' does not change at all (which is expected!), but now the IPv4 addresses do not any more have access:

+++
#!/usr/sbin/nft -f
flush set inet filter whitelist_ipv4_tcp
flush set inet filter whitelist_ipv4_udp
table inet filter {
	set whitelist_ipv4_tcp {
		type inet_service . ipv4_addr; flags interval;
		elements = {
			22 . 111.222.333.444/32,
			22 . 555.666.777.888/32,
		}
	}
	set whitelist_ipv4_udp {
		type inet_service . ipv4_addr; flags interval;
	}
}

+++

I have double-checked that in the logs:

+++
nft dropped: [...] SRC=111.222.333.444 DST=194.48.XXX.XX [...] DPT=22 [...]
+++

This problem is not present in Debian 10 (Buster) which uses nftables 0.9.0. But it is present in Debian 11 (Bullseye) which uses nftables 0.9.8. Because I even got a segmentation fault here I cannot reproduce yet, I tried it with nftables 1.0.1 - but still no success.

I don't understand how the 'nft list ruleset' output can be identical after using 'nft -f ...'. In any case, the IPv4 addresses are part of the whitelist set and are unexpectedly blocked.

Kind Regards,
etkaar

Re: nftables >= 0.9.8: atomic update (nft -f ...) of a set not possible any more

[Florian Westphal]

etkaar <lists.netfilter.org@prvy.eu> wrote:

[ CC Stefano ]

> Dear colleagues,
> 
> given is following perfectly working ruleset (nft list ruleset), which drops almost all of the IPv4 traffic, but grants access to port 22 (SSH) for two IPv4 addresses provided by the set named 'whitelist_ipv4_tcp':

Thanks for reporting, I can reproduce this.

> +++
> table inet filter {
> 	set whitelist_ipv4_tcp {
> 		type inet_service . ipv4_addr
> 		flags interval
> 		elements = { 22 . 111.222.333.444,
> 			     22 . 555.666.777.888 }
> 	}

I can repro this, looks like missing scratchpad cloning in the set
backend.

I can see that after second 'nft -f', avx2_lookup takes the 'if (unlikely(!scratch)) {' branch.

Can you try this (kernel) patch below?

As a workaround, you could try removing the 'interval' flag so that
kernel uses a hash table as set backend instead.

Stefano, does that patch make sense to you?
Thanks!

diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
--- a/net/netfilter/nft_set_pipapo.c
+++ b/net/netfilter/nft_set_pipapo.c
@@ -1271,7 +1271,7 @@ static struct nft_pipapo_match *pipapo_clone(struct nft_pipapo_match *old)
 {
 	struct nft_pipapo_field *dst, *src;
 	struct nft_pipapo_match *new;
-	int i;
+	int i, err;
 
 	new = kmalloc(sizeof(*new) + sizeof(*dst) * old->field_count,
 		      GFP_KERNEL);
@@ -1291,6 +1291,14 @@ static struct nft_pipapo_match *pipapo_clone(struct nft_pipapo_match *old)
 		goto out_scratch;
 #endif
 
+	err = pipapo_realloc_scratch(new, old->bsize_max);
+	if (err) {
+#ifdef NFT_PIPAPO_ALIGN
+		free_percpu(new->scratch_aligned);
+#endif
+		goto out_scratch;
+	}
+
 	rcu_head_init(&new->rcu);
 
 	src = old->f;

Re: nftables >= 0.9.8: atomic update (nft -f ...) of a set not possible any more

[Stefano Brivio]

Hi Florian,

On Tue, 4 Jan 2022 20:57:28 +0100
Florian Westphal <fw@strlen.de> wrote:

> etkaar <lists.netfilter.org@prvy.eu> wrote:
> 
> [ CC Stefano ]
> 
> > Dear colleagues,
> > 
> > given is following perfectly working ruleset (nft list ruleset), which drops almost all of the IPv4 traffic, but grants access to port 22 (SSH) for two IPv4 addresses provided by the set named 'whitelist_ipv4_tcp':  
> 
> Thanks for reporting, I can reproduce this.
> 
> > +++
> > table inet filter {
> > 	set whitelist_ipv4_tcp {
> > 		type inet_service . ipv4_addr
> > 		flags interval
> > 		elements = { 22 . 111.222.333.444,
> > 			     22 . 555.666.777.888 }
> > 	}  
> 
> I can repro this, looks like missing scratchpad cloning in the set
> backend.
> 
> I can see that after second 'nft -f', avx2_lookup takes the 'if (unlikely(!scratch)) {' branch.
> 
> Can you try this (kernel) patch below?
> 
> As a workaround, you could try removing the 'interval' flag so that
> kernel uses a hash table as set backend instead.
> 
> Stefano, does that patch make sense to you?
> Thanks!

Thanks for checking and fixing this!

Yes, it makes sense, a clone without a subsequent new insertion
wouldn't have a scratchpad otherwise -- I wonder how I missed this.
Just perhaps:

> diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
> --- a/net/netfilter/nft_set_pipapo.c
> +++ b/net/netfilter/nft_set_pipapo.c
> @@ -1271,7 +1271,7 @@ static struct nft_pipapo_match *pipapo_clone(struct nft_pipapo_match *old)
>  {
>  	struct nft_pipapo_field *dst, *src;
>  	struct nft_pipapo_match *new;
> -	int i;
> +	int i, err;
>  
>  	new = kmalloc(sizeof(*new) + sizeof(*dst) * old->field_count,
>  		      GFP_KERNEL);
> @@ -1291,6 +1291,14 @@ static struct nft_pipapo_match *pipapo_clone(struct nft_pipapo_match *old)
>  		goto out_scratch;
>  #endif
>  
> +	err = pipapo_realloc_scratch(new, old->bsize_max);
> +	if (err) {
> +#ifdef NFT_PIPAPO_ALIGN
> +		free_percpu(new->scratch_aligned);
> +#endif

I would use another label for this, "out_scratch_aligned", for
consistency with the rest of the error handling, but it's not a strong
preference.

> +		goto out_scratch;
> +	}
> +
>  	rcu_head_init(&new->rcu);
>  
>  	src = old->f;
> 

-- 
Stefano

Re: nftables >= 0.9.8: atomic update (nft -f ...) of a set not possible any more

[etkaar]

Dear colleagues,

am I right if I assume that this has been patched into the Kernel of 5.10.106-1 (2022-03-17)?

I am not able anymore to reproduce the issue, but I just want to double-check that.

Kind Regards,
etkaar

Re: nftables >= 0.9.8: atomic update (nft -f ...) of a set not possible any more

[etkaar]

Dear Florian and Stefano,

thanks so much for your fast response!

Unfortunately I am not really able yet to apply a kernel patch, because that would be the first time at all I modify the kernel and I am afraid of being locked-out again in case Debian afterwards updates the kernel with a newer version which still is affected of the bug. I am not so familiar with that, because for stability and security reasons I usually always leave my Debian installation as they are.

However, I am able to compile nftables itself and replace Debian 11 Bullseyes version 0.9.8 with 1.0.1, but this version still contains the bug. Once there is an update to 1.0.2 and this bug is fixed, compiling and replacing the 0.9.8 version with it should be enough, or is still a kernel patch required in that case?

Kind Regards,
etkaar