[GIT PULL] VFIO fix for v6.0-rc5

[GIT PULL] VFIO fix for v6.0-rc5

[Alex Williamson]

Hi Linus,

The following changes since commit b90cb1053190353cc30f0fef0ef1f378ccc063c5:

  Linux 6.0-rc3 (2022-08-28 15:05:29 -0700)

are available in the Git repository at:

  https://github.com/awilliam/linux-vfio.git tags/vfio-v6.0-rc5

for you to fetch changes up to 873aefb376bbc0ed1dd2381ea1d6ec88106fdbd4:

  vfio/type1: Unpin zero pages (2022-08-31 08:57:30 -0600)

----------------------------------------------------------------
VFIO fix for v6.0-rc5

 - Fix zero page refcount leak (Alex Williamson)

----------------------------------------------------------------
Alex Williamson (1):
      vfio/type1: Unpin zero pages

 drivers/vfio/vfio_iommu_type1.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Re: [GIT PULL] VFIO fix for v6.0-rc5

[Linus Torvalds]

On Fri, Sep 9, 2022 at 6:52 AM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> VFIO fix for v6.0-rc5
>
>  - Fix zero page refcount leak (Alex Williamson)

Ugh. This is disgusting.

Don't get me wrong - I've pulled this, but I think there's some deeper
problem that made this patch required.

Why is pin_user_pages_remote() taking a reference to a reserved page?
Maybe it just shouldn't (and then obviously we should fix the unpin
case to match too).

Adding a few GUP people to the participants for comments.

Anybody?

            Linus

Re: [GIT PULL] VFIO fix for v6.0-rc5

[David Hildenbrand]

On 09.09.22 13:53, Linus Torvalds wrote:
> On Fri, Sep 9, 2022 at 6:52 AM Alex Williamson
> <alex.williamson@redhat.com> wrote:
>>
>> VFIO fix for v6.0-rc5
>>
>>   - Fix zero page refcount leak (Alex Williamson)
> 
> Ugh. This is disgusting.
> 
> Don't get me wrong - I've pulled this, but I think there's some deeper
> problem that made this patch required.
> 
> Why is pin_user_pages_remote() taking a reference to a reserved page?
> Maybe it just shouldn't (and then obviously we should fix the unpin
> case to match too).
> 
> Adding a few GUP people to the participants for comments.
> 
> Anybody?

I mentioned in an offline discussion to Alex that we should teach the 
pin/unpin interface to not mess with the zeropage at all (i.e., not 
adjust the refcount and eventually overflow it).

We decided that the unbalanced pin/unpin should be fixed independently, 
such that the refcount handling change on pin/unpin stays GUP internal.


-- 
Thanks,

David / dhildenb

Re: [GIT PULL] VFIO fix for v6.0-rc5

[John Hubbard]

On 9/9/22 05:02, David Hildenbrand wrote:
> On 09.09.22 13:53, Linus Torvalds wrote:
>> On Fri, Sep 9, 2022 at 6:52 AM Alex Williamson
>> <alex.williamson@redhat.com> wrote:
>>>
>>> VFIO fix for v6.0-rc5
>>>
>>>   - Fix zero page refcount leak (Alex Williamson)
>>
>> Ugh. This is disgusting.
>>
>> Don't get me wrong - I've pulled this, but I think there's some deeper
>> problem that made this patch required.
>>
>> Why is pin_user_pages_remote() taking a reference to a reserved page?
>> Maybe it just shouldn't (and then obviously we should fix the unpin
>> case to match too).
>>
>> Adding a few GUP people to the participants for comments.
>>
>> Anybody?
> 
> I mentioned in an offline discussion to Alex that we should teach the 
> pin/unpin interface to not mess with the zeropage at all (i.e., not 
> adjust the refcount and eventually overflow it).

I don't think this is part of the problem. And I sure hope that it's
not. If you can make such a change, and contain it within the gup.c
layer so that callers can still think they are pinning the zero page,
then OK, that works.

But generally, callers expect to be able to pin the zero page, or to at
least believe that they've done so. :)  Sorting it out and treating it
separately requires larger changes to probably quite a lot of subsystems.

