| Message ID | 20220721180214.3223778-4-dlatypov@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e562e309d1d4ac05457c1454b6007071f13b5684 |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | [1/4] kunit: string-stream: Simplify resource use | expand |
(Nit: typo in the subject line "knuit_free" --> "kunit_free" On Fri, Jul 22, 2022 at 2:02 AM Daniel Latypov <dlatypov@google.com> wrote: > > kunit_kfree() can only work on data ("resources") allocated by KUnit. > > Currently for code like this, > > void *ptr = kmalloc(4, GFP_KERNEL); > > kunit_kfree(test, ptr); > kunit_kfree() will segfault. > > It'll try and look up the kunit_resource associated with `ptr` and get a > NULL back, but it won't check for this. This means we also segfault if > you double-free. Personally, I don't think the case of people calling kunit_kfree() on pointers allocated with kmalloc() is too worrying, but I do think we should error more gracefully in cases like double-frees (and maybe handle kfree(NULL) situations). > > Change kunit_kfree() so it'll notice these invalid pointers and respond > by failing the test. > > Implementation: kunit_destroy_resource() does what kunit_kfree() does, > but is more generic and returns -ENOENT when it can't find the resource. > Sadly, unlike just letting it crash, this means we don't get a stack > trace. But kunit_kfree() is so infrequently used it shouldn't be hard to > track down the bad callsite anyways. One day we should look into printing stacktraces on failed expectations... It could be spammy in some cases, but it'd be nice to have the option for things like this. > > After this change, the above code gives: > > # example_simple_test: EXPECTATION FAILED at lib/kunit/test.c:702 > > kunit_kfree: 00000000626ec200 already freed or not allocated by kunit > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > --- Looks good to me: this is both more correct and so much simpler as a function. I can live without the nitpicks fixed. Reviewed-by: David Gow <davidgow@google.com> Thanks! -- David > lib/kunit/test.c | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index 82019a78462e..c7ca87484968 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -698,18 +698,8 @@ static inline bool kunit_kfree_match(struct kunit *test, > > void kunit_kfree(struct kunit *test, const void *ptr) > { > - struct kunit_resource *res; > - > - res = kunit_find_resource(test, kunit_kfree_match, (void *)ptr); > - > - /* > - * Removing the resource from the list of resources drops the > - * reference count to 1; the final put will trigger the free. > - */ > - kunit_remove_resource(test, res); > - > - kunit_put_resource(res); > - > + if (kunit_destroy_resource(test, kunit_kfree_match, (void *)ptr)) > + KUNIT_FAIL(test, "kunit_kfree: %px already freed or not allocated by kunit", ptr); _Maybe_ we should no-op if ptr is NULL. I think it's legal for free()/kfree(), and while I don't see much use of it for kunit tests, maybe it'll save someone confusion down the road. But I could live with it either way... > } > EXPORT_SYMBOL_GPL(kunit_kfree); > > -- > 2.37.1.359.gd136c6c3e2-goog >
On Fri, Jul 22, 2022 at 12:35 AM David Gow <davidgow@google.com> wrote: > _Maybe_ we should no-op if ptr is NULL. I think it's legal for > free()/kfree(), and while I don't see much use of it for kunit tests, > maybe it'll save someone confusion down the road. > > But I could live with it either way... That's a good point. kfree(NULL) is indeed a no-op. I can see someone writing a parameterized test w/ some code like char *buffer = NULL; if (param->use_buffer) buffer = kunit_kzalloc(test, 10, GFP_KERNEL); ... kunit_kfree(test, buffer); and they'd have every reason to think this should just work. I think I'll tack this on as an extra patch and send a v2 w/ the commit subject for this one fixed.
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 82019a78462e..c7ca87484968 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -698,18 +698,8 @@ static inline bool kunit_kfree_match(struct kunit *test, void kunit_kfree(struct kunit *test, const void *ptr) { - struct kunit_resource *res; - - res = kunit_find_resource(test, kunit_kfree_match, (void *)ptr); - - /* - * Removing the resource from the list of resources drops the - * reference count to 1; the final put will trigger the free. - */ - kunit_remove_resource(test, res); - - kunit_put_resource(res); - + if (kunit_destroy_resource(test, kunit_kfree_match, (void *)ptr)) + KUNIT_FAIL(test, "kunit_kfree: %px already freed or not allocated by kunit", ptr); } EXPORT_SYMBOL_GPL(kunit_kfree);
kunit_kfree() can only work on data ("resources") allocated by KUnit. Currently for code like this, > void *ptr = kmalloc(4, GFP_KERNEL); > kunit_kfree(test, ptr); kunit_kfree() will segfault. It'll try and look up the kunit_resource associated with `ptr` and get a NULL back, but it won't check for this. This means we also segfault if you double-free. Change kunit_kfree() so it'll notice these invalid pointers and respond by failing the test. Implementation: kunit_destroy_resource() does what kunit_kfree() does, but is more generic and returns -ENOENT when it can't find the resource. Sadly, unlike just letting it crash, this means we don't get a stack trace. But kunit_kfree() is so infrequently used it shouldn't be hard to track down the bad callsite anyways. After this change, the above code gives: > # example_simple_test: EXPECTATION FAILED at lib/kunit/test.c:702 > kunit_kfree: 00000000626ec200 already freed or not allocated by kunit Signed-off-by: Daniel Latypov <dlatypov@google.com> --- lib/kunit/test.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)