linux-kernel - Re: [PATCH v4 17/22] lib/stackdepot: allow users to evict stack traces

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: Thu, 4 Jan 2024 09:52:53 +0100
From: Oscar Salvador <osalvador@...e.de>
To: andrey.konovalov@...ux.dev
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	Andrey Konovalov <andreyknvl@...il.com>,
	Marco Elver <elver@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	Vlastimil Babka <vbabka@...e.cz>, kasan-dev@...glegroups.com,
	Evgenii Stepanov <eugenis@...gle.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org,
	Andrey Konovalov <andreyknvl@...gle.com>
Subject: Re: [PATCH v4 17/22] lib/stackdepot: allow users to evict stack
 traces

On Mon, Nov 20, 2023 at 06:47:15PM +0100, andrey.konovalov@...ux.dev wrote:
> From: Andrey Konovalov <andreyknvl@...gle.com>
> 
> Add stack_depot_put, a function that decrements the reference counter
> on a stack record and removes it from the stack depot once the counter
> reaches 0.
> 
> Internally, when removing a stack record, the function unlinks it from
> the hash table bucket and returns to the freelist.
> 
> With this change, the users of stack depot can call stack_depot_put
> when keeping a stack trace in the stack depot is not needed anymore.
> This allows avoiding polluting the stack depot with irrelevant stack
> traces and thus have more space to store the relevant ones before the
> stack depot reaches its capacity.
> 
> Signed-off-by: Andrey Konovalov <andreyknvl@...gle.com>

I yet have to review the final bits of this series, but I'd like to
comment on something below

  
> +void stack_depot_put(depot_stack_handle_t handle)
> +{
> +	struct stack_record *stack;
> +	unsigned long flags;
> +
> +	if (!handle || stack_depot_disabled)
> +		return;
> +
> +	write_lock_irqsave(&pool_rwlock, flags);
> +
> +	stack = depot_fetch_stack(handle);
> +	if (WARN_ON(!stack))
> +		goto out;
> +
> +	if (refcount_dec_and_test(&stack->count)) {
> +		/* Unlink stack from the hash table. */
> +		list_del(&stack->list);
> +
> +		/* Free stack. */
> +		depot_free_stack(stack);

It would be great if stack_depot_put would also accept a boolean,
which would determine whether we want to erase the stack or not.

For the feature I'm working on page_ower [1], I need to keep track
of how many times we allocated/freed from a certain path, which may
expose a potential leak, and I was using the refcount to do that,
but I don't want the record to be erased, because this new
functionality won't be exclusive with the existing one.

e.g:  you can check /sys/kernel/debug/page_owner AND
/sys/kernel/debug/page_owner_stacks

So, while the new functionaliy won't care if a record has been erased,
the old one will, so information will be lost.

[1] https://patchwork.kernel.org/project/linux-mm/cover/20231120084300.4368-1-osalvador@suse.de/

 

-- 
Oscar Salvador
SUSE Labs