On Fri, May 23, 2025 at 10:54:24AM +0200, Oscar Salvador wrote:
On Fri, May 23, 2025 at 09:56:18AM +0200, Ricardo Cañuelo Navarro wrote:
If, during a mremap() operation for a hugetlb-backed memory mapping, copy_vma() fails after the source vma has been duplicated and opened (ie. vma_link() fails), the error is handled by closing the new vma. This updates the hugetlbfs reservation counter of the reservation map which at this point is referenced by both the source vma and the new copy. As a result, once the new vma has been freed and copy_vma() returns, the reservation counter for the source vma will be incorrect.
This patch addresses this corner case by clearing the hugetlb private page reservation reference for the new vma and decrementing the reference before closing the vma, so that vma_close() won't update the reservation counter.
The issue was reported by a private syzbot instance, see the error report log [1] and reproducer [2]. Possible duplicate of public syzbot report [3].
Signed-off-by: Ricardo Cañuelo Navarro rcn@igalia.com Cc: stable@vger.kernel.org # 6.12+ Link: https://people.igalia.com/rcn/kernel_logs/20250422__WARNING_in_page_counter_... [1] Link: https://people.igalia.com/rcn/kernel_logs/20250422__WARNING_in_page_counter_... [2] Link: https://lore.kernel.org/all/67000a50.050a0220.49194.048d.GAE@google.com/ [3]
mm/vma.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/mm/vma.c b/mm/vma.c index 839d12f02c885d3338d8d233583eb302d82bb80b..9d9f699ace977c9c869e5da5f88f12be183adcfb 100644 --- a/mm/vma.c +++ b/mm/vma.c @@ -1834,6 +1834,8 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap, return new_vma;
out_vma_link:
if (is_vm_hugetlb_page(new_vma))
clear_vma_resv_huge_pages(new_vma);vma_close(new_vma);
if (new_vma->vm_file)
Sigh, I do not think Lorenzo will be happy about having yet another hugetlb check around vma code :-).
NAAAA... nah only joking ;)
But you do know me well sir, and indeed I hate doing this here.
Maybe this is good as is as a quick fix, but we really need to re-assest this situation
Yes. We really need to find a way to avoid arbitrarily putting in branches like:
if ([hugetlb/uffd/dax/blah blah]) some_seemingly_random_thing_that_is_an_implementation_detail();
. I will have a look once I managed to finish a couple of other things.
Thanks.
-- Oscar Salvador SUSE Labs
Let me have a look at this fix-wise as obviously that's a priority here and we might need to live with some ugliness for that reason.
But I want to make sure this is correct (I mean it kind of rings true).
Can you confirm at least on the theoretical side this makes sense? I was actually going to cc you if you weren't already (hadn't checked yet :)
Cheers, Lorenzo