linux-kernel - Re: [PATCH] mm/page_alloc: fix deadlock on cpu_hotplug_lock in __accept

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] [day] [month] [year] [list]

Message-ID: <mc7rnftmisbx5fpefwaiobngzpbh66yk5535xrfxope4gobu36@2wite5exlfcd>
Date: Tue, 1 Apr 2025 10:25:02 +0300
From: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
To: Dave Hansen <dave.hansen@...el.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, 
	Mike Rapoport <rppt@...nel.org>, David Hildenbrand <david@...hat.com>, 
	Vlastimil Babka <vbabka@...e.cz>, Mel Gorman <mgorman@...hsingularity.net>, 
	Tom Lendacky <thomas.lendacky@....com>, "Kalra, Ashish" <ashish.kalra@....com>, 
	Rick Edgecombe <rick.p.edgecombe@...el.com>, linux-mm@...ck.org, linux-coco@...ts.linux.dev, 
	linux-kernel@...r.kernel.org, Srikanth Aithal <sraithal@....com>
Subject: Re: [PATCH] mm/page_alloc: fix deadlock on cpu_hotplug_lock in
 __accept_page()

On Mon, Mar 31, 2025 at 12:07:07PM -0700, Dave Hansen wrote:
> On 3/29/25 10:10, Kirill A. Shutemov wrote:
> > +		if (system_wq)
> > +			schedule_work(&zone->unaccepted_cleanup);
> > +		else
> > +			unaccepted_cleanup_work(&zone->unaccepted_cleanup);
> > +	}
> >  }
> 
> The 'system_wq' check seems like an awfully big hack. No other
> schedule_work() user does anything similar that I can find across the tree.

I don't see how it is "an awfully big hack". It is "use system_wq if it is
ready".

Maybe it is going to be marginally cleaner if schedule_work() would be
open-coded:

		if (system_wq)
		        queue_work(system_wq, &zone->unaccepted_cleanup);
		else
			unaccepted_cleanup_work(&zone->unaccepted_cleanup);

?

> 
> Instead of hacking in some internal state, could you use 'system_state',
> like:
> 
> 	if (system_state == SYSTEM_BOOTING)
> 		unaccepted_cleanup_work(&zone->unaccepted_cleanup);
> 	else
> 		schedule_work(&zone->unaccepted_cleanup);

Really? The transition points between these states are arbitrary defined.
Who said that if we are out of SYSTEM_BOOTING we can use system_wq?
Tomorrow we can introduce additional state between BOOTING and SCHEDULING
and this code will be silently broken. The same for any new state before
BOOTING.

> The other method would be to make it more opportunistic? Basically,
> detect when it might deadlock:
> 
> bool try_to_dec()
> {
> 	if (!cpus_read_trylock())
> 		return false;
> 
> 	static_branch_dec_cpuslocked(&zones_with_unaccepted_pages);
> 	cpus_read_unlock();
> 
> 	return true;
> }
> 
> That still requires a bit in the zone to say whether the
> static_branch_dec() was deferred or not, though. It's kinda open-coding
> schedule_work().

It will also require special handling for soft CPU online/offline.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov