Re: [PATCH net] netfilter: nf_nat: fix action not being set for all ct states

From: Simon Horman <horms@kernel.org>
To: Brad Cowie <brad@faucet.nz>
Cc: netdev@vger.kernel.org, dev@openvswitch.org, fw@strlen.de,
	linux-kernel@vger.kernel.org, kadlec@netfilter.org,
	edumazet@google.com, netfilter-devel@vger.kernel.org,
	kuba@kernel.org, pabeni@redhat.com, davem@davemloft.net,
	pablo@netfilter.org, Xin Long <lucien.xin@gmail.com>,
	Aaron Conole <aconole@redhat.com>,
	coreteam@netfilter.org
Subject: Re: [PATCH net] netfilter: nf_nat: fix action not being set for all ct states
Date: Sat, 23 Dec 2023 21:13:06 +0000	[thread overview]
Message-ID: <20231223211306.GA215659@kernel.org> (raw)
In-Reply-To: <20231221224311.130319-1-brad@faucet.nz>

+ Xin Long <lucien.xin@gmail.com>
  Aaron Conole <aconole@redhat.com>
  coreteam@netfilter.org

On Fri, Dec 22, 2023 at 11:43:11AM +1300, Brad Cowie wrote:
> This fixes openvswitch's handling of nat packets in the related state.
> 
> In nf_ct_nat_execute(), which is called from nf_ct_nat(), ICMP/ICMPv6
> packets in the IP_CT_RELATED or IP_CT_RELATED_REPLY state, which have
> not been dropped, will follow the goto, however the placement of the
> goto label means that updating the action bit field will be bypassed.
> 
> This causes ovs_nat_update_key() to not be called from ovs_ct_nat()
> which means the openvswitch match key for the ICMP/ICMPv6 packet is not
> updated and the pre-nat value will be retained for the key, which will
> result in the wrong openflow rule being matched for that packet.
> 
> Move the goto label above where the action bit field is being set so
> that it is updated in all cases where the packet is accepted.
> 
> Fixes: ebddb1404900 ("net: move the nat function to nf_nat_ovs for ovs and tc")
> Signed-off-by: Brad Cowie <brad@faucet.nz>

Thanks Brad,

I agree with your analysis and that the problem appears to
have been introduced by the cited commit.

I am curious to know what use case triggers this /
why it when unnoticed for a year.

But in any case, this fix looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

> ---
>  net/netfilter/nf_nat_ovs.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_nat_ovs.c b/net/netfilter/nf_nat_ovs.c
> index 551abd2da614..0f9a559f6207 100644
> --- a/net/netfilter/nf_nat_ovs.c
> +++ b/net/netfilter/nf_nat_ovs.c
> @@ -75,9 +75,10 @@ static int nf_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
>  	}
>  
>  	err = nf_nat_packet(ct, ctinfo, hooknum, skb);
> +out:
>  	if (err == NF_ACCEPT)
>  		*action |= BIT(maniptype);
> -out:
> +
>  	return err;
>  }
>  
> -- 
> 2.34.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 