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

Date:   Mon, 15 May 2023 14:00:53 -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 Mon, May 15, 2023 at 11:48:17AM -0400, Peter Xu wrote:

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

I don't know if it is so confusing, the flags still make sense when
composed together even if one is a NOP.

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

I don't really like it..

The FOLL_ flags are supposed to be statements about what the caller is
expecting. In this context the caller is clearly perfectly happy with
unlocking the mmap sem during operation. It should set the flag.

That the underlying code can't possibly do that when FOLL_NOWAIT is
set too doesn't really matter to the caller.

If there is something that needs tidying I'd say it is adjusting the
FAULT_FLAG logic to not make the combination, but I don't think it is
actually that confusing. "don't sleep" & "allow kill if you do sleep"
are still logically combinable operations.

Jason