On 11/2/24 4:07 AM, Andrew Morton wrote:
> On Fri, 01 Nov 2024 21:08:02 +0900 Asahi Lina <lina@xxxxxxxxxxxxx> 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
