Re: [PATCH 5/5] mm/gup: limit pagecache folio refcount when grabbing references

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

On 11/6/26 14:11, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [High] mm/gup: try_get_folio() calls capable() with IRQs disabled in
>   GUP-fast
> --
> 
> commit 9290efad216cdf3b5ee1cdfafab0a5d9928a1976
> Author: David Hildenbrand (Arm) <david@kernel.org>
> 
> mm/gup: limit pagecache folio refcount when grabbing references
> 
> This commit replaces the negative refcount safety checks in try_get_folio()
> and try_grab_folio() with folio_ref_count_over_limit() to enforce folio
> refcount limits in fast and slow GUP before acquiring references or pins.
> 
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 2ab42fc6834e5..db9db6b2a4662 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -77,7 +77,7 @@ static inline struct folio *try_get_folio(struct page *page, int refs)
>>  
>>  retry:
>>  	folio = page_folio(page);
>> -	if (WARN_ON_ONCE(folio_ref_count(folio) < 0))
>> +	if (unlikely(folio_ref_count_over_limit(folio)))
>>  		return NULL;
> 
> [Severity: High]
> Is it safe for try_get_folio() to call folio_ref_count_over_limit() in the
> fast GUP path?
> 
> In mm/gup.c, gup_fast() disables hardware interrupts across the page table
> walk:
> 
> mm/gup.c:gup_fast() {
> 	...
> 	local_irq_save(flags);
> 	nr_pinned = gup_fast_pgd_range(start, end, gup_flags, pages);
> 	local_irq_restore(flags);
> 	...
> }
> 
> which calls into try_grab_folio_fast() -> try_get_folio() with IRQs
> disabled:
> 
> mm/gup.c:try_grab_folio_fast() {
> 	...
> 	/* Raise warn if it is not called in fast GUP */
> 	VM_WARN_ON_ONCE(!irqs_disabled());
> 	...
> }
> 
> Furthermore, get_user_page_fast_only() -> gup_fast() -> try_get_folio() is
> invoked from hardirq and NMI contexts, such as perf_prepare_sample() ->
> perf_virt_to_phys() in kernel/events/core.c.
> 
> When folio_ref_count(folio) is between FOLIO_REF_COUNT_LIMIT and
> FOLIO_REF_COUNT_ADMIN_LIMIT and current is not a kernel thread,
> folio_ref_count_over_limit() in include/linux/mm.h calls
> capable(CAP_SYS_RESOURCE):
> 
> include/linux/mm.h:folio_ref_count_over_limit() {
> 	const unsigned int ref_count = folio_ref_count(folio);
> 
> 	if (likely(ref_count < FOLIO_REF_COUNT_LIMIT))
> 		return false;
> 	if (ref_count >= FOLIO_REF_COUNT_ADMIN_LIMIT)
> 		return true;
> 	/* Background threads must not consume admin reserves. */
> 	return (current->flags & PF_KTHREAD) || !capable(CAP_SYS_RESOURCE);
> }
> 
> Calling capable(CAP_SYS_RESOURCE) -> ns_capable_common() ->
> security_capable() in these contexts appears to cause several issues:
> 
> 1. LSM capability hooks (such as selinux_capable() and apparmor_capable())
>    perform audit logging via common_lsm_audit(), which allocates memory and
>    acquires spinlock_t, rwlock_t, and SLUB local_lock_t locks. On
>    CONFIG_PREEMPT_RT, these are sleeping rt_mutex locks, so acquiring them
>    with hardware interrupts disabled in gup_fast() will sleep in an invalid
>    atomic context.
> 
> 2. When get_user_page_fast_only() is called from hardirq or NMI context,
>    acquiring non-IRQ/NMI-safe locks in the LSM, audit, or SLUB paths can
>    self-deadlock if the interrupted context on the same CPU already holds
>    one of those locks.
> 
> 3. In security/apparmor/capability.c, audit_caps() accesses the per-CPU
>    audit_cache using get_cpu_var(), which only disables preemption:
> 
>    security/apparmor/capability.c:audit_caps() {
> 	...
> 	ent = &get_cpu_var(audit_cache);
> 	if (ad->subj_cred == ent->ad_subj_cred &&
> 	    ktime_get_ns() <= ent->ktime_ns_expiration[cap]) {
> 		...
> 	} else {
> 		put_cred(ent->ad_subj_cred);
> 		ent->ad_subj_cred = get_cred(ad->subj_cred);
> 		ent->ktime_ns_expiration[cap] = ...;
> 	}
> 	put_cpu_var(audit_cache);
> 	...
>    }
> 
>    If an IRQ or NMI invokes get_user_page_fast_only() while the interrupted
>    context is inside audit_caps() on the same CPU, both contexts can race on
>    ent->ad_subj_cred and cause a double put_cred().
> 
> 4. In kernel/capability.c, ns_capable_common() updates current->flags
>    non-atomically:
> 
>    kernel/capability.c:ns_capable_common() {
> 	...
> 	capable = security_capable(current_cred(), ns, cap, opts);
> 	if (capable == 0) {
> 		current->flags |= PF_SUPERPRIV;
> 		return true;
> 	}
> 	...
>    }
> 
>    When called from hardirq or NMI context, this mutates the interrupted
>    task's flags and can race with process-context updates to current->flags.
> 
> Should try_get_folio() avoid calling capable() in fast GUP (for example, by
> falling back to slow GUP once the folio refcount reaches
> FOLIO_REF_COUNT_LIMIT)?

What a minefiled :)

Best to throw in something like

	if (!in_task() || irqs_disabled())
		true;

In the (new) slow helper to just reject admin reserves in such
context.

-- 
Cheers,

David