Re: [PATCH 1/3] mm: add functions folio_in_range() and folio_within_vma()

[Yin Fengwei]

On 8/3/23 17:58, 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 <fengwei.yin@xxxxxxxxx>
>> ---
>>  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);
>> +
> 
> Hi Yin,
> 
> I wanted to take a step back and consolidate my concerns about this function. I
> should say that my understanding of the pgoff and index stuff is shaky and I
> could therefore be wrong about some of this; if this is the case, then sorry for
> the noise! But something about this function doesn't smell right to me, so I
> figure its better to raise it...
No worries. And thank you for looking at the code ans share the comments.
That helps me a lot. Really appreciate it.

> 
>> +/*
>> + * Check whether the folio is in specific range
> 
> What exactly is the function trying to do? I *think* the intention is to figure
> out if a folio is fully and contiguously mapped within a range of virtual
> addresses belonging to a specific virtual address space? And I assume you want
> the answer to be precise? I'm assuming 'yes' for the below.
> 
> I think the only way to do this is to actually check each PTE. And that causes a
> problem, because a folio can straddle multiple PTE tables, which causes PTL
> locking issues, and means having just a *pte pointer is insufficient if we need
> to traverse multiple pte tables. So perhaps you need to allow for a false
> negative in the case that the folio is not contained within a single pte table.
Let's check the users of this function first:
 mlock/munlock: Needs only care whether the folio is in the range of VM_LOCKED VMA
 madvise: Needs to check whether the folio is in the range of VMA and
          range [start, end). This range is from user space. It could contain
          VMA range (in this case, we only need to check VMA range) or is contained
          by VMA range.

So we check:
  1. Whether the folio is in the range.

     To do this, we need to first check whether the folio->index is in the
     VMA range. If not, we know the folio is not in VMA range. Just return false
     without further check.

     Then, we will check whether the folio is in the range which is defined as
     [max(start, vma->vm_start), min(end, vma->vm_end)).


     This is the version in RFC as I was not aware of mremap case and forgot the
     page cache case. So if no mremap with an anonymous folio, this check is enough.
     But we need to add PTE check for mremap and page cache cases.

  2. Check PTE for mremap in middle of large folio and page cache
     If the folio is normal 4K and if the folio is in the range, it's not possible
     the folio is partially mapped to two different VMA. So we are sure this folio
     is in the range.

     Currently, check PTE didn't handle the case you mentioned. But my plan is
     checking whether the folio is mapped cross page table (by checking whether
     the folio start vaddr and end vaddr has same value for low aligned to PMD_SIZE).
     If it cross, I will treat it as out of VMA range. Otherwise, we can reuse
     current check because we know the PTE pointer is always valid.

     Obviously, the PTE checking needs hold pte lock as you pointed out.


My understanding:
pgoff is important if we have folio and VMA and want to know the virtual address of
the folio mapped to. Like first, check whether pgoff of folio belongs to VMA, then get
vaddr by:
      addr = vma->vm_start + ((pgoff - vma->vm_pgoff) << PAGE_SHIFT);
rmap_walk() also depends on pgoff. You can check the vma_address() and rmap_walk()
implementation.

> 
>> + *
>> + * 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)
> 
> The prototype looks odd to me; Fundamentally it seems to me that you need the
> folio, start and end virtual addresses (the range you want to check that it is
> in), the pte pointer and the virtual address that the pte represents. That
> virtual address allows you to figure out the offset between the pa and va. Then
> you can look at all the ptes to figure out if they reference the folio's pfns,
> and use the va to pa mapping info to figure out if its within the specified range.
> 
> I don't really understand why the vma is useful.
> 
>> +{
>> +	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;
> 
> Why is this pgoff calculation helpful? Surely it's only useful if the folio
> belongs to the same file that the vma is mapping? Otherwise the folio's pgoff
> might be referring to a completely different file than the vma's vm_pgoff. So
> you will get spurious results.
> 
>> +
>> +	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;
> 
> I don't understand why you don't need to check the pte for a small folio? As
> above, if the folio doesn't belong to the file that the vma is mapping the above
> checks seem wrong and you can't conclude that the folio is mapped in the range
> without looking at the pte?
> 
>> +
>> +	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);
> 
> I think this could wander off the front or back of the pte table into arbitrary
> memory if the folio is strddling multiple pte tables.
> 
>> +	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)
> 
> Doesn't !pte_present() also cover pte_none()? So I think the pte_none() check is
> redundant?
I think you are right. pte_none() can be removed here.


Regards
Yin, Fengwei

> 
> Thanks,
> Ryan
> 
> 
>> +			return false;
>> +	}
>> +
>> +	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,
>