On 03.05.23 02:31, Matthew Rosato wrote:
On 5/2/23 6:51 PM, Lorenzo Stoakes wrote:
Writing to file-backed mappings which require folio dirty tracking using
GUP is a fundamentally broken operation, as kernel write access to GUP
mappings do not adhere to the semantics expected by a file system.
A GUP caller uses the direct mapping to access the folio, which does not
cause write notify to trigger, nor does it enforce that the caller marks
the folio dirty.
The problem arises when, after an initial write to the folio, writeback
results in the folio being cleaned and then the caller, via the GUP
interface, writes to the folio again.
As a result of the use of this secondary, direct, mapping to the folio no
write notify will occur, and if the caller does mark the folio dirty, this
will be done so unexpectedly.
For example, consider the following scenario:-
1. A folio is written to via GUP which write-faults the memory, notifying
the file system and dirtying the folio.
2. Later, writeback is triggered, resulting in the folio being cleaned and
the PTE being marked read-only.
3. The GUP caller writes to the folio, as it is mapped read/write via the
direct mapping.
4. The GUP caller, now done with the page, unpins it and sets it dirty
(though it does not have to).
This change updates both the PUP FOLL_LONGTERM slow and fast APIs. As
pin_user_pages_fast_only() does not exist, we can rely on a slightly
imperfect whitelisting in the PUP-fast case and fall back to the slow case
should this fail.
v8:
- Fixed typo writeable -> writable.
- Fixed bug in writable_file_mapping_allowed() - must check combination of
FOLL_PIN AND FOLL_LONGTERM not either/or.
- Updated vma_needs_dirty_tracking() to include write/shared to account for
MAP_PRIVATE mappings.
- Move to open-coding the checks in folio_pin_allowed() so we can
READ_ONCE() the mapping and avoid unexpected compiler loads. Rename to
account for fact we now check flags here.
- Disallow mapping == NULL or mapping & PAGE_MAPPING_FLAGS other than
anon. Defer to slow path.
- Perform GUP-fast check _after_ the lowest page table level is confirmed to
be stable.
- Updated comments and commit message for final patch as per Jason's
suggestions.
Tested again on s390 using QEMU with a memory backend file (on ext4) and vfio-pci -- This time both vfio_pin_pages_remote (which will call pin_user_pages_remote(flags | FOLL_LONGTERM)) and the pin_user_pages_fast(FOLL_WRITE | FOLL_LONGTERM) in kvm_s390_pci_aif_enable are being allowed (e.g. returning positive pin count)
At least it's consistent now ;) And it might be working as expected ...
In v7:
* pin_user_pages_fast() succeeded
* vfio_pin_pages_remote() failed
But also in v7:
* GUP-fast allows pinning (anonymous) pages in MAP_PRIVATE file
mappings
* Ordinary GUP allows pinning pages in MAP_PRIVATE file mappings
In v8:
* pin_user_pages_fast() succeeds
* vfio_pin_pages_remote() succeeds
But also in v8:
* GUP-fast allows pinning (anonymous) pages in MAP_PRIVATE file
mappings
* Ordinary GUP allows pinning pages in MAP_PRIVATE file mappings
I have to speculate, but ... could it be that you are using a private
mapping?
In QEMU, unfortunately, the default for memory-backend-file is
"share=off" (private) ... for memory-backend-memfd it is "share=on"
(shared). The default is stupid ...
If you invoke QEMU manually, can you specify "share=on" for the
memory-backend-file? I thought libvirt would always default to
"share=on" for file mappings (everything else doesn't make much sense)
... but you might have to specify
<access mode="shared"/>
in addition to
<source type="file"/>
--
Thanks,
David / dhildenb
