Re: [PATCH 6/6] x86/mm: Avoid mmap lock for shadow stack pop fast path

[Dave Hansen]

On 5/4/26 16:15, Edgecombe, Rick P wrote:
> 
> I guess the problem is the lock ordering. Not sure if there is any slow path
> avoidance details that could make this splat a false positive. But how about
> this simpler munmap() case:
> 
> Shadow stack signal                          munmap()
> -------------------                          --------
> vma_start_read() (VM_SHADOW_STACK check)
>                                              mmap_write_lock()
> mmap_read_lock() (user fault) <- deadlock
>                                              vma_start_write() <-deadlock

It's a little more complicated than that in practice, but I think you're
right.

I'm not sure when this would happen in practice because the fault is
actually on the VMA that's being held for read. So I think another
writer would have had to sneak in there and zap the VMA.

The funny thing is that the fault handler is really just trying to find
the VMA. The thing causing the fault *has* the VMA. So it's as simple as
just passing the VMA down into the fault handler, right? How hard could
it be? ;)

There are still games to play, but they all involve dropping locks and
retrying, like:

retry:
	vma = lock_vma_under_rcu()
	// muck with VMA
	pagefault_disable() // avoid deadlock
	ret = copy_from_user()
	pagefault_enable()
	vma_end_read();

	if (!ret)
		return SUCCESS;

	mmap_read_lock()
	vma = vma_lookup()
	mmap_read_unlock() // avoid deadlock before touching userspace
	// check for valid VMA to avoid looping when there is no VMA
	if (!vma)
		return -ERRNO;

	// uh oh, slow path, something faulted
	get_user_pages()??
	//or
	copy_from_user() without the VMA??

	goto retry;


This also needs some very careful thought, but something like this
should work, where we avoid fault handling (and lock taking) in the
actual #PF and do it in a context where the VMA lock is held:

	vma = lock_vma_under_rcu();
	pagefault_disable() // avoid deadlock

	while (1) {
		ret = copy_from_user()
		if (!ret)
			break;
		handle_mm_fault(vma, address, FAULT_FLAG_VMA_LOCK...);
	};

	pagefault_enable()
	vma_end_read();

That's effectively just short-circuiting the #PF code which does the:

        vma = lock_vma_under_rcu(mm, address);
	...
        fault = handle_mm_fault(vma, address, ... FAULT_FLAG_VMA_LOCK)

sequence _itself_.