RE: [PATCH] vfio/type1: Unpin zero pages

[Tian, Kevin]

Hi, Alex,

> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Sent: Tuesday, August 30, 2022 11:06 AM
> 
> There's currently a reference count leak on the zero page.  We increment
> the reference via pin_user_pages_remote(), but the page is later handled
> as an invalid/reserved page, therefore it's not accounted against the
> user and not unpinned by our put_pfn().
> 
> Introducing special zero page handling in put_pfn() would resolve the
> leak, but without accounting of the zero page, a single user could
> still create enough mappings to generate a reference count overflow.
> 
> The zero page is always resident, so for our purposes there's no reason
> to keep it pinned.  Therefore, add a loop to walk pages returned from
> pin_user_pages_remote() and unpin any zero pages.
> 

We found an interesting issue on zero page and wonder whether we
should instead find a way to not use zero page in vfio pinning path.

The observation - the 'pc.bios' region (0xfffc0000) is always mapped
RO to zero page in the IOMMU page table even after the mapping in
the CPU page table has been changed after Qemu loads the guest
bios image into that region (which is mmap'ed as RW).

In reality this may not cause real problem as I don't expect any sane
usage would want to DMA read from the bios region. This is probably
the reason why nobody ever notes it.

But in concept it is incorrect.

Fixing Qemu to update/setup the VFIO mapping after loading the bios
image could mitigate this problem. But we never document such ABI
restriction on RO mappings and in concept the pinning semantics
should apply to all paths (RO in DMA and RW in CPU) which the
application uses to access the pinned memory hence the sequence
shouldn't matter from user p.o.v

And old Qemu/VMM still have this issue.

Having a notifier to implicitly fix the IOMMU mapping within the
kernel violates the semantics of pinning, and makes vfio page
accounting even more tricky.

So I wonder instead of continuing to fix trickiness around the zero
page whether it is a better idea to pursue allocating a normal
page from the beginning for pinned RO mappings?

Thanks
Kevin