Re: [syzbot] WARNING in hugetlb_wp

[David Hildenbrand <david@redhat.com>]

On 30.10.22 09:53, David Hildenbrand wrote:
> On 30.10.22 02:35, Mike Kravetz wrote:
>> On 10/28/22 22:15, syzbot wrote:
>>> Hello,
>>>
>>> syzbot found the following issue on:
>>>
>>> HEAD commit:    247f34f7b803 Linux 6.1-rc2
>>> git tree:       upstream
>>> console output: https://syzkaller.appspot.com/x/log.txt?x=14a9efd2880000
>>> kernel config:  https://syzkaller.appspot.com/x/.config?x=a66c6c673fb555e8
>>> dashboard link: https://syzkaller.appspot.com/bug?extid=f0b97304ef90f0d0b1dc
>>> compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
>>> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=112217b4880000
>>> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=105f4616880000
>>>
>>> Downloadable assets:
>>> disk image: https://storage.googleapis.com/syzbot-assets/de212436b09b/disk-247f34f7.raw.xz
>>> vmlinux: https://storage.googleapis.com/syzbot-assets/63c4feda220f/vmlinux-247f34f7.xz
>>>
>>> IMPORTANT: if you fix the issue, please add the following tag to the commit:
>>> Reported-by: syzbot+f0b97304ef90f0d0b1dc@syzkaller.appspotmail.com
>>>
>>> ------------[ cut here ]------------
>>> WARNING: CPU: 1 PID: 3612 at mm/hugetlb.c:5313 hugetlb_wp+0x20a/0x1af0 mm/hugetlb.c:5313
>>
>> This warning was added with commit 1d8d14641fd94 ("mm/hugetlb: support write-faults
>> in shared mappings").  It is there 'by design' as this this specific
>> type of write fault is not supported.
>>
>> 	/*
>> 	 * hugetlb does not support FOLL_FORCE-style write faults that keep the
>> 	 * PTE mapped R/O such as maybe_mkwrite() would do.
>> 	 */
>> 	if (WARN_ON_ONCE(!unshare && !(vma->vm_flags & VM_WRITE)))
>> 		return VM_FAULT_SIGSEGV;
>>
>> If there is an actual use case for this support, we can look at adding it.
> 
> Right, it's by design and in retrospective it was the right approach to add this
> check there. We seem to have a way to trigger a hugetlb write
> fault without VM_WRITE set from GUP. We have to fence that.
> 
> 
> Interestingly, I thought I tried to trigger that exact scenario.
> 
> a) Have a MAP_PRIVATE, PROT_READ hugetlb mapping
> b) Try writing to it via /proc/self/mem, triggering debug access with FOLL_FORCE
> 
> The expectation is that this will fail with -EFAULT on hugetlb. I could have
> sworn that it did the right thing when I tried :)
> 
> 
> But staring at follow_hugetlb_page(), I think we will end up triggering a
> write fault (FAULT_FLAG_WRITE) on hugetlb.
> 
> 
> The easiest fix might be to special-case hugetlb VMA in check_vma_flags():
> 
> 
>   From 39d2a525cae62e7d2766ecfc4337ed4d59555d9d Mon Sep 17 00:00:00 2001
> From: David Hildenbrand <david@redhat.com>
> Date: Sun, 30 Oct 2022 09:45:50 +0100
> Subject: [PATCH] mm/gup: disallow FOLL_FORCE on hugetlb mappings
> 
> TODO
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>    mm/gup.c | 3 +++
>    1 file changed, 3 insertions(+)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index fe195d47de74..b934687efdec 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1076,6 +1076,9 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
>    			 */
>    			if (!is_cow_mapping(vm_flags))
>    				return -EFAULT;
> +			/* hugetlb does not support FOLL_FORCE. */
> +			if (is_vm_hugetlb_page(vma))
> +				return -EFAULT;
>    		}
>    	} else if (!(vm_flags & VM_READ)) {
>    		if (!(gup_flags & FOLL_FORCE))


A simple reproducer:


#include <stdio.h>
#include <stdlib.h>
#include <fcntl.h>
#include <unistd.h>
#include <errno.h>
#include <stdint.h>
#include <sys/mman.h>
#include <linux/mman.h>

int main(int argc, char **argv)
{
         char *map;
         int mem_fd;

         map = mmap(NULL, 2 * 1024 * 1024u, PROT_READ,
                    MAP_PRIVATE|MAP_ANON|MAP_HUGETLB|MAP_HUGE_2MB, -1, 0);
         if (map == MAP_FAILED) {
                 fprintf(stderr, "mmap() failed: %d\n", errno);
                 return 1;
         }

         mem_fd = open("/proc/self/mem", O_RDWR);
         if (mem_fd < 0) {
                 fprintf(stderr, "open(/proc/self/mem) failed: %d\n", errno);
                 return 1;
         }

         if (pwrite(mem_fd, "0", 1, (uintptr_t) map) == 1) {
                 fprintf(stderr, "write() succeeded, which is unexpected\n");
                 return 1;
         }

         printf("write() failed as expected: %d\n", errno);
         return 0;
}



I started looking at the follow_hugetlb_page() call in __get_user_pages() and
my head seriously started to hurt.

Why are we storing to "i" and error from follow_hugetlb_page()->hugetlb_fault()
and then eventually happily continuing?

I'm afraid of touching that code, it looks too fragile.

Hopefully I am missing something important and it's all perfectly
fine code.


-- 
Thanks,

David / dhildenb