linux-kernel - Re: [PATCH v5 2/3] mm, page_owner: Add page_owner_stacks file to print out only stacks and their counte

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 22 May 2023 10:27:41 +0200
From:   Alexander Potapenko <glider@...gle.com>
To:     Oscar Salvador <osalvador@...e.de>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Michal Hocko <mhocko@...e.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Waiman Long <longman@...hat.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Marco Elver <elver@...gle.com>,
        Andrey Konovalov <andreyknvl@...il.com>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH v5 2/3] mm, page_owner: Add page_owner_stacks file to
 print out only stacks and their counte

On Tue, May 16, 2023 at 8:25 PM Oscar Salvador <osalvador@...e.de> wrote:

I am still hesitant about adding this functionality to stackdepot,
because page_owner is the only user of the stack counters that look
orthogonal to the rest of stackdepot.
One indicator of that is the fact that you keep adding dependencies on
page_owner to stackdepot code.

> We might be only interested in knowing about stacks <-> count
> relationship, so instead of having to fiddle with page_owner
> output and screen through pfns, let us add a new file called
> 'page_owner_stacks' that does just that.
> By cating such file, we will get all the stacktraces followed by

"cating"?

> +#ifdef CONFIG_PAGE_OWNER
> +void *stack_start(struct seq_file *m, loff_t *ppos);
> +void *stack_next(struct seq_file *m, void *v, loff_t *ppos);
> +int stack_print(struct seq_file *m, void *v);
> +#endif

Code depending on CONFIG_PAGE_OWNER should not belong here.
It is fine to have generic iterators to traverse the stack depot in
stackdepot.h without #ifdefs.
Perhaps they don't need to implement the whole interface of seq_file.


> @@ -486,6 +487,77 @@ static struct stack_record *stack_depot_getstack(depot_stack_handle_t handle)
>         return stack;
>  }
>
> +#ifdef CONFIG_PAGE_OWNER

Ditto - no CONFIG_PAGE_OWNER, please


> +
> +int stack_print(struct seq_file *m, void *v)
> +{
> +       char *buf;
> +       int ret = 0;
> +       struct stack_record *stack = v;
> +
> +       if (!stack->size || stack->size < 0 ||
> +           stack->size > PAGE_SIZE || stack->handle.valid != 1 ||
> +           refcount_read(&stack->count) < 1)
> +               return 0;
> +
> +       buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +       ret += stack_trace_snprint(buf, PAGE_SIZE, stack->entries, stack->size, 0);
> +       scnprintf(buf + ret, PAGE_SIZE - ret, "stack count: %d\n\n",
> +                 refcount_read(&stack->count));
> +       seq_printf(m, buf);
> +       seq_puts(m, "\n\n");
> +       kfree(buf);
> +
> +       return 0;
> +}

Maybe stack_print() should be in mm/page_owner.c instead?



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg