Re: [PATCHv2 net-next 3/5] net: sched: return NF_ACCEPT when fails to add nat ext in tcf_ct_act_nat

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Tue, Nov 22, 2022 at 12:32:19PM -0500, Xin Long wrote:
> This patch changes to return NF_ACCEPT when fails to add nat
> ext before doing NAT in tcf_ct_act_nat(), to keep consistent
> with OVS' processing in ovs_ct_nat().
> 
> Reviewed-by: Saeed Mahameed <saeed@kernel.org>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sched/act_ct.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index da0b7f665277..8869b3ef6642 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -994,7 +994,7 @@ static int tcf_ct_act_nat(struct sk_buff *skb,
>  
>  	/* Add NAT extension if not confirmed yet. */
>  	if (!nf_ct_is_confirmed(ct) && !nf_ct_nat_ext_add(ct))
> -		return NF_DROP;   /* Can't NAT. */
> +		return NF_ACCEPT;   /* Can't NAT. */

I'm wondering if the fix should actually be in OVS, to make it drop
the packet? Aaron, Eelco?

If the user asked for NAT, and it can't NAT, it doesn't seem right to
forward the packet while not performing the asked action.

If we follow the code here, it may even commit the entry without the
NAT extension, rendering the connection useless/broken per the first
if condition above. It just won't try again.

>  
>  	if (ctinfo != IP_CT_NEW && (ct->status & IPS_NAT_MASK) &&
>  	    (ctinfo != IP_CT_RELATED || commit)) {
> -- 
> 2.31.1
>