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

[Kleber Souza]

Hello Frode,

Can you please re-submit these patches following the procedures
specified at https://wiki.ubuntu.com/Kernel/Dev/StablePatchFormat ?

The main issues to be fixed being:
* The patch should be in its own email, the SRU justification goes
   into a separate cover letter.
* The patch should contain the BugLink as well.
* Please group all the submissions related to the same bug report
   under a single email thread, this makes it easier for us to
   review and apply the patches.


Thanks,
Kleber

On 06.07.22 11:57, Frode Nordahl wrote:
> Buglink: https://launchpad.net/bugs/1967856
> 
> SRU Justification:
> 
> [Impact]
> 
> Users of Open vSwitch on Jammy will be affected by this bug and
> have no user space fix available.
> 
> [Fix]
> 
> * cba7c76ea1e15fddb95706eb64659644a6a02b38 "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 ed8afedbe49c5528ed2937bad5f9debf8292062b 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>
> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> (cherry picked from commit cba7c76ea1e15fddb95706eb64659644a6a02b38 5.15.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 8955f31fa47e..aca6e2b599c8 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -373,6 +373,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;
>   }
>   
> @@ -420,6 +421,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]));
>   }
>   
> @@ -660,6 +662,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;
>   }
> @@ -699,6 +702,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);
> @@ -761,6 +765,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 f2b64cab9af7..1f5bccd6a1dc 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -1336,7 +1336,9 @@ int ovs_ct_clear(struct sk_buff *skb, struct sw_flow_key *key)
>   
>   	nf_ct_put(ct);
>   	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;
>   }