Re: [ovs-dev] [PATCH 2/2] bond: Fix bug that writes to freed memory

2017-12-20 Thread Ben Pfaff 
Thank you.

I applied this to master and backported it as far as branch-2.3.

On Wed, Dec 20, 2017 at 09:56:27AM -0800, Yifeng Sun wrote:
> Hi Ben,
> 
> I think that simply moving up update_recirc_rules__ brings some confusion
> because if looking into update_recirc_rules__, at line 345, when bond->hash
> is valid, update_recirc_rules__ may use bond->hash to invoke add_pr_rule.
> I feel it is quite hard to figure out what is going on.
> 
> Thanks,
> Yifeng
> 
> 
> On Wed, Dec 20, 2017 at 9:39 AM, Ben Pfaff  wrote:
> 
> > On Mon, Dec 11, 2017 at 05:44:07AM -0800, Yifeng Sun wrote:
> > > pr_op->pr_rule is pointing to memory in bond->hash. It shouldn't be
> > written
> > > if bond->hash is already freed.
> > >
> > > This bug is reported by running kernel path testsuite under valgrind:
> > > Invalid write of size 8
> > >at 0x413D16: update_recirc_rules__ (bond.c:392)
> > >by 0x414CA0: bond_unref (bond.c:290)
> > >by 0x427E3C: bundle_destroy (ofproto-dpif.c:3002)
> > >by 0x429EF4: bundle_set (ofproto-dpif.c:3023)
> > >by 0x40858B: port_destroy (bridge.c:4087)
> > >by 0x40BD04: bridge_destroy (bridge.c:3266)
> > >by 0x410528: bridge_exit (bridge.c:506)
> > >by 0x4072EE: main (ovs-vswitchd.c:135)
> > >  Address 0xb5a85f0 is 5,360 bytes inside a block of size 12,288 free'd
> > >at 0x4C2EDEB: free (/usr/lib/valgrind/vgpreload_
> > memcheck-amd64-linux.so)
> > >by 0x414C8D: bond_unref (bond.c:288)
> > >by 0x427E3C: bundle_destroy (ofproto-dpif.c:3002)
> > >by 0x429EF4: bundle_set (ofproto-dpif.c:3023)
> > >by 0x40858B: port_destroy (bridge.c:4087)
> > >by 0x40BD04: bridge_destroy (bridge.c:3266)
> > >by 0x410528: bridge_exit (bridge.c:506)
> > >by 0x4072EE: main (ovs-vswitchd.c:135)
> > >  Block was alloc'd at
> > >at 0x4C2DB8F: malloc (/usr/lib/valgrind/vgpreload_
> > memcheck-amd64-linux.so)
> > >by 0x516C04: xmalloc (util.c:120)
> > >by 0x414FD1: bond_entry_reset (bond.c:1651)
> > >by 0x414FD1: bond_reconfigure (bond.c:470)
> > >by 0x41507D: bond_create (bond.c:245)
> > >by 0x429D5D: bundle_set (ofproto-dpif.c:3194)
> > >by 0x408AC8: port_configure (bridge.c:1052)
> > >by 0x40CD87: bridge_reconfigure (bridge.c:682)
> > >by 0x410775: bridge_run (bridge.c:2998)
> > >by 0x407244: main (ovs-vswitchd.c:119)
> > >
> > > Signed-off-by: Yifeng Sun 
> > > ---
> > >  ofproto/bond.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/ofproto/bond.c b/ofproto/bond.c
> > > index 8ecd22c7d5d3..6f3d7b5b3817 100644
> > > --- a/ofproto/bond.c
> > > +++ b/ofproto/bond.c
> > > @@ -389,7 +389,9 @@ update_recirc_rules__(struct bond *bond)
> > >  }
> > >
> > >  hmap_remove(>pr_rule_ops, _op->hmap_node);
> > > -*pr_op->pr_rule = NULL;
> > > +if (bond->hash) {
> > > +*pr_op->pr_rule = NULL;
> > > +}
> > >  free(pr_op);
> > >  break;
> > >  }
> >
> > Thank you for the bug fix.
> >
> > This bug triggers along the bond destruction path.  I think that another
> > alternative would be to free bond->hash later, like the following.  Does
> > that also fix the problem?  Do you have an opinion on which version is
> > easier to understand and to maintain?
> >
> > Thanks,
> >
> > Ben.
> >
> > diff --git a/ofproto/bond.c b/ofproto/bond.c
> > index 6f3d7b5b3817..9d46758dc652 100644
> > --- a/ofproto/bond.c
> > +++ b/ofproto/bond.c
> > @@ -285,9 +285,9 @@ bond_unref(struct bond *bond)
> >  recirc_free_id(bond->recirc_id);
> >  bond->recirc_id = 0;
> >  }
> > +update_recirc_rules__(bond);
> >  free(bond->hash);
> >  bond->hash = NULL;
> > -update_recirc_rules__(bond);
> >
> >  hmap_destroy(>pr_rule_ops);
> >  free(bond->name);
> >
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] bond: Fix bug that writes to freed memory

2017-12-20 Thread Yifeng Sun 
Hi Ben,

I think that simply moving up update_recirc_rules__ brings some confusion
because if looking into update_recirc_rules__, at line 345, when bond->hash
is valid, update_recirc_rules__ may use bond->hash to invoke add_pr_rule.
I feel it is quite hard to figure out what is going on.

Thanks,
Yifeng


On Wed, Dec 20, 2017 at 9:39 AM, Ben Pfaff  wrote:

> On Mon, Dec 11, 2017 at 05:44:07AM -0800, Yifeng Sun wrote:
> > pr_op->pr_rule is pointing to memory in bond->hash. It shouldn't be
> written
> > if bond->hash is already freed.
> >
> > This bug is reported by running kernel path testsuite under valgrind:
> > Invalid write of size 8
> >at 0x413D16: update_recirc_rules__ (bond.c:392)
> >by 0x414CA0: bond_unref (bond.c:290)
> >by 0x427E3C: bundle_destroy (ofproto-dpif.c:3002)
> >by 0x429EF4: bundle_set (ofproto-dpif.c:3023)
> >by 0x40858B: port_destroy (bridge.c:4087)
> >by 0x40BD04: bridge_destroy (bridge.c:3266)
> >by 0x410528: bridge_exit (bridge.c:506)
> >by 0x4072EE: main (ovs-vswitchd.c:135)
> >  Address 0xb5a85f0 is 5,360 bytes inside a block of size 12,288 free'd
> >at 0x4C2EDEB: free (/usr/lib/valgrind/vgpreload_
> memcheck-amd64-linux.so)
> >by 0x414C8D: bond_unref (bond.c:288)
> >by 0x427E3C: bundle_destroy (ofproto-dpif.c:3002)
> >by 0x429EF4: bundle_set (ofproto-dpif.c:3023)
> >by 0x40858B: port_destroy (bridge.c:4087)
> >by 0x40BD04: bridge_destroy (bridge.c:3266)
> >by 0x410528: bridge_exit (bridge.c:506)
> >by 0x4072EE: main (ovs-vswitchd.c:135)
> >  Block was alloc'd at
> >at 0x4C2DB8F: malloc (/usr/lib/valgrind/vgpreload_
> memcheck-amd64-linux.so)
> >by 0x516C04: xmalloc (util.c:120)
> >by 0x414FD1: bond_entry_reset (bond.c:1651)
> >by 0x414FD1: bond_reconfigure (bond.c:470)
> >by 0x41507D: bond_create (bond.c:245)
> >by 0x429D5D: bundle_set (ofproto-dpif.c:3194)
> >by 0x408AC8: port_configure (bridge.c:1052)
> >by 0x40CD87: bridge_reconfigure (bridge.c:682)
> >by 0x410775: bridge_run (bridge.c:2998)
> >by 0x407244: main (ovs-vswitchd.c:119)
> >
> > Signed-off-by: Yifeng Sun 
> > ---
> >  ofproto/bond.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/ofproto/bond.c b/ofproto/bond.c
> > index 8ecd22c7d5d3..6f3d7b5b3817 100644
> > --- a/ofproto/bond.c
> > +++ b/ofproto/bond.c
> > @@ -389,7 +389,9 @@ update_recirc_rules__(struct bond *bond)
> >  }
> >
> >  hmap_remove(>pr_rule_ops, _op->hmap_node);
> > -*pr_op->pr_rule = NULL;
> > +if (bond->hash) {
> > +*pr_op->pr_rule = NULL;
> > +}
> >  free(pr_op);
> >  break;
> >  }
>
> Thank you for the bug fix.
>
> This bug triggers along the bond destruction path.  I think that another
> alternative would be to free bond->hash later, like the following.  Does
> that also fix the problem?  Do you have an opinion on which version is
> easier to understand and to maintain?
>
> Thanks,
>
> Ben.
>
> diff --git a/ofproto/bond.c b/ofproto/bond.c
> index 6f3d7b5b3817..9d46758dc652 100644
> --- a/ofproto/bond.c
> +++ b/ofproto/bond.c
> @@ -285,9 +285,9 @@ bond_unref(struct bond *bond)
>  recirc_free_id(bond->recirc_id);
>  bond->recirc_id = 0;
>  }
> +update_recirc_rules__(bond);
>  free(bond->hash);
>  bond->hash = NULL;
> -update_recirc_rules__(bond);
>
>  hmap_destroy(>pr_rule_ops);
>  free(bond->name);
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] bond: Fix bug that writes to freed memory

