Message-ID: <cc6e17ad-1afd-5c52-a06e-1d89d1978368@redhat.com>
Date:   Thu, 5 Jan 2023 09:59:28 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Nadav Amit <nadav.amit@...il.com>, Peter Xu <peterx@...hat.com>
Cc:     Linux-MM <linux-mm@...ck.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Muchun Song <songmuchun@...edance.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        James Houghton <jthoughton@...gle.com>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 3/3] mm/uffd: Detect pgtable allocation failures

On 05.01.23 04:10, Nadav Amit wrote:
> 
>> On Jan 4, 2023, at 2:52 PM, Peter Xu <peterx@...hat.com> wrote:
>>
>> Before this patch, when there's any pgtable allocation issues happened
>> during change_protection(), the error will be ignored from the syscall.
>> For shmem, there will be an error dumped into the host dmesg.  Two issues
>> with that:
>>
>>   (1) Doing a trace dump when allocation fails is not anything close to
>>       grace..
>>
>>   (2) The user should be notified with any kind of such error, so the user
>>       can trap it and decide what to do next, either by retrying, or stop
>>       the process properly, or anything else.
>>
>> For userfault users, this will change the API of UFFDIO_WRITEPROTECT when
>> pgtable allocation failure happened.  It should not normally break anyone,
>> though.  If it breaks, then in good ways.
>>
>> One man-page update will be on the way to introduce the new -ENOMEM for
>> UFFDIO_WRITEPROTECT.  Not marking stable so we keep the old behavior on the
>> 5.19-till-now kernels.
> 
> I understand that the current assumption is that change_protection() should
> fully succeed or fail, and I guess this is the current behavior.
> 
> However, to be more “future-proof” perhaps this needs to be revisited.
> 
> For instance, UFFDIO_WRITEPROTECT can benefit from the ability to (based on
> userspace request) prevent write-protection of pages that are pinned. This is
> necessary to allow userspace uffd monitor to avoid write-protection of
> O_DIRECT’d memory, for instance, that might change even if a uffd monitor
> considers it write-protected.

Just a note that this is pretty tricky IMHO, because:

a) We cannot distinguished "pinned readable" from "pinned writable"
b) We can have false positives ("pinned") even for compound pages due to
    concurrent GUP-fast.
c) Synchronizing against GUP-fast is pretty tricky ... as we learned.
    Concurrent pinning is usually problematic.
d) O_DIRECT still uses FOLL_GET and we cannot identify that. (at least
    that should be figured out at one point)

I have a patch lying around for a very long time that removes that 
special-pinned handling from softdirty code, because of the above 
reasons (and because it forgets THP). For now I didn't send it because 
for softdirty, it's acceptable to over-indicate and it hasn't been 
reported to be an actual problem so far.

For existing UFFDIO_WRITEPROTECT users, however, it might be very 
harmful (especially for existing users) to get false protection errors. 
Failing due to ENOMEM is different to failing due to some temporary 
concurrency issues.

Having that said, I started thinking about alternative ways of detecting 
that in that past, without much outcome so far: that latest idea was 
indicating "this MM has had pinned pages at one point, be careful 
because any techniques that use write-protection (softdirty, mprotect, 
uffd-wp) won't be able to catch writes via pinned pages reliably".

Hm.

-- 
Thanks,

David / dhildenb

Powered by blists - more mailing lists