| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ju7huixzwdjoz2jvnhdwc3mxrrbm7uvj73wezbpedcossijgi6@3la34kfiaval>
Date: Mon, 6 Nov 2023 14:46:40 +0100
From: Maxime Ripard <mripard@...nel.org>
To: Marco Pagani <marpagan@...hat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>,
Sumit Semwal <sumit.semwal@...aro.org>,
Christian Koenig <christian.koenig@....com>,
linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
linux-media@...r.kernel.org, linaro-mm-sig@...ts.linaro.org
Subject: Re: [RFC PATCH] drm/test: add a test suite for GEM objects backed by
shmem
On Mon, Oct 30, 2023 at 11:58:20AM +0100, Marco Pagani wrote:
> On 2023-10-25 10:43, Maxime Ripard wrote:
> > On Tue, Oct 24, 2023 at 07:14:25PM +0200, Marco Pagani wrote:
> >>>> +static void drm_gem_shmem_test_obj_create_private(struct kunit *test)
> >>>> +{
> >>>> + struct fake_dev *fdev = test->priv;
> >>>> + struct drm_gem_shmem_object *shmem;
> >>>> + struct drm_gem_object *gem_obj;
> >>>> + struct dma_buf buf_mock;
> >>>> + struct dma_buf_attachment attach_mock;
> >>>> + struct sg_table *sgt;
> >>>> + char *buf;
> >>>> + int ret;
> >>>> +
> >>>> + /* Create a mock scatter/gather table */
> >>>> + buf = kunit_kzalloc(test, TEST_SIZE, GFP_KERNEL);
> >>>> + KUNIT_ASSERT_NOT_NULL(test, buf);
> >>>> +
> >>>> + sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
> >>>> + KUNIT_ASSERT_NOT_NULL(test, sgt);
> >>>> +
> >>>> + ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
> >>>> + KUNIT_ASSERT_EQ(test, ret, 0);
> >>>> + sg_init_one(sgt->sgl, buf, TEST_SIZE);
> >>>> +
> >>>> + /* Init a mock DMA-BUF */
> >>>> + buf_mock.size = TEST_SIZE;
> >>>> + attach_mock.dmabuf = &buf_mock;
> >>>> +
> >>>> + gem_obj = drm_gem_shmem_prime_import_sg_table(&fdev->drm_dev, &attach_mock, sgt);
> >>>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gem_obj);
> >>>> + KUNIT_ASSERT_EQ(test, gem_obj->size, TEST_SIZE);
> >>>> + KUNIT_ASSERT_NULL(test, gem_obj->filp);
> >>>> + KUNIT_ASSERT_NOT_NULL(test, gem_obj->funcs);
> >>>> +
> >>>> + shmem = to_drm_gem_shmem_obj(gem_obj);
> >>>> + KUNIT_ASSERT_PTR_EQ(test, shmem->sgt, sgt);
> >>>> +
> >>>> + /* The scatter/gather table is freed by drm_gem_shmem_free */
> >>>> + drm_gem_shmem_free(shmem);
> >>>> +}
> >>>
> >>> KUNIT_ASSERT_* will stop the execution of the test on failure, you
> >>> should probably use a bit more of KUNIT_EXPECT_* calls otherwise you'll
> >>> leak resources.
> >>>
> >>> You also probably want to use a kunit_action to clean up and avoid that
> >>> whole discussion
> >>>
> >>
> >> You are right. I slightly prefer using KUnit expectations (unless actions
> >> are strictly necessary) since I feel using actions makes test cases a bit
> >> less straightforward to understand. Is this okay for you?
> >
> > I disagree. Actions make it easier to reason about, even when comparing
> > assertion vs expectation
> >
> > Like, for the call to sg_alloc_table and
> > drm_gem_shmem_prime_import_sg_table(), the reasonable use of assert vs
> > expect would be something like:
> >
> > sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
> > KUNIT_ASSERT_NOT_NULL(test, sgt);
> >
> > ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
> > KUNIT_ASSERT_EQ(test, ret, 0);
> >
> > /*
> > * Here, it's already not super clear whether you want to expect vs
> > * assert. expect will make you handle the failure case later, assert will
> > * force you to call kfree on sgt. Both kind of suck in their own ways.
> > */
> >
> > sg_init_one(sgt->sgl, buf, TEST_SIZE);
> >
> > gem_obj = drm_gem_shmem_prime_import_sg_table(&fdev->drm_dev, &attach_mock, sgt);
> > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gem_obj);
> >
> > /*
> > * If the assert fails, we forgot to call sg_free_table(sgt) and kfree(sgt).
> > */
> >
> > KUNIT_EXPECT_EQ(test, gem_obj->size, TEST_SIZE);
> > KUNIT_EXPECT_NULL(test, gem_obj->filp);
> > KUNIT_EXPECT_NOT_NULL(test, gem_obj->funcs);
> >
> > /*
> > * And here we have to handle the case where the expectation was wrong,
> > * but the test still continued.
> > */
> >
> > But if you're not using an action, you still have to call kfree(sgt),
> > which means that you might still
> >
> > shmem = to_drm_gem_shmem_obj(gem_obj);
> > KUNIT_ASSERT_PTR_EQ(test, shmem->sgt, sgt);
> >
> > /*
> > * If the assertion fails, we now have to call drm_gem_shmem_free(shmem)
> > */
> >
> > /* The scatter/gather table is freed by drm_gem_shmem_free */
> > drm_gem_shmem_free(shmem);
> >
> > /* everything's fine now */
> >
> > The semantics around drm_gem_shmem_free make it a bit convoluted, but
> > doing it using goto/labels, plus handling the assertions and error
> > reporting would be difficult.
> >
> > Using actions, we have:
> >
> > sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
> > KUNIT_ASSERT_NOT_NULL(test, sgt);
> >
> > ret = kunit_add_action_or_reset(test, kfree_wrapper, sgt);
> > KUNIT_ASSERT_EQ(test, ret, 0);
> >
> > ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
> > KUNIT_ASSERT_EQ(test, ret, 0);
> >
> > ret = kunit_add_action_or_reset(test, sg_free_table_wrapper, sgt);
> > KUNIT_ASSERT_EQ(test, ret, 0);
> >
> > sg_init_one(sgt->sgl, buf, TEST_SIZE);
> >
> > gem_obj = drm_gem_shmem_prime_import_sg_table(&fdev->drm_dev, &attach_mock, sgt);
> > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gem_obj);
> > KUNIT_EXPECT_EQ(test, gem_obj->size, TEST_SIZE);
> > KUNIT_EXPECT_NULL(test, gem_obj->filp);
> > KUNIT_EXPECT_NOT_NULL(test, gem_obj->funcs);
> >
> > /* drm_gem_shmem_free will free the struct sg_table itself */
> > kunit_remove_action(test, sg_free_table_wrapper, sgt);
> > kunit_remove_action(test, kfree_wrapper, sgt);
>
> I agree that using actions makes error handling cleaner. However, I still
> have some concerns about the additional complexity that actions introduce.
> For instance, I feel these two lines make the testing harness more complex
> without asserting any additional property of the component under test.
If anything, the API makes it more difficult to deal with. It would
actually be harder/messier to handle without an action.
> In some sense, I wonder if it is worth worrying about memory leaks when
> a test case fails. At that point, the system is already in an inconsistent
> state due to a bug in the component under test, so it is unsafe to continue
> anyway.
I guess the larger issue is: once that code will be merged, we're going
to have patches to convert to actions because they make it nicer and fix
a couple of issues anyway.
So, if it's still the state we're going to end up in, why not doing it
right from the beginning?
Maxime
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists