Re: [PATCH] mm/memfd: reserve hugetlb folios before allocation

[Steven Sistare <steven.sistare@oracle.com>]

On 1/10/2025 1:17 AM, Kasireddy, Vivek wrote:
> Hi Steve,
> 
>> Subject: Re: [PATCH] mm/memfd: reserve hugetlb folios before allocation
>>
>>>>> There are cases when we try to pin a folio but discover that it has
>>>>> not been faulted-in. So, we try to allocate it in memfd_alloc_folio()
>>>>> but there is a chance that we might encounter a crash/failure
>>>>> (VM_BUG_ON(!h->resv_huge_pages)) if there are no active reservations
>>>>> at that instant. This issue was reported by syzbot:
>>>>>
>>>>> kernel BUG at mm/hugetlb.c:2403!
>>>>> Oops: invalid opcode: 0000 [#1] PREEMPT SMP KASAN NOPTI
>>>>> CPU: 0 UID: 0 PID: 5315 Comm: syz.0.0 Not tainted
>>>>> 6.13.0-rc5-syzkaller-00161-g63676eefb7a0 #0
>>>>> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
>>>>> 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014
>>>>> RIP: 0010:alloc_hugetlb_folio_reserve+0xbc/0xc0 mm/hugetlb.c:2403
>>>>> Code: 1f eb 05 e8 56 18 a0 ff 48 c7 c7 40 56 61 8e e8 ba 21 cc 09 4c 89
>>>>> f0 5b 41 5c 41 5e 41 5f 5d c3 cc cc cc cc e8 35 18 a0 ff 90 <0f> 0b 66
>>>>> 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f
>>>>> RSP: 0018:ffffc9000d3d77f8 EFLAGS: 00010087
>>>>> RAX: ffffffff81ff6beb RBX: 0000000000000000 RCX: 0000000000100000
>>>>> RDX: ffffc9000e51a000 RSI: 00000000000003ec RDI: 00000000000003ed
>>>>> RBP: 1ffffffff34810d9 R08: ffffffff81ff6ba3 R09: 1ffffd4000093005
>>>>> R10: dffffc0000000000 R11: fffff94000093006 R12: dffffc0000000000
>>>>> R13: dffffc0000000000 R14: ffffea0000498000 R15: ffffffff9a4086c8
>>>>> FS:  00007f77ac12e6c0(0000) GS:ffff88801fc00000(0000)
>>>>> knlGS:0000000000000000
>>>>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>> CR2: 00007f77ab54b170 CR3: 0000000040b70000 CR4:
>> 0000000000352ef0
>>>>> DR0: 0000000000000000 DR1: 0000000000000000 DR2:
>> 0000000000000000
>>>>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>>>>> Call Trace:
>>>>>     <TASK>
>>>>>     memfd_alloc_folio+0x1bd/0x370 mm/memfd.c:88
>>>>>     memfd_pin_folios+0xf10/0x1570 mm/gup.c:3750
>>>>>     udmabuf_pin_folios drivers/dma-buf/udmabuf.c:346 [inline]
>>>>>     udmabuf_create+0x70e/0x10c0 drivers/dma-buf/udmabuf.c:443
>>>>>     udmabuf_ioctl_create drivers/dma-buf/udmabuf.c:495 [inline]
>>>>>     udmabuf_ioctl+0x301/0x4e0 drivers/dma-buf/udmabuf.c:526
>>>>>     vfs_ioctl fs/ioctl.c:51 [inline]
>>>>>     __do_sys_ioctl fs/ioctl.c:906 [inline]
>>>>>     __se_sys_ioctl+0xf5/0x170 fs/ioctl.c:892
>>>>>     do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>>>>>     do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
>>>>>     entry_SYSCALL_64_after_hwframe+0x77/0x7f
>>>>>
>>>>> Therefore, to avoid this situation and fix this issue, we just need
>>>>> to make a reservation before we try to allocate the folio. While at
>>>>> it, also remove the VM_BUG_ON() as there is no need to crash the
>>>>> system in this scenario and instead we could just fail the allocation.
>>>>>
>>>>> Fixes: 26a8ea80929c ("mm/hugetlb: fix memfd_pin_folios
>> resv_huge_pages
>>>> leak")
>>>>> Reported-by:
>> syzbot+a504cb5bae4fe117ba94@syzkaller.appspotmail.com
>>>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>>>> Cc: Steve Sistare <steven.sistare@oracle.com>
>>>>> Cc: Muchun Song <muchun.song@linux.dev>
>>>>> Cc: David Hildenbrand <david@redhat.com>
>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>> ---
>>>>>     mm/hugetlb.c | 9 ++++++---
>>>>>     mm/memfd.c   | 5 +++++
>>>>>     2 files changed, 11 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>>>> index c498874a7170..e46c461210a4 100644
>>>>> --- a/mm/hugetlb.c
>>>>> +++ b/mm/hugetlb.c
>>>>> @@ -2397,12 +2397,15 @@ struct folio
>> *alloc_hugetlb_folio_reserve(struct
>>>> hstate *h, int preferred_nid,
>>>>>     	struct folio *folio;
>>>>>
>>>>>     	spin_lock_irq(&hugetlb_lock);
>>>>> +	if (!h->resv_huge_pages) {
>>>>> +		spin_unlock_irq(&hugetlb_lock);
>>>>> +		return NULL;
>>>>> +	}
>>>>
>>>> This should be the entire fix, plus deleting the VM_BUG_ON.  See below.
>>>>
>>>>> +
>>>>>     	folio = dequeue_hugetlb_folio_nodemask(h, gfp_mask,
>>>> preferred_nid,
>>>>>     					       nmask);
>>>>> -	if (folio) {
>>>>> -		VM_BUG_ON(!h->resv_huge_pages);
>>>>> +	if (folio)
>>>>>     		h->resv_huge_pages--;
>>>>> -	}
>>>>>
>>>>>     	spin_unlock_irq(&hugetlb_lock);
>>>>>     	return folio;
>>>>> diff --git a/mm/memfd.c b/mm/memfd.c
>>>>> index 35a370d75c9a..a3012c444285 100644
>>>>> --- a/mm/memfd.c
>>>>> +++ b/mm/memfd.c
>>>>> @@ -85,6 +85,10 @@ struct folio *memfd_alloc_folio(struct file
>> *memfd,
>>>> pgoff_t idx)
>>>>>     		gfp_mask &= ~(__GFP_HIGHMEM | __GFP_MOVABLE);
>>>>>     		idx >>= huge_page_order(h);
>>>>>
>>>>> +		if (!hugetlb_reserve_pages(file_inode(memfd),
>>>>> +					   idx, idx + 1, NULL, 0))
>>>>> +			return ERR_PTR(-ENOMEM);
>>>>
>>>> I believe it is wrong to force a reservation here.
>>> Is there any particular reason why you believe a reservation here would be
>> wrong?
>>> AFAICS, at the moment, we are not doing any region/subpool accounting
>> before
>>> our folio allocation and this gets flagged in the form of elevated
>> resv_huge_pages
>>> value (hugetlb_acct_memory() elevates it based on the return value of
>> region_del())
>>> when hugetlb_unreserve_pages() eventually gets called.
>>>
>>>> Pages should have already been
>>>> reserved at this point, eg by calls from hugetlbfs_file_mmap or
>> hugetlb_file_setup.
>>> hugetlb_file_setup() does not reserve any pages as it passes in
>> VM_NORESERVE.
>>> And, the case we are trying to address is exactly when
>> hugetlbfs_file_mmap() does
>>> not get called before pinning.
>>
>> But you must not break the case where hugetlbfs_file_mmap was called first, which
> AFAICS, this patch does not break the primary use-case where hugetlbfs_file_mmap()
> gets called first.
> 
>> reserves, then memfd_alloc_folio is called, which reserves again with your fix. Does
>> that work correctly, or do the counts go bad?
> Yes it works correctly and ` cat /proc/meminfo|grep Huge` does not show any
> unexpected counts.
> 
>>
>>> So, when hugetlbfs_file_mmap() does eventually
>>> get called, I don't see any problem if it calls hugetlb_reserve_pages() again
>> for the
>>> same range or overlapping ranges.
>>
>> Does that work correctly, or do the counts go bad?
> It works correctly as expected. And, as mentioned to David in another email,
> if a range is already reserved, and when hugetlb_reserve_pages() gets called
> again for the same range (in this use-case), it would mostly become a no-op
> as region_chg() and region_add() return 0 (based on my light testing).

Great, thanks.

>> Please try those scenarios with your test program:  mmap +
>> memfd_alloc_folio, and
>> memfd_alloc_folio + mmap.
> Here are the scenarios I tried (with the calls in the exact order as below):
> 1) hugetlbfs_file_mmap()
>      memfd_alloc_folio()
>      hugetlb_fault()
> 
> 2) memfd_alloc_folio()
>      hugetlbfs_file_mmap()
>      hugetlb_fault()
> 
> 3) hugetlbfs_file_mmap()
>      hugetlb_fault()
>          alloc_hugetlb_folio()
> 
> memfd_alloc_folio() does not get called in the last case as the folio is already
> allocated (and is present in the page cache). While testing all these cases, I
> did not notice any splats or messed-up counts.
> 
> If it is not too much trouble, could you please try this patch with your test-cases?

Done, and all tests pass.  I will review your V2 patch.
Thanks for working on this bug.

- Steve

>>>> syzcaller has forced its way down this path without calling those pre-
>> requisites,
>>>> doing weird stuff as it should.
>>> This issue is not very hard to reproduce. If we have free_huge_pages > 0
>> and
>>> resv_huge_pages = 0, and then we call memfd_pin_folios() before mmap()/
>>> hugetlbfs_file_mmap() we can easily encounter this issue. Furthermore, we
>>> should be able to allocate a folio in this scenario (as available_huge_pages
>>> 0),
>>> which we would not be able to do if we don't call hugetlb_reserve_pages().
>>> Note that hugetlb_reserve_pages() actually elevates resv_huge_pages in
>>> this case and kind of gives a go-ahead for the allocation.
>>>
>>> I have used a slightly modified udmabuf selftest to reproduce this issue
>> which
>>> I'll send out as part of v2.
>>>
>>> Thanks,
>>> Vivek
>>>
>>>>
>>>> To fix, I suggest you simply fix alloc_hugetlb_folio_reserve as above.
>>>>
>>>> - Steve
>>>>
>>>>> +
>>>>>     		folio = alloc_hugetlb_folio_reserve(h,
>>>>>     						    numa_node_id(),
>>>>>     						    NULL,
>>>>> @@ -100,6 +104,7 @@ struct folio *memfd_alloc_folio(struct file
>> *memfd,
>>>> pgoff_t idx)
>>>>>     			folio_unlock(folio);
>>>>>     			return folio;
>>>>>     		}
>>>>> +		hugetlb_unreserve_pages(file_inode(memfd), idx, idx + 1, 1);
>>>>>     		return ERR_PTR(-ENOMEM);
>>>>>     	}
>>>>>     #endif
>>>
>