Re: [PATCH v2 2/2] xen/gntdev: Accommodate VMA splitting

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4174 bytes --]

On 03.10.22 00:20, M. Vefa Bicakci wrote:
> Prior to this commit, the gntdev driver code did not handle the
> following scenario correctly with paravirtualized (PV) Xen domains:
> 
> * User process sets up a gntdev mapping composed of two grant mappings
>    (i.e., two pages shared by another Xen domain).
> * User process munmap()s one of the pages.
> * User process munmap()s the remaining page.
> * User process exits.
> 
> In the scenario above, the user process would cause the kernel to log
> the following messages in dmesg for the first munmap(), and the second
> munmap() call would result in similar log messages:
> 
>    BUG: Bad page map in process doublemap.test  pte:... pmd:...
>    page:0000000057c97bff refcount:1 mapcount:-1 \
>      mapping:0000000000000000 index:0x0 pfn:...
>    ...
>    page dumped because: bad pte
>    ...
>    file:gntdev fault:0x0 mmap:gntdev_mmap [xen_gntdev] readpage:0x0
>    ...
>    Call Trace:
>     <TASK>
>     dump_stack_lvl+0x46/0x5e
>     print_bad_pte.cold+0x66/0xb6
>     unmap_page_range+0x7e5/0xdc0
>     unmap_vmas+0x78/0xf0
>     unmap_region+0xa8/0x110
>     __do_munmap+0x1ea/0x4e0
>     __vm_munmap+0x75/0x120
>     __x64_sys_munmap+0x28/0x40
>     do_syscall_64+0x38/0x90
>     entry_SYSCALL_64_after_hwframe+0x61/0xcb
>     ...
> 
> For each munmap() call, the Xen hypervisor (if built with CONFIG_DEBUG)
> would print out the following and trigger a general protection fault in
> the affected Xen PV domain:
> 
>    (XEN) d0v... Attempt to implicitly unmap d0's grant PTE ...
>    (XEN) d0v... Attempt to implicitly unmap d0's grant PTE ...
> 
> As of this writing, gntdev_grant_map structure's vma field (referred to
> as map->vma below) is mainly used for checking the start and end
> addresses of mappings. However, with split VMAs, these may change, and
> there could be more than one VMA associated with a gntdev mapping.
> Hence, remove the use of map->vma and rely on map->pages_vm_start for
> the original start address and on (map->count << PAGE_SHIFT) for the
> original mapping size. Let the invalidate() and find_special_page()
> hooks use these.
> 
> Also, given that there can be multiple VMAs associated with a gntdev
> mapping, move the "mmu_interval_notifier_remove(&map->notifier)" call to
> the end of gntdev_put_map, so that the MMU notifier is only removed
> after the closing of the last remaining VMA.
> 
> Finally, use an atomic to prevent inadvertent gntdev mapping re-use,
> instead of using the map->live_grants atomic counter and/or the map->vma
> pointer (the latter of which is now removed). This prevents the
> userspace from mmap()'ing (with MAP_FIXED) a gntdev mapping over the
> same address range as a previously set up gntdev mapping. This scenario
> can be summarized with the following call-trace, which was valid prior
> to this commit:
> 
>    mmap
>      gntdev_mmap
>    mmap (repeat mmap with MAP_FIXED over the same address range)
>      gntdev_invalidate
>        unmap_grant_pages (sets 'being_removed' entries to true)
>          gnttab_unmap_refs_async
>      unmap_single_vma
>      gntdev_mmap (maps the shared pages again)
>    munmap
>      gntdev_invalidate
>        unmap_grant_pages
>          (no-op because 'being_removed' entries are true)
>      unmap_single_vma (For PV domains, Xen reports that a granted page
>        is being unmapped and triggers a general protection fault in the
>        affected domain, if Xen was built with CONFIG_DEBUG)
> 
> The fix for this last scenario could be worth its own commit, but we
> opted for a single commit, because removing the gntdev_grant_map
> structure's vma field requires guarding the entry to gntdev_mmap(), and
> the live_grants atomic counter is not sufficient on its own to prevent
> the mmap() over a pre-existing mapping.
> 
> Link: https://github.com/QubesOS/qubes-issues/issues/7631
> Fixes: ab31523c2fca ("xen/gntdev: allow usermode to map granted pages")
> Cc: stable@vger.kernel.org
> Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]