2017-12-20 Thread Ben Pfaff 
On Mon, Dec 11, 2017 at 05:44:07AM -0800, Yifeng Sun wrote:
> pr_op->pr_rule is pointing to memory in bond->hash. It shouldn't be written
> if bond->hash is already freed.
> 
> This bug is reported by running kernel path testsuite under valgrind:
> Invalid write of size 8
>at 0x413D16: update_recirc_rules__ (bond.c:392)
>by 0x414CA0: bond_unref (bond.c:290)
>by 0x427E3C: bundle_destroy (ofproto-dpif.c:3002)
>by 0x429EF4: bundle_set (ofproto-dpif.c:3023)
>by 0x40858B: port_destroy (bridge.c:4087)
>by 0x40BD04: bridge_destroy (bridge.c:3266)
>by 0x410528: bridge_exit (bridge.c:506)
>by 0x4072EE: main (ovs-vswitchd.c:135)
>  Address 0xb5a85f0 is 5,360 bytes inside a block of size 12,288 free'd
>at 0x4C2EDEB: free (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>by 0x414C8D: bond_unref (bond.c:288)
>by 0x427E3C: bundle_destroy (ofproto-dpif.c:3002)
>by 0x429EF4: bundle_set (ofproto-dpif.c:3023)
>by 0x40858B: port_destroy (bridge.c:4087)
>by 0x40BD04: bridge_destroy (bridge.c:3266)
>by 0x410528: bridge_exit (bridge.c:506)
>by 0x4072EE: main (ovs-vswitchd.c:135)
>  Block was alloc'd at
>at 0x4C2DB8F: malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>by 0x516C04: xmalloc (util.c:120)
>by 0x414FD1: bond_entry_reset (bond.c:1651)
>by 0x414FD1: bond_reconfigure (bond.c:470)
>by 0x41507D: bond_create (bond.c:245)
>by 0x429D5D: bundle_set (ofproto-dpif.c:3194)
>by 0x408AC8: port_configure (bridge.c:1052)
>by 0x40CD87: bridge_reconfigure (bridge.c:682)
>by 0x410775: bridge_run (bridge.c:2998)
>by 0x407244: main (ovs-vswitchd.c:119)
> 
> Signed-off-by: Yifeng Sun 
> ---
>  ofproto/bond.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/ofproto/bond.c b/ofproto/bond.c
> index 8ecd22c7d5d3..6f3d7b5b3817 100644
> --- a/ofproto/bond.c
> +++ b/ofproto/bond.c
> @@ -389,7 +389,9 @@ update_recirc_rules__(struct bond *bond)
>  }
>  
>  hmap_remove(>pr_rule_ops, _op->hmap_node);
> -*pr_op->pr_rule = NULL;
> +if (bond->hash) {
> +*pr_op->pr_rule = NULL;
> +}
>  free(pr_op);
>  break;
>  }

Thank you for the bug fix.

This bug triggers along the bond destruction path.  I think that another
alternative would be to free bond->hash later, like the following.  Does
that also fix the problem?  Do you have an opinion on which version is
easier to understand and to maintain?

Thanks,

Ben.

diff --git a/ofproto/bond.c b/ofproto/bond.c
index 6f3d7b5b3817..9d46758dc652 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -285,9 +285,9 @@ bond_unref(struct bond *bond)
 recirc_free_id(bond->recirc_id);
 bond->recirc_id = 0;
 }
+update_recirc_rules__(bond);
 free(bond->hash);
 bond->hash = NULL;
-update_recirc_rules__(bond);
 
 hmap_destroy(>pr_rule_ops);
 free(bond->name);
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] bond: Fix bug that writes to freed memory

2017-12-12 Thread Gregory Rose 

On 12/11/2017 5:44 AM, Yifeng Sun wrote:

pr_op->pr_rule is pointing to memory in bond->hash. It shouldn't be written
if bond->hash is already freed.

This bug is reported by running kernel path testsuite under valgrind:
Invalid write of size 8
at 0x413D16: update_recirc_rules__ (bond.c:392)
by 0x414CA0: bond_unref (bond.c:290)
by 0x427E3C: bundle_destroy (ofproto-dpif.c:3002)
by 0x429EF4: bundle_set (ofproto-dpif.c:3023)
by 0x40858B: port_destroy (bridge.c:4087)
by 0x40BD04: bridge_destroy (bridge.c:3266)
by 0x410528: bridge_exit (bridge.c:506)
by 0x4072EE: main (ovs-vswitchd.c:135)
  Address 0xb5a85f0 is 5,360 bytes inside a block of size 12,288 free'd
