| Message ID | 1512999847-24034-2-git-send-email-pkusunyifeng@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,1/2] valgrind: Add support to run kernel datapath testsuite under valgrind | expand |
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 <pkusunyifeng@gmail.com> > --- > 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(&bond->pr_rule_ops, &pr_op->hmap_node); > - *pr_op->pr_rule = NULL; > + if (bond->hash) { > + *pr_op->pr_rule = NULL; > + } > free(pr_op); > break; > } Tested-by: Greg Rose <gvrose8192@gmail.com> Reviewed-by: Greg Rose <gvrose8192@gmail.com>
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 <pkusunyifeng@gmail.com> > --- > 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(&bond->pr_rule_ops, &pr_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(&bond->pr_rule_ops); free(bond->name);
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 <blp@ovn.org> 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 <pkusunyifeng@gmail.com> > > --- > > 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(&bond->pr_rule_ops, &pr_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(&bond->pr_rule_ops); > free(bond->name); >
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 <blp@ovn.org> 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 <pkusunyifeng@gmail.com> > > > --- > > > 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(&bond->pr_rule_ops, &pr_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(&bond->pr_rule_ops); > > free(bond->name); > >
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(&bond->pr_rule_ops, &pr_op->hmap_node); - *pr_op->pr_rule = NULL; + if (bond->hash) { + *pr_op->pr_rule = NULL; + } free(pr_op); break; }
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 <pkusunyifeng@gmail.com> --- ofproto/bond.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)