[PATCH] nf_conntrack_proto_udp: do not accept packets with IPS_NAT_CLASH

[PATCH] nf_conntrack_proto_udp: do not accept packets with IPS_NAT_CLASH

[Hannes Reinecke]

Commit c46172147ebb changed the logic when to move to ASSURED if
a NAT CLASH is detected. In particular, it moved to ASSURED even
if a NAT CLASH had been detected, with the idea that they will
time out soon (and then the state will be cleared).
However, under high load this caused the timeout to happen too
slow causing an IPVS malfunction.
This patch revert part of that patch, as for NAT CLASH we
should not move to ASSURED at all.

Fixes: c46172147ebb ("netfilter: conntrack: do not auto-delete clash entries on reply")

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 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.35.3

Re: [PATCH] nf_conntrack_proto_udp: do not accept packets with IPS_NAT_CLASH

[Florian Westphal]

Hannes Reinecke <hare@kernel.org> wrote:
> Commit c46172147ebb changed the logic when to move to ASSURED if
> a NAT CLASH is detected. In particular, it moved to ASSURED even
> if a NAT CLASH had been detected,

I'm not following.  The code you are removing returns early
for nat clash case.

Where does it move to assured if nat clash is detected?

> However, under high load this caused the timeout to happen too
> slow causing an IPVS malfunction.

Can you elaborate?

> This patch revert part of that patch, as for NAT CLASH we
> should not move to ASSURED at all.

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

AFAICS with this patch we now do move to assured unconditionally?

The changelog and patch seem contradictory to me.

Re: [PATCH] nf_conntrack_proto_udp: do not accept packets with IPS_NAT_CLASH

[Yadan Fan]

On 9/30/24 17:29, Florian Westphal wrote:
> Hannes Reinecke <hare@kernel.org> wrote:
>> Commit c46172147ebb changed the logic when to move to ASSURED if
>> a NAT CLASH is detected. In particular, it moved to ASSURED even
>> if a NAT CLASH had been detected,
> 
> I'm not following.  The code you are removing returns early
> for nat clash case.
> 
> Where does it move to assured if nat clash is detected?
> 
>> However, under high load this caused the timeout to happen too
>> slow causing an IPVS malfunction.
> 
> Can you elaborate?

Hi Florian,

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 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.

Yadan,
SUSE L3

> 
>> This patch revert part of that patch, as for NAT CLASH we
>> should not move to ASSURED at all.
> 
>>   		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);
> 
> AFAICS with this patch we now do move to assured unconditionally?
> 
> The changelog and patch seem contradictory to me.

Re: [PATCH] nf_conntrack_proto_udp: do not accept packets with IPS_NAT_CLASH

[Florian Westphal]

Yadan Fan <ydfan@suse.com> wrote:
> On 9/30/24 17:29, Florian Westphal wrote:
> > Hannes Reinecke <hare@kernel.org> wrote:
> > > Commit c46172147ebb changed the logic when to move to ASSURED if
> > > a NAT CLASH is detected. In particular, it moved to ASSURED even
> > > if a NAT CLASH had been detected,
> > 
> > I'm not following.  The code you are removing returns early
> > for nat clash case.
> > 
> > Where does it move to assured if nat clash is detected?
> > 
> > > However, under high load this caused the timeout to happen too
> > > slow causing an IPVS malfunction.
> > 
> > Can you elaborate?
> 
> Hi Florian,
> 
> 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 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.

I think the only thing remaining is to rewrite the commit message to
say that not setting assured will drop NAT_CLASH replies in case server
is very busy and early_drop logic kicks in.

Re: [PATCH] nf_conntrack_proto_udp: do not accept packets with IPS_NAT_CLASH

[Yadan Fan]

On 10/9/24 00:45, Florian Westphal wrote:
> Yadan Fan <ydfan@suse.com> wrote:
>> On 9/30/24 17:29, Florian Westphal wrote:
>>> Hannes Reinecke <hare@kernel.org> wrote:
>>>> Commit c46172147ebb changed the logic when to move to ASSURED if
>>>> a NAT CLASH is detected. In particular, it moved to ASSURED even
>>>> if a NAT CLASH had been detected,
>>>
>>> I'm not following.  The code you are removing returns early
>>> for nat clash case.
>>>
>>> Where does it move to assured if nat clash is detected?
>>>
>>>> However, under high load this caused the timeout to happen too
>>>> slow causing an IPVS malfunction.
>>>
>>> Can you elaborate?
>>
>> Hi Florian,
>>
>> 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 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.
> 
> I think the only thing remaining is to rewrite the commit message to
> say that not setting assured will drop NAT_CLASH replies in case server
> is very busy and early_drop logic kicks in.

Thanks, I will submit a new patch with the new commit message
continuing the work from Hannes.