| 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: <3e32dbc2-c93f-45a1-a872-4e1798141a70@redhat.com>
Date: Mon, 30 Oct 2023 11:58:20 +0100
From: Marco Pagani <marpagan@...hat.com>
To: Maxime Ripard <mripard@...nel.org>
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 2023-10-25 10:43, Maxime Ripard wrote:
> Hi,
>
> 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.
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.
>
> shmem = to_drm_gem_shmem_obj(gem_obj);
> KUNIT_ASSERT_PTR_EQ(test, shmem->sgt, sgt);
>
> ret = kunit_add_action_or_reset(test, drm_gem_shmem_free_wrapper, shmem);
> KUNIT_ASSERT_EQ(test, ret, 0);
>
> The last one is arguable, but for the previous ones it makes error
> handling much more convenient and easy to reason about.
>
> The wrappers are also a bit inconvenient to use, but it's mostly there
> to avoid a compiler warning at the moment.
>
> This patch will help hopefully:
> https://lore.kernel.org/linux-kselftest/20230915050125.3609689-1-davidgow@google.com/
>
> Maxime
Thanks,
Marco
Powered by blists - more mailing lists