linux-kernel - Re: [PATCH] mm/gup: Fixes FOLL_UNLOCKABLE against FOLL

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 12 May 2023 14:06:36 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Peter Xu <peterx@...hat.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        John Hubbard <jhubbard@...dia.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        Lorenzo Stoakes <lstoakes@...il.com>,
        David Hildenbrand <david@...hat.com>
Subject: Re: [PATCH] mm/gup: Fixes FOLL_UNLOCKABLE against FOLL_NOWAIT

On Thu, May 11, 2023 at 08:31:02PM -0400, Peter Xu wrote:

> E.g., with current code we could at last have FAULT_FLAG_RETRY_NOWAIT set
> even if with a FOLL_UNLOCKABLE gup which doesn't make a lot of
> sense.

I would say NOWAIT and UNLOCKABLE are different things. UNLOCKABLE
says the mmap sem is allowed to be unlocked, which is true, and NOWAIT
says it shouldn't "wait" (which is something more nebulous than just
sleep). In FOLL_ flag terms it would be fine if the mmap sem was
unlocked while doing NOWAIT - even though the fault hanlder will not
doe this.

The only caller is fine with this too.

!UNLOCKABLE literally means not to ever drop the mmap lock which is
not something KVM needs at all.

So I'd say it is fine as is. A caller should never assume that calling
an unlocked function or passing null locked means that the mmap sem
won't be unlocked while running indirectly because of other GUP
flags. If it wants this behavior it needs to ask for it explicitly
with a locked GUP call and a NULL locked.

> Since at it, the same commit added unconditional FOLL_UNLOCKABLE in
> faultin_vma_page_range(), which is code-wise correct becuase the helper
> only has one user right now and it always has "locked" set.  

Not quite, it is correct because that is the API contract of this
function. The caller must provide a non-NULL locked and non-NULL
locked at the external interfaces always mean it can be unlocked while
running.

> However it can be abused if someone reuse faultin_vma_page_range()
> in other call sites in the future.  Add a sanity check for that,
> also add the missing comment for UNLOCKABLE.

Then we have bigger problems because the API has become confusing if a
non-NULL locked somehow means 'don't ever unlock', but only sometimes.

Jason