linux-kernel - Re: [PATCH v2 1/2] mm,page_owner: Fix refcount imbalance

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: Wed, 20 Mar 2024 06:49:05 +0100
From: Oscar Salvador <osalvador@...e.de>
To: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
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>, Marco Elver <elver@...gle.com>,
	Andrey Konovalov <andreyknvl@...il.com>,
	Alexander Potapenko <glider@...gle.com>,
	syzbot+41bbfdb8d41003d12c0f@...kaller.appspotmail.com
Subject: Re: [PATCH v2 1/2] mm,page_owner: Fix refcount imbalance

On Wed, Mar 20, 2024 at 01:40:05PM +0900, Tetsuo Handa wrote:
> Hmm, I guess that this is not an expected user of refcount API.
> If it is correct behavior that refcount becomes 0 here, you need to explain like
> 
> -		refcount_sub_and_test(nr_base_pages, &stack_record->count);
> +		if (refcount_sub_and_test(nr_base_pages, &stack_record->count)) {
> +			// Explain why nothing to do here, and explain where/how
> +			// refcount again becomes positive value using refcount_set().
> +		}
> 
> or replace refcount_t with atomic_t where it is legal to make refcount positive
> without using atomic_set().

No, it is not expected for the refcount to become 0.
I do know why, but I lost a chunk in the middle of a rebase.
This should have the follwing on top:

 diff --git a/mm/page_owner.c b/mm/page_owner.c
 index 2613805cb665..e477a71d6adc 100644
 --- a/mm/page_owner.c
 +++ b/mm/page_owner.c
 @@ -222,8 +222,11 @@ static void dec_stack_record_count(depot_stack_handle_t handle,
  {
         struct stack_record *stack_record = __stack_depot_get_stack_record(handle);
  
 -       if (stack_record)
 -               refcount_sub_and_test(nr_base_pages, &stack_record->count);
 +       if (!stack_record)
 +               return;
 +
 +       if (refcount_sub_and_test(nr_base_pages, &stack_record->count))
 +               WARN(1, "%s refcount went to 0 for %u handle\n", __func__, handle);
  }


-- 
Oscar Salvador
SUSE Labs