Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1631172276-82914-1-git-send-email-xiyuyang19@fudan.edu.cn> <CAGS_qxp2e3pgZ6nGW+5=qneeVw_Pd_ex9kX4jZqA9bweoHXxFA@mail.gmail.com>
In-Reply-To: <CAGS_qxp2e3pgZ6nGW+5=qneeVw_Pd_ex9kX4jZqA9bweoHXxFA@mail.gmail.com>
From:   Daniel Latypov <dlatypov@google.com>
Date:   Thu, 9 Sep 2021 10:15:31 -0700
Message-ID: <CAGS_qxqFY3oaUC=m_5nDtozgBPgqU9zdiB3=7h5QPF0HBaLaqw@mail.gmail.com>
Subject: Re: [PATCH v2] kunit: fix reference count leak in kfree_at_end
To:     Xiyu Yang <xiyuyang19@fudan.edu.cn>
Cc:     Brendan Higgins <brendanhiggins@google.com>,
        linux-kselftest@vger.kernel.org, kunit-dev@googlegroups.com,
        linux-kernel@vger.kernel.org, yuanxzhang@fudan.edu.cn,
        Xin Tan <tanxin.ctf@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.