Received-SPF: pass (google.com: domain of linux-kernel+bounces-14542-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: Aaron Conole <aconole@redhat.com>
To: Simon Horman <horms@kernel.org>
Cc: Brad Cowie <brad@faucet.nz>,  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>,  coreteam@netfilter.org
Subject: Re: [PATCH net] netfilter: nf_nat: fix action not being set for all
 ct states
References: <20231221224311.130319-1-brad@faucet.nz>
	<20231223211306.GA215659@kernel.org>
Date: Tue, 02 Jan 2024 10:10:15 -0500
In-Reply-To: <20231223211306.GA215659@kernel.org> (Simon Horman's message of
	"Sat, 23 Dec 2023 21:13:06 +0000")
Message-ID: <f7tle97eppk.fsf@redhat.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.3 (gnu/linux)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

Simon Horman <horms@kernel.org> writes:

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

LGTM.  I guess we should try to codify the specific flows that were used
to flag this into the ovs selftest - we clearly have a missing case
after NAT lookup.

I'll add it to my (ever growing) list.

Meanwhile,

Acked-by: Aaron Conole <aconole@redhat.com>

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