(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