Re: [PATCH 0/2] mm/memfd: add F_SEAL_PUNCH_HOLE

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

On 10/31/26 04:03, Samiullah Khawaja wrote:
> On Fri, Oct 30, 2026 at 03:43:53PM -0700, Andy Lutomirski wrote:
>> On Fri, Oct 30, 2026 at 10:40 AM Samiullah Khawaja <skhawaja@google.com> wrote:
>>>
>>> >On 10/30/26 13:11, Jason Gunthorpe wrote:
>>> >> On Fri, Oct 30, 2026 at 11:20:30AM +0100, David Hildenbrand (Arm) wrote:
>>> >>>
>>> >>> We have the exact same situation when userspace maps anonymous folios
>>> into vfio
>>> >>> and then desides to MADV_DONTNEED the pages. Or any other memory backend
>>> that is
>>> >>> used for such purposes.
>>> >>>
>>> >>> User space must not zap/punch things that might be pinned elsewhere.
>>> >>>
>>> >>> That's not specific to memfd, udmabuf, iommufd or anything else.
>>> >>>
>>> >>> I don't see a reason to make userspace (that should not shoot itself into
>>> the
>>> >>> foot) a kernel feature it must activate itself to not shoot itself into
>>> the foot.
>>>
>>> Agreed, for normal use this is userspace's problem. The seal was meant
>>> for the kernel to cheaply verify at preserve time that nothing was
>>> punched.
>>>
>>> >>
>>> >> The issue is live update doesn't preserve those background punched
>>> >> folios and now we have created an UAF attack on the kernel across
>>> >> kexec if you don't do something about it.
>>> >>
>>> >> There was some notion here that with the right sealing flags we could
>>> >> tell if everything was 'live update compatible' and then know that
>>> >> these attacks are impossible.
>>> >Just remember that there was any hole punch and refuse to live update this
>>> instance.
>>> >
>>> >We could likely implement "we saw hole punch on a pinned folio" as well, but I
>>> >don't think that's required initially.
>>>
>>> If I understand correctly, you mean recording on the memfd that a hole
>>> punch happened, and refusing the live update if it did? That would work
>>> and is much cheaper than what we do today, where iommufd walks all the
>>> mapped pages at preserve time. For truncate, the existing F_SEAL_SHRINK
>>> is enough or I can record that also on the memfd. I will send patches
>>> for this.
>>
>> This entire situation continues to give me the willies.  After much
>> prodding and contemplating (in my own brain), I got a friendly LLM to
>> report:
>>
>> --- begin AI output ---
>>
>> IOMMU_IOAS_MAP_FILE uses memfd_pin_folios(). Collapse [MADV_COLLAPSE
>> or khugepaged] checks the old folio’s reference count, so a pin
>> acquired before that check makes collapse fail.
>>
>>  The problematic interleaving is:
>>
>>  1. collapse_file() locks and unmaps the old folio, then passes its
>> reference-count check (mm/khugepaged.c:2446).
>>  2. memfd_pin_folios() (mm/gup.c:3483) obtains the old folio from the
>> file’s XArray and pins it, without taking the folio lock.
>>  3. Collapse copies the contents and replaces the old cache entries
>> with the new huge folio. It does not repeat the reference check or
>> freeze the old count.
>>  4. IOAS retains the old physical pages; accesses through the file
>> use the new folio.
>>
>> --- end AI output ---
>>
>> Is this correct?  I haven't personally verified it.  But this whole
> 
> Yes, I think this is correct. memfd_pin_folios() pins the folios
> directly without taking the folio lock. Other page cache lookups lock
> the folio and recheck folio->mapping, e.g. shmem_get_folio_gfp(). Only
> memfd_pin_folios() misses it.
> 
> This scenario is different from punch hole or MADV_DONTNEED where the
> user decides to remove the pinned memory. Here the kernel is removing
> the pinned page without the user knowing. Collapse already checks the
> refcount to avoid this.
> 
> The recent fix [1] by Chris already takes the folio lock for !uptodate
> folios. I think we can take the lock unconditionally and recheck
> folio->mapping, that should fix this. I can send a patch for this fix,
> if folks agree.

I have another patch from Anthropic on my "rework schlop" queue that should fix
this:

"mm/gup: pin memfd folios under the folio lock".


We'd do just what we do in other context now, which is checking under folio lock
whether the folio was truncated.

Care to send a proper fix?

-- 
Cheers,

David