Re: [PATCH] nf_conntrack_proto_udp: Set ASSURED for NAT_CLASH entries to avoid packets dropped

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

Hi,

On Thu, Oct 10, 2024 at 08:19:16PM +0800, Yadan Fan wrote:
> c46172147ebb brought the logic that never setting ASSURED to drop NAT_CLASH replies
> in case server is very busy and early_drop logic kicks in.
> 
> However, this will drop all subsequent UDP packets that sent through multiple threads
> of application, we already had a customer reported this issue that impacts their business,
> so deleting this logic to avoid this issue at the moment.
> 
> Fixes: c46172147ebb ("netfilter: conntrack: do not auto-delete clash entries on reply")
> 
> Signed-off-by: Yadan Fan <ydfan@suse.com>
> ---
>  net/netfilter/nf_conntrack_proto_udp.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_proto_udp.c b/net/netfilter/nf_conntrack_proto_udp.c
> index 0030fbe8885c..def3e06430eb 100644
> --- a/net/netfilter/nf_conntrack_proto_udp.c
> +++ b/net/netfilter/nf_conntrack_proto_udp.c
> @@ -116,10 +116,6 @@ int nf_conntrack_udp_packet(struct nf_conn *ct,
>  
>  		nf_ct_refresh_acct(ct, ctinfo, skb, extra);
>  
> -		/* never set ASSURED for IPS_NAT_CLASH, they time out soon */
> -		if (unlikely((status & IPS_NAT_CLASH)))
> -			return NF_ACCEPT;

This update is obsoleting several comments in the code, such as:

        /* IPS_NAT_CLASH removes the entry automatically on the first
         * reply.  Also prevents UDP tracker from moving the entry to
         * ASSURED state, i.e. the entry can always be evicted under
         * pressure.
         */
        loser_ct->status |= IPS_FIXED_TIMEOUT | IPS_NAT_CLASH;

According to 6a757c07e51f ("netfilter: conntrack: allow insertion of
clashing entries"), the idea is to let this packet go through (no
drop!) while next packets will already find the conntrack entry that
won race.

You mentioned this fix is for IPVS, you said:

> We have a customer who encountered an issue that UDP packets kept in
> UNREPLIED in conntrack table when there is large number of UDP packets
> sent from their application, the application send packets through multiple
> threads, it caused NAT clash because the same SNATs were used for multiple connections

Hm. But this IPS_NAT_CLASH entry should not be ever found by other
packets, this is just a dummy entry for _this_ packet that has a
clashing entry, to let it go through.

> setup, so that initial packets will be flagged with IPS_NAT_CLASH, and this snippet
> of codes just makes IPS_NAT_CLASH flagged packets never be marked as ASSURED, which
> caused all subsequent UDP packets got dropped.
> Issue just disappeared after deleting this portion.

Something is missing here, I still don't see how all this make sense.

> -
>  		/* Also, more likely to be important, and not a probe */
>  		if (stream && !test_and_set_bit(IPS_ASSURED_BIT, &ct->status))
>  			nf_conntrack_event_cache(IPCT_ASSURED, ct);
> -- 
> 2.34.1