at 0x4C2EDEB: free (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x414C8D: bond_unref (bond.c:288)
by 0x427E3C: bundle_destroy (ofproto-dpif.c:3002)
by 0x429EF4: bundle_set (ofproto-dpif.c:3023)
by 0x40858B: port_destroy (bridge.c:4087)
by 0x40BD04: bridge_destroy (bridge.c:3266)
by 0x410528: bridge_exit (bridge.c:506)
by 0x4072EE: main (ovs-vswitchd.c:135)
  Block was alloc'd at
at 0x4C2DB8F: malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x516C04: xmalloc (util.c:120)
by 0x414FD1: bond_entry_reset (bond.c:1651)
by 0x414FD1: bond_reconfigure (bond.c:470)
by 0x41507D: bond_create (bond.c:245)
by 0x429D5D: bundle_set (ofproto-dpif.c:3194)
by 0x408AC8: port_configure (bridge.c:1052)
by 0x40CD87: bridge_reconfigure (bridge.c:682)
by 0x410775: bridge_run (bridge.c:2998)
by 0x407244: main (ovs-vswitchd.c:119)

Signed-off-by: Yifeng Sun 
---
  ofproto/bond.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ofproto/bond.c b/ofproto/bond.c
index 8ecd22c7d5d3..6f3d7b5b3817 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -389,7 +389,9 @@ update_recirc_rules__(struct bond *bond)
  }
  
  hmap_remove(>pr_rule_ops, _op->hmap_node);

-*pr_op->pr_rule = NULL;
+if (bond->hash) {
+*pr_op->pr_rule = NULL;
+}
  free(pr_op);
  break;
  }


Tested-by: Greg Rose 
Reviewed-by: Greg Rose 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 2/2] bond: Fix bug that writes to freed memory

2017-12-11 Thread Yifeng Sun 
pr_op->pr_rule is pointing to memory in bond->hash. It shouldn't be written
if bond->hash is already freed.

This bug is reported by running kernel path testsuite under valgrind:
Invalid write of size 8
   at 0x413D16: update_recirc_rules__ (bond.c:392)
   by 0x414CA0: bond_unref (bond.c:290)
   by 0x427E3C: bundle_destroy (ofproto-dpif.c:3002)
   by 0x429EF4: bundle_set (ofproto-dpif.c:3023)
   by 0x40858B: port_destroy (bridge.c:4087)
   by 0x40BD04: bridge_destroy (bridge.c:3266)
   by 0x410528: bridge_exit (bridge.c:506)
   by 0x4072EE: main (ovs-vswitchd.c:135)
 Address 0xb5a85f0 is 5,360 bytes inside a block of size 12,288 free'd
   at 0x4C2EDEB: free (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x414C8D: bond_unref (bond.c:288)
   by 0x427E3C: bundle_destroy (ofproto-dpif.c:3002)
   by 0x429EF4: bundle_set (ofproto-dpif.c:3023)
   by 0x40858B: port_destroy (bridge.c:4087)
   by 0x40BD04: bridge_destroy (bridge.c:3266)
   by 0x410528: bridge_exit (bridge.c:506)
   by 0x4072EE: main (ovs-vswitchd.c:135)
 Block was alloc'd at
   at 0x4C2DB8F: malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x516C04: xmalloc (util.c:120)
   by 0x414FD1: bond_entry_reset (bond.c:1651)
   by 0x414FD1: bond_reconfigure (bond.c:470)
   by 0x41507D: bond_create (bond.c:245)
   by 0x429D5D: bundle_set (ofproto-dpif.c:3194)
   by 0x408AC8: port_configure (bridge.c:1052)
   by 0x40CD87: bridge_reconfigure (bridge.c:682)
   by 0x410775: bridge_run (bridge.c:2998)
   by 0x407244: main (ovs-vswitchd.c:119)

Signed-off-by: Yifeng Sun 
---
 ofproto/bond.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ofproto/bond.c b/ofproto/bond.c
index 8ecd22c7d5d3..6f3d7b5b3817 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -389,7 +389,9 @@ update_recirc_rules__(struct bond *bond)
 }
 
 hmap_remove(>pr_rule_ops, _op->hmap_node);
-*pr_op->pr_rule = NULL;
+if (bond->hash) {
+*pr_op->pr_rule = NULL;
+}
 free(pr_op);
 break;
 }
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev