On Wed, Mar 20, 2019 at 08:16:18PM +0800, Li Wei wrote: > > After inserting/removing a bucket, we don't update the bucket counter. > When we call ovs-ofctl dump-group-stats br-int, a panic happened.
Thanks for the patch! It looks correct to me. Thank you for adding a test, too. I took a closer look and I saw that 'n_buckets' is not very useful, because it is only used in cases where the code is already O(n_buckets). I think that we can just remove it. Then it cannot get out-of-sync. What do you think of this variation of your patch? --8<--------------------------cut here-------------------------->8-- From: Li Wei <[email protected]> Date: Wed, 20 Mar 2019 20:16:18 +0800 Subject: [PATCH] ofproto: fix the bug of bucket counter is not updated After inserting/removing a bucket, we don't update the bucket counter. When we call ovs-ofctl dump-group-stats br-int, a panic happened. Reproduce steps: 1. ovs-ofctl -O OpenFlow15 add-group br-int "group_id=1, type=select, selection_method=hash bucket=bucket_id=1,weight:100,actions=output:1" 2. ovs-ofctl insert-buckets br-int "group_id=1, command_bucket_id=last, bucket=bucket_id=7,weight:800,actions=output:1" 3. ovs-ofctl dump-group-stats br-int gdb) bt at ../sysdeps/posix/libc_fatal.c:175 ar_ptr=<optimized out>) at malloc.c:5049 group_id=<optimized out>, cb=cb@entry=0x55cab8fd6cd0 <append_group_stats>) at ofproto/ofproto.c:6790 Signed-off-by: solomon <[email protected]> Signed-off-by: Ben Pfaff <[email protected]> --- ofproto/ofproto-dpif.c | 2 +- ofproto/ofproto-provider.h | 1 - ofproto/ofproto.c | 11 ++++------- tests/ofproto.at | 16 ++++++++++++++++ 4 files changed, 21 insertions(+), 9 deletions(-) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 21dd54bab09b..cc6dbc70fe41 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4754,7 +4754,7 @@ static bool group_setup_dp_hash_table(struct group_dpif *group, size_t max_hash) { struct ofputil_bucket *bucket; - uint32_t n_buckets = group->up.n_buckets; + uint32_t n_buckets = ovs_list_size(&group->up.buckets); uint64_t total_weight = 0; uint16_t min_weight = UINT16_MAX; struct webster { diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index d1a87a59e47e..a71d911199f4 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -573,7 +573,6 @@ struct ofgroup { const long long int modified; /* Time of last modification. */ const struct ovs_list buckets; /* Contains "struct ofputil_bucket"s. */ - const uint32_t n_buckets; struct ofputil_group_props props; diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 40780e2766ab..ff3b8410bf49 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -6979,11 +6979,12 @@ append_group_stats(struct ofgroup *group, struct ovs_list *replies) long long int now = time_msec(); int error; - ogs.bucket_stats = xmalloc(group->n_buckets * sizeof *ogs.bucket_stats); + size_t n_buckets = ovs_list_size(&group->buckets); + ogs.bucket_stats = xmalloc(n_buckets * sizeof *ogs.bucket_stats); /* Provider sets the packet and byte counts, we do the rest. */ ogs.ref_count = rule_collection_n(&group->rules); - ogs.n_buckets = group->n_buckets; + ogs.n_buckets = n_buckets; error = (ofproto->ofproto_class->group_get_stats ? ofproto->ofproto_class->group_get_stats(group, &ogs) @@ -6991,8 +6992,7 @@ append_group_stats(struct ofgroup *group, struct ovs_list *replies) if (error) { ogs.packet_count = UINT64_MAX; ogs.byte_count = UINT64_MAX; - memset(ogs.bucket_stats, 0xff, - ogs.n_buckets * sizeof *ogs.bucket_stats); + memset(ogs.bucket_stats, 0xff, n_buckets * sizeof *ogs.bucket_stats); } ogs.group_id = group->group_id; @@ -7212,9 +7212,6 @@ init_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm, &(*ofgroup)->buckets), &gm->buckets, NULL); - *CONST_CAST(uint32_t *, &(*ofgroup)->n_buckets) = - ovs_list_size(&(*ofgroup)->buckets); - ofputil_group_properties_copy(CONST_CAST(struct ofputil_group_props *, &(*ofgroup)->props), &gm->props); diff --git a/tests/ofproto.at b/tests/ofproto.at index 213fb262360b..a810dd6042fd 100644 --- a/tests/ofproto.at +++ b/tests/ofproto.at @@ -1223,6 +1223,22 @@ OFPST_GROUP reply (OF1.5): OVS_VSWITCHD_STOP AT_CLEANUP +dnl This is used to find that the bucket counter is not updated. +AT_SETUP([ofproto - group stats after insert a new bucket (OpenFlow 1.5)]) +OVS_VSWITCHD_START +AT_DATA([groups.txt], [dnl +group_id=1234,type=select,selection_method=hash bucket=bucket_id=1,weight:100,actions=output:10 +]) +AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn add-groups br0 groups.txt]) +AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 'group_id=1234, command_bucket_id=last, bucket=bucket_id=2,weight:100,actions=output:10']) +AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-group-stats br0 group_id=1234], [0], [stdout]) +AT_CHECK([strip_xids < stdout | sed 's/duration=[[0-9.]]*s/duration=?s/' | sort], [0], [dnl + group_id=1234,duration=?s,ref_count=0,packet_count=0,byte_count=0,bucket0:packet_count=0,byte_count=0,bucket1:packet_count=0,byte_count=0 +OFPST_GROUP reply (OF1.5): +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + dnl This found a use-after-free error in bridge destruction in the dnl presence of groups. AT_SETUP([ofproto - group add then bridge delete (OpenFlow 1.3)]) -- 2.20.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
