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

Wed Jul 6 12:38:28 UTC 2022

Hello Kleber,

See replies in-line below.

On Wed, Jul 6, 2022 at 12:32 PM Kleber Souza
<kleber.sacilotto.de.souza at canonical.com> wrote:
>
> Hello Frode,
>
> Can you please re-submit these patches following the procedures
> specified at https://wiki.ubuntu.com/Kernel/Dev/StablePatchFormat ?

I did, however it was not clear to me that you wanted the SRU
justification in a cover letter after reading that page.

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

Thank you for pointing these out and please welcome my new series
where your concerns have been addressed.

-- 
Frode Nordahl

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