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

[David Hildenbrand (Arm)]

On 11/13/26 20:45, Matthew Wilcox wrote:
> On Thu, Nov 12, 2026 at 11:53:20PM +0100, David Hildenbrand (Arm) wrote:
>> Being able to overflow the refcount is not really big news, but it's about
>> time that we do something about it because some modern technology might
>> find ways to abuse it :)

Hi Willy,

thanks for the review!

Be aware that I want to get this fixed now asap. So I will push back against any
bigger changes on this for now unless there are real issues to fix.

I want to get this sent early next week to Linus.

With future changes I am planning, mappings will no longer be an issue. I will
then drop admin reserves (as the chance for DOS will go down drastically) and we
can use the negative refcount as our limit.

> 
> Yes.  My primary worry is actually it finding a path where we forget to
> put the refcount on a folio and that gets exploited into a UAF.  I'm not
> sure the "reserve for root" aspect really helps recover from that, but
> I'm not too opposed to it.
> 
>> As a short-term solution, let's introduce two folio refcount limits:
>> one for ordinary consumers and one for admins. Leave some headroom
>> before the refcount goes negative, which we really don't want (having
>> the mapcount go negative is also not expected).
> 
> I don't think a negative refcount or mapcount is really a problem.  What
> matters is that we don't get to zero from below (for the refcount)
> or that we don't get to 0xf000'0000 (for mapcount).

They are a problems in current code with both negative refcount and negative
mapcounts.

Nothing I want to sort out in this patch set.

So if we reach a negative mapcount of refcount today, we messed up.

> 
>> +/* 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))
> 
> I think these specific values will generate bad code.

Again, we can worry about fine tuning once we have a proper fix in place.

> 
>> +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;
> 
> I would replace this with a simple signed comparison against 0.

As per above, this is something for the future.

> 
>>  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);
>>  }
> 
> And I would drop this hunk.

Similarly, for the futue.

> 
>> diff --git a/mm/util.c b/mm/util.c
>> index 80f6c714844f2..c9c3dc4b490fa 100644
>> --- a/mm/util.c
>> +++ b/mm/util.c
> 
> Now we have mm/folio.c, I think it should live in there.

I can move it.

> 
>> @@ -791,6 +791,32 @@ int folio_mc_copy(struct folio *dst, struct folio *src)
>>  }
>>  EXPORT_SYMBOL(folio_mc_copy);
>>  
>> +bool folio_ref_count_over_limit_slowpath(const struct folio *folio,
>> +		unsigned int ref_count)
>> +{
>> +	if (ref_count >= FOLIO_REF_COUNT_ADMIN_LIMIT)
>> +		goto over_limit;
> 
> Maybe let this go up to 0x8100'0000?
> 
>> +	/* Background threads must not consume admin reserves. */
>> +	if (current->flags & PF_KTHREAD)
>> +		goto over_limit;
>> +	/* Neither must interrupt contexts on behalf of the interrupted task. */
>> +	if (!in_task())
>> +		goto over_limit;
>> +	/* Capability checks are not safe with IRQs disabled. */
>> +	if (irqs_disabled())
>> +		goto over_limit;
>> +	/* Use the task's credentials, which filesystem overrides cannot change. */
>> +	if (has_capability_noaudit(current, CAP_SYS_RESOURCE))
>> +		return false;
>> +
>> +over_limit:
>> +	pr_warn_ratelimited("folio:%p refcount:%d mapcount:%d mapping:%p reached refcount limit\n",
>> +			    folio, (int)ref_count, folio_mapcount(folio),
>> +			    folio_mapping(folio));
> 
> If it's worth doing folio->mapping, maybe also print folio->index?
> In fact,

I can do.

> 
>         pr_warn("page: refcount:%d mapcount:%d mapping:%p index:%#lx pfn:%#lx\n",
>                         folio_ref_count(folio), mapcount, mapping,
>                         folio->index + idx, pfn);
> 
> might be worth pulling out into a mini helper from __dump_folio()?

I want a pr_warn_ratelimited though.

Again, thanks for the review, I want to get this fixed asap now.

-- 
Cheers,

David