From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CF1A32C21DD for ; Tue, 21 Jul 2026 16:07:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784650041; cv=none; b=rZJNlbXCrOGncnXKyIsZvQmB/pJSiAw29HrucLzODicqD+DDxIWI02yEcUop4reOYc8aF7V0CjUF1bgwy50ENzkN1HRJzSt9dmisK2CpqmuGHMIz+lGJDAGDWSxkTt8mxTwT3nk5hxCWDwA1Hr0EY6JCsAxeKYqwBHkXUJWesNM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784650041; c=relaxed/simple; bh=t0kzVU95J8OpK9giua6UOZ4ryrD4aPD70A3d26rOIvo=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=QfiJ/z3vb3KvzCj3idOMvOx/ZDi6Ws6XgmPcgcdwYE04FnKdD4fZtHe4KHUIvMR3HmtS02Qxfqh1ycf3mgYpIhG4Owhq2CRcxAvixbe1qG1QtFvEJoZfONlqtVR4iYHGoHHMUU9Fx7vREklDmMQsTHGgJ/V2rZyRfOsVaz0tEIY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=PJTSnxlZ; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="PJTSnxlZ" Received: by smtp.kernel.org (Postfix) with ESMTPSA id EB2461F00A3F; Tue, 21 Jul 2026 16:07:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1784650039; bh=4p9k0hlLGDY/qPU1IJexARY+JeFaJLu5uDqSMtXY9B4=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=PJTSnxlZneCnDin3BpxNW27U6slc2AwV9nKJGK62/afX5n7heJDdXQIcMGofDlqYQ +2DGkk8YX0d8O6ghTG2Z6ISuRdkrN8DieLr8wZdWKQi1bVF0c7rR8g1cmcc+BGqI9J 2N9pU3OP65KJT3tRB3UkrGXWueHyOUlpN7HbmLTvbDRj/1CsrGjpBjAAmJBxWVj4ni Lq449/SSe6hDQSKPmBDeZx9POwWxddADz5Z/PhEjoTK3Wfth2wgkhibi7SY5GNcxaA CL4fqI5Fj6aRcV3gx1MV8r9lPWpr3Q7vZlEISXxtE67knyiSXYizSre3pPcPQ9fAxr E1y1n3JsHjaDw== Date: Tue, 21 Jul 2026 17:07:05 +0100 From: "Lorenzo Stoakes (ARM)" To: "Denis V. Lunev" Cc: Dave Hansen , Andy Lutomirski , Peter Zijlstra , x86@kernel.org, Thomas Gleixner , Ingo Molnar , Borislav Petkov , "H . Peter Anvin" , Mike Rapoport , Juergen Gross , Kiryl Shutsemau , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] x86/mm/pat: take cpa_lock around large-page collapse Message-ID: References: <20260715183453.2381141-1-den@openvz.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260715183453.2381141-1-den@openvz.org> On Wed, Jul 15, 2026 at 08:34:52PM +0200, Denis V. Lunev wrote: > Loading and unloading modules concurrently on several CPUs on a KASAN > build, with a short delay injected at the CPA page-table lookup to > widen the window, faults within minutes: > > BUG: KASAN: use-after-free in __change_page_attr+0x7cc/0x7e0 > Write of size 8 at addr ffff888181139718 by task modprobe > ... > The buggy address belongs to the physical page: > pfn:0x181139 ... page_type: f2(table) > > cpa_collapse_large_pages() rebuilds a leaf PMD from its 4K PTEs and > frees the old PTE-table pages, while __change_page_attr() fetches a > PTE pointer from a lockless lookup_address_in_pgd_attr() and writes > it with set_pte_atomic() only later. When module text is served from > a shared large ROX mapping the two run on the same PMD: > > CPU A (module load) CPU B (module finalize) > ------------------- ----------------------- > execmem_make_temp_rw > set_memory_nx > __change_page_attr > split 2M -> 4K table P > kpte = &P[i] (lockless) > execmem_restore_rox > set_memory_rox (CPA_COLLAPSE) > cpa_collapse_large_pages > rebuild leaf PMD > flush_tlb_all > pagetable_free(P) > set_pte_atomic(kpte, ...) > -> writes into freed P So now this is resolved via: init_mm read lock init_mm write lock > > P is a page-table page (page_type: table), reused at once, so the > write corrupts whatever got the page next: a bad-pte or bad-page > splat, or a fatal fault once P has been turned into read-only text. > > The flush_tlb_all() before the free does not close this: its IPI only > serializes against page-table walkers that run with interrupts off > (e.g. GUP-fast); the walk in __change_page_attr() runs with interrupts > on, so nothing stops it from holding a stale pointer into P. > > Serialize the collapse - the PMD rebuild, TLB flush and PTE-table > free - under cpa_lock, the same lock __change_page_attr() now takes > unconditionally since commit ("x86/mm/pat: stop gating cpa_lock on > debug_pagealloc_enabled()"), so a concurrent walker can no longer > hold a pointer into a table the collapse is about to free. So this isn't quite correct any more; the init_mm locks fix this race in my series ([0]), annotated above. I feel terrible to have raced with your patch, and the commit message is gloriously well-written and the patch is really good, it's just unfortunate that ptdump ALSO races and can't use cpa_lock. And my series ended up growing into a monstrous rabbit hole in general :) However importantly, this patch does not conflict with mine, as the latest revision holds the init_mm write lock over what was cpa_collapse_large_pages() and what is now __cpa_collapse_large_pages() so the spin lock is nested within the rwsem. (There'll be a small merge conflict there, easily resolved.) But, I wonder whether it's achieving much at this stage? But I think there might be other races here potentially with lockless walkers, which are probably worth looking at. But certainly perhaps the commit message should be altered to reflect that. > > Fixes: 41d88484c71c ("x86/mm/pat: restore large ROX pages after fragmentation") > Signed-off-by: Denis V. Lunev > Acked-by: Kiryl Shutsemau (Meta) > --- > v2: > - drop the debug_pagealloc_enabled() skip and its comment: now that > __change_page_attr() takes cpa_lock unconditionally, the skip is no > longer needed to avoid locking only one side of the race So there's also an unfortunate issues as a result of doing this: As commented over there, Mike really needs to convert the locks in his patch ([1]) to be irq-safe since debug pagealloc wonderfully calls __change_page_attr_set_clr() from potentially-atomic context. (And obviously then your spin locks should also be irq-safe) But with all of the spin locks converted, you potentially deadlock, since you're holding cpa_lock across flush_tlb_all() which can end up doing an IPI: CPU 0 -----------------------------------|---------------------------------------- < GFP_ATOMIC context > | __free_pages_prepare() | set_memory_rox() -> debug_pagealloc_unmap_pages() | -> ... -> __kernel_map_pages() | -> __cpa_collapse_large_pages() -> __change_page_attr_set_clr() | -> cpa_lock ACQUIRED IRQs off -> cpa_lock spins [irqs off] | -> IPI every CPU... BUT I think you can fix that by just reinstating from v1 the 'give up on collapse if debug_pagealloc_enabled()' thing, as that's the only way the locks can be held by anything in irq context. Then perhaps it's actually valid to use spin_[un]lock() here also and we need only update those code paths accessible with debug page alloc enabled? > > arch/x86/mm/pat/set_memory.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c > index e8316f5ffa8a..6dda81a629d6 100644 > --- a/arch/x86/mm/pat/set_memory.c > +++ b/arch/x86/mm/pat/set_memory.c > @@ -417,6 +417,8 @@ static void cpa_collapse_large_pages(struct cpa_data *cpa) > int collapsed = 0; > int i; > > + spin_lock(&cpa_lock); > + > if (cpa->flags & (CPA_PAGES_ARRAY | CPA_ARRAY)) { > for (i = 0; i < cpa->numpages; i++) > collapsed += collapse_large_pages(__cpa_addr(cpa, i), > @@ -430,8 +432,10 @@ static void cpa_collapse_large_pages(struct cpa_data *cpa) > collapsed += collapse_large_pages(addr, &pgtables); > } > > - if (!collapsed) > + if (!collapsed) { > + spin_unlock(&cpa_lock); > return; > + } > > flush_tlb_all(); > > @@ -439,6 +443,8 @@ static void cpa_collapse_large_pages(struct cpa_data *cpa) > list_del(&ptdesc->pt_list); > pagetable_free(ptdesc); > } > + > + spin_unlock(&cpa_lock); > } > > static void cpa_flush(struct cpa_data *cpa, int cache) > > base-commit: 4a0e2d0aa6a44155f9bf0289be3dd94024cee3b4 > -- > 2.53.0 > > Thanks, Lorenzo [0]:https://lore.kernel.org/linux-mm/20260716-series-vmap-race-fix-v4-0-8c108c4317df@kernel.org/ [1]:https://lore.kernel.org/all/20260715144519.934289-1-rppt@kernel.org/