linux-kernel - Re: [PATCH] mm/gup: add missing gup_must_unshare() check to gup_huge

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]

Message-ID: <2dbc82e9-30dc-5187-cdd6-166fa53213c4@redhat.com>
Date:   Mon, 8 May 2023 02:45:12 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Andrew Morton <akpm@...ux-foundation.org>,
        Lorenzo Stoakes <lstoakes@...il.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/gup: add missing gup_must_unshare() check to
 gup_huge_pgd()

On 08.05.23 02:30, Andrew Morton wrote:
> On Sat,  6 May 2023 15:05:25 +0100 Lorenzo Stoakes <lstoakes@...il.com> wrote:
> 
>> All other instances of gup_huge_pXd() perform the unshare check, so update
>> the PGD-specific function to do so as well.
>>
>> While checking pgd_write() might seem unusual, this function already
>> performs such a check via pgd_access_permitted() so this is in line with
>> the existing implementation.
> 
> Rationale seems strange.  "Other sites do it so this should also".  Why
> is this a desirable change?  Maybe the "other instances" shouldn't be
> performing this check either?

This change makes unshare handling across all GUP-fast variants consistent,
which is desirable as GUP-fast is complicated enough already even when
consistent :)

This function was the only one I seemed to have missed (or left out and forgot
why -- maybe because it's really dead code for now). The COW selftest would
identify the problem, so far there was no report. Either the selftest wasn't
run on corresponding architectures with that hugetlb size, or that code is still
dead code and unused by architectures.

I suspect the latter.


It might be worth to add a reference to the original commit(s) that added unsharing,
because they also explain why we care about these checks:

commit a7f226604170acd6b142b76472c1a49c12ebb83d
Author: David Hildenbrand <david@...hat.com>
Date:   Mon May 9 18:20:45 2022 -0700

     mm/gup: trigger FAULT_FLAG_UNSHARE when R/O-pinning a possibly shared anonymous page


commit 84209e87c6963f928194a890399e24e8ad299db1
Author: David Hildenbrand <david@...hat.com>
Date:   Wed Nov 16 11:26:48 2022 +0100

     mm/gup: reliable R/O long-term pinning in COW mappings


Acked-by: David Hildenbrand <david@...hat.com>

-- 
Thanks,

David / dhildenb