Re: [PATCH v3 1/9] mm: limit pagecache folio refcount when mapping pages

["David Hildenbrand (Arm)" <david@kernel.org>]

> Mostly LGTM, with some comments:
> 
> Reviewed-by: Pedro Falcato <pfalcato@suse.de>

Thanks for the review, Pedro!

> 
>> ---
>>  include/linux/mm.h | 35 +++++++++++++++++++++++++++++++----
>>  mm/filemap.c       |  6 ++++++
>>  mm/huge_memory.c   | 29 +++++++++++++++++++++++++++--
>>  mm/memory.c        | 33 +++++++++++++++++++++++++++++----
>>  mm/migrate.c       | 11 +++++++++++
>>  mm/userfaultfd.c   |  4 ++++
>>  mm/util.c          | 18 ++++++++++++++++++
>>  7 files changed, 126 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index e6da76fbe8e83..6c7c29a086bd1 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -2229,9 +2229,36 @@ vm_fault_t finish_fault(struct vm_fault *vmf);
>>   *   back into memory.
>>   */
>>  
>> -/* 127: arbitrary random number, small enough to assemble well */
>> -#define folio_ref_zero_or_close_to_overflow(folio) \
>> -	((unsigned int) folio_ref_count(folio) + 127u <= 127u)
>> +/* Leave separate reserves for administrators and concurrent reference gains. */
>> +#define FOLIO_REF_COUNT_LIMIT		(INT_MAX - (1U << 26))
>> +#define FOLIO_REF_COUNT_ADMIN_LIMIT	(INT_MAX - (1U << 25))
>> +
>> +bool __folio_ref_count_over_limit(unsigned int ref_count);
>> +
>> +/**
>> + * folio_ref_count_over_limit - Check whether to refuse more folio references.
>> + * @folio: The folio.
>> + *
>> + * Check before adding page table mappings or other significant refcount users
>> + * to pagecache folios (!hugetlb), either before acquiring references or
>> + * afterwards with rollback if the operation is refused.
>> + *
>> + * Checking other folios does no harm.
>> + *
>> + * This check is an approximation; we leave some headroom such that concurrent
>> + * increments, batches, temporary references, or other bounded refcount
>> + * consumers are not a problem.
>> + *
>> + * Return: True if the caller should refuse the operation, false otherwise.
>> + */
>> +static inline bool folio_ref_count_over_limit(const struct folio *folio)
>> +{
>> +	const unsigned int ref_count = folio_ref_count(folio);
>> +
>> +	if (likely(ref_count < FOLIO_REF_COUNT_LIMIT))
>> +		return false;
>> +	return __folio_ref_count_over_limit(ref_count);
>> +}
>>  
>>  /**
>>   * folio_get - Increment the reference count on a folio.
>> @@ -2243,7 +2270,7 @@ vm_fault_t finish_fault(struct vm_fault *vmf);
>>   */
>>  static inline void folio_get(struct folio *folio)
>>  {
>> -	VM_BUG_ON_FOLIO(folio_ref_zero_or_close_to_overflow(folio), folio);
>> +	VM_WARN_ON_FOLIO(folio_ref_count(folio) <= 0, folio);
>>  	folio_ref_inc(folio);
>>  }
>>  
>> diff --git a/mm/filemap.c b/mm/filemap.c
>> index 9871b4f32a916..0f6d57432a7e5 100644
>> --- a/mm/filemap.c
>> +++ b/mm/filemap.c
>> @@ -3968,6 +3968,11 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,
>>  		unsigned long end;
>>  		vm_fault_t map_ret;
>>  
>> +		if (unlikely(folio_ref_count_over_limit(folio))) {
>> +			folio_ref_dec(folio);
>> +			goto next;
>> +		}
>> +
> 
> It's a bit odd that you take a ref, only to check later if we're over the
> limit, and ditch the folio. Could you move this to next_uptodate_folio()?

