NAK: [SRU][I][PATCH] net: openvswitch: fix misuse of the cached connection on tuple changes

[Kleber Souza]

Please comment for the Jammy submission.

On 06.07.22 11:57, Frode Nordahl wrote:
> Buglink: https://launchpad.net/bugs/1967856
> 
> SRU Justification:
> 
> [Impact]
> 
> Users of Open vSwitch on Impish will be affected by this bug and
> have no user space fix available.
> 
> [Fix]
> 
> * 6bb3c77c74f5de28c78c74ef23af4c63e06e881e "net: openvswitch: fix
> misuse of the cached connection on tuple changes"
> 
> [Test Plan]
> 
> A reproducer has been attached to the bug and can be executed with
> the OVS system testsuite.
> 
> [Where problems could occur]
> 
> The regression potential can be considered as low because:
> - The calls added in the openvswitch kernel datapath code would
>    prior to Open vSwitch 2.16.0 have been initiated from the
>    userspace code and by chance concealed this bug.
> - After an optimization done in 2.16.0 the kernel bug was
>    revealed and these calls now must be made from the kernel
>    datapath to retain functionality in use in the wild.
> 
>  From b5a07004998db55c18789caac1faa61fb6268c94 Mon Sep 17 00:00:00 2001
> From: Ilya Maximets <i.maximets at ovn.org>
> Date: Tue, 7 Jun 2022 00:11:40 +0200
> Subject: [SRU][J][PATCH] net: openvswitch: fix misuse of the cached connection
>   on tuple changes
> To: kernel-team at lists.ubuntu.com
> 
> commit 2061ecfdf2350994e5b61c43e50e98a7a70e95ee upstream.
> 
> If packet headers changed, the cached nfct is no longer relevant
> for the packet and attempt to re-use it leads to the incorrect packet
> classification.
> 
> This issue is causing broken connectivity in OpenStack deployments
> with OVS/OVN due to hairpin traffic being unexpectedly dropped.
> 
> The setup has datapath flows with several conntrack actions and tuple
> changes between them:
> 
>    actions:ct(commit,zone=8,mark=0/0x1,nat(src)),
>            set(eth(src=00:00:00:00:00:01,dst=00:00:00:00:00:06)),
>            set(ipv4(src=172.18.2.10,dst=192.168.100.6,ttl=62)),
>            ct(zone=8),recirc(0x4)
> 
> After the first ct() action the packet headers are almost fully
> re-written.  The next ct() tries to re-use the existing nfct entry
> and marks the packet as invalid, so it gets dropped later in the
> pipeline.
> 
> Clearing the cached conntrack entry whenever packet tuple is changed
> to avoid the issue.
> 
> The flow key should not be cleared though, because we should still
> be able to match on the ct_state if the recirculation happens after
> the tuple change but before the next ct() action.
> 
> Cc: stable at vger.kernel.org
> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
> Reported-by: Frode Nordahl <frode.nordahl at canonical.com>
> Link: https://mail.openvswitch.org/pipermail/ovs-discuss/2022-May/051829.html
> Link: https://bugs.launchpad.net/ubuntu/+source/ovn/+bug/1967856
> Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
> Link: https://lore.kernel.org/r/20220606221140.488984-1-i.maximets@ovn.org
> Signed-off-by: Jakub Kicinski <kuba at kernel.org>
> [Backport to 5.10: minor rebase in ovs_ct_clear function.
>   This version also applicable to and tested on 5.4 and 4.19.]
> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> (backported from commit 6bb3c77c74f5de28c78c74ef23af4c63e06e881e 5.4.y)
> Signed-off-by: Frode Nordahl <frode.nordahl at canonical.com>
> ---
>   net/openvswitch/actions.c   | 6 ++++++
>   net/openvswitch/conntrack.c | 4 +++-
>   2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 0c49b63f3e2a..4afe83ec1057 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -372,6 +372,7 @@ static void set_ip_addr(struct sk_buff *skb, struct iphdr *nh,
>   	update_ip_l4_checksum(skb, nh, *addr, new_addr);
>   	csum_replace4(&nh->check, *addr, new_addr);
>   	skb_clear_hash(skb);
> +	ovs_ct_clear(skb, NULL);
>   	*addr = new_addr;
>   }
>   
> @@ -419,6 +420,7 @@ static void set_ipv6_addr(struct sk_buff *skb, u8 l4_proto,
>   		update_ipv6_checksum(skb, l4_proto, addr, new_addr);
>   
>   	skb_clear_hash(skb);
> +	ovs_ct_clear(skb, NULL);
>   	memcpy(addr, new_addr, sizeof(__be32[4]));
>   }
>   
> @@ -659,6 +661,7 @@ static int set_nsh(struct sk_buff *skb, struct sw_flow_key *flow_key,
>   static void set_tp_port(struct sk_buff *skb, __be16 *port,
>   			__be16 new_port, __sum16 *check)
>   {
> +	ovs_ct_clear(skb, NULL);
>   	inet_proto_csum_replace2(check, skb, *port, new_port, false);
>   	*port = new_port;
>   }
> @@ -698,6 +701,7 @@ static int set_udp(struct sk_buff *skb, struct sw_flow_key *flow_key,
>   		uh->dest = dst;
>   		flow_key->tp.src = src;
>   		flow_key->tp.dst = dst;
> +		ovs_ct_clear(skb, NULL);
>   	}
>   
>   	skb_clear_hash(skb);
> @@ -760,6 +764,8 @@ static int set_sctp(struct sk_buff *skb, struct sw_flow_key *flow_key,
>   	sh->checksum = old_csum ^ old_correct_csum ^ new_csum;
>   
>   	skb_clear_hash(skb);
> +	ovs_ct_clear(skb, NULL);
> +
>   	flow_key->tp.src = sh->source;
>   	flow_key->tp.dst = sh->dest;
>   
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index cadb6a29b285..95c835e014d5 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -1332,7 +1332,9 @@ int ovs_ct_clear(struct sk_buff *skb, struct sw_flow_key *key)
>   	if (skb_nfct(skb)) {
>   		nf_conntrack_put(skb_nfct(skb));
>   		nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
> -		ovs_ct_fill_key(skb, key, false);
> +
> +		if (key)
> +		    ovs_ct_fill_key(skb, key, false);
>   	}
>   
>   	return 0;