[ovs-dev] [PATCH 2/2] bond: Fix bug that writes to freed memory
Yifeng Sun
pkusunyifeng at gmail.com
Wed Dec 20 17:56:27 UTC 2017
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 at 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 at 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);
>
More information about the dev
mailing list