That might be even better (and avoids one conflict with Andrew's tree). Let me
take a look.

> 
>>  		addr += (xas.xa_index - last_pgoff) << PAGE_SHIFT;
>>  		vmf->pte += xas.xa_index - last_pgoff;
>>  		last_pgoff = xas.xa_index;
>> @@ -4009,6 +4014,7 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,
>>  					   mmap_miss - 1);
>>  		}
>>  
>> +next:
>>  		folio_unlock(folio);
>>  	} while ((folio = next_uptodate_folio(&xas, mapping, end_pgoff)) != NULL);
>>  	add_mm_counter(vma->vm_mm, folio_type, rss);
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 4608540e40d46..439cfe446cea4 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -1626,6 +1626,7 @@ static vm_fault_t insert_pmd(struct vm_area_struct *vma, unsigned long addr,
>>  {
>>  	struct mm_struct *mm = vma->vm_mm;
>>  	pgtable_t pgtable = NULL;
>> +	vm_fault_t ret = VM_FAULT_NOPAGE;
>>  	spinlock_t *ptl;
>>  	pmd_t entry;
>>  
>> @@ -1662,6 +1663,10 @@ static vm_fault_t insert_pmd(struct vm_area_struct *vma, unsigned long addr,
>>  		if (is_huge_zero_folio(fop.folio)) {
>>  			entry = pmd_mkspecial(entry);
>>  		} else {
>> +			if (unlikely(folio_ref_count_over_limit(fop.folio))) {
>> +				ret = VM_FAULT_SIGBUS;
>> +				goto out_unlock;
>> +			}
>>  			folio_get(fop.folio);
> 
> I see you use this pattern often. Would it make sense to have a
> bool folio_get_unless_overflow()?

I commented somewhere (in v2 I think) that I played with using
folio_get_checked() from #2, but didn't quite like it so I discarded it as

(a) it cannot be used everywhere here, as I'd need more functions to add
multiple references. next_uptodate_folio() also could use it as I would need yet
another temporary helper for a try_get().

(b) these changes here are going to get reverted fairly soon once I change the
way how mappings are refcounted (which is, unfortunately, not suitable for
stable, otherwise I wouldn't be throwing in this temporary change in the first
place)

So everything that is expected to stay in later patches uses the proper helper.

[...]

>>  
>> +		if (unlikely(folio_ref_count_over_limit(folio))) {
>> +			/* Skip this PUD; it can be faulted in later. */
>> +			ret = (dst_vma->vm_flags & VM_MIXEDMAP) ? -ENOMEM : 0;
> 
> A comment as to what's going on here with VM_MIXEDMAP would be useful. I guess
> the idea is that faulting on a mixedmap later will not necessarily hand you the
> exact same page fault result?

I can throw in a comment, yes.

[...]

>> +bool __folio_ref_count_over_limit(unsigned int ref_count)
>> +{
>> +	if (ref_count >= FOLIO_REF_COUNT_ADMIN_LIMIT)
>> +		return true;
>> +	/* Background threads must not consume admin reserves. */
>> +	if (current->flags & PF_KTHREAD)
>> +		return true;
>> +	/* Neither must interrupt contexts on behalf of the interrupted task. */
>> +	if (!in_task())
>> +		return true;
>> +	/* Capability checks are not safe with IRQs disabled. */
>> +	if (irqs_disabled())
>> +		return true;
>> +	/* Use the task's credentials, which filesystem overrides cannot change. */
>> +	return !has_capability_noaudit(current, CAP_SYS_RESOURCE);
>> +}
> 
> In case this function ever returns over limit, we probably want to log this
> in the kernel log. If this function gets the folio you could even print
> the problematic file offset/inode/filesystem/etc. Ratelimited of course.

You mean, if this function returns "true", right? Is printing always fine?
(e.g., irqs disabled?)

> 
>> +EXPORT_SYMBOL_GPL(__folio_ref_count_over_limit);
> 
> I'm fairly sure this will break the build for a number of non-GPL drivers
> when calling folio_get() with CONFIG_DEBUG_VM=y.

You mean, breaking OOT drivers, right? I can switch to EXPORT_SYMBOL if that's a
concern.

-- 
Cheers,

David