linux-kernel - Re: [PATCH v9 4/7] mm,page_owner: Implement the tracking of the stacks count

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]

Message-ID: <9fc95f61-827f-40ee-a823-576cdcad7939@suse.cz>
Date: Thu, 15 Feb 2024 12:08:53 +0100
From: Vlastimil Babka <vbabka@...e.cz>
To: Oscar Salvador <osalvador@...e.de>,
 Andrew Morton <akpm@...ux-foundation.org>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
 Michal Hocko <mhocko@...e.com>, Marco Elver <elver@...gle.com>,
 Andrey Konovalov <andreyknvl@...il.com>,
 Alexander Potapenko <glider@...gle.com>
Subject: Re: [PATCH v9 4/7] mm,page_owner: Implement the tracking of the
 stacks count

On 2/14/24 18:01, Oscar Salvador wrote:
> Implement {inc,dec}_stack_record_count() which increments or
> decrements on respective allocation and free operations, via
> __reset_page_owner() (free operation) and __set_page_owner() (alloc
> operation).
> Newly allocated stack_record structs will be added to the list stack_list
> via add_stack_record_to_list().
> Modifications on the list are protected via a spinlock with irqs
> disabled, since this code can also be reached from IRQ context.
> 
> Signed-off-by: Oscar Salvador <osalvador@...e.de>
> Reviewed-by: Marco Elver <elver@...gle.com>

Reviewed-by: Vlastimil Babka <vbabka@...e.cz>
Note:

> +static void inc_stack_record_count(depot_stack_handle_t handle, gfp_t gfp_mask)
> +{
> +	struct stack_record *stack_record = __stack_depot_get_stack_record(handle);
> +
> +	if (!stack_record)
> +		return;
> +
> +	/*
> +	 * New stack_record's that do not use STACK_DEPOT_FLAG_GET start
> +	 * with REFCOUNT_SATURATED to catch spurious increments of their
> +	 * refcount.
> +	 * Since we do not use STACK_DEPOT_FLAG_GET API, let us
> +	 * set a refcount of 1 ourselves.
> +	 */
> +	if (refcount_read(&stack_record->count) == REFCOUNT_SATURATED) {
> +		int old = REFCOUNT_SATURATED;
> +
> +		if (atomic_try_cmpxchg_relaxed(&stack_record->count.refs, &old, 1))
> +			/* Add the new stack_record to our list */
> +			add_stack_record_to_list(stack_record, gfp_mask);
			
			Not returning here...

> +	}
> +	refcount_inc(&stack_record->count);

.. means we'll increase the count to 2 on the first store, so there's a
bias. Which would be consistent with the failure and dummy stacks that also
start with a refcount of 1. But then the stack count reporting should
decrement by 1 to prevent confusion? (in the following patch). Imagine
somebody debugging an allocation stack where there are not so many of them,
but the allocation is large, and being sidetracked by an off-by-one error.