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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 9 Sep 2022 11:58:18 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     David Hildenbrand <david@...hat.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Alex Williamson <alex.williamson@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [GIT PULL] VFIO fix for v6.0-rc5

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@...hat.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