On 5/12/21 1:14 PM, Peter Xu wrote: > On Wed, May 12, 2021 at 12:42:32PM -0700, Mina Almasry wrote: >>>>> @@ -4868,30 +4869,39 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, >>>>> + WARN_ON(*pagep); >>>> >>>> I don't think this warning works, because we do set *pagep, in the >>>> copy_huge_page_from_user failure case. In that case, the following >>>> happens: >>>> >>>> 1. We set *pagep, and return immediately. >>>> 2. Our caller notices this particular error, drops mmap_lock, and then >>>> calls us again with *pagep set. >>>> >>>> In this path, we're supposed to just re-use this existing *pagep >>>> instead of allocating a second new page. >>>> >>>> I think this also means we need to keep the "else" case where *pagep >>>> is set below. >>>> >>> >>> +1 to Peter's comment. >>> Apologies to Axel (and Peter) as that comment was from Axel. >> >> Gah, sorry about that. I'll fix in v2. > > I have a question regarding v1: how do you guarantee huge_add_to_page_cache() > won't fail again even if checked before page alloc? Say, what if the page > cache got inserted after hugetlbfs_pagecache_present() (which is newly added in > your v1) but before huge_add_to_page_cache()? In the caller (__mcopy_atomic_hugetlb) we obtain the hugetlb fault mutex before calling this routine. This should prevent changes to the cache while in the routine. However, things get complicated in the case where copy_huge_page_from_user fails. In this case, we will return to the caller which will drop mmap_lock and the hugetlb fault mutex before doing the copy. After dropping the mutex, someone could populate the cache. This would result in the same situation where two reserves are 'temporarily' consumed for the same mapping offset. By the time we get to the second call to hugetlb_mcopy_atomic_pte where the previously allocated page is passed in, it is too late. -- Mike Kravetz
