| Message ID | 1453188448-2437-2-git-send-email-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Jan 18, 2016, at 11:26 PM, Ben Pfaff <blp@ovn.org> wrote: > > In handle_group_mod() cases where adding a group failed, nothing freed the > list of buckets, causing a leak. The same was true in every case of > modifying a group. This commit fixes the problem by changing add_group() > to never steal or free the buckets (modify_group() already acted this way) > and then making handle_group_mod() always free the buckets when it's done. > > This approach might at first raise objections, because it makes add_group() > copy the buckets instead of just take the existing ones. But it actually > fixes a worse problem too: when OF1.4+ REQUESTFORWARD is enabled, the > group_mod is reused for the request forwarding. Until now, for a group_mod > that adds a new group and that has some buckets, the previous stealing of > buckets in add_group() meant that the group_mod's buckets were no longer > valid; in practice, the list of buckets became linked in a way that > iteration never terminated, which caused memory to be exhausted while > composing the requestforward message. By making add_group() no longer > modify the group_mod, we also fix this problem. > > The requestforward test in the testsuite did not find the latter problem > because it only added a group without any buckets. This commit also > updates the testsuite to include a bucket in its group_mod, which would > have found the problem. > > Found by pain and suffering. > > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > ofproto/ofproto.c | 17 +++++++++++------ > tests/ofproto.at | 16 ++++++++-------- > 2 files changed, 19 insertions(+), 14 deletions(-) > > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 528d5ac..386009e 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -297,7 +297,8 @@ static bool ofproto_group_exists__(const struct ofproto *ofproto, > static bool ofproto_group_exists(const struct ofproto *ofproto, > uint32_t group_id) > OVS_EXCLUDED(ofproto->groups_rwlock); > -static enum ofperr add_group(struct ofproto *, struct ofputil_group_mod *); > +static enum ofperr add_group(struct ofproto *, > + const struct ofputil_group_mod *); > static void handle_openflow(struct ofconn *, const struct ofpbuf *); > static enum ofperr ofproto_flow_mod_start(struct ofproto *, > struct ofproto_flow_mod *) > @@ -6280,7 +6281,7 @@ handle_queue_get_config_request(struct ofconn *ofconn, > } > > static enum ofperr > -init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm, > +init_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm, > struct ofgroup **ofgroup) > { > enum ofperr error; > @@ -6306,7 +6307,9 @@ init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm, > *CONST_CAST(long long int *, &((*ofgroup)->modified)) = now; > ovs_refcount_init(&(*ofgroup)->ref_count); > > - list_move(&(*ofgroup)->buckets, &gm->buckets); > + list_init(&(*ofgroup)->buckets); > + ofputil_bucket_clone_list(&(*ofgroup)->buckets, &gm->buckets, NULL); > + > *CONST_CAST(uint32_t *, &(*ofgroup)->n_buckets) = > list_size(&(*ofgroup)->buckets); > > @@ -6326,7 +6329,7 @@ init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm, > * 'ofproto''s group table. Returns 0 on success or an OpenFlow error code on > * failure. */ > static enum ofperr > -add_group(struct ofproto *ofproto, struct ofputil_group_mod *gm) > +add_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm) > { > struct ofgroup *ofgroup; > enum ofperr error; > @@ -6474,7 +6477,7 @@ copy_buckets_for_remove_bucket(const struct ofgroup *ofgroup, > * ofproto's ofgroup hash map. Thus, the group is never altered while users of > * the xlate module hold a pointer to the group. */ > static enum ofperr > -modify_group(struct ofproto *ofproto, struct ofputil_group_mod *gm) > +modify_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm) > { > struct ofgroup *ofgroup, *new_ofgroup, *retiring; > enum ofperr error; > @@ -6643,7 +6646,7 @@ handle_group_mod(struct ofconn *ofconn, const struct ofp_header *oh) > VLOG_INFO_RL(&rl, "%s: Invalid group_mod command type %d", > ofproto->name, gm.command); > } > - return OFPERR_OFPGMFC_BAD_COMMAND; > + error = OFPERR_OFPGMFC_BAD_COMMAND; > } > > if (!error) { > @@ -6653,6 +6656,8 @@ handle_group_mod(struct ofconn *ofconn, const struct ofp_header *oh) > rf.group_mod = &gm; > connmgr_send_requestforward(ofproto->connmgr, ofconn, &rf); > } > + ofputil_bucket_list_destroy(&gm.buckets); > + > return error; > } > > diff --git a/tests/ofproto.at b/tests/ofproto.at > index b3b8a0f..787def1 100644 > --- a/tests/ofproto.at > +++ b/tests/ofproto.at > @@ -3144,25 +3144,25 @@ check_async () { > shift > > # OFPGC_ADD > - ovs-appctl -t `pwd`/c2 ofctl/send 050f0010000000020000000000000001 > + ovs-appctl -t `pwd`/c2 ofctl/send "050f0020000000020000000000000001 00100000 ffffffffffffffff 00000000" > if test X"$1" = X"OFPGC_ADD"; then shift; > echo >>expout2 "send: OFPT_GROUP_MOD (OF1.4): > - ADD group_id=1,type=all" > + ADD group_id=1,type=all,bucket=actions=drop" > echo >>expout1 "OFPT_REQUESTFORWARD (OF1.5): reason=group_mod > - ADD group_id=1,type=all" > + ADD group_id=1,type=all,bucket=bucket_id:0,actions=drop" > echo >>expout3 "OFPT_REQUESTFORWARD (OF1.4): reason=group_mod > - ADD group_id=1,type=all" > + ADD group_id=1,type=all,bucket=actions=drop" > fi > > # OFPGC_MODIFY > - ovs-appctl -t `pwd`/c2 ofctl/send 050f0010000000020001010000000001 > + ovs-appctl -t `pwd`/c2 ofctl/send "050f0020000000020001010000000001 00100000 ffffffffffffffff 00000000" > if test X"$1" = X"OFPGC_MODIFY"; then shift; > echo >>expout2 "send: OFPT_GROUP_MOD (OF1.4): > - MOD group_id=1,type=select" > + MOD group_id=1,type=select,bucket=weight:0,actions=drop" > echo >>expout1 "OFPT_REQUESTFORWARD (OF1.5): reason=group_mod > - MOD group_id=1,type=select" > + MOD group_id=1,type=select,bucket=bucket_id:0,weight:0,actions=drop" > echo >>expout3 "OFPT_REQUESTFORWARD (OF1.4): reason=group_mod > - MOD group_id=1,type=select" > + MOD group_id=1,type=select,bucket=weight:0,actions=drop" > fi > > ovs-appctl -t `pwd`/c1 ofctl/barrier > -- > 2.1.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
Thanks. I applied this to master, branch-2.5, and branch-2.4. On Tue, Jan 19, 2016 at 11:25:03AM -0800, Jarno Rajahalme wrote: > Acked-by: Jarno Rajahalme <jarno@ovn.org> > > > On Jan 18, 2016, at 11:26 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > In handle_group_mod() cases where adding a group failed, nothing freed the > > list of buckets, causing a leak. The same was true in every case of > > modifying a group. This commit fixes the problem by changing add_group() > > to never steal or free the buckets (modify_group() already acted this way) > > and then making handle_group_mod() always free the buckets when it's done. > > > > This approach might at first raise objections, because it makes add_group() > > copy the buckets instead of just take the existing ones. But it actually > > fixes a worse problem too: when OF1.4+ REQUESTFORWARD is enabled, the > > group_mod is reused for the request forwarding. Until now, for a group_mod > > that adds a new group and that has some buckets, the previous stealing of > > buckets in add_group() meant that the group_mod's buckets were no longer > > valid; in practice, the list of buckets became linked in a way that > > iteration never terminated, which caused memory to be exhausted while > > composing the requestforward message. By making add_group() no longer > > modify the group_mod, we also fix this problem. > > > > The requestforward test in the testsuite did not find the latter problem > > because it only added a group without any buckets. This commit also > > updates the testsuite to include a bucket in its group_mod, which would > > have found the problem. > > > > Found by pain and suffering. > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > ofproto/ofproto.c | 17 +++++++++++------ > > tests/ofproto.at | 16 ++++++++-------- > > 2 files changed, 19 insertions(+), 14 deletions(-) > > > > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > > index 528d5ac..386009e 100644 > > --- a/ofproto/ofproto.c > > +++ b/ofproto/ofproto.c > > @@ -297,7 +297,8 @@ static bool ofproto_group_exists__(const struct ofproto *ofproto, > > static bool ofproto_group_exists(const struct ofproto *ofproto, > > uint32_t group_id) > > OVS_EXCLUDED(ofproto->groups_rwlock); > > -static enum ofperr add_group(struct ofproto *, struct ofputil_group_mod *); > > +static enum ofperr add_group(struct ofproto *, > > + const struct ofputil_group_mod *); > > static void handle_openflow(struct ofconn *, const struct ofpbuf *); > > static enum ofperr ofproto_flow_mod_start(struct ofproto *, > > struct ofproto_flow_mod *) > > @@ -6280,7 +6281,7 @@ handle_queue_get_config_request(struct ofconn *ofconn, > > } > > > > static enum ofperr > > -init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm, > > +init_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm, > > struct ofgroup **ofgroup) > > { > > enum ofperr error; > > @@ -6306,7 +6307,9 @@ init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm, > > *CONST_CAST(long long int *, &((*ofgroup)->modified)) = now; > > ovs_refcount_init(&(*ofgroup)->ref_count); > > > > - list_move(&(*ofgroup)->buckets, &gm->buckets); > > + list_init(&(*ofgroup)->buckets); > > + ofputil_bucket_clone_list(&(*ofgroup)->buckets, &gm->buckets, NULL); > > + > > *CONST_CAST(uint32_t *, &(*ofgroup)->n_buckets) = > > list_size(&(*ofgroup)->buckets); > > > > @@ -6326,7 +6329,7 @@ init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm, > > * 'ofproto''s group table. Returns 0 on success or an OpenFlow error code on > > * failure. */ > > static enum ofperr > > -add_group(struct ofproto *ofproto, struct ofputil_group_mod *gm) > > +add_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm) > > { > > struct ofgroup *ofgroup; > > enum ofperr error; > > @@ -6474,7 +6477,7 @@ copy_buckets_for_remove_bucket(const struct ofgroup *ofgroup, > > * ofproto's ofgroup hash map. Thus, the group is never altered while users of > > * the xlate module hold a pointer to the group. */ > > static enum ofperr > > -modify_group(struct ofproto *ofproto, struct ofputil_group_mod *gm) > > +modify_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm) > > { > > struct ofgroup *ofgroup, *new_ofgroup, *retiring; > > enum ofperr error; > > @@ -6643,7 +6646,7 @@ handle_group_mod(struct ofconn *ofconn, const struct ofp_header *oh) > > VLOG_INFO_RL(&rl, "%s: Invalid group_mod command type %d", > > ofproto->name, gm.command); > > } > > - return OFPERR_OFPGMFC_BAD_COMMAND; > > + error = OFPERR_OFPGMFC_BAD_COMMAND; > > } > > > > if (!error) { > > @@ -6653,6 +6656,8 @@ handle_group_mod(struct ofconn *ofconn, const struct ofp_header *oh) > > rf.group_mod = &gm; > > connmgr_send_requestforward(ofproto->connmgr, ofconn, &rf); > > } > > + ofputil_bucket_list_destroy(&gm.buckets); > > + > > return error; > > } > > > > diff --git a/tests/ofproto.at b/tests/ofproto.at > > index b3b8a0f..787def1 100644 > > --- a/tests/ofproto.at > > +++ b/tests/ofproto.at > > @@ -3144,25 +3144,25 @@ check_async () { > > shift > > > > # OFPGC_ADD > > - ovs-appctl -t `pwd`/c2 ofctl/send 050f0010000000020000000000000001 > > + ovs-appctl -t `pwd`/c2 ofctl/send "050f0020000000020000000000000001 00100000 ffffffffffffffff 00000000" > > if test X"$1" = X"OFPGC_ADD"; then shift; > > echo >>expout2 "send: OFPT_GROUP_MOD (OF1.4): > > - ADD group_id=1,type=all" > > + ADD group_id=1,type=all,bucket=actions=drop" > > echo >>expout1 "OFPT_REQUESTFORWARD (OF1.5): reason=group_mod > > - ADD group_id=1,type=all" > > + ADD group_id=1,type=all,bucket=bucket_id:0,actions=drop" > > echo >>expout3 "OFPT_REQUESTFORWARD (OF1.4): reason=group_mod > > - ADD group_id=1,type=all" > > + ADD group_id=1,type=all,bucket=actions=drop" > > fi > > > > # OFPGC_MODIFY > > - ovs-appctl -t `pwd`/c2 ofctl/send 050f0010000000020001010000000001 > > + ovs-appctl -t `pwd`/c2 ofctl/send "050f0020000000020001010000000001 00100000 ffffffffffffffff 00000000" > > if test X"$1" = X"OFPGC_MODIFY"; then shift; > > echo >>expout2 "send: OFPT_GROUP_MOD (OF1.4): > > - MOD group_id=1,type=select" > > + MOD group_id=1,type=select,bucket=weight:0,actions=drop" > > echo >>expout1 "OFPT_REQUESTFORWARD (OF1.5): reason=group_mod > > - MOD group_id=1,type=select" > > + MOD group_id=1,type=select,bucket=bucket_id:0,weight:0,actions=drop" > > echo >>expout3 "OFPT_REQUESTFORWARD (OF1.4): reason=group_mod > > - MOD group_id=1,type=select" > > + MOD group_id=1,type=select,bucket=weight:0,actions=drop" > > fi > > > > ovs-appctl -t `pwd`/c1 ofctl/barrier > > -- > > 2.1.3 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev >
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 528d5ac..386009e 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -297,7 +297,8 @@ static bool ofproto_group_exists__(const struct ofproto *ofproto, static bool ofproto_group_exists(const struct ofproto *ofproto, uint32_t group_id) OVS_EXCLUDED(ofproto->groups_rwlock); -static enum ofperr add_group(struct ofproto *, struct ofputil_group_mod *); +static enum ofperr add_group(struct ofproto *, + const struct ofputil_group_mod *); static void handle_openflow(struct ofconn *, const struct ofpbuf *); static enum ofperr ofproto_flow_mod_start(struct ofproto *, struct ofproto_flow_mod *) @@ -6280,7 +6281,7 @@ handle_queue_get_config_request(struct ofconn *ofconn, } static enum ofperr -init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm, +init_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm, struct ofgroup **ofgroup) { enum ofperr error; @@ -6306,7 +6307,9 @@ init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm, *CONST_CAST(long long int *, &((*ofgroup)->modified)) = now; ovs_refcount_init(&(*ofgroup)->ref_count); - list_move(&(*ofgroup)->buckets, &gm->buckets); + list_init(&(*ofgroup)->buckets); + ofputil_bucket_clone_list(&(*ofgroup)->buckets, &gm->buckets, NULL); + *CONST_CAST(uint32_t *, &(*ofgroup)->n_buckets) = list_size(&(*ofgroup)->buckets); @@ -6326,7 +6329,7 @@ init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm, * 'ofproto''s group table. Returns 0 on success or an OpenFlow error code on * failure. */ static enum ofperr -add_group(struct ofproto *ofproto, struct ofputil_group_mod *gm) +add_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm) { struct ofgroup *ofgroup; enum ofperr error; @@ -6474,7 +6477,7 @@ copy_buckets_for_remove_bucket(const struct ofgroup *ofgroup, * ofproto's ofgroup hash map. Thus, the group is never altered while users of * the xlate module hold a pointer to the group. */ static enum ofperr -modify_group(struct ofproto *ofproto, struct ofputil_group_mod *gm) +modify_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm) { struct ofgroup *ofgroup, *new_ofgroup, *retiring; enum ofperr error; @@ -6643,7 +6646,7 @@ handle_group_mod(struct ofconn *ofconn, const struct ofp_header *oh) VLOG_INFO_RL(&rl, "%s: Invalid group_mod command type %d", ofproto->name, gm.command); } - return OFPERR_OFPGMFC_BAD_COMMAND; + error = OFPERR_OFPGMFC_BAD_COMMAND; } if (!error) { @@ -6653,6 +6656,8 @@ handle_group_mod(struct ofconn *ofconn, const struct ofp_header *oh) rf.group_mod = &gm; connmgr_send_requestforward(ofproto->connmgr, ofconn, &rf); } + ofputil_bucket_list_destroy(&gm.buckets); + return error; } diff --git a/tests/ofproto.at b/tests/ofproto.at index b3b8a0f..787def1 100644 --- a/tests/ofproto.at +++ b/tests/ofproto.at @@ -3144,25 +3144,25 @@ check_async () { shift # OFPGC_ADD - ovs-appctl -t `pwd`/c2 ofctl/send 050f0010000000020000000000000001 + ovs-appctl -t `pwd`/c2 ofctl/send "050f0020000000020000000000000001 00100000 ffffffffffffffff 00000000" if test X"$1" = X"OFPGC_ADD"; then shift; echo >>expout2 "send: OFPT_GROUP_MOD (OF1.4): - ADD group_id=1,type=all" + ADD group_id=1,type=all,bucket=actions=drop" echo >>expout1 "OFPT_REQUESTFORWARD (OF1.5): reason=group_mod - ADD group_id=1,type=all" + ADD group_id=1,type=all,bucket=bucket_id:0,actions=drop" echo >>expout3 "OFPT_REQUESTFORWARD (OF1.4): reason=group_mod - ADD group_id=1,type=all" + ADD group_id=1,type=all,bucket=actions=drop" fi # OFPGC_MODIFY - ovs-appctl -t `pwd`/c2 ofctl/send 050f0010000000020001010000000001 + ovs-appctl -t `pwd`/c2 ofctl/send "050f0020000000020001010000000001 00100000 ffffffffffffffff 00000000" if test X"$1" = X"OFPGC_MODIFY"; then shift; echo >>expout2 "send: OFPT_GROUP_MOD (OF1.4): - MOD group_id=1,type=select" + MOD group_id=1,type=select,bucket=weight:0,actions=drop" echo >>expout1 "OFPT_REQUESTFORWARD (OF1.5): reason=group_mod - MOD group_id=1,type=select" + MOD group_id=1,type=select,bucket=bucket_id:0,weight:0,actions=drop" echo >>expout3 "OFPT_REQUESTFORWARD (OF1.4): reason=group_mod - MOD group_id=1,type=select" + MOD group_id=1,type=select,bucket=weight:0,actions=drop" fi ovs-appctl -t `pwd`/c1 ofctl/barrier
In handle_group_mod() cases where adding a group failed, nothing freed the list of buckets, causing a leak. The same was true in every case of modifying a group. This commit fixes the problem by changing add_group() to never steal or free the buckets (modify_group() already acted this way) and then making handle_group_mod() always free the buckets when it's done. This approach might at first raise objections, because it makes add_group() copy the buckets instead of just take the existing ones. But it actually fixes a worse problem too: when OF1.4+ REQUESTFORWARD is enabled, the group_mod is reused for the request forwarding. Until now, for a group_mod that adds a new group and that has some buckets, the previous stealing of buckets in add_group() meant that the group_mod's buckets were no longer valid; in practice, the list of buckets became linked in a way that iteration never terminated, which caused memory to be exhausted while composing the requestforward message. By making add_group() no longer modify the group_mod, we also fix this problem. The requestforward test in the testsuite did not find the latter problem because it only added a group without any buckets. This commit also updates the testsuite to include a bucket in its group_mod, which would have found the problem. Found by pain and suffering. Signed-off-by: Ben Pfaff <blp@ovn.org> --- ofproto/ofproto.c | 17 +++++++++++------ tests/ofproto.at | 16 ++++++++-------- 2 files changed, 19 insertions(+), 14 deletions(-)