[ovs-dev] datapath: Revert "datapath: Fix template leak in error cases."
diff mbox series

Message ID 1554310153-2544-1-git-send-email-pkusunyifeng@gmail.com
State New
Headers show
Series
  • [ovs-dev] datapath: Revert "datapath: Fix template leak in error cases."
Related show

Commit Message

Yifeng Sun April 3, 2019, 4:49 p.m. UTC
From: Flavio Leitner <fbl@redhat.com>

Upstream commit:
    commit 7f6d6558ae44bc193eb28df3617c364d3bb6df39
    Author: Flavio Leitner <fbl@redhat.com>
    Date:   Fri Sep 28 14:55:34 2018 -0300

    Revert "openvswitch: Fix template leak in error cases."
    This reverts commit 90c7afc.

    When the commit was merged, the code used nf_ct_put() to free
    the entry, but later on commit 7664423 ("openvswitch: Free
    tmpl with tmpl_free.") replaced that with nf_ct_tmpl_free which
    is a more appropriate. Now the original problem is removed.

    Then 44d6e2f ("net: Replace NF_CT_ASSERT() with WARN_ON().")
    replaced a debug assert with a WARN_ON() which is trigged now.

    Signed-off-by: Flavio Leitner <fbl@redhat.com>
    Acked-by: Joe Stringer <joe@ovn.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>

This patch backports this upstream patch to OVS.

Cc: Flavio Leitner <fbl@redhat.com>
Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
---
 datapath/conntrack.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

0-day Robot April 3, 2019, 4:59 p.m. UTC | #1
Bleep bloop.  Greetings Yifeng Sun, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Author Flavio Leitner <fbl@redhat.com> needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Yifeng Sun <pkusunyifeng@gmail.com>
Lines checked: 61, Warnings: 1, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Anand Kumar via dev April 3, 2019, 6:34 p.m. UTC | #2
On Wed, Apr 03, 2019 at 09:49:13AM -0700, Yifeng Sun wrote:
> From: Flavio Leitner <fbl@redhat.com>
> 
> Upstream commit:
>     commit 7f6d6558ae44bc193eb28df3617c364d3bb6df39
>     Author: Flavio Leitner <fbl@redhat.com>
>     Date:   Fri Sep 28 14:55:34 2018 -0300
> 
>     Revert "openvswitch: Fix template leak in error cases."
>     This reverts commit 90c7afc.
> 
>     When the commit was merged, the code used nf_ct_put() to free
>     the entry, but later on commit 7664423 ("openvswitch: Free
>     tmpl with tmpl_free.") replaced that with nf_ct_tmpl_free which
>     is a more appropriate. Now the original problem is removed.
> 
>     Then 44d6e2f ("net: Replace NF_CT_ASSERT() with WARN_ON().")
>     replaced a debug assert with a WARN_ON() which is trigged now.
> 
>     Signed-off-by: Flavio Leitner <fbl@redhat.com>
>     Acked-by: Joe Stringer <joe@ovn.org>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> This patch backports this upstream patch to OVS.
> 
> Cc: Flavio Leitner <fbl@redhat.com>
> Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
> ---

LGTM
Acked-by: Flavio Leitner <fbl@sysclose.org>
Ben Pfaff April 15, 2019, 11:08 p.m. UTC | #3
On Wed, Apr 03, 2019 at 03:34:34PM -0300, Flavio Leitner via dev wrote:
> On Wed, Apr 03, 2019 at 09:49:13AM -0700, Yifeng Sun wrote:
> > From: Flavio Leitner <fbl@redhat.com>
> > 
> > Upstream commit:
> >     commit 7f6d6558ae44bc193eb28df3617c364d3bb6df39
> >     Author: Flavio Leitner <fbl@redhat.com>
> >     Date:   Fri Sep 28 14:55:34 2018 -0300
> > 
> >     Revert "openvswitch: Fix template leak in error cases."
> >     This reverts commit 90c7afc.
> > 
> >     When the commit was merged, the code used nf_ct_put() to free
> >     the entry, but later on commit 7664423 ("openvswitch: Free
> >     tmpl with tmpl_free.") replaced that with nf_ct_tmpl_free which
> >     is a more appropriate. Now the original problem is removed.
> > 
> >     Then 44d6e2f ("net: Replace NF_CT_ASSERT() with WARN_ON().")
> >     replaced a debug assert with a WARN_ON() which is trigged now.
> > 
> >     Signed-off-by: Flavio Leitner <fbl@redhat.com>
> >     Acked-by: Joe Stringer <joe@ovn.org>
> >     Signed-off-by: David S. Miller <davem@davemloft.net>
> > 
> > This patch backports this upstream patch to OVS.
> > 
> > Cc: Flavio Leitner <fbl@redhat.com>
> > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
> > ---
> 
> LGTM
> Acked-by: Flavio Leitner <fbl@sysclose.org>

I applied this to master.  I would appreciate advice on backporting.
Yifeng Sun April 16, 2019, 6:12 p.m. UTC | #4
The more appropriate patch (7664423) mentioned in the revert message
was introduced in ovs-2.5.
So I think this revert patch should be backported to 2.5.

Thanks,
Yifeng

On Mon, Apr 15, 2019 at 4:08 PM Ben Pfaff <blp@ovn.org> wrote:
>
> On Wed, Apr 03, 2019 at 03:34:34PM -0300, Flavio Leitner via dev wrote:
> > On Wed, Apr 03, 2019 at 09:49:13AM -0700, Yifeng Sun wrote:
> > > From: Flavio Leitner <fbl@redhat.com>
> > >
> > > Upstream commit:
> > >     commit 7f6d6558ae44bc193eb28df3617c364d3bb6df39
> > >     Author: Flavio Leitner <fbl@redhat.com>
> > >     Date:   Fri Sep 28 14:55:34 2018 -0300
> > >
> > >     Revert "openvswitch: Fix template leak in error cases."
> > >     This reverts commit 90c7afc.
> > >
> > >     When the commit was merged, the code used nf_ct_put() to free
> > >     the entry, but later on commit 7664423 ("openvswitch: Free
> > >     tmpl with tmpl_free.") replaced that with nf_ct_tmpl_free which
> > >     is a more appropriate. Now the original problem is removed.
> > >
> > >     Then 44d6e2f ("net: Replace NF_CT_ASSERT() with WARN_ON().")
> > >     replaced a debug assert with a WARN_ON() which is trigged now.
> > >
> > >     Signed-off-by: Flavio Leitner <fbl@redhat.com>
> > >     Acked-by: Joe Stringer <joe@ovn.org>
> > >     Signed-off-by: David S. Miller <davem@davemloft.net>
> > >
> > > This patch backports this upstream patch to OVS.
> > >
> > > Cc: Flavio Leitner <fbl@redhat.com>
> > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
> > > ---
> >
> > LGTM
> > Acked-by: Flavio Leitner <fbl@sysclose.org>
>
> I applied this to master.  I would appreciate advice on backporting.
Ben Pfaff April 16, 2019, 7:27 p.m. UTC | #5
OK, I backported as far as 2.6.  On 2.5, the original patch hadn't been
applied, so there was nothing to revert.

On Tue, Apr 16, 2019 at 11:12:11AM -0700, Yifeng Sun wrote:
> The more appropriate patch (7664423) mentioned in the revert message
> was introduced in ovs-2.5.
> So I think this revert patch should be backported to 2.5.
> 
> Thanks,
> Yifeng
> 
> On Mon, Apr 15, 2019 at 4:08 PM Ben Pfaff <blp@ovn.org> wrote:
> >
> > On Wed, Apr 03, 2019 at 03:34:34PM -0300, Flavio Leitner via dev wrote:
> > > On Wed, Apr 03, 2019 at 09:49:13AM -0700, Yifeng Sun wrote:
> > > > From: Flavio Leitner <fbl@redhat.com>
> > > >
> > > > Upstream commit:
> > > >     commit 7f6d6558ae44bc193eb28df3617c364d3bb6df39
> > > >     Author: Flavio Leitner <fbl@redhat.com>
> > > >     Date:   Fri Sep 28 14:55:34 2018 -0300
> > > >
> > > >     Revert "openvswitch: Fix template leak in error cases."
> > > >     This reverts commit 90c7afc.
> > > >
> > > >     When the commit was merged, the code used nf_ct_put() to free
> > > >     the entry, but later on commit 7664423 ("openvswitch: Free
> > > >     tmpl with tmpl_free.") replaced that with nf_ct_tmpl_free which
> > > >     is a more appropriate. Now the original problem is removed.
> > > >
> > > >     Then 44d6e2f ("net: Replace NF_CT_ASSERT() with WARN_ON().")
> > > >     replaced a debug assert with a WARN_ON() which is trigged now.
> > > >
> > > >     Signed-off-by: Flavio Leitner <fbl@redhat.com>
> > > >     Acked-by: Joe Stringer <joe@ovn.org>
> > > >     Signed-off-by: David S. Miller <davem@davemloft.net>
> > > >
> > > > This patch backports this upstream patch to OVS.
> > > >
> > > > Cc: Flavio Leitner <fbl@redhat.com>
> > > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
> > > > ---
> > >
> > > LGTM
> > > Acked-by: Flavio Leitner <fbl@sysclose.org>
> >
> > I applied this to master.  I would appreciate advice on backporting.

Patch
diff mbox series

diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index a7dc9e0c3513..52208bad3029 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -1691,10 +1691,6 @@  int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
 		OVS_NLERR(log, "Failed to allocate conntrack template");
 		return -ENOMEM;
 	}
-
-	__set_bit(IPS_CONFIRMED_BIT, &ct_info.ct->status);
-	nf_conntrack_get(&ct_info.ct->ct_general);
-
 	if (helper) {
 		err = ovs_ct_add_helper(&ct_info, helper, key, log);
 		if (err)
@@ -1706,6 +1702,8 @@  int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
 	if (err)
 		goto err_free_ct;
 
+	__set_bit(IPS_CONFIRMED_BIT, &ct_info.ct->status);
+	nf_conntrack_get(&ct_info.ct->ct_general);
 	return 0;
 err_free_ct:
 	__ovs_ct_free_action(&ct_info);