Re: [PATCH v3 1/2] ARM: mm: fix use-after-free in __do_user_fault() under CONFIG_DEBUG_USER

[Lorenzo Stoakes <ljs@kernel.org>]

On Fri, Jul 10, 2026 at 10:32:29AM +0800, Qi Xi wrote:
>
> On 07/07/2026 23:34, Russell King wrote:
> > On Tue, Jul 07, 2026 at 02:20:19PM +0100, Lorenzo Stoakes wrote:
> > > On Tue, Jul 07, 2026 at 09:14:09PM +0800, Xie Yuanbin wrote:
> > > > On Tue, 7 Jul 2026 12:57:45 +0100, Russell King wrote:
> > > > > No. This information is useful debug for kernel oops.
> > > > For kernel oops, I think it should be `!user_mode(regs)`, Qi Xi's reply:
> > > >
> > > > On Tue, 7 Jul 2026 19:48:12 +0800, Qi Xi wrote:
> > > > > For do_DataAbort() fallback:
> > > > >
> > > > > 	if (user_mode(regs)) {
> > > > > 		if (addr < TASK_SIZE) {
> > > > > 			mmap_read_lock(current->mm);
> > > > > 			show_pte(KERN_ALERT, current->mm, addr);
> > > > > 			mmap_read_unlock(current->mm);
> > > > > 		}
> > > > > 	} else {
> > > > > 		show_pte(KERN_ALERT, current->mm, addr);
> > > > > 	}
> > > > changes nothing to kernel oops. It only skip show_pte() for user-mode
> > > > faults, and the fault addr is a kernel address, which means a user
> > > > program is trying to access a kernel address.
> > > > I think it is reasonable to skip show_pte() in this case?
> > > Well the whole reason you're faulting here might be because a userland process
> > > did that right? The page tables should tell you (presumably on ARM32 :)
> > >
> > > And I hate to repeat myself, maybe you didn't read the whole thread but... just
> > > use mmap_write_lock(), this isn't necessary?
> > >
> > > What is this trying to achieve?
> > >
> > > You're not in a hotpath, why are you bothering to conditionally take/not take
> > > the lock?
> > Unconditionally taking the lock could lead to a deadlock. Consider
> > the case where the mmap lock is held, and we get an unrecognised
> > abort from the kernel.
> >
> > If we try to take the mmap lock again, we'll deadlock, which will
> > result in very little debug information being output - and the
> > system locks up. The only thing that would save such a case would
> > be if the user had decided to use a hardware watchdog, or is
> > physically present to press the reset button.

Yeah right, ugh.

We don't currently have an mmap_write_trylock(), that was removed in commit
cf95e337cb63 ("mm: delete mmap_write_trylock() and vma_try_start_write()")
but I think it's legit to bring _only_ the mmap_write_trylock() back (not
vma_try_start_write()), AND updated to account for how VMA locking works
currently (we have to update a seqcount on mmap write lock acquisition).

To make life easy - I've attached a patch that you can add as part of a
series (please keep attribution etc. to me so I can be blamed/villified if
this is insane :)

That way we avoid the deadlock and also avoid concurrent downgraded
munmap() oopsing in show_pte().

>
> We are preparing a v4 and would like to confirm the approach for
> the do_DataAbort() fallback path (__do_user_fault() is similar).
>
> As Lorenzo noted, an mmap write lock is required here because
> munmap() downgrades the write lock to a read lock before tearing
> down page tables.
>
> -	show_pte(KERN_ALERT, current->mm, addr);
> +	if (user_mode(regs)) {
> +		if (addr < TASK_SIZE) {
> +			mmap_write_lock(current->mm);
> +			show_pte(KERN_ALERT, current->mm, addr);
> +			mmap_write_unlock(current->mm);
> +		}
> +	} else {
> +		show_pte(KERN_ALERT, current->mm, addr);
> +	}
>
> The lock is taken only for user_mode(regs) + addr < TASK_SIZE, so
> kernel aborts that may already hold the mmap lock are left unchanged.
> For user-mode faults on kernel addresses (addr >= TASK_SIZE), as
> Yuanbin noted, it is reasonable to skip show_pte().
>
> Please let us know if you see any issues with this approach, or if you
> would suggest a different way to handle it.
>

In general yeah but with mmap_write_trylock() after you add the attached
patch to a series (making sure to cc- the right people etc.)... but also,
wouldn't you just generally want this in show_pte()?

Seems to me best way is to put the existing show_pte() into a __show_pte()
and then do show_pte() like (untested top of my head thing):

void show_pte(const char *lvl, struct mm_struct *mm, unsigned long addr,
	      bool is_user)
{
	/* Write lock needed to account for concurrent downgraded munmap(). */
	if (is_user && !mmap_write_trylock(mm)) {
		printk("%s[%08lx] unable to acquire lock for PTE output\n",
			lvl, addr);
		return;
	}
	__show_pte(lvl, mm, addr);
	if (is_user)
		mmap_write_unlock(mm);
}

Cheers, Lorenzo

----8<----
From 2d0906c9ca962361fcdf2a033deb2a37252c486a Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <ljs@kernel.org>
Date: Sat, 11 Jul 2026 08:07:27 +0100
Subject: [PATCH] mm: add mmap_write_trylock()

Commit cf95e337cb63 ("mm: delete mmap_write_trylock() and
vma_try_start_write()") removed mmap_write_trylock() as there were no
users.

Re-add this helper and update to have it update the mm's write lock
sequence count number as per the current VMA lock implementation.

The use cases for this function are very narrow - in the vast majority of
cases an inability to acquire the write lock is terminal so you want either
an unconditional or killable variant of the mmap write lock.

However this is being added so a subsequent patch can safely dump PTE
information when an unhandled fault arises in arm32.

Signed-off-by: Lorenzo Stoakes <ljs@kernel.org>
---
 include/linux/mmap_lock.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
index 04b8f61ece5d..fe14d4babae9 100644
--- a/include/linux/mmap_lock.h
+++ b/include/linux/mmap_lock.h
@@ -546,6 +546,18 @@ static inline void mmap_write_lock_nested(struct mm_struct *mm, int subclass)
 	__mmap_lock_trace_acquire_returned(mm, true, true);
 }

+static inline bool __must_check mmap_write_trylock(struct mm_struct *mm)
+{
+	bool ret;
+
+	__mmap_lock_trace_start_locking(mm, true);
+	ret = down_write_trylock(&mm->mmap_lock) != 0;
+	if (ret)
+		mm_lock_seqcount_begin(mm);
+	__mmap_lock_trace_acquire_returned(mm, true, ret);
+	return ret;
+}
+
 static inline int __must_check mmap_write_lock_killable(struct mm_struct *mm)
 {
 	int ret;
--
2.55.0