| Message ID | 20230903071028.1518913-3-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4b00920da1dd2bbb33baeb2e7b9808af4c68de97 |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: Fix some bugs in kunit_filter_suites() | expand |
Hi Jinjie, kernel test robot noticed the following build warnings: [auto build test WARNING on linus/master] [also build test WARNING on next-20230831] [cannot apply to v6.5] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Jinjie-Ruan/kunit-Fix-wild-memory-access-bug-in-kunit_free_suite_set/20230903-151137 base: linus/master patch link: https://lore.kernel.org/r/20230903071028.1518913-3-ruanjinjie%40huawei.com patch subject: [PATCH v2 2/4] kunit: Fix the wrong err path and add goto labels in kunit_filter_suites() config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230903/202309031733.usGHpnSR-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230903/202309031733.usGHpnSR-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202309031733.usGHpnSR-lkp@intel.com/ All warnings (new ones prefixed by >>): lib/kunit/executor.c: In function 'kunit_filter_suites': >> lib/kunit/executor.c:227:1: warning: label 'free_copy' defined but not used [-Wunused-label] 227 | free_copy: | ^~~~~~~~~ >> lib/kunit/executor.c:221:1: warning: label 'free_parsed_glob' defined but not used [-Wunused-label] 221 | free_parsed_glob: | ^~~~~~~~~~~~~~~~ vim +/free_copy +227 lib/kunit/executor.c 132 133 struct kunit_suite_set 134 kunit_filter_suites(const struct kunit_suite_set *suite_set, 135 const char *filter_glob, 136 char *filters, 137 char *filter_action, 138 int *err) 139 { 140 int i, j, k; 141 int filter_count = 0; 142 struct kunit_suite **copy, **copy_start, *filtered_suite, *new_filtered_suite; 143 struct kunit_suite_set filtered = {NULL, NULL}; 144 struct kunit_glob_filter parsed_glob; 145 struct kunit_attr_filter *parsed_filters = NULL; 146 147 const size_t max = suite_set->end - suite_set->start; 148 149 copy = kmalloc_array(max, sizeof(*filtered.start), GFP_KERNEL); 150 if (!copy) { /* won't be able to run anything, return an empty set */ 151 return filtered; 152 } 153 copy_start = copy; 154 155 if (filter_glob) 156 kunit_parse_glob_filter(&parsed_glob, filter_glob); 157 158 /* Parse attribute filters */ 159 if (filters) { 160 filter_count = kunit_get_filter_count(filters); 161 parsed_filters = kcalloc(filter_count, sizeof(*parsed_filters), GFP_KERNEL); 162 if (!parsed_filters) { 163 kfree(copy); 164 return filtered; 165 } 166 for (j = 0; j < filter_count; j++) 167 parsed_filters[j] = kunit_next_attr_filter(&filters, err); 168 if (*err) 169 goto free_parsed_filters; 170 } 171 172 for (i = 0; &suite_set->start[i] != suite_set->end; i++) { 173 filtered_suite = suite_set->start[i]; 174 if (filter_glob) { 175 if (!glob_match(parsed_glob.suite_glob, filtered_suite->name)) 176 continue; 177 filtered_suite = kunit_filter_glob_tests(filtered_suite, 178 parsed_glob.test_glob); 179 if (IS_ERR(filtered_suite)) { 180 *err = PTR_ERR(filtered_suite); 181 goto free_parsed_filters; 182 } 183 } 184 if (filter_count > 0 && parsed_filters != NULL) { 185 for (k = 0; k < filter_count; k++) { 186 new_filtered_suite = kunit_filter_attr_tests(filtered_suite, 187 parsed_filters[k], filter_action, err); 188 189 /* Free previous copy of suite */ 190 if (k > 0 || filter_glob) { 191 kfree(filtered_suite->test_cases); 192 kfree(filtered_suite); 193 } 194 195 filtered_suite = new_filtered_suite; 196 197 if (*err) 198 goto free_parsed_filters; 199 200 if (IS_ERR(filtered_suite)) { 201 *err = PTR_ERR(filtered_suite); 202 goto free_parsed_filters; 203 } 204 if (!filtered_suite) 205 break; 206 } 207 } 208 209 if (!filtered_suite) 210 continue; 211 212 *copy++ = filtered_suite; 213 } 214 filtered.start = copy_start; 215 filtered.end = copy; 216 217 free_parsed_filters: 218 if (filter_count) 219 kfree(parsed_filters); 220 > 221 free_parsed_glob: 222 if (filter_glob) { 223 kfree(parsed_glob.suite_glob); 224 kfree(parsed_glob.test_glob); 225 } 226 > 227 free_copy: 228 if (*err) 229 kfree(copy); 230 231 return filtered; 232 } 233
Hi Jinjie, kernel test robot noticed the following build warnings: [auto build test WARNING on linus/master] [also build test WARNING on next-20230831] [cannot apply to v6.5] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Jinjie-Ruan/kunit-Fix-wild-memory-access-bug-in-kunit_free_suite_set/20230903-151137 base: linus/master patch link: https://lore.kernel.org/r/20230903071028.1518913-3-ruanjinjie%40huawei.com patch subject: [PATCH v2 2/4] kunit: Fix the wrong err path and add goto labels in kunit_filter_suites() config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20230903/202309031713.CDgtY4ZB-lkp@intel.com/config) compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230903/202309031713.CDgtY4ZB-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202309031713.CDgtY4ZB-lkp@intel.com/ All warnings (new ones prefixed by >>): >> lib/kunit/executor.c:221:1: warning: unused label 'free_parsed_glob' [-Wunused-label] free_parsed_glob: ^~~~~~~~~~~~~~~~~ >> lib/kunit/executor.c:227:1: warning: unused label 'free_copy' [-Wunused-label] free_copy: ^~~~~~~~~~ 2 warnings generated. vim +/free_parsed_glob +221 lib/kunit/executor.c 132 133 struct kunit_suite_set 134 kunit_filter_suites(const struct kunit_suite_set *suite_set, 135 const char *filter_glob, 136 char *filters, 137 char *filter_action, 138 int *err) 139 { 140 int i, j, k; 141 int filter_count = 0; 142 struct kunit_suite **copy, **copy_start, *filtered_suite, *new_filtered_suite; 143 struct kunit_suite_set filtered = {NULL, NULL}; 144 struct kunit_glob_filter parsed_glob; 145 struct kunit_attr_filter *parsed_filters = NULL; 146 147 const size_t max = suite_set->end - suite_set->start; 148 149 copy = kmalloc_array(max, sizeof(*filtered.start), GFP_KERNEL); 150 if (!copy) { /* won't be able to run anything, return an empty set */ 151 return filtered; 152 } 153 copy_start = copy; 154 155 if (filter_glob) 156 kunit_parse_glob_filter(&parsed_glob, filter_glob); 157 158 /* Parse attribute filters */ 159 if (filters) { 160 filter_count = kunit_get_filter_count(filters); 161 parsed_filters = kcalloc(filter_count, sizeof(*parsed_filters), GFP_KERNEL); 162 if (!parsed_filters) { 163 kfree(copy); 164 return filtered; 165 } 166 for (j = 0; j < filter_count; j++) 167 parsed_filters[j] = kunit_next_attr_filter(&filters, err); 168 if (*err) 169 goto free_parsed_filters; 170 } 171 172 for (i = 0; &suite_set->start[i] != suite_set->end; i++) { 173 filtered_suite = suite_set->start[i]; 174 if (filter_glob) { 175 if (!glob_match(parsed_glob.suite_glob, filtered_suite->name)) 176 continue; 177 filtered_suite = kunit_filter_glob_tests(filtered_suite, 178 parsed_glob.test_glob); 179 if (IS_ERR(filtered_suite)) { 180 *err = PTR_ERR(filtered_suite); 181 goto free_parsed_filters; 182 } 183 } 184 if (filter_count > 0 && parsed_filters != NULL) { 185 for (k = 0; k < filter_count; k++) { 186 new_filtered_suite = kunit_filter_attr_tests(filtered_suite, 187 parsed_filters[k], filter_action, err); 188 189 /* Free previous copy of suite */ 190 if (k > 0 || filter_glob) { 191 kfree(filtered_suite->test_cases); 192 kfree(filtered_suite); 193 } 194 195 filtered_suite = new_filtered_suite; 196 197 if (*err) 198 goto free_parsed_filters; 199 200 if (IS_ERR(filtered_suite)) { 201 *err = PTR_ERR(filtered_suite); 202 goto free_parsed_filters; 203 } 204 if (!filtered_suite) 205 break; 206 } 207 } 208 209 if (!filtered_suite) 210 continue; 211 212 *copy++ = filtered_suite; 213 } 214 filtered.start = copy_start; 215 filtered.end = copy; 216 217 free_parsed_filters: 218 if (filter_count) 219 kfree(parsed_filters); 220 > 221 free_parsed_glob: 222 if (filter_glob) { 223 kfree(parsed_glob.suite_glob); 224 kfree(parsed_glob.test_glob); 225 } 226 > 227 free_copy: 228 if (*err) 229 kfree(copy); 230 231 return filtered; 232 } 233
On Sun, 3 Sept 2023 at 15:11, 'Jinjie Ruan' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > Take the last kfree(parsed_filters) and add it to be the first. Take > the first kfree(copy) and add it to be the last. The Best practice is to > return these errors reversely. > > And as David suggested, add several labels which target only the things > which actually have been allocated so far. > > Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes") > Fixes: abbf73816b6f ("kunit: fix possible memory leak in kunit_filter_suites()") > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > Reviewed-by: Rae Moar <rmoar@google.com> > Suggested-by: David Gow <davidgow@google.com> > --- > v2: > - Add err path labels. > - Update the commit message and title. > --- This looks good to me. The kernel test robot does complain about unused labels, so it might make sense to wait to introduce the labels in the later patches (or merge patches 2,3,4 together), but personally I'm okay with it as-is, as these should be merged in one go. Reviewed-by: David Gow <davidgow@google.com> -- David > lib/kunit/executor.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c > index 5181aa2e760b..0eda42b0c9bb 100644 > --- a/lib/kunit/executor.c > +++ b/lib/kunit/executor.c > @@ -166,7 +166,7 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, > for (j = 0; j < filter_count; j++) > parsed_filters[j] = kunit_next_attr_filter(&filters, err); > if (*err) > - goto err; > + goto free_parsed_filters; > } > > for (i = 0; &suite_set->start[i] != suite_set->end; i++) { > @@ -178,7 +178,7 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, > parsed_glob.test_glob); > if (IS_ERR(filtered_suite)) { > *err = PTR_ERR(filtered_suite); > - goto err; > + goto free_parsed_filters; > } > } > if (filter_count > 0 && parsed_filters != NULL) { > @@ -195,10 +195,11 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, > filtered_suite = new_filtered_suite; > > if (*err) > - goto err; > + goto free_parsed_filters; > + > if (IS_ERR(filtered_suite)) { > *err = PTR_ERR(filtered_suite); > - goto err; > + goto free_parsed_filters; > } > if (!filtered_suite) > break; > @@ -213,17 +214,19 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, > filtered.start = copy_start; > filtered.end = copy; > > -err: > - if (*err) > - kfree(copy); > +free_parsed_filters: > + if (filter_count) > + kfree(parsed_filters); > > +free_parsed_glob: > if (filter_glob) { > kfree(parsed_glob.suite_glob); > kfree(parsed_glob.test_glob); > } > > - if (filter_count) > - kfree(parsed_filters); > +free_copy: > + if (*err) > + kfree(copy); > > return filtered; > } > -- > 2.34.1 > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230903071028.1518913-3-ruanjinjie%40huawei.com.
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c index 5181aa2e760b..0eda42b0c9bb 100644 --- a/lib/kunit/executor.c +++ b/lib/kunit/executor.c @@ -166,7 +166,7 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, for (j = 0; j < filter_count; j++) parsed_filters[j] = kunit_next_attr_filter(&filters, err); if (*err) - goto err; + goto free_parsed_filters; } for (i = 0; &suite_set->start[i] != suite_set->end; i++) { @@ -178,7 +178,7 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, parsed_glob.test_glob); if (IS_ERR(filtered_suite)) { *err = PTR_ERR(filtered_suite); - goto err; + goto free_parsed_filters; } } if (filter_count > 0 && parsed_filters != NULL) { @@ -195,10 +195,11 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, filtered_suite = new_filtered_suite; if (*err) - goto err; + goto free_parsed_filters; + if (IS_ERR(filtered_suite)) { *err = PTR_ERR(filtered_suite); - goto err; + goto free_parsed_filters; } if (!filtered_suite) break; @@ -213,17 +214,19 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, filtered.start = copy_start; filtered.end = copy; -err: - if (*err) - kfree(copy); +free_parsed_filters: + if (filter_count) + kfree(parsed_filters); +free_parsed_glob: if (filter_glob) { kfree(parsed_glob.suite_glob); kfree(parsed_glob.test_glob); } - if (filter_count) - kfree(parsed_filters); +free_copy: + if (*err) + kfree(copy); return filtered; }
Take the last kfree(parsed_filters) and add it to be the first. Take the first kfree(copy) and add it to be the last. The Best practice is to return these errors reversely. And as David suggested, add several labels which target only the things which actually have been allocated so far. Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes") Fixes: abbf73816b6f ("kunit: fix possible memory leak in kunit_filter_suites()") Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> Reviewed-by: Rae Moar <rmoar@google.com> Suggested-by: David Gow <davidgow@google.com> --- v2: - Add err path labels. - Update the commit message and title. --- lib/kunit/executor.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)