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:   Mon, 15 May 2023 11:48:17 -0400
From:   Peter Xu <peterx@...hat.com>
To:     Jason Gunthorpe <jgg@...dia.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 Fri, May 12, 2023 at 02:06:36PM -0300, Jason Gunthorpe wrote:
> 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.

The problem is FOLL_NOWAIT implies FAULT_FLAG_RETRY_NOWAIT internally.

Then we'll have FAULT_FLAG_RETRY_NOWAIT+FAULT_FLAG_KILLABLE which makes it
very confusing, because RETRY_NOWAIT means we never release mmap lock or
retry, then KILL means "if we wait, allow us to be killed".

Considering FOLL_UNLOCKABLE is an internal flag while FOLL_NOWAIT a public
(even if only with a single caller...), I'd still think it makes more sense
and cleaner to just remove FOLL_UNLOCKABLE if FOLL_NOWAIT, no?

Again, nothing to blame for previous commit (I explained in the commit
message too that we don't need fixes, but simply a cleanup), but it seems
removing this confusion of NOWAIT+UNLOCKABLE could be helpful to me.

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

Hmm yes, that's the contract.  But then it makes more sense to assert on
that contract (by checking locked)?

How about I rework the commit message but keep the change (which literally
only add the assertion)?

-- 
Peter Xu