| Message ID | 20220319055600.3471875-1-davidgow@google.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: Rework kunit_resource allocation policy | expand |
On Sat, Mar 19, 2022 at 12:56 AM David Gow <davidgow@google.com> wrote: > > KUnit's test-managed resources can be created in two ways: > - Using the kunit_add_resource() family of functions, which accept a > struct kunit_resource pointer, typically allocated statically or on > the stack during the test. > - Using the kunit_alloc_resource() family of functions, which allocate a > struct kunit_resource using kzalloc() behind the scenes. > > Both of these families of functions accept a 'free' function to be > called when the resource is finally disposed of. > > At present, KUnit will kfree() the resource if this 'free' function is > specified, and will not if it is NULL. However, this can lead > kunit_alloc_resource() to leak memory (if no 'free' function is passed > in), or kunit_add_resource() to incorrectly kfree() memory which was > allocated by some other means (on the stack, as part of a larger > allocation, etc), if a 'free' function is provided. Trying it with this: static void noop_free_resource(struct kunit_resource *) {} struct kunit_resource global_res; static void example_simple_test(struct kunit *test) { kunit_add_resource(test, NULL, noop_free_resource, &global_res, test); } Running then with $ run_kunit --kunitconfig=lib/kunit --arch=x86_64 --build_dir=kunit_x86/ --kconfig_add=CONFIG_KASAN=y Before: BUG: KASAN: double-free or invalid-free in kunit_cleanup+0x51/0xb0 After: Passes > > Instead, always kfree() if the resource was allocated with > kunit_alloc_resource(), and never kfree() if it was passed into > kunit_add_resource() by the user. (If the user of kunit_add_resource() > wishes the resource be kfree()ed, they can call kfree() on the resource > from within the 'free' function. > > This is implemented by adding a 'should_free' member to nit: would `should_kfree` be a bit better? `should_free` almost sounds like "should we invoke res->free" (as nonsensical as that might be) > struct kunit_resource and setting it appropriately. To facilitate this, > the various resource add/alloc functions have been refactored somewhat, > making them all call a __kunit_add_resource() helper after setting the > 'should_free' member appropriately. In the process, all other functions > have been made static inline functions. > > Signed-off-by: David Gow <davidgow@google.com> Tested-by: Daniel Latypov <dlatypov@google.com> > --- > include/kunit/test.h | 135 +++++++++++++++++++++++++++++++++++-------- > lib/kunit/test.c | 65 +++------------------ > 2 files changed, 120 insertions(+), 80 deletions(-) > > diff --git a/include/kunit/test.h b/include/kunit/test.h > index 00b9ff7783ab..5a3aacbadda2 100644 > --- a/include/kunit/test.h > +++ b/include/kunit/test.h > @@ -36,11 +36,14 @@ typedef void (*kunit_resource_free_t)(struct kunit_resource *); > * struct kunit_resource - represents a *test managed resource* > * @data: for the user to store arbitrary data. > * @name: optional name > - * @free: a user supplied function to free the resource. Populated by > - * kunit_resource_alloc(). > + * @free: a user supplied function to free the resource. > * > * Represents a *test managed resource*, a resource which will automatically be > - * cleaned up at the end of a test case. > + * cleaned up at the end of a test case. This cleanup is performed by the 'free' > + * function. The resource itself is allocated with kmalloc() and freed with > + * kfree() if created with kunit_alloc_{,and_get_}resource(), otherwise it must > + * be freed by the user, typically with the 'free' function, or automatically if > + * it's allocated on the stack. I'm not a fan of this complexity, but I'm not sure if we have a way around it, esp. w/ stack-allocated data. Perhaps this would be a bit easier to read if we tweaked it a bit like: "freed with kfree() if allocated by KUnit (via kunit_alloc..." Maybe we can drop the "or automatically, if it's allocated on the stack" as well. A bigger way to simplify: perhaps we should get rid of kunit_alloc_and_get_resource() first? It's only used in KUnit's tests for itself. They could instead use kunit_alloc_resource() + kunit_find_resource(test, kunit_resource_instance_match, data). We could even define the helper with the same name in kunit-test.c (the only place it's used). Alternatively, we could make it an internal helper and define kunit_alloc_resource() as void *kunit_alloc_resource(...) { struct kunit_resource *res = _kunit_alloc_and_get_resource(...) if (res) return res->data; return NULL; } ?
On Tue, Mar 22, 2022 at 9:57 AM 'Daniel Latypov' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > On Sat, Mar 19, 2022 at 12:56 AM David Gow <davidgow@google.com> wrote: > > > > KUnit's test-managed resources can be created in two ways: > > - Using the kunit_add_resource() family of functions, which accept a > > struct kunit_resource pointer, typically allocated statically or on > > the stack during the test. > > - Using the kunit_alloc_resource() family of functions, which allocate a > > struct kunit_resource using kzalloc() behind the scenes. > > > > Both of these families of functions accept a 'free' function to be > > called when the resource is finally disposed of. > > > > At present, KUnit will kfree() the resource if this 'free' function is > > specified, and will not if it is NULL. However, this can lead > > kunit_alloc_resource() to leak memory (if no 'free' function is passed > > in), or kunit_add_resource() to incorrectly kfree() memory which was > > allocated by some other means (on the stack, as part of a larger > > allocation, etc), if a 'free' function is provided. > > Trying it with this: > > static void noop_free_resource(struct kunit_resource *) {} > > struct kunit_resource global_res; > > static void example_simple_test(struct kunit *test) > { > kunit_add_resource(test, NULL, noop_free_resource, &global_res, test); > } > > Running then with > $ run_kunit --kunitconfig=lib/kunit --arch=x86_64 > --build_dir=kunit_x86/ --kconfig_add=CONFIG_KASAN=y > > Before: > BUG: KASAN: double-free or invalid-free in kunit_cleanup+0x51/0xb0 > > After: > Passes > Phew! :-) I'm glad it works. > > > > Instead, always kfree() if the resource was allocated with > > kunit_alloc_resource(), and never kfree() if it was passed into > > kunit_add_resource() by the user. (If the user of kunit_add_resource() > > wishes the resource be kfree()ed, they can call kfree() on the resource > > from within the 'free' function. > > > > This is implemented by adding a 'should_free' member to > > nit: would `should_kfree` be a bit better? > `should_free` almost sounds like "should we invoke res->free" (as > nonsensical as that might be) > I think I had it as should_kfree at some point. I agree it's a little clearer. I'll rename it back. The other option I considered was to have a "flags" member, of which SHOULD_KFREE could be one. Though I eventually decided to leave that until we needed another flag. > > struct kunit_resource and setting it appropriately. To facilitate this, > > the various resource add/alloc functions have been refactored somewhat, > > making them all call a __kunit_add_resource() helper after setting the > > 'should_free' member appropriately. In the process, all other functions > > have been made static inline functions. > > > > Signed-off-by: David Gow <davidgow@google.com> > > Tested-by: Daniel Latypov <dlatypov@google.com> > > > > --- > > include/kunit/test.h | 135 +++++++++++++++++++++++++++++++++++-------- > > lib/kunit/test.c | 65 +++------------------ > > 2 files changed, 120 insertions(+), 80 deletions(-) > > > > diff --git a/include/kunit/test.h b/include/kunit/test.h > > index 00b9ff7783ab..5a3aacbadda2 100644 > > --- a/include/kunit/test.h > > +++ b/include/kunit/test.h > > @@ -36,11 +36,14 @@ typedef void (*kunit_resource_free_t)(struct kunit_resource *); > > * struct kunit_resource - represents a *test managed resource* > > * @data: for the user to store arbitrary data. > > * @name: optional name > > - * @free: a user supplied function to free the resource. Populated by > > - * kunit_resource_alloc(). > > + * @free: a user supplied function to free the resource. > > * > > * Represents a *test managed resource*, a resource which will automatically be > > - * cleaned up at the end of a test case. > > + * cleaned up at the end of a test case. This cleanup is performed by the 'free' > > + * function. The resource itself is allocated with kmalloc() and freed with > > + * kfree() if created with kunit_alloc_{,and_get_}resource(), otherwise it must > > + * be freed by the user, typically with the 'free' function, or automatically if > > + * it's allocated on the stack. > > I'm not a fan of this complexity, but I'm not sure if we have a way > around it, esp. w/ stack-allocated data. > The other option is to make all resources allocated with kunit_alloc_resource() require a non-NULL 'free' function which calls kfree() itself. This is much simpler on the KUnit side, but does put some of that burden on the user (and may prevent a free() function from being shared between allocated and non-allocated resources). > Perhaps this would be a bit easier to read if we tweaked it a bit like: > "freed with kfree() if allocated by KUnit (via kunit_alloc..." > > Maybe we can drop the "or automatically, if it's allocated on the > stack" as well. Yeah: I'm not 100% happy with that wording. I wanted to make it clear that there are cases where no automatic freeing is needed, but I agree it's really just making things more confusing. > > A bigger way to simplify: perhaps we should get rid of > kunit_alloc_and_get_resource() first? > It's only used in KUnit's tests for itself. > They could instead use kunit_alloc_resource() + > kunit_find_resource(test, kunit_resource_instance_match, data). > We could even define the helper with the same name in kunit-test.c > (the only place it's used). > > Alternatively, we could make it an internal helper and define > kunit_alloc_resource() as > > void *kunit_alloc_resource(...) > { > struct kunit_resource *res = _kunit_alloc_and_get_resource(...) > if (res) return res->data; > return NULL; > } > > ? > I was thinking about this a bit this morning, and I think we should do the opposite: get rid of kunit_alloc_resource() and leave only kunit_alloc_and_get_resource(). Then, split the resource system basically in two: - The system for managing "findable" resources, whose main purpose is for cases like the KASAN integration and the stub stuff where main goal is tying some named bit of data to a test, and reference counting it so it can safely be retrieved and used throughout the kernel if need be. - The simpler "free this on test exit" system, which could be as simple as a kunit_defer(func, context) function built on top of the former. This wouldn't need detailed tracking of reference counts, etc, (tl;dr: I think that kunit_alloc_resource() is broken, refcount-wise, if we're trying to implement the first kind of system, but useful for the second, and this is quite confusing. So kunit_alloc_resource() probably shouldn't be used alongside kunit_find_resource(), as there could be a potential race condition. Now, this shouldn't happen in practice, as most tests are single threaded and none are doing fancy things with kunit_remove_resource(), but kunit_alloc_and_get_resource() should be safer, as you're not playing with a resource you don't have a reference to according to the refcount.) That's a more complicated refactor and redesign of the resources system, though, so I'd rather fix this first. Cheers, -- David
On Mon, Mar 21, 2022 at 11:10 PM David Gow <davidgow@google.com> wrote: > > On Tue, Mar 22, 2022 at 9:57 AM 'Daniel Latypov' via KUnit Development > <kunit-dev@googlegroups.com> wrote: > > > > On Sat, Mar 19, 2022 at 12:56 AM David Gow <davidgow@google.com> wrote: > > > > > > KUnit's test-managed resources can be created in two ways: > > > - Using the kunit_add_resource() family of functions, which accept a > > > struct kunit_resource pointer, typically allocated statically or on > > > the stack during the test. > > > - Using the kunit_alloc_resource() family of functions, which allocate a > > > struct kunit_resource using kzalloc() behind the scenes. > > > > > > Both of these families of functions accept a 'free' function to be > > > called when the resource is finally disposed of. > > > > > > At present, KUnit will kfree() the resource if this 'free' function is > > > specified, and will not if it is NULL. However, this can lead > > > kunit_alloc_resource() to leak memory (if no 'free' function is passed > > > in), or kunit_add_resource() to incorrectly kfree() memory which was > > > allocated by some other means (on the stack, as part of a larger > > > allocation, etc), if a 'free' function is provided. > > > > Trying it with this: > > > > static void noop_free_resource(struct kunit_resource *) {} > > > > struct kunit_resource global_res; > > > > static void example_simple_test(struct kunit *test) > > { > > kunit_add_resource(test, NULL, noop_free_resource, &global_res, test); > > } > > > > Running then with > > $ run_kunit --kunitconfig=lib/kunit --arch=x86_64 > > --build_dir=kunit_x86/ --kconfig_add=CONFIG_KASAN=y > > > > Before: > > BUG: KASAN: double-free or invalid-free in kunit_cleanup+0x51/0xb0 > > > > After: > > Passes > > > > Phew! :-) > I'm glad it works. > > > > > > > Instead, always kfree() if the resource was allocated with > > > kunit_alloc_resource(), and never kfree() if it was passed into > > > kunit_add_resource() by the user. (If the user of kunit_add_resource() > > > wishes the resource be kfree()ed, they can call kfree() on the resource > > > from within the 'free' function. > > > > > > This is implemented by adding a 'should_free' member to > > > > nit: would `should_kfree` be a bit better? > > `should_free` almost sounds like "should we invoke res->free" (as > > nonsensical as that might be) > > > > I think I had it as should_kfree at some point. I agree it's a little > clearer. I'll rename it back. > > The other option I considered was to have a "flags" member, of which > SHOULD_KFREE could be one. Though I eventually decided to leave that > until we needed another flag. > > > > struct kunit_resource and setting it appropriately. To facilitate this, > > > the various resource add/alloc functions have been refactored somewhat, > > > making them all call a __kunit_add_resource() helper after setting the > > > 'should_free' member appropriately. In the process, all other functions > > > have been made static inline functions. > > > > > > Signed-off-by: David Gow <davidgow@google.com> > > > > Tested-by: Daniel Latypov <dlatypov@google.com> > > > > > > > --- > > > include/kunit/test.h | 135 +++++++++++++++++++++++++++++++++++-------- > > > lib/kunit/test.c | 65 +++------------------ > > > 2 files changed, 120 insertions(+), 80 deletions(-) > > > > > > diff --git a/include/kunit/test.h b/include/kunit/test.h > > > index 00b9ff7783ab..5a3aacbadda2 100644 > > > --- a/include/kunit/test.h > > > +++ b/include/kunit/test.h > > > @@ -36,11 +36,14 @@ typedef void (*kunit_resource_free_t)(struct kunit_resource *); > > > * struct kunit_resource - represents a *test managed resource* > > > * @data: for the user to store arbitrary data. > > > * @name: optional name > > > - * @free: a user supplied function to free the resource. Populated by > > > - * kunit_resource_alloc(). > > > + * @free: a user supplied function to free the resource. > > > * > > > * Represents a *test managed resource*, a resource which will automatically be > > > - * cleaned up at the end of a test case. > > > + * cleaned up at the end of a test case. This cleanup is performed by the 'free' > > > + * function. The resource itself is allocated with kmalloc() and freed with > > > + * kfree() if created with kunit_alloc_{,and_get_}resource(), otherwise it must > > > + * be freed by the user, typically with the 'free' function, or automatically if > > > + * it's allocated on the stack. > > > > I'm not a fan of this complexity, but I'm not sure if we have a way > > around it, esp. w/ stack-allocated data. > > > The other option is to make all resources allocated with > kunit_alloc_resource() require a non-NULL 'free' function which calls > kfree() itself. This is much simpler on the KUnit side, but does put > some of that burden on the user (and may prevent a free() function > from being shared between allocated and non-allocated resources). Overall, I'm ambivalent. To be honest, I'm not sure how real the user burden would be (it's basically 0 right now). This would only add about 6 more lines to add a kfree version: static void free_stack_resource(struct kunit_resource *res) { ... } static void free_heap_resource(struct kunit_resource *res) { free_stack_resource(res); kfree(res); } So far, this function is only ever used w/ non-NULL free functions (even in the under-review stubbing patches). So now would be the time to make such a change. But I'm slightly against such a change. It slightly complicates the "resources as storage" usecase in favor of simplifying the "resources as memory wranglers". Maybe it'd be fine if we added a helper they could use, e.g. void kunit_resource_default_free(struct kunit_resource *res) { kfree(res); } but it > > > Perhaps this would be a bit easier to read if we tweaked it a bit like: > > "freed with kfree() if allocated by KUnit (via kunit_alloc..." > > > > Maybe we can drop the "or automatically, if it's allocated on the > > stack" as well. > > Yeah: I'm not 100% happy with that wording. I wanted to make it clear > that there are cases where no automatic freeing is needed, but I agree > it's really just making things more confusing. > > > > A bigger way to simplify: perhaps we should get rid of > > kunit_alloc_and_get_resource() first? > > It's only used in KUnit's tests for itself. > > They could instead use kunit_alloc_resource() + > > kunit_find_resource(test, kunit_resource_instance_match, data). > > We could even define the helper with the same name in kunit-test.c > > (the only place it's used). > > > > Alternatively, we could make it an internal helper and define > > kunit_alloc_resource() as > > > > void *kunit_alloc_resource(...) > > { > > struct kunit_resource *res = _kunit_alloc_and_get_resource(...) > > if (res) return res->data; > > return NULL; > > } > > > > ? > > > > I was thinking about this a bit this morning, and I think we should do > the opposite: get rid of kunit_alloc_resource() and leave only > kunit_alloc_and_get_resource(). > Then, split the resource system basically in two: > - The system for managing "findable" resources, whose main purpose is > for cases like the KASAN integration and the stub stuff where main > goal is tying some named bit of data to a test, and reference counting > it so it can safely be retrieved and used throughout the kernel if > need be. > - The simpler "free this on test exit" system, which could be as > simple as a kunit_defer(func, context) function built on top of the > former. This wouldn't need detailed tracking of reference counts, etc, Agree that there's two distinct usecases here. One wants a replacement for global variables (which thus need "finding") and the other just wants to ensure some function like kfree() gets called. The latter ~never need to get "found" (e.g. kunit_kmalloc() users). The one exception: when people use kunit_kfree() to free things early, which requires us to "find" these resources we otherwise wouldn't care about. So I don't know how we can split the API unless we get rid of kunit_kfree(). Its presence means kunit_kmalloc() and friends need refcounting. Can we drop it? Maybe. Looking at the uses of kunit_kfree(), they're all internal to kunit except one. 111 static void ne_misc_dev_test_merge_phys_contig_memory_regions(struct kunit *test) 112 { ... 117 phys_contig_mem_regions.regions = kunit_kcalloc(test, MAX_PHYS_REGIONS, 118 sizeof(*phys_contig_mem_regions.regions), 119 GFP_KERNEL); ... 140 141 kunit_kfree(test, phys_contig_mem_regions.regions); 142 } Hmm, that looks redundant since it's right before the end of the test case. We can drop that call, I think. But I think kunit_kfree() can serve a purpose. E.g. for short-lived allocations where assertions are used. buf = kunit_kzalloc(test, sizeof(*buf), GFP_KERNEL); KUNIT_ASSERT_EQ(test, do_stuff(buf), 0); KUNIT_EXPECT_EQ(test, <something about buf>); kunit_kfree(buf); // do more stuff Sure we can drop kunit_kfree() and have `buf` stick around longer than needed. Or we could rewrite it like buf = kzalloc(sizeof(*buf), GFP_KERNEL); if (do_stuff(buf)) { KUNIT_FAIL(test, "do_stuff() failed"); } else { KUNIT_EXPECT_EQ(test, <something about buf>); } kfree(buf); but I think the kunit_kfree() code is cleaner. > > (tl;dr: I think that kunit_alloc_resource() is broken, refcount-wise, > if we're trying to implement the first kind of system, but useful for > the second, and this is quite confusing. So kunit_alloc_resource() > probably shouldn't be used alongside kunit_find_resource(), as there > could be a potential race condition. Now, this shouldn't happen in > practice, as most tests are single threaded and none are doing fancy > things with kunit_remove_resource(), but > kunit_alloc_and_get_resource() should be safer, as you're not playing > with a resource you don't have a reference to according to the > refcount.) > > That's a more complicated refactor and redesign of the resources > system, though, so I'd rather fix this first. > > Cheers, > -- David
On Tue, Mar 22, 2022 at 3:06 AM Daniel Latypov <dlatypov@google.com> wrote: > > On Mon, Mar 21, 2022 at 11:10 PM David Gow <davidgow@google.com> wrote: > > > > On Tue, Mar 22, 2022 at 9:57 AM 'Daniel Latypov' via KUnit Development > > <kunit-dev@googlegroups.com> wrote: > > > > > > On Sat, Mar 19, 2022 at 12:56 AM David Gow <davidgow@google.com> wrote: > > > > > > > > KUnit's test-managed resources can be created in two ways: > > > > - Using the kunit_add_resource() family of functions, which accept a > > > > struct kunit_resource pointer, typically allocated statically or on > > > > the stack during the test. > > > > - Using the kunit_alloc_resource() family of functions, which allocate a > > > > struct kunit_resource using kzalloc() behind the scenes. > > > > > > > > Both of these families of functions accept a 'free' function to be > > > > called when the resource is finally disposed of. > > > > > > > > At present, KUnit will kfree() the resource if this 'free' function is > > > > specified, and will not if it is NULL. However, this can lead > > > > kunit_alloc_resource() to leak memory (if no 'free' function is passed > > > > in), or kunit_add_resource() to incorrectly kfree() memory which was > > > > allocated by some other means (on the stack, as part of a larger > > > > allocation, etc), if a 'free' function is provided. > > > > > > Trying it with this: > > > > > > static void noop_free_resource(struct kunit_resource *) {} > > > > > > struct kunit_resource global_res; > > > > > > static void example_simple_test(struct kunit *test) > > > { > > > kunit_add_resource(test, NULL, noop_free_resource, &global_res, test); > > > } > > > > > > Running then with > > > $ run_kunit --kunitconfig=lib/kunit --arch=x86_64 > > > --build_dir=kunit_x86/ --kconfig_add=CONFIG_KASAN=y > > > > > > Before: > > > BUG: KASAN: double-free or invalid-free in kunit_cleanup+0x51/0xb0 > > > > > > After: > > > Passes > > > > > > > Phew! :-) > > I'm glad it works. > > > > > > > > > > Instead, always kfree() if the resource was allocated with > > > > kunit_alloc_resource(), and never kfree() if it was passed into > > > > kunit_add_resource() by the user. (If the user of kunit_add_resource() > > > > wishes the resource be kfree()ed, they can call kfree() on the resource > > > > from within the 'free' function. > > > > > > > > This is implemented by adding a 'should_free' member to > > > > > > nit: would `should_kfree` be a bit better? > > > `should_free` almost sounds like "should we invoke res->free" (as > > > nonsensical as that might be) > > > > > > > I think I had it as should_kfree at some point. I agree it's a little > > clearer. I'll rename it back. > > > > The other option I considered was to have a "flags" member, of which > > SHOULD_KFREE could be one. Though I eventually decided to leave that > > until we needed another flag. > > > > > > struct kunit_resource and setting it appropriately. To facilitate this, > > > > the various resource add/alloc functions have been refactored somewhat, > > > > making them all call a __kunit_add_resource() helper after setting the > > > > 'should_free' member appropriately. In the process, all other functions > > > > have been made static inline functions. > > > > > > > > Signed-off-by: David Gow <davidgow@google.com> > > > > > > Tested-by: Daniel Latypov <dlatypov@google.com> > > > > > > > > > > --- > > > > include/kunit/test.h | 135 +++++++++++++++++++++++++++++++++++-------- > > > > lib/kunit/test.c | 65 +++------------------ > > > > 2 files changed, 120 insertions(+), 80 deletions(-) > > > > > > > > diff --git a/include/kunit/test.h b/include/kunit/test.h > > > > index 00b9ff7783ab..5a3aacbadda2 100644 > > > > --- a/include/kunit/test.h > > > > +++ b/include/kunit/test.h > > > > @@ -36,11 +36,14 @@ typedef void (*kunit_resource_free_t)(struct kunit_resource *); > > > > * struct kunit_resource - represents a *test managed resource* > > > > * @data: for the user to store arbitrary data. > > > > * @name: optional name > > > > - * @free: a user supplied function to free the resource. Populated by > > > > - * kunit_resource_alloc(). > > > > + * @free: a user supplied function to free the resource. > > > > * > > > > * Represents a *test managed resource*, a resource which will automatically be > > > > - * cleaned up at the end of a test case. > > > > + * cleaned up at the end of a test case. This cleanup is performed by the 'free' > > > > + * function. The resource itself is allocated with kmalloc() and freed with > > > > + * kfree() if created with kunit_alloc_{,and_get_}resource(), otherwise it must > > > > + * be freed by the user, typically with the 'free' function, or automatically if > > > > + * it's allocated on the stack. > > > > > > I'm not a fan of this complexity, but I'm not sure if we have a way > > > around it, esp. w/ stack-allocated data. > > > > > The other option is to make all resources allocated with > > kunit_alloc_resource() require a non-NULL 'free' function which calls > > kfree() itself. This is much simpler on the KUnit side, but does put > > some of that burden on the user (and may prevent a free() function > > from being shared between allocated and non-allocated resources). > > Overall, I'm ambivalent. > > To be honest, I'm not sure how real the user burden would be (it's > basically 0 right now). > > This would only add about 6 more lines to add a kfree version: > static void free_stack_resource(struct kunit_resource *res) { ... } > > static void free_heap_resource(struct kunit_resource *res) > { > free_stack_resource(res); > kfree(res); > } > > So far, this function is only ever used w/ non-NULL free functions > (even in the under-review stubbing patches). > So now would be the time to make such a change. > > But I'm slightly against such a change. > It slightly complicates the "resources as storage" usecase in favor of > simplifying the "resources as memory wranglers". > Maybe it'd be fine if we added a helper they could use, e.g. > void kunit_resource_default_free(struct kunit_resource *res) { kfree(res); } > but it I agree. I am not a fan of requiring a non-null free function. I think the solution is better captured by splitting up the resource API, like you suggest elsewhere as a long term solution. In the short term, I like what you did here with the should_kfree. > > > Perhaps this would be a bit easier to read if we tweaked it a bit like: > > > "freed with kfree() if allocated by KUnit (via kunit_alloc..." > > > > > > Maybe we can drop the "or automatically, if it's allocated on the > > > stack" as well. > > > > Yeah: I'm not 100% happy with that wording. I wanted to make it clear > > that there are cases where no automatic freeing is needed, but I agree > > it's really just making things more confusing. > > > > > > A bigger way to simplify: perhaps we should get rid of > > > kunit_alloc_and_get_resource() first? > > > It's only used in KUnit's tests for itself. > > > They could instead use kunit_alloc_resource() + > > > kunit_find_resource(test, kunit_resource_instance_match, data). > > > We could even define the helper with the same name in kunit-test.c > > > (the only place it's used). > > > > > > Alternatively, we could make it an internal helper and define > > > kunit_alloc_resource() as > > > > > > void *kunit_alloc_resource(...) > > > { > > > struct kunit_resource *res = _kunit_alloc_and_get_resource(...) > > > if (res) return res->data; > > > return NULL; > > > } > > > > > > ? > > > > > > > I was thinking about this a bit this morning, and I think we should do > > the opposite: get rid of kunit_alloc_resource() and leave only > > kunit_alloc_and_get_resource(). > > Then, split the resource system basically in two: > > - The system for managing "findable" resources, whose main purpose is > > for cases like the KASAN integration and the stub stuff where main > > goal is tying some named bit of data to a test, and reference counting > > it so it can safely be retrieved and used throughout the kernel if > > need be. > > - The simpler "free this on test exit" system, which could be as > > simple as a kunit_defer(func, context) function built on top of the > > former. This wouldn't need detailed tracking of reference counts, etc, > > Agree that there's two distinct usecases here. > One wants a replacement for global variables (which thus need > "finding") and the other just wants to ensure some function like > kfree() gets called. Agreed. > The latter ~never need to get "found" (e.g. kunit_kmalloc() users). > The one exception: when people use kunit_kfree() to free things early, > which requires us to "find" these resources we otherwise wouldn't care > about. > > So I don't know how we can split the API unless we get rid of kunit_kfree(). > Its presence means kunit_kmalloc() and friends need refcounting. Do we need to choose between dropping kunit_kfree() and refcounting? I think this is semantically different from other findable resources, and I think it fairly obviously entails the complexity of using it. > Can we drop it? Maybe. > Looking at the uses of kunit_kfree(), they're all internal to kunit except one. > > 111 static void > ne_misc_dev_test_merge_phys_contig_memory_regions(struct kunit *test) > 112 { > ... > 117 phys_contig_mem_regions.regions = kunit_kcalloc(test, > MAX_PHYS_REGIONS, > 118 > sizeof(*phys_contig_mem_regions.regions), > 119 GFP_KERNEL); > ... > 140 > 141 kunit_kfree(test, phys_contig_mem_regions.regions); > 142 } > > Hmm, that looks redundant since it's right before the end of the test case. > We can drop that call, I think. > > But I think kunit_kfree() can serve a purpose. > E.g. for short-lived allocations where assertions are used. > buf = kunit_kzalloc(test, sizeof(*buf), GFP_KERNEL); > KUNIT_ASSERT_EQ(test, do_stuff(buf), 0); > KUNIT_EXPECT_EQ(test, <something about buf>); > kunit_kfree(buf); > // do more stuff > > Sure we can drop kunit_kfree() and have `buf` stick around longer than needed. > Or we could rewrite it like > buf = kzalloc(sizeof(*buf), GFP_KERNEL); > if (do_stuff(buf)) { > KUNIT_FAIL(test, "do_stuff() failed"); > } else { > KUNIT_EXPECT_EQ(test, <something about buf>); > } > kfree(buf); > but I think the kunit_kfree() code is cleaner. > > > > > (tl;dr: I think that kunit_alloc_resource() is broken, refcount-wise, > > if we're trying to implement the first kind of system, but useful for > > the second, and this is quite confusing. So kunit_alloc_resource() > > probably shouldn't be used alongside kunit_find_resource(), as there > > could be a potential race condition. Now, this shouldn't happen in > > practice, as most tests are single threaded and none are doing fancy > > things with kunit_remove_resource(), but > > kunit_alloc_and_get_resource() should be safer, as you're not playing > > with a resource you don't have a reference to according to the > > refcount.) > > > > That's a more complicated refactor and redesign of the resources > > system, though, so I'd rather fix this first. > > > > Cheers, > > -- David
On Wed, Mar 23, 2022 at 2:22 AM Brendan Higgins <brendanhiggins@google.com> wrote: > <snip> > > > The latter ~never need to get "found" (e.g. kunit_kmalloc() users). > > The one exception: when people use kunit_kfree() to free things early, > > which requires us to "find" these resources we otherwise wouldn't care > > about. > > > > So I don't know how we can split the API unless we get rid of kunit_kfree(). > > Its presence means kunit_kmalloc() and friends need refcounting. > > Do we need to choose between dropping kunit_kfree() and refcounting? I > think this is semantically different from other findable resources, > and I think it fairly obviously entails the complexity of using it. Yes, they're different. We could do something different and just have a atomic bool "is_freed" for the kunit_kmalloc() style resources. But is it worth it? Currently kunit_kfree() is defined as 697:void kunit_kfree(struct kunit *test, const void *ptr) 698-{ 699- struct kunit_resource *res; 700- 701- res = kunit_find_resource(test, kunit_resource_instance_match, 702- (void *)ptr); 703- 704- /* 705- * Removing the resource from the list of resources drops the 706- * reference count to 1; the final put will trigger the free. 707- */ 708- kunit_remove_resource(test, res); 709- 710- kunit_put_resource(res); 711- 712-} i.e. the overhead of using a refcount is that we need to call kunit_put_resource() bc we called kunit_find_resource(). IMO, this less semantic overhead than adding a different mechanism specifically for kunit_kfree(). Tangent: Huh, it segfaults if you call kunit_kfree() on a non-kunit allocated ptr. res == NULL on 701 in that case, but kunit_remove_resource() doesn't guard against that. It also happens if you call kunit_free() twice. That's analogous to how kfree() works, so I guess that's fine. A difference though is kfree(NULL); // is fine kunit_free(test, NULL); // segfaults, res == NULL above But thinking on it more, someone could register a resource w/ data == NULL. I.e. a named resource which just acts as a flag via presence/absence. kunit_kfree(test, NULL) would the most recent such resource though. Should we do the trick/hack where we check the free function first in kunit_kfree() to avoid such confusion?
diff --git a/include/kunit/test.h b/include/kunit/test.h index 00b9ff7783ab..5a3aacbadda2 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -36,11 +36,14 @@ typedef void (*kunit_resource_free_t)(struct kunit_resource *); * struct kunit_resource - represents a *test managed resource* * @data: for the user to store arbitrary data. * @name: optional name - * @free: a user supplied function to free the resource. Populated by - * kunit_resource_alloc(). + * @free: a user supplied function to free the resource. * * Represents a *test managed resource*, a resource which will automatically be - * cleaned up at the end of a test case. + * cleaned up at the end of a test case. This cleanup is performed by the 'free' + * function. The resource itself is allocated with kmalloc() and freed with + * kfree() if created with kunit_alloc_{,and_get_}resource(), otherwise it must + * be freed by the user, typically with the 'free' function, or automatically if + * it's allocated on the stack. * * Resources are reference counted so if a resource is retrieved via * kunit_alloc_and_get_resource() or kunit_find_resource(), we need @@ -97,6 +100,7 @@ struct kunit_resource { /* private: internal use only. */ struct kref refcount; struct list_head node; + bool should_free; }; struct kunit; @@ -385,16 +389,6 @@ static inline int kunit_run_all_tests(void) enum kunit_status kunit_suite_has_succeeded(struct kunit_suite *suite); -/* - * Like kunit_alloc_resource() below, but returns the struct kunit_resource - * object that contains the allocation. This is mostly for testing purposes. - */ -struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test, - kunit_resource_init_t init, - kunit_resource_free_t free, - gfp_t internal_gfp, - void *context); - /** * kunit_get_resource() - Hold resource for use. Should not need to be used * by most users as we automatically get resources @@ -416,10 +410,14 @@ static inline void kunit_release_resource(struct kref *kref) refcount); /* If free function is defined, resource was dynamically allocated. */ - if (res->free) { + if (res->free) res->free(res); + + /* 'res' is valid here, as if should_free is set, res->free may not free + * 'res' itself, just res->data + */ + if (res->should_free) kfree(res); - } } /** @@ -440,7 +438,9 @@ static inline void kunit_put_resource(struct kunit_resource *res) } /** - * kunit_add_resource() - Add a *test managed resource*. + * __kunit_add_resource() - Internal helper to add a resource. + * + * res->should_free is not initialised. * @test: The test context object. * @init: a user-supplied function to initialize the result (if needed). If * none is supplied, the resource data value is simply set to @data. @@ -449,12 +449,34 @@ static inline void kunit_put_resource(struct kunit_resource *res) * @res: The resource. * @data: value to pass to init function or set in resource data field. */ -int kunit_add_resource(struct kunit *test, +int __kunit_add_resource(struct kunit *test, kunit_resource_init_t init, kunit_resource_free_t free, struct kunit_resource *res, void *data); +/** + * kunit_add_resource() - Add a *test managed resource*. + * @test: The test context object. + * @init: a user-supplied function to initialize the result (if needed). If + * none is supplied, the resource data value is simply set to @data. + * If an init function is supplied, @data is passed to it instead. + * @free: a user-supplied function to free the resource (if needed). + * @res: The resource. + * @data: value to pass to init function or set in resource data field. + */ +static inline int kunit_add_resource(struct kunit *test, + kunit_resource_init_t init, + kunit_resource_free_t free, + struct kunit_resource *res, + void *data) +{ + res->should_free = false; + return __kunit_add_resource(test, init, free, res, data); +} + +static inline struct kunit_resource * +kunit_find_named_resource(struct kunit *test, const char *name); /** * kunit_add_named_resource() - Add a named *test managed resource*. * @test: The test context object. @@ -464,18 +486,84 @@ int kunit_add_resource(struct kunit *test, * @name: name to be set for resource. * @data: value to pass to init function or set in resource data field. */ -int kunit_add_named_resource(struct kunit *test, +static inline int kunit_add_named_resource(struct kunit *test, + kunit_resource_init_t init, + kunit_resource_free_t free, + struct kunit_resource *res, + const char *name, + void *data) +{ + struct kunit_resource *existing; + + if (!name) + return -EINVAL; + + existing = kunit_find_named_resource(test, name); + if (existing) { + kunit_put_resource(existing); + return -EEXIST; + } + + res->name = name; + res->should_free = false; + + return __kunit_add_resource(test, init, free, res, data); +} + +/** + * kunit_alloc_and_get_resource() - Allocates and returns a *test managed resource*. + * @test: The test context object. + * @init: a user supplied function to initialize the resource. + * @free: a user supplied function to free the resource (if needed). + * @internal_gfp: gfp to use for internal allocations, if unsure, use GFP_KERNEL + * @context: for the user to pass in arbitrary data to the init function. + * + * Allocates a *test managed resource*, a resource which will automatically be + * cleaned up at the end of a test case. See &struct kunit_resource for an + * example. + * + * This is effectively identical to kunit_alloc_resource, but returns the + * struct kunit_resource pointer, not just the 'data' pointer. It therefore + * also increments the resource's refcount, so kunit_put_resource() should be + * called when you've finished with it. + * + * Note: KUnit needs to allocate memory for a kunit_resource object. You must + * specify an @internal_gfp that is compatible with the use context of your + * resource. + */ +static inline struct kunit_resource * +kunit_alloc_and_get_resource(struct kunit *test, kunit_resource_init_t init, kunit_resource_free_t free, - struct kunit_resource *res, - const char *name, - void *data); + gfp_t internal_gfp, + void *context) +{ + struct kunit_resource *res; + int ret; + + res = kzalloc(sizeof(*res), internal_gfp); + if (!res) + return NULL; + + res->should_free = true; + + ret = __kunit_add_resource(test, init, free, res, context); + if (!ret) { + /* + * bump refcount for get; kunit_resource_put() should be called + * when done. + */ + kunit_get_resource(res); + return res; + } + return NULL; +} /** * kunit_alloc_resource() - Allocates a *test managed resource*. * @test: The test context object. * @init: a user supplied function to initialize the resource. - * @free: a user supplied function to free the resource. + * @free: a user supplied function to free the resource (if needed). * @internal_gfp: gfp to use for internal allocations, if unsure, use GFP_KERNEL * @context: for the user to pass in arbitrary data to the init function. * @@ -499,7 +587,8 @@ static inline void *kunit_alloc_resource(struct kunit *test, if (!res) return NULL; - if (!kunit_add_resource(test, init, free, res, context)) + res->should_free = true; + if (!__kunit_add_resource(test, init, free, res, context)) return res->data; return NULL; diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 3bca3bf5c15b..b4f10329abb8 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -596,18 +596,19 @@ EXPORT_SYMBOL_GPL(__kunit_test_suites_exit); * Used for static resources and when a kunit_resource * has been created by * kunit_alloc_resource(). When an init function is supplied, @data is passed * into the init function; otherwise, we simply set the resource data field to - * the data value passed in. + * the data value passed in. Doesn't initialize res->should_free. */ -int kunit_add_resource(struct kunit *test, - kunit_resource_init_t init, - kunit_resource_free_t free, - struct kunit_resource *res, - void *data) +int __kunit_add_resource(struct kunit *test, + kunit_resource_init_t init, + kunit_resource_free_t free, + struct kunit_resource *res, + void *data) { int ret = 0; unsigned long flags; res->free = free; + res->should_free = false; kref_init(&res->refcount); if (init) { @@ -625,57 +626,7 @@ int kunit_add_resource(struct kunit *test, return ret; } -EXPORT_SYMBOL_GPL(kunit_add_resource); - -int kunit_add_named_resource(struct kunit *test, - kunit_resource_init_t init, - kunit_resource_free_t free, - struct kunit_resource *res, - const char *name, - void *data) -{ - struct kunit_resource *existing; - - if (!name) - return -EINVAL; - - existing = kunit_find_named_resource(test, name); - if (existing) { - kunit_put_resource(existing); - return -EEXIST; - } - - res->name = name; - - return kunit_add_resource(test, init, free, res, data); -} -EXPORT_SYMBOL_GPL(kunit_add_named_resource); - -struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test, - kunit_resource_init_t init, - kunit_resource_free_t free, - gfp_t internal_gfp, - void *data) -{ - struct kunit_resource *res; - int ret; - - res = kzalloc(sizeof(*res), internal_gfp); - if (!res) - return NULL; - - ret = kunit_add_resource(test, init, free, res, data); - if (!ret) { - /* - * bump refcount for get; kunit_resource_put() should be called - * when done. - */ - kunit_get_resource(res); - return res; - } - return NULL; -} -EXPORT_SYMBOL_GPL(kunit_alloc_and_get_resource); +EXPORT_SYMBOL_GPL(__kunit_add_resource); void kunit_remove_resource(struct kunit *test, struct kunit_resource *res) {
KUnit's test-managed resources can be created in two ways: - Using the kunit_add_resource() family of functions, which accept a struct kunit_resource pointer, typically allocated statically or on the stack during the test. - Using the kunit_alloc_resource() family of functions, which allocate a struct kunit_resource using kzalloc() behind the scenes. Both of these families of functions accept a 'free' function to be called when the resource is finally disposed of. At present, KUnit will kfree() the resource if this 'free' function is specified, and will not if it is NULL. However, this can lead kunit_alloc_resource() to leak memory (if no 'free' function is passed in), or kunit_add_resource() to incorrectly kfree() memory which was allocated by some other means (on the stack, as part of a larger allocation, etc), if a 'free' function is provided. Instead, always kfree() if the resource was allocated with kunit_alloc_resource(), and never kfree() if it was passed into kunit_add_resource() by the user. (If the user of kunit_add_resource() wishes the resource be kfree()ed, they can call kfree() on the resource from within the 'free' function. This is implemented by adding a 'should_free' member to struct kunit_resource and setting it appropriately. To facilitate this, the various resource add/alloc functions have been refactored somewhat, making them all call a __kunit_add_resource() helper after setting the 'should_free' member appropriately. In the process, all other functions have been made static inline functions. Signed-off-by: David Gow <davidgow@google.com> --- include/kunit/test.h | 135 +++++++++++++++++++++++++++++++++++-------- lib/kunit/test.c | 65 +++------------------ 2 files changed, 120 insertions(+), 80 deletions(-)