| Message ID | 1631172276-82914-1-git-send-email-xiyuyang19@fudan.edu.cn (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | [v2] kunit: fix reference count leak in kfree_at_end | expand |
On Thu, Sep 9, 2021 at 12:26 AM 'Xiyu Yang' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > The reference counting issue happens in the normal path of > kfree_at_end(). When kunit_alloc_and_get_resource() is invoked, the > function forgets to handle the returned resource object, whose refcount > increased inside, causing a refcount leak. > > Fix this issue by calling kunit_alloc_resource() instead of > kunit_alloc_and_get_resource(). > > Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn> > Signed-off-by: Xin Tan <tanxin.ctf@gmail.com> Reviewed-by: Daniel Latypov <dlatypov@google.com> Ah, thanks for finding and fixing this! We really should have better documentation/otherwise make it clearer that people shouldn't use the "_and_get" version. I went and added some pr_info() calls to verify that these were being leaked before and they're fixed now. I copy-pasted this mistake into https://lore.kernel.org/linux-kselftest/20210831171926.3832806-2-dlatypov@google.com/. I'll send a v3 fix for that patch as well. > --- > lib/kunit/executor_test.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c > index cdbe54b16501..c2dcfb1f6e97 100644 > --- a/lib/kunit/executor_test.c > +++ b/lib/kunit/executor_test.c > @@ -116,7 +116,7 @@ static void kfree_at_end(struct kunit *test, const void *to_free) > /* kfree() handles NULL already, but avoid allocating a no-op cleanup. */ > if (IS_ERR_OR_NULL(to_free)) > return; > - kunit_alloc_and_get_resource(test, NULL, kfree_res_free, GFP_KERNEL, > + kunit_alloc_resource(test, NULL, kfree_res_free, GFP_KERNEL, > (void *)to_free); > } > > -- > 2.7.4 > > -- > 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/1631172276-82914-1-git-send-email-xiyuyang19%40fudan.edu.cn.
On Thu, Sep 9, 2021 at 9:59 AM Daniel Latypov <dlatypov@google.com> wrote: > > On Thu, Sep 9, 2021 at 12:26 AM 'Xiyu Yang' via KUnit Development > <kunit-dev@googlegroups.com> wrote: > > > > The reference counting issue happens in the normal path of > > kfree_at_end(). When kunit_alloc_and_get_resource() is invoked, the > > function forgets to handle the returned resource object, whose refcount > > increased inside, causing a refcount leak. > > > > Fix this issue by calling kunit_alloc_resource() instead of > > kunit_alloc_and_get_resource(). > > > > Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn> > > Signed-off-by: Xin Tan <tanxin.ctf@gmail.com> > > Reviewed-by: Daniel Latypov <dlatypov@google.com> > > Ah, thanks for finding and fixing this! > We really should have better documentation/otherwise make it clearer > that people shouldn't use the "_and_get" version. > > I went and added some pr_info() calls to verify that these were being > leaked before and they're fixed now. > > I copy-pasted this mistake into > https://lore.kernel.org/linux-kselftest/20210831171926.3832806-2-dlatypov@google.com/. > I'll send a v3 fix for that patch as well. Fixed that patch: https://lore.kernel.org/linux-kselftest/20210909171052.3192326-2-dlatypov@google.com/ I assume this patch and that one shouldn't have merge conflicts, so we don't need these to be applied in any specific order. > > > --- > > lib/kunit/executor_test.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c > > index cdbe54b16501..c2dcfb1f6e97 100644 > > --- a/lib/kunit/executor_test.c > > +++ b/lib/kunit/executor_test.c > > @@ -116,7 +116,7 @@ static void kfree_at_end(struct kunit *test, const void *to_free) > > /* kfree() handles NULL already, but avoid allocating a no-op cleanup. */ > > if (IS_ERR_OR_NULL(to_free)) > > return; > > - kunit_alloc_and_get_resource(test, NULL, kfree_res_free, GFP_KERNEL, > > + kunit_alloc_resource(test, NULL, kfree_res_free, GFP_KERNEL, > > (void *)to_free); > > } > > > > -- > > 2.7.4 > > > > -- > > 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/1631172276-82914-1-git-send-email-xiyuyang19%40fudan.edu.cn.
On Thu, Sep 9, 2021 at 12:26 AM Xiyu Yang <xiyuyang19@fudan.edu.cn> wrote: > > The reference counting issue happens in the normal path of > kfree_at_end(). When kunit_alloc_and_get_resource() is invoked, the > function forgets to handle the returned resource object, whose refcount > increased inside, causing a refcount leak. > > Fix this issue by calling kunit_alloc_resource() instead of > kunit_alloc_and_get_resource(). > > Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn> > Signed-off-by: Xin Tan <tanxin.ctf@gmail.com> Reviewed-by: Brendan Higgins <brendanhiggins@google.com> Thanks!
On 9/28/21 3:27 PM, Brendan Higgins wrote: > On Thu, Sep 9, 2021 at 12:26 AM Xiyu Yang <xiyuyang19@fudan.edu.cn> wrote: >> >> The reference counting issue happens in the normal path of >> kfree_at_end(). When kunit_alloc_and_get_resource() is invoked, the >> function forgets to handle the returned resource object, whose refcount >> increased inside, causing a refcount leak. >> >> Fix this issue by calling kunit_alloc_resource() instead of >> kunit_alloc_and_get_resource(). >> >> Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn> >> Signed-off-by: Xin Tan <tanxin.ctf@gmail.com> > > Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > > Thanks! > Thank you for the fix. Applied now after fixing a checkpatch alignment check to git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/ kunit-fixes branch. Please remember to run checkpatch.pl before sending patches. thanks, -- Shuah
diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c index cdbe54b16501..c2dcfb1f6e97 100644 --- a/lib/kunit/executor_test.c +++ b/lib/kunit/executor_test.c @@ -116,7 +116,7 @@ static void kfree_at_end(struct kunit *test, const void *to_free) /* kfree() handles NULL already, but avoid allocating a no-op cleanup. */ if (IS_ERR_OR_NULL(to_free)) return; - kunit_alloc_and_get_resource(test, NULL, kfree_res_free, GFP_KERNEL, + kunit_alloc_resource(test, NULL, kfree_res_free, GFP_KERNEL, (void *)to_free); }