Re: [PATCH] mm: Fix __wp_page_copy_user fallback path for remote mm

[Asahi Lina <lina@asahilina.net>]

On 11/2/24 4:07 AM, Andrew Morton wrote:
> On Fri, 01 Nov 2024 21:08:02 +0900 Asahi Lina <lina@asahilina.net> wrote:
> 
>> If the source page is a PFN mapping, we copy back from userspace.
>> However, if this fault is a remote access, we cannot use
>> __copy_from_user_inatomic. Instead, use access_remote_vm() in this case.
>>
>> Fixes WARN and incorrect zero-filling when writing to CoW mappings in
>> a remote process, such as when using gdb on a binary present on a DAX
>> filesystem.
>>
>> [  143.683782] ------------[ cut here ]------------
>> [  143.683784] WARNING: CPU: 1 PID: 350 at mm/memory.c:2904 __wp_page_copy_user+0x120/0x2bc
>>
>> ...
>>
> 
> Thanks.  I assume we should backport this into earlier kernels?
> 
> If so, a Fixes: target is desired, to tell people how far back in time
> it should be ported.

I think so? I'm not sure how back the bug goes though, possibly a long
time...

> I think it's
> 
> 83d116c53058 ("mm: fix double page fault on arm64 if PTE_AF is cleared").

That doesn't sound right. The old code prior to the patch still had the
__copy_from_user_inatomic() fallback path so it should still have the
same problem. That fallback goes back to:

  6aab341e0a28 ("mm: re-architect the VM_UNPAGED logic")

But the ptrace code back then doesn't seem to be using that codepath at
all, so that's meaningless. I think this is the proper tag:

  3565fce3a659 ("mm, x86: get_user_pages() for dax mappings")

That's when GUP started working for DAX mappings at all, and if my
reading of the code is correct, at that point do_wp_page() was only
grabbing the struct page for normal pages to pass to wp_page_copy()
(triggering the fallback path for DAX mappings). The code has moved
around a lot today but has the same logic, so I think it's been broken
since then.

Should I resend it with the Fixes tag?

> 
> 
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -3081,13 +3081,18 @@ static inline int __wp_page_copy_user(struct page *dst, struct page *src,
>>  			update_mmu_cache_range(vmf, vma, addr, vmf->pte, 1);
>>  	}
>>  
>> +	/* If the mm is a remote mm, copy in the page using access_remote_vm() */
>> +	if (current->mm != mm) {
>> +		if (access_remote_vm(mm, (unsigned long)uaddr, kaddr, PAGE_SIZE, 0) != PAGE_SIZE)
>> +			goto warn;
>> +	}
>>  	/*
>>  	 * This really shouldn't fail, because the page is there
>>  	 * in the page tables. But it might just be unreadable,
>>  	 * in which case we just give up and fill the result with
>>  	 * zeroes.
>>  	 */
>> -	if (__copy_from_user_inatomic(kaddr, uaddr, PAGE_SIZE)) {
>> +	else if (__copy_from_user_inatomic(kaddr, uaddr, PAGE_SIZE)) {
>>  		if (vmf->pte)
>>  			goto warn;
>>  
> 
> The coding style ends up being unconventional.  I made these changes:
> 
> --- a/mm/memory.c~mm-fix-__wp_page_copy_user-fallback-path-for-remote-mm-fix
> +++ a/mm/memory.c
> @@ -3081,18 +3081,20 @@ static inline int __wp_page_copy_user(st
>  			update_mmu_cache_range(vmf, vma, addr, vmf->pte, 1);
>  	}
>  
> -	/* If the mm is a remote mm, copy in the page using access_remote_vm() */
> -	if (current->mm != mm) {
> -		if (access_remote_vm(mm, (unsigned long)uaddr, kaddr, PAGE_SIZE, 0) != PAGE_SIZE)
> -			goto warn;
> -	}
>  	/*
> -	 * This really shouldn't fail, because the page is there
> -	 * in the page tables. But it might just be unreadable,
> -	 * in which case we just give up and fill the result with
> -	 * zeroes.
> +	 * If the mm is a remote mm, copy in the page using access_remote_vm()
>  	 */
> -	else if (__copy_from_user_inatomic(kaddr, uaddr, PAGE_SIZE)) {
> +	if (current->mm != mm) {
> +		if (access_remote_vm(mm, (unsigned long)uaddr, kaddr,
> +				     PAGE_SIZE, 0) != PAGE_SIZE)
> +			goto warn;
> +	} else if (__copy_from_user_inatomic(kaddr, uaddr, PAGE_SIZE)) {
> +		/*
> +		 * This really shouldn't fail, because the page is there
> +		 * in the page tables. But it might just be unreadable,
> +		 * in which case we just give up and fill the result with
> +		 * zeroes.
> +		 */
>  		if (vmf->pte)
>  			goto warn;
>  
> _
> 
> I'll queue this for testing and shall await further review.
> 

~~ Lina