As a case in point, very close to my heart, I'm about to add a
pin_user_page*() caller to block/bio, that pins the zero page, after
discussing it with block/fs people [1]. 

> 
> We decided that the unbalanced pin/unpin should be fixed independently, 
> such that the refcount handling change on pin/unpin stays GUP internal.
> 
> 

OK, good.

[1] https://lore.kernel.org/all/20220124221709.kzsaqkdp3gmjie3z@quack3.lan/

thanks,

-- 
John Hubbard
NVIDIA

Re: [GIT PULL] VFIO fix for v6.0-rc5

[Alex Williamson]

On Fri, 9 Sep 2022 07:53:17 -0400
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Sep 9, 2022 at 6:52 AM Alex Williamson
> <alex.williamson@redhat.com> wrote:
> >
> > VFIO fix for v6.0-rc5
> >
> >  - Fix zero page refcount leak (Alex Williamson)  
> 
> Ugh. This is disgusting.
> 
> Don't get me wrong - I've pulled this, but I think there's some deeper
> problem that made this patch required.
> 
> Why is pin_user_pages_remote() taking a reference to a reserved page?
> Maybe it just shouldn't (and then obviously we should fix the unpin
> case to match too).
> 
> Adding a few GUP people to the participants for comments.
> 
> Anybody?

Yes, David is working on allocating pages rather than pinning the zero
page, however that's going to have some user visible locked memory
accounting changes.  This isn't the long term solution, it's only meant
to close the shared zero page refcount holes we have currently.  Thanks,

Alex

Re: [GIT PULL] VFIO fix for v6.0-rc5

[Jason Gunthorpe]

On Fri, Sep 09, 2022 at 06:08:32AM -0600, Alex Williamson wrote:
> On Fri, 9 Sep 2022 07:53:17 -0400
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Fri, Sep 9, 2022 at 6:52 AM Alex Williamson
> > <alex.williamson@redhat.com> wrote:
> > >
> > > VFIO fix for v6.0-rc5
> > >
> > >  - Fix zero page refcount leak (Alex Williamson)  
> > 
> > Ugh. This is disgusting.
> > 
> > Don't get me wrong - I've pulled this, but I think there's some deeper
> > problem that made this patch required.

The basic issue is that VFIO is using the reserved page as an
indication that pin_user_pages_remote() didn't process the page and
instead VFIO's bad follow_pfn() path was used.

It really shouldn't be working like this, which path was used to
obtain the PFN should be recorded independently. VFIO just doesn't
have a datastructure to accommodate it.

Aside from that, most longterm users don't ever even see the zero page
because that typically creates that GUP incoherence we've talked about
alot. The ugly FOLL_FORCE prevents it from happening. VFIO doesn't do
this either so it suffers the incoherence bug along with exposure to
the zero pages :(

David will fix the FOLL_FORCE issue and this will almost go away.

We had a long thread about this and discussed fixing it by simply
setting FOLL_FORCE as other pin_user_page users do. This reached an
interesting point that I'm curious on your view about ABI stability.

It is related to mlock accounting. When vfio pins pages it accounts
for them in mlock. Alex says there are users that set a mlock
limit. VFIO has this historical bug where it doesn't account for every
page it is asked to pin in mlock, eg it ignores the zero page. So, if
we change how GUP or VFIO does page pinning in a way that increases
its mlock charge is this an ABI break?

On one hand users with an very strict limit may find their environment
requires a limit increase after a kernel upgrade. So it fails the
'everything keeps working after kernel upgrade' test.

On the other hand, treating these sorts of limits as ABI means that a
lot of internal stuff is suddenly ABI - eg with a memory cgroup is
increasing the charge on GFP_KERNEL_ACCOUNT also ABI breaking? That
seems unworkable.

Thanks,
Jason

Re: [GIT PULL] VFIO fix for v6.0-rc5

[pr-tracker-bot]

The pull request you sent on Fri, 9 Sep 2022 04:52:25 -0600:

> https://github.com/awilliam/linux-vfio.git tags/vfio-v6.0-rc5

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/725f3f3b2708d8f3fe56df8113bfdc7380d52dc9

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html