| 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: <dbx84ivb6asr.fsf@ynaffit-andsys.c.googlers.com>
Date: Wed, 16 Jul 2025 15:16:52 -0700
From: Tiffany Yang <ynaffit@...gle.com>
To: Carlos Llamas <cmllamas@...gle.com>
Cc: linux-kernel@...r.kernel.org, keescook@...gle.com, kernel-team@...roid.com,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Arve Hjønnevåg" <arve@...roid.com>, Todd Kjos <tkjos@...roid.com>, Martijn Coenen <maco@...roid.com>,
Joel Fernandes <joelagnelf@...dia.com>, Christian Brauner <brauner@...nel.org>,
Suren Baghdasaryan <surenb@...gle.com>, Brendan Higgins <brendan.higgins@...ux.dev>,
David Gow <davidgow@...gle.com>, Rae Moar <rmoar@...gle.com>, linux-kselftest@...r.kernel.org,
kunit-dev@...glegroups.com
Subject: Re: [PATCH v3 4/6] binder: Scaffolding for binder_alloc KUnit tests
Carlos Llamas <cmllamas@...gle.com> writes:
> On Mon, Jul 14, 2025 at 11:53:17AM -0700, Tiffany Yang wrote:
>> Add setup and teardown for testing binder allocator code with KUnit.
>> Include minimal test cases to verify that tests are initialized
>> correctly.
>> Tested-by: Rae Moar <rmoar@...gle.com>
>> Signed-off-by: Tiffany Yang <ynaffit@...gle.com>
>> ---
>> v2:
>> * Added tested-by tag
>> v3:
>> * Split kunit lib change into separate change
>> ---
>> drivers/android/Kconfig | 11 ++
>> drivers/android/Makefile | 1 +
>> drivers/android/binder.c | 5 +-
>> drivers/android/binder_alloc.c | 15 +-
>> drivers/android/binder_alloc.h | 6 +
>> drivers/android/binder_internal.h | 4 +
>> drivers/android/tests/.kunitconfig | 3 +
>> drivers/android/tests/Makefile | 3 +
>> drivers/android/tests/binder_alloc_kunit.c | 166 +++++++++++++++++++++
>> 9 files changed, 208 insertions(+), 6 deletions(-)
>> create mode 100644 drivers/android/tests/.kunitconfig
>> create mode 100644 drivers/android/tests/Makefile
>> create mode 100644 drivers/android/tests/binder_alloc_kunit.c
>> diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
>> index 07aa8ae0a058..b1bc7183366c 100644
>> --- a/drivers/android/Kconfig
>> +++ b/drivers/android/Kconfig
>> @@ -47,4 +47,15 @@ config ANDROID_BINDER_IPC_SELFTEST
>> exhaustively with combinations of various buffer sizes and
>> alignments.
>> +config ANDROID_BINDER_ALLOC_KUNIT_TEST
> I think is OK to have "alloc" in the name. However, this is probably
> going to grow into just "binder" at some point (hopefully). Then we can
> just rename the kunit config as such.
My original thinking was that it would be nice to have multiple configs
for various binder tests (i.e., if we wanted to have binderfs unit tests
and other binder driver unit tests), but upon reflection, I'm not sure
what purpose that would serve... I'll leave the "alloc" in it for now,
but I agree we'll probably remove it for the future binder kunit tests.
>> + tristate "KUnit Tests for Android Binder Alloc" if !KUNIT_ALL_TESTS
>> + depends on ANDROID_BINDER_IPC && KUNIT
>> + default KUNIT_ALL_TESTS
>> + help
>> + This feature builds the binder alloc KUnit tests.
>> +
>> + Each test case runs using a pared-down binder_alloc struct and
>> + test-specific freelist, which allows this KUnit module to be
>> loaded
>> + for testing without interfering with a running system.
>> +
>> endmenu
>> diff --git a/drivers/android/Makefile b/drivers/android/Makefile
>> index c9d3d0c99c25..74d02a335d4e 100644
>> --- a/drivers/android/Makefile
>> +++ b/drivers/android/Makefile
>> @@ -4,3 +4,4 @@ ccflags-y += -I$(src) # needed for trace
>> events
>> obj-$(CONFIG_ANDROID_BINDERFS) += binderfs.o
>> obj-$(CONFIG_ANDROID_BINDER_IPC) += binder.o binder_alloc.o
>> obj-$(CONFIG_ANDROID_BINDER_IPC_SELFTEST) += binder_alloc_selftest.o
>> +obj-$(CONFIG_ANDROID_BINDER_ALLOC_KUNIT_TEST) += tests/
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index c463ca4a8fff..9dfe90c284fc 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -68,6 +68,8 @@
>> #include <linux/sizes.h>
>> #include <linux/ktime.h>
>> +#include <kunit/visibility.h>
>> +
>> #include <uapi/linux/android/binder.h>
>> #include <linux/cacheflush.h>
>> @@ -5956,10 +5958,11 @@ static void binder_vma_close(struct
>> vm_area_struct *vma)
>> binder_alloc_vma_close(&proc->alloc);
>> }
>> -static vm_fault_t binder_vm_fault(struct vm_fault *vmf)
>> +VISIBLE_IF_KUNIT vm_fault_t binder_vm_fault(struct vm_fault *vmf)
>> {
>> return VM_FAULT_SIGBUS;
>> }
>> +EXPORT_SYMBOL_IF_KUNIT(binder_vm_fault);
>> static const struct vm_operations_struct binder_vm_ops = {
>> .open = binder_vma_open,
>> diff --git a/drivers/android/binder_alloc.c
>> b/drivers/android/binder_alloc.c
>> index 2e89f9127883..c79e5c6721f0 100644
>> --- a/drivers/android/binder_alloc.c
>> +++ b/drivers/android/binder_alloc.c
>> @@ -23,6 +23,7 @@
>> #include <linux/uaccess.h>
>> #include <linux/highmem.h>
>> #include <linux/sizes.h>
>> +#include <kunit/visibility.h>
>> #include "binder_alloc.h"
>> #include "binder_trace.h"
>> @@ -57,13 +58,14 @@ static struct binder_buffer
>> *binder_buffer_prev(struct binder_buffer *buffer)
>> return list_entry(buffer->entry.prev, struct binder_buffer, entry);
>> }
>> -static size_t binder_alloc_buffer_size(struct binder_alloc *alloc,
>> - struct binder_buffer *buffer)
>> +VISIBLE_IF_KUNIT size_t binder_alloc_buffer_size(struct binder_alloc
>> *alloc,
>> + struct binder_buffer
>> *buffer)
>> {
>> if (list_is_last(&buffer->entry, &alloc->buffers))
>> return alloc->vm_start + alloc->buffer_size -
>> buffer->user_data;
>> return binder_buffer_next(buffer)->user_data - buffer->user_data;
>> }
>> +EXPORT_SYMBOL_IF_KUNIT(binder_alloc_buffer_size);
>> static void binder_insert_free_buffer(struct binder_alloc *alloc,
>> struct binder_buffer *new_buffer)
>> @@ -959,7 +961,7 @@ int binder_alloc_mmap_handler(struct binder_alloc
>> *alloc,
>> failure_string, ret);
>> return ret;
>> }
>> -
>> +EXPORT_SYMBOL_IF_KUNIT(binder_alloc_mmap_handler);
>> void binder_alloc_deferred_release(struct binder_alloc *alloc)
>> {
>> @@ -1028,6 +1030,7 @@ void binder_alloc_deferred_release(struct
>> binder_alloc *alloc)
>> "%s: %d buffers %d, pages %d\n",
>> __func__, alloc->pid, buffers, page_count);
>> }
>> +EXPORT_SYMBOL_IF_KUNIT(binder_alloc_deferred_release);
>> /**
>> * binder_alloc_print_allocated() - print buffer info
>> @@ -1122,6 +1125,7 @@ void binder_alloc_vma_close(struct binder_alloc
>> *alloc)
>> {
>> binder_alloc_set_mapped(alloc, false);
>> }
>> +EXPORT_SYMBOL_IF_KUNIT(binder_alloc_vma_close);
>> /**
>> * binder_alloc_free_page() - shrinker callback to free pages
>> @@ -1229,8 +1233,8 @@ binder_shrink_scan(struct shrinker *shrink, struct
>> shrink_control *sc)
>> static struct shrinker *binder_shrinker;
>> -static void __binder_alloc_init(struct binder_alloc *alloc,
>> - struct list_lru *freelist)
>> +VISIBLE_IF_KUNIT void __binder_alloc_init(struct binder_alloc *alloc,
>> + struct list_lru *freelist)
>> {
>> alloc->pid = current->group_leader->pid;
>> alloc->mm = current->mm;
>> @@ -1239,6 +1243,7 @@ static void __binder_alloc_init(struct
>> binder_alloc *alloc,
>> INIT_LIST_HEAD(&alloc->buffers);
>> alloc->freelist = freelist;
>> }
>> +EXPORT_SYMBOL_IF_KUNIT(__binder_alloc_init);
>> /**
>> * binder_alloc_init() - called by binder_open() for per-proc
>> initialization
>> diff --git a/drivers/android/binder_alloc.h
>> b/drivers/android/binder_alloc.h
>> index aa05a9df1360..dc8dce2469a7 100644
>> --- a/drivers/android/binder_alloc.h
>> +++ b/drivers/android/binder_alloc.h
>> @@ -188,5 +188,11 @@ int binder_alloc_copy_from_buffer(struct
>> binder_alloc *alloc,
>> binder_size_t buffer_offset,
>> size_t bytes);
>> +#if IS_ENABLED(CONFIG_KUNIT)
>> +void __binder_alloc_init(struct binder_alloc *alloc, struct list_lru
>> *freelist);
>> +size_t binder_alloc_buffer_size(struct binder_alloc *alloc,
>> + struct binder_buffer *buffer);
>> +#endif
>> +
>> #endif /* _LINUX_BINDER_ALLOC_H */
>> diff --git a/drivers/android/binder_internal.h
>> b/drivers/android/binder_internal.h
>> index 1ba5caf1d88d..b5d3014fb4dc 100644
>> --- a/drivers/android/binder_internal.h
>> +++ b/drivers/android/binder_internal.h
>> @@ -592,4 +592,8 @@ void binder_add_device(struct binder_device *device);
>> */
>> void binder_remove_device(struct binder_device *device);
>> +#if IS_ENABLED(CONFIG_KUNIT)
>> +vm_fault_t binder_vm_fault(struct vm_fault *vmf);
>> +#endif
>> +
>> #endif /* _LINUX_BINDER_INTERNAL_H */
>> diff --git a/drivers/android/tests/.kunitconfig
>> b/drivers/android/tests/.kunitconfig
>> new file mode 100644
>> index 000000000000..a73601231049
>> --- /dev/null
>> +++ b/drivers/android/tests/.kunitconfig
>> @@ -0,0 +1,3 @@
>> +CONFIG_KUNIT=y
>> +CONFIG_ANDROID_BINDER_IPC=y
>> +CONFIG_ANDROID_BINDER_ALLOC_KUNIT_TEST=y
>> diff --git a/drivers/android/tests/Makefile
>> b/drivers/android/tests/Makefile
>> new file mode 100644
>> index 000000000000..6780967e573b
>> --- /dev/null
>> +++ b/drivers/android/tests/Makefile
>> @@ -0,0 +1,3 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
> All the new files need the copyright notice. Something like:
> "Copyright 2025 Google LCC"
Done.
>> +
>> +obj-$(CONFIG_ANDROID_BINDER_ALLOC_KUNIT_TEST) +=
>> binder_alloc_kunit.o
>> diff --git a/drivers/android/tests/binder_alloc_kunit.c
>> b/drivers/android/tests/binder_alloc_kunit.c
>> new file mode 100644
>> index 000000000000..4b68b5687d33
>> --- /dev/null
>> +++ b/drivers/android/tests/binder_alloc_kunit.c
>> @@ -0,0 +1,166 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Test cases for binder allocator code
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <kunit/test.h>
>> +#include <linux/anon_inodes.h>
>> +#include <linux/err.h>
>> +#include <linux/file.h>
>> +#include <linux/fs.h>
>> +#include <linux/mm.h>
>> +#include <linux/mman.h>
>> +#include <linux/sizes.h>
>> +
>> +#include "../binder_alloc.h"
>> +#include "../binder_internal.h"
>> +
>> +MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING");
>> +
>> +#define BINDER_MMAP_SIZE SZ_128K
>> +
>> +struct binder_alloc_test {
>> + struct binder_alloc alloc;
>> + struct list_lru binder_test_freelist;
>> + struct file *filp;
>> + unsigned long mmap_uaddr;
>> +};
>> +
>> +static void binder_alloc_test_init_freelist(struct kunit *test)
>> +{
>> + struct binder_alloc_test *priv = test->priv;
>> +
>> + KUNIT_EXPECT_PTR_EQ(test, priv->alloc.freelist,
>> + &priv->binder_test_freelist);
>> +}
>> +
>> +static void binder_alloc_test_mmap(struct kunit *test)
>> +{
>> + struct binder_alloc_test *priv = test->priv;
>> + struct binder_alloc *alloc = &priv->alloc;
>> + struct binder_buffer *buf;
>> + struct rb_node *n;
>> +
>> + KUNIT_EXPECT_EQ(test, alloc->mapped, true);
>> + KUNIT_EXPECT_EQ(test, alloc->buffer_size, BINDER_MMAP_SIZE);
>> +
>> + n = rb_first(&alloc->allocated_buffers);
>> + KUNIT_EXPECT_PTR_EQ(test, n, NULL);
>> +
>> + n = rb_first(&alloc->free_buffers);
>> + buf = rb_entry(n, struct binder_buffer, rb_node);
>> + KUNIT_EXPECT_EQ(test, binder_alloc_buffer_size(alloc, buf),
>> + BINDER_MMAP_SIZE);
>> + KUNIT_EXPECT_TRUE(test, list_is_last(&buf->entry,
>> &alloc->buffers));
>> +}
>> +
>> +/* ===== End test cases ===== */
>> +
>> +static void binder_alloc_test_vma_close(struct vm_area_struct *vma)
>> +{
>> + struct binder_alloc *alloc = vma->vm_private_data;
>> +
>> + binder_alloc_vma_close(alloc);
>> +}
>> +
>> +static const struct vm_operations_struct binder_alloc_test_vm_ops = {
>> + .close = binder_alloc_test_vma_close,
>> + .fault = binder_vm_fault,
>> +};
>> +
>> +static int binder_alloc_test_mmap_handler(struct file *filp,
>> + struct vm_area_struct *vma)
>> +{
>> + struct binder_alloc *alloc = filp->private_data;
>> +
>> + vm_flags_mod(vma, VM_DONTCOPY | VM_MIXEDMAP, VM_MAYWRITE);
>> +
>> + vma->vm_ops = &binder_alloc_test_vm_ops;
>> + vma->vm_private_data = alloc;
>> +
>> + return binder_alloc_mmap_handler(alloc, vma);
>> +}
>> +
>> +static const struct file_operations binder_alloc_test_fops = {
>> + .mmap = binder_alloc_test_mmap_handler,
>> +};
>> +
>> +static int binder_alloc_test_init(struct kunit *test)
>> +{
>> + struct binder_alloc_test *priv;
>> + int ret;
>> +
>> + priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> + test->priv = priv;
>> +
>> + ret = list_lru_init(&priv->binder_test_freelist);
>> + if (ret) {
>> + kunit_err(test, "Failed to initialize test freelist\n");
>> + return ret;
>> + }
>> +
>> + /* __binder_alloc_init requires mm to be attached */
>> + ret = kunit_attach_mm();
>> + if (ret) {
>> + kunit_err(test, "Failed to attach mm\n");
>> + return ret;
>> + }
>> + __binder_alloc_init(&priv->alloc, &priv->binder_test_freelist);
>> +
>> + priv->filp = anon_inode_getfile("binder_alloc_kunit",
>> + &binder_alloc_test_fops,
>> &priv->alloc,
>> + O_RDWR | O_CLOEXEC);
>> + if (IS_ERR_OR_NULL(priv->filp)) {
>> + kunit_err(test, "Failed to open binder alloc test driver
>> file\n");
>> + return priv->filp ? PTR_ERR(priv->filp) : -ENOMEM;
>> + }
>> +
>> + priv->mmap_uaddr = kunit_vm_mmap(test, priv->filp, 0,
>> BINDER_MMAP_SIZE,
>> + PROT_READ, MAP_PRIVATE |
>> MAP_NORESERVE,
>> + 0);
>> + if (!priv->mmap_uaddr) {
>> + kunit_err(test, "Could not map the test's transaction
>> memory\n");
>> + return -ENOMEM;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void binder_alloc_test_exit(struct kunit *test)
>> +{
>> + struct binder_alloc_test *priv = test->priv;
>> +
>> + /* Close the backing file to make sure binder_alloc_vma_close runs
>> */
>> + if (!IS_ERR_OR_NULL(priv->filp))
>> + fput(priv->filp);
>> +
>> + if (priv->alloc.mm)
>> + binder_alloc_deferred_release(&priv->alloc);
>> +
>> + /* Make sure freelist is empty */
>> + KUNIT_EXPECT_EQ(test, list_lru_count(&priv->binder_test_freelist),
>> 0);
>> + list_lru_destroy(&priv->binder_test_freelist);
>> +}
>> +
>> +static struct kunit_case binder_alloc_test_cases[] = {
>> + KUNIT_CASE(binder_alloc_test_init_freelist),
>> + KUNIT_CASE(binder_alloc_test_mmap),
>> + {}
>> +};
>> +
>> +static struct kunit_suite binder_alloc_test_suite = {
>> + .name = "binder_alloc",
>> + .test_cases = binder_alloc_test_cases,
>> + .init = binder_alloc_test_init,
>> + .exit = binder_alloc_test_exit,
>> +};
>> +
>> +kunit_test_suite(binder_alloc_test_suite);
>> +
>> +MODULE_AUTHOR("Tiffany Yang <ynaffit@...gle.com>");
>> +MODULE_DESCRIPTION("Binder Alloc KUnit tests");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.50.0.727.gbf7dc18ff4-goog
> Aside from the missing Copyright, feel free to add:
> Acked-by: Carlos Llamas <cmllamas@...gle.com>
Thanks for the review!
--
Tiffany Y. Yang
Powered by blists - more mailing lists