linux-kernel - Re: [PATCH v8 2/5] 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: <ZctjNn7i3atRPccE@localhost.localdomain>
Date: Tue, 13 Feb 2024 13:40:22 +0100
From: Oscar Salvador <osalvador@...e.de>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Marco Elver <elver@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	Michal Hocko <mhocko@...e.com>,
	Andrey Konovalov <andreyknvl@...il.com>,
	Alexander Potapenko <glider@...gle.com>
Subject: Re: [PATCH v8 2/5] mm,page_owner: Implement the tracking of the
 stacks count

On Tue, Feb 13, 2024 at 12:34:55PM +0100, Vlastimil Babka wrote:
> On 2/13/24 10:21, Marco Elver wrote:
> > On Tue, 13 Feb 2024 at 10:16, Vlastimil Babka <vbabka@...e.cz> wrote:
> >> Isn't this racy? Shouldn't we use some atomic cmpxchg operation to change
> >> from REFCOUNT_SATURATED to 1?
> > 
> > If 2 threads race here, both will want to add it to the list as well
> > and take the lock. So this could just be solved with double-checked
> > locking:
> > 
> > if (count == REFCOUNT_SATURATED) {
> >   spin_lock(...);
> 
> Yeah probably stack_list_lock could be taken here already. But then the
> kmalloc() of struct stack must happen also here, before taking the lock.

I am thinking what would be a less expensive and safer option here.
Of course, taking the spinlock is easier, but having the allocation
inside is tricky, and having it outside could mean that we might free
the struct once we checked __within__ the lock that the refcount
is no longer REFCOUNT_SATURATED. No big deal, but a bit sub-optimal.

On the other hand, IIUC, cmpxchg has some memory ordering, like
store_and_release/load_acquire do, so would it be safe to use it
instead of taking the lock?
 

-- 
Oscar Salvador
SUSE Labs