| 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: <Z7y5vK1M5IOizIWR@google.com>
Date: Mon, 24 Feb 2025 18:26:04 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Brendan Jackman <jackmanb@...gle.com>
Cc: Brendan Higgins <brendan.higgins@...ux.dev>,
David Gow <davidgow@...gle.com>, Rae Moar <rmoar@...gle.com>,
Andrew Morton <akpm@...ux-foundation.org>,
David Hildenbrand <david@...hat.com>,
Oscar Salvador <osalvador@...e.de>,
Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
Vlastimil Babka <vbabka@...e.cz>, Michal Hocko <mhocko@...nel.org>,
linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com,
linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH RFC 4/4] mm/page_alloc_test: Add smoke-test for page
allocation
On Mon, Feb 24, 2025 at 02:47:14PM +0000, Brendan Jackman wrote:
> This is the bare minimum to illustrate what KUnit code would look like
> that covers the page allocator.
>
> Even this trivial test illustrates a couple of nice things that are
> possible when testing via KUnit
>
> 1. We can directly assert that the correct zone was used.
> (Although note due to the simplistic setup, you can have any zone you
> like as long as it's ZONE_NORMAL).
>
> 2. We can assert that a page got freed. It's probably pretty unlikely
> that we'd have a bug that actually causes a page to get leaked by the
> allocator, but it serves as a good example of the kind of assertions
> we can make by judicously peeking at allocator internals.
>
> Signed-off-by: Brendan Jackman <jackmanb@...gle.com>
> ---
> mm/page_alloc_test.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 138 insertions(+), 1 deletion(-)
>
> diff --git a/mm/page_alloc_test.c b/mm/page_alloc_test.c
> index c6bcfcaf61b57ca35ad1b5fc48fd07d0402843bc..0c4effb151f4cd31ec6a696615a9b6ae4964b332 100644
> --- a/mm/page_alloc_test.c
> +++ b/mm/page_alloc_test.c
> @@ -26,6 +26,139 @@
> } \
> })
>
> +#define EXPECT_WITHIN_ZONE(test, page, zone) ({ \
ultranit: the '\' above is unaligned with the ones below.
> + unsigned long pfn = page_to_pfn(page); \
> + unsigned long start_pfn = zone->zone_start_pfn; \
> + unsigned long end_pfn = start_pfn + zone->spanned_pages; \
> + \
> + KUNIT_EXPECT_TRUE_MSG(test, \
> + pfn >= start_pfn && pfn < end_pfn, \
> + "Wanted PFN 0x%lx - 0x%lx, got 0x%lx", \
> + start_pfn, end_pfn, pfn); \
> + KUNIT_EXPECT_PTR_EQ_MSG(test, page_zone(page), zone, \
> + "Wanted %px (%s), got %px (%s)", \
> + zone, zone->name, page_zone(page), page_zone(page)->name); \
> +})
> +
> +static void action_nodemask_free(void *ctx)
> +{
> + NODEMASK_FREE(ctx);
> +}
> +
> +/*
> + * Call __alloc_pages_noprof with a nodemask containing only the nid.
> + *
> + * Never returns NULL.
> + */
> +static inline struct page *alloc_pages_force_nid(struct kunit *test,
> + gfp_t gfp, int order, int nid)
> +{
> + NODEMASK_ALLOC(nodemask_t, nodemask, GFP_KERNEL);
For the sake of the test can't we just put the nodemask on the stack?
> + struct page *page;
> +
> + KUNIT_ASSERT_NOT_NULL(test, nodemask);
> + kunit_add_action(test, action_nodemask_free, &nodemask);
Why aren't we just freeing the nodemask after using it, before we make
any assertions?
> + nodes_clear(*nodemask);
> + node_set(nid, *nodemask);
> +
> + page = __alloc_pages_noprof(GFP_KERNEL, 0, nid, nodemask);
> + KUNIT_ASSERT_NOT_NULL(test, page);
> + return page;
> +}
> +
> +static inline bool page_on_buddy_list(struct page *want_page, struct list_head *head)
> +{
> + struct page *found_page;
nit: This is just an iterator, we can probably just call it 'page'.
'found' is confusing for me because we didn't really search for it.
> +
> + list_for_each_entry(found_page, head, buddy_list) {
> + if (found_page == want_page)
> + return true;
> + }
> +
> + return false;
> +}
> +
> +/* Test case parameters that are independent of alloc order. */
> +static const struct {
> + gfp_t gfp_flags;
> + enum zone_type want_zone;
> +} alloc_fresh_gfps[] = {
> + /*
> + * The way we currently set up the isolated node, everything ends up in
> + * ZONE_NORMAL.
> + */
> + { .gfp_flags = GFP_KERNEL, .want_zone = ZONE_NORMAL },
> + { .gfp_flags = GFP_ATOMIC, .want_zone = ZONE_NORMAL },
> + { .gfp_flags = GFP_USER, .want_zone = ZONE_NORMAL },
> + { .gfp_flags = GFP_DMA32, .want_zone = ZONE_NORMAL },
> +};
> +
> +struct alloc_fresh_test_case {
> + int order;
> + int gfp_idx;
> +};
> +
> +/* Generate test cases as the cross product of orders and alloc_fresh_gfps. */
> +static const void *alloc_fresh_gen_params(const void *prev, char *desc)
> +{
> + /* Buffer to avoid allocations. */
> + static struct alloc_fresh_test_case tc;
> +
> + if (!prev) {
> + /* First call */
> + tc.order = 0;
> + tc.gfp_idx = 0;
> + return &tc;
> + }
We need to set 'tc' here to whatever 'prev' is pointing at, right?
> +
> + tc.gfp_idx++;
> + if (tc.gfp_idx >= ARRAY_SIZE(alloc_fresh_gfps)) {
> + tc.gfp_idx = 0;
> + tc.order++;
> + }
> + if (tc.order > MAX_PAGE_ORDER)
> + /* Finished. */
> + return NULL;
> +
> + snprintf(desc, KUNIT_PARAM_DESC_SIZE, "order %d %pGg\n",
> + tc.order, &alloc_fresh_gfps[tc.gfp_idx].gfp_flags);
> + return &tc;
> +}
> +
> +/* Smoke test: allocate from a node where everything is in a pristine state. */
> +static void test_alloc_fresh(struct kunit *test)
> +{
> + const struct alloc_fresh_test_case *tc = test->param_value;
> + gfp_t gfp_flags = alloc_fresh_gfps[tc->gfp_idx].gfp_flags;
> + enum zone_type want_zone_type = alloc_fresh_gfps[tc->gfp_idx].want_zone;
> + struct zone *want_zone = &NODE_DATA(isolated_node)->node_zones[want_zone_type];
> + struct list_head *buddy_list;
> + struct per_cpu_pages *pcp;
> + struct page *page, *merged_page;
> + int cpu;
> +
> + page = alloc_pages_force_nid(test, gfp_flags, tc->order, isolated_node);
> +
> + EXPECT_WITHIN_ZONE(test, page, want_zone);
> +
> + cpu = get_cpu();
> + __free_pages(page, 0);
> + pcp = per_cpu_ptr(want_zone->per_cpu_pageset, cpu);
> + put_cpu();
> +
> + /*
> + * Should end up back in the free area when drained. Because everything
> + * is free, it should get buddy-merged up to the maximum order.
> + */
> + drain_zone_pages(want_zone, pcp);
> + KUNIT_EXPECT_TRUE(test, PageBuddy(page));
> + KUNIT_EXPECT_EQ(test, buddy_order(page), MAX_PAGE_ORDER);
> + KUNIT_EXPECT_TRUE(test, list_empty(&pcp->lists[MIGRATE_UNMOVABLE]));
> + merged_page = pfn_to_page(round_down(page_to_pfn(page), 1 << MAX_PAGE_ORDER));
> + buddy_list = &want_zone->free_area[MAX_PAGE_ORDER].free_list[MIGRATE_UNMOVABLE];
> + KUNIT_EXPECT_TRUE(test, page_on_buddy_list(merged_page, buddy_list));
> +}
> +
> static void action_drain_pages_all(void *unused)
> {
> int cpu;
> @@ -144,7 +277,11 @@ static void depopulate_isolated_node(struct kunit_suite *suite)
> WARN_ON(add_memory(0, start, size, MMOP_ONLINE));
> WARN_ON(walk_memory_blocks(start, size, NULL, memory_block_online_cb));
> }
> -static struct kunit_case test_cases[] = { {} };
> +
> +static struct kunit_case test_cases[] = {
> + KUNIT_CASE_PARAM(test_alloc_fresh, alloc_fresh_gen_params),
> + {}
> +};
>
> struct kunit_suite page_alloc_test_suite = {
> .name = "page_alloc",
>
> --
> 2.48.1.601.g30ceb7b040-goog
>
Powered by blists - more mailing lists