Received-SPF: pass (google.com: domain of linux-kernel+bounces-190664-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <9f3371fa-0d46-4e93-a00b-0f5cb30b58bb@redhat.com>
Date: Mon, 27 May 2024 15:17:01 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] mm/hugetlb: Move vmf_anon_prepare upfront in hugetlb_wp
To: Oscar Salvador <osalvador@suse.de>,
 Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
 Muchun Song <muchun.song@linux.dev>, Vishal Moola <vishal.moola@gmail.com>
References: <20240521073446.23185-1-osalvador@suse.de>
 <ZlRKI_tJ2CYhmekw@localhost.localdomain>
From: David Hildenbrand <dhildenb@redhat.com>
Content-Language: en-US
In-Reply-To: <ZlRKI_tJ2CYhmekw@localhost.localdomain>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


Am 27.05.24 um 10:53 schrieb Oscar Salvador:
> On Tue, May 21, 2024 at 09:34:46AM +0200, Oscar Salvador wrote:
>> I did not hit this bug, I just spotted this because I was looking at hugetlb_wp
>> for some other reason. And I did not want to get creative to see if I could
>> trigger this so I could get a backtrace.
>> My assumption is that we could trigger this if 1) this was a shared mapping,
>> so no anon_vma and 2) we call in GUP code with FOLL_WRITE, which would cause
>> the FLAG_UNSHARE to be passed, so we will end up in hugetlb_wp().
> 
> So I checked this again and I have to confess I am bit confused.
> 
> hugetlb_wp() can be called from either hugetlb_fault() or hugetlb_no_page().
> 
> hugetlb_fault()->hugetlb_wp() upon FAULT_FLAG_{WRITE,UNSHARE}
> hugetlb_no_page->hugetlb_wp()-> upon FAULT_FLAG_WRITE && !VM_SHARED
> 
> hugetlb_no_page()->vmf_anon_prepare() upon !VM_SHARED, which means that VM_SHARED
> mappings do not have vma->anon_vma, while others do.
> 
> hugetlb_wp() will call set_huge_ptep_writable() right away and return if it sees
> that the mapping is shared.
> So the only other we have to end up in hugetlb_wp() is via FAULT_FLAG_UNSHARE.
> For that to happen gup_must_unshare() must return true, which means the following
> assumptions must hold.
> 
> - For Anonymous pages:
>    1) !PageAnonExclusive
> - For Filebacked pages:
>    2) We do not have a vma
>    3) It is a COW mapping
> 
> 1) If gup_must_unshare() returns true for Anonymous pages because the page is not
>     exclusive and must be unshared, hugetlb_wp() will already see the
>     vma->anon_prepare being initialized because of the previous
>     hugetlb_no_page()->vmf_anon_prepare.
> 
> 2) I do not quite understand this case.

gup_must_unshare() without a VMA is only used for GUP-fast. Before triggering a 
page fault we always fallback to GUP-slow first, where we have a VMA.

IMHO we better not make assumptions that hugetlb_wp() will always already have 
an anon VMA.

-- 
Thanks,

David / dhildenb