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

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

[Yadan Fan]

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;
-
 		/* 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

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

[Florian Westphal]

Yadan Fan <ydfan@suse.com> 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>


Acked-by: Florian Westphal <fw@strlen.de>

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

[Hannes Reinecke]

On 10/10/24 14:47, Florian Westphal wrote:
> Yadan Fan <ydfan@suse.com> 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>
> 
> 
> Acked-by: Florian Westphal <fw@strlen.de>

You probably need mine:

Signed-off-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

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

[Yadan Fan]

Hi,

This patch is still not processed further:

https://patchwork.ozlabs.org/project/netfilter-devel/list/?submitter=89472

May I ask when this patch is planed to be merged?

Thanks,
Yadan Fan

On 10/10/24 20:19, 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;
> -
>  		/* 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);

-- 
Yadan Fan,
SUSE L3

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

[Pablo Neira Ayuso]

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