diff mbox series

nf_conntrack_proto_udp: Set ASSURED for NAT_CLASH entries to avoid packets dropped

Message ID fd991e87-a97b-49af-892f-685b93833bd8@suse.com
State New
Headers show
Series nf_conntrack_proto_udp: Set ASSURED for NAT_CLASH entries to avoid packets dropped | expand

Commit Message

Yadan Fan Oct. 10, 2024, 12:19 p.m. UTC
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(-)

Comments

Florian Westphal Oct. 10, 2024, 12:47 p.m. UTC | #1
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>
Hannes Reinecke Oct. 10, 2024, 12:49 p.m. UTC | #2
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
Yadan Fan Nov. 10, 2024, 11:16 a.m. UTC | #3
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);
Pablo Neira Ayuso Nov. 11, 2024, 9:56 a.m. UTC | #4
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
diff mbox series

Patch

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);