From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id C7747C04A6A for ; Wed, 2 Aug 2023 14:59:20 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 64F10280187; Wed, 2 Aug 2023 10:59:20 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5FEF0280143; Wed, 2 Aug 2023 10:59:20 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4EE54280187; Wed, 2 Aug 2023 10:59:20 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 3D335280143 for ; Wed, 2 Aug 2023 10:59:20 -0400 (EDT) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id E1496140690 for ; Wed, 2 Aug 2023 14:59:19 +0000 (UTC) X-FDA: 81079472838.10.C6B8D14 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf20.hostedemail.com (Postfix) with ESMTP id 9CC2A1C002A for ; Wed, 2 Aug 2023 14:59:17 +0000 (UTC) Authentication-Results: imf20.hostedemail.com; dkim=none; spf=pass (imf20.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1690988358; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=f2cf0LMnp6GIj8Qdfbfx5zHqQUebehBwg78S/KXRqtc=; b=IgqkcqQZT2jmXGqa5wo6Mftwgnzs2b7nYTr7Ng3eaXRGU4vufW99IJ6lUJEEXqrORGaOyS 30kQMgW0R90UeZxNr0efM070P7WJlzfaUelVPIG/vUNy3goUKHYTT6YzB5lc7hyweiV87L gDKpmJ2m54+P4hsF4BJl617JFvoWFXo= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1690988358; a=rsa-sha256; cv=none; b=HHy0Yv+LaLUVyBR3sDWInRsUe+LvpN0uw4+I4go2djLxpreYYuV3SiCU0jCNByI8tjalyR Ffps76oE9MbK59otQD338AYI+RawmACsc0RzCRJwshWG0gJUXLgCrI+pDY070ZsElILC9J IWOi7Y0914o3vHs+hRzpBnuGJZcSzKA= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=none; spf=pass (imf20.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id DAA73113E; Wed, 2 Aug 2023 07:59:59 -0700 (PDT) Received: from [10.57.77.90] (unknown [10.57.77.90]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 3AEDB3F5A1; Wed, 2 Aug 2023 07:59:15 -0700 (PDT) Message-ID: Date: Wed, 2 Aug 2023 15:59:13 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH 1/3] mm: add functions folio_in_range() and folio_within_vma() To: "Yin, Fengwei" , linux-mm@kvack.org, linux-kernel@vger.kernel.org, akpm@linux-foundation.org, yuzhao@google.com, willy@infradead.org, david@redhat.com, shy828301@gmail.com, hughd@google.com References: <20230728070929.2487065-1-fengwei.yin@intel.com> <20230728070929.2487065-2-fengwei.yin@intel.com> <55c9e3f7-099d-6f57-32da-1f318a9688a0@arm.com> <65a36b41-d69e-4072-cfd2-253ed6e4e040@arm.com> <286cbca6-ab5e-ad06-ea2a-89ea08ee53d4@intel.com> <8dcf002a-088e-32de-7868-5dc5ca6b1206@arm.com> <6f5301b4-57f5-bcef-45d4-68b0efbd4e67@intel.com> From: Ryan Roberts In-Reply-To: <6f5301b4-57f5-bcef-45d4-68b0efbd4e67@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 9CC2A1C002A X-Rspam-User: X-Stat-Signature: jzqab3pm5u4rgwgkywm78bazim57ype1 X-Rspamd-Server: rspam03 X-HE-Tag: 1690988357-598209 X-HE-Meta: U2FsdGVkX1+HyTLAgnWRcFdlq94kdo/9RJI8Hw7le7b1Ob7dNdRGVF2GRv2IsGnV9b/uW1mHf7zkqlSpkY2OGRRsBjsRip/TikywmjNISLNV+z7RXF7tmHfjb+Ki4UJKVl5OJ5fzIthOEEspipE4p/3UmXQcs0SBrMm2P16SJzuN8pY7dKK0qkeeUIEJqCanRBLuScY2JA7BMhQ5yI+cmk9Ysfu8fM7t9ebfquMCxyMZyab6NkP1stbONenlIl/bOdvNMpdxCqG8M0gfnNWuYlMRU99H/k/UjuVUriGI2suQvlqOQLJ6+ONrPzOnJ2m56W9fv51QIcNAwgBzTQx28rdegoTsnvnLlhmcCt1AH/9EvfsmKVEVwS49zD0Y4GVVomy1/FEGMnArZGqgJ6KwXUG3q4aYqP407vyKS1yqFC5ykevBfiwDxaR7uAgVSZX+wMec23dpdyTQYVUh234hrLmSRv9JXQpGC80Cgk2MERVvvY1ceWp0J17N7CPMCasjjbj3IfC9+AsaOFZNYvEbAo46TH5j9ekhdSmRgbTMEVi0m+7LWAPOfsTV3i2FnyuW1gCUHgjFwCxcczaHNG/gW8mGTjqUWXFkWsFJcBJe8IVfyOe3CUj2Oo2XKpkQFX8JsN9dfu20hVnMENKO/HVV8kxVkowcJBNZpyEXljZ8XuWZlRF8UwuPG1ZtPtMcyG17DgW/pQ0d9P728TT3Ux6cIJviVxWGcv+Vs8wgINR3dCp+Zo58Sx0mfJo5oY3N/E/uOGAPfyWB1gwQm7j475+vChacdnDM7mI5abLosbPdPgW/x199NjWRwkbABS+0gm7ndgFm/jQfGwdjcb/8dKM4i38tqUSmU4BNqXnwYJa5rSOglek41+uftQUtuUdux/nHq13/XNvdldYqUV6H5XdGyy+NU/mconv+SxVuYNq3EQ+DcNW12FDJBtwH4/I7Wtyt6LKLWm4MheBMyhdDRYv 6IwoHuFf 7PdRPsawnAWP793Gajlq62IYHrCOtwFvxqxYRyz3whwccRWBYJMXENoTVZn0UJhg63EWnSJODIGbGzvGyHyGKXRluEawACZfIQy+DMHBLMSykU0gbJ0u8JgpsKMSZ/RXiS8r0TsWOtWbG9bHkaQfULXQHF8FsptS5MXi9ccta0dBLn4H9w27Xpxvhxg== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 02/08/2023 15:14, Yin, Fengwei wrote: > > > On 8/2/2023 10:08 PM, Ryan Roberts wrote: >> On 02/08/2023 14:46, Yin, Fengwei wrote: >>> >>> >>> On 8/2/2023 9:09 PM, Ryan Roberts wrote: >>>> On 02/08/2023 13:50, Yin, Fengwei wrote: >>>>> >>>>> >>>>> On 8/2/2023 7:14 PM, Ryan Roberts wrote: >>>>>> On 28/07/2023 08:09, Yin Fengwei wrote: >>>>>>> It will be used to check whether the folio is mapped to specific >>>>>>> VMA and whether the mapping address of folio is in the range. >>>>>>> >>>>>>> Also a helper function folio_within_vma() to check whether folio >>>>>>> is in the range of vma based on folio_in_range(). >>>>>>> >>>>>>> Signed-off-by: Yin Fengwei >>>>>>> --- >>>>>>> mm/internal.h | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>> 1 file changed, 69 insertions(+) >>>>>>> >>>>>>> diff --git a/mm/internal.h b/mm/internal.h >>>>>>> index 5a03bc4782a2..63de32154a48 100644 >>>>>>> --- a/mm/internal.h >>>>>>> +++ b/mm/internal.h >>>>>>> @@ -585,6 +585,75 @@ extern long faultin_vma_page_range(struct vm_area_struct *vma, >>>>>>> bool write, int *locked); >>>>>>> extern bool mlock_future_ok(struct mm_struct *mm, unsigned long flags, >>>>>>> unsigned long bytes); >>>>>>> + >>>>>>> +/* >>>>>>> + * Check whether the folio is in specific range >>>>>>> + * >>>>>>> + * First, check whether the folio is in the range of vma. >>>>>>> + * Then, check whether the folio is mapped to the range of [start, end]. >>>>>>> + * In the end, check whether the folio is fully mapped to the range. >>>>>>> + * >>>>>>> + * @pte page table pointer will be checked whether the large folio >>>>>>> + * is fully mapped to. Currently, if mremap in the middle of >>>>>>> + * large folio, the large folio could be mapped to to different >>>>>>> + * VMA and address check can't identify this situation. >>>>>>> + */ >>>>>>> +static inline bool >>>>>>> +folio_in_range(struct folio *folio, struct vm_area_struct *vma, >>>>>>> + unsigned long start, unsigned long end, pte_t *pte) >>>>>> >>>>>> This api seems a bit redundant to me. Wouldn't it be better to remove the vma >>>>>> parameter and instead fix up the start/end addresses in folio_within_vma()? >>>>> My understanding is it's necessary. As for madvise, we need to check whether >>>>> the folio is both in the range of VMA and also in the range of [start, end). >>>> >>>> But in folio_within_vma() you pass start as vma->vm_start and end as >>>> vma->vm_end. And in this function, you narrow start/end to be completely >>>> contained in vma. So surely there is only really one start/end you are >>>> interested in? Just seems a bit odd to me. >>> madvise() will call filio_in_range() with VMA and real range [start, end) passed >>> from user space. >>> >>>> >>>>> >>>>>> >>>>>>> +{ >>>>>>> + pte_t ptent; >>>>>>> + unsigned long i, nr = folio_nr_pages(folio); >>>>>>> + pgoff_t pgoff, addr; >>>>>>> + unsigned long vma_pglen = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; >>>>>>> + >>>>>>> + VM_WARN_ON_FOLIO(folio_test_ksm(folio), folio); >>>>>>> + >>>>>>> + if (start < vma->vm_start) >>>>>>> + start = vma->vm_start; >>>>>>> + if (end > vma->vm_end) >>>>>>> + end = vma->vm_end; >>>>>>> + >>>>>>> + pgoff = folio_pgoff(folio); >>>>>>> + /* if folio start address is not in vma range */ >>>>>>> + if (pgoff < vma->vm_pgoff || pgoff > vma->vm_pgoff + vma_pglen) >>>>>>> + return false; >>>>>>> + >>>>>>> + addr = vma->vm_start + ((pgoff - vma->vm_pgoff) << PAGE_SHIFT); >>>>>>> + if (addr < start || end - addr < folio_size(folio)) >>>>>>> + return false; >>>>>>> + >>>>>>> + /* not necessary to check pte for none large folio */ >>>>>>> + if (!folio_test_large(folio)) >>>>>>> + return true; >>>>>>> + >>>>>>> + if (!pte) >>>>>>> + return false; >>>>>>> + >>>>>>> + /* check whether parameter pte is associated with folio */ >>>>>>> + ptent = ptep_get(pte); >>>>>>> + if (pte_none(ptent) || !pte_present(ptent) || >>>>>>> + pte_pfn(ptent) - folio_pfn(folio) >= nr) >>>>>>> + return false; >>>>>>> + >>>>>>> + pte -= pte_pfn(ptent) - folio_pfn(folio); >>>>>>> + for (i = 0; i < nr; i++, pte++) { >>>>>>> + ptent = ptep_get(pte); >>>>>>> + >>>>>>> + if (pte_none(ptent) || !pte_present(ptent) || >>>>>>> + pte_pfn(ptent) - folio_pfn(folio) >= nr) >>>>>>> + return false; >>>>>>> + } >>>>>> >>>>>> I don't think I see anything to ensure you don't wander off the end (or start) >>>>>> of the pgtable? If the folio is mremapped so that it straddles multiple tables >>>>>> (or is bigger than a single table?) then I think pte can become invalid? Perhaps >>>>>> you intended start/end to always be within the same pgtable, but that is not >>>>>> guarranteed in the case that folio_within_vma() is making the call. >>>>> If pte is invalid for any reason (pass wrong parameter, not fully mapped etc), this >>>>> function just return false in page table entry check phase. >>>> >>>> Sorry I don't think this covers the issue I'm describing. If you have a >>>> pte-mapped THP that gets mremapped to straddle 2 pte tables, don't you have a >>>> problem? >>>> >>>> example for 4K base page set up: >>>> >>>> folio_nr_pages = 512 >>>> first page of folio mapped at vaddr = 2M - 4K = 0x1FF000 >>>> >>>> If you then call this function with the pte pointer for the second page in the >>>> folio, which is mapped at address 0x200000, that pte is pointing to the first >>>> pte entry in the table pointed to by the second pmd entry. The pte pointer can >>>> be legitimately manipulated to point to any entry within that table, >>>> corrsponding to vaddrs [0x200000, 0x400000). But you will end up subtracting 1 >>>> from the pointer, intending that it now points to the pte entry that represents >>>> vaddr 0x1FF000. But actually it has fallen off the front of the table into some >>>> other arbitrary memory in the linear map. 0x1FF000 is represented in a different >>>> table, pointed to by the first pmd entry. >>> Yes. This can be an issue as hold the second page table lock can't prevent the first >>> part unmapped. Let me add another check vaddr align to folio_size in next version. >> >> Locking is a problem but its not the only problem. The 2 tables are almost >> certainly not contiguous in virtual memory. So once you have moved the pointer >> to before the start of the second table, then you are pointing to arbitrary memory. > If vaddr is aligned to folio_size, suppose we are OK here (I have assumption that > large folio will not be larger than PMD size. Or it's possible on ARM platform?). I *think* your assumption that a folio will never be bigger than PMD size is ok. (I'm guessing page cache never allocates bigger folios than that?). But its a bad assumption to assume folios are always mapped in a naturally aligned manner. mremapping a thp will cause non-natural alignment. User space requesting a file (that is in a large folio in pagecache) to be mapped to arbitrary (page-aligned) address will do that. > > > Regards > Yin, Fengwei > >> >>> >>> Regards >>> Yin, Fengwei >>> >>>> >>>> >>>>> >>>>>> >>>>>> Also I want to check that this function is definitely always called under the >>>>>> PTL for the table that pte belongs to? >>>>> Yes. I should spell it out. Thanks. >>>>> >>>>> >>>>> Regards >>>>> Yin, Fengwei >>>>> >>>>>> >>>>>>> + >>>>>>> + return true; >>>>>>> +} >>>>>>> + >>>>>>> +static inline bool >>>>>>> +folio_within_vma(struct folio *folio, struct vm_area_struct *vma, pte_t *pte) >>>>>>> +{ >>>>>>> + return folio_in_range(folio, vma, vma->vm_start, vma->vm_end, pte); >>>>>>> +} >>>>>>> + >>>>>>> /* >>>>>>> * mlock_vma_folio() and munlock_vma_folio(): >>>>>>> * should be called with vma's mmap_lock held for read or write, >>>>>> >>>> >>