Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230101230042.244286-1-jthoughton@google.com> <Y7SA839SDcpf+Ll0@monkey>
In-Reply-To: <Y7SA839SDcpf+Ll0@monkey>
From:   James Houghton <jthoughton@google.com>
Date:   Tue, 3 Jan 2023 20:26:01 +0000
Message-ID: <CADrL8HVeOkj0QH5VZZbRzybNE8CG-tEGFshnA+bG9nMgcWtBSg@mail.gmail.com>
Subject: Re: [PATCH] hugetlb: unshare some PMDs when splitting VMAs
To:     Mike Kravetz <mike.kravetz@oracle.com>
Cc:     Muchun Song <songmuchun@bytedance.com>,
        Peter Xu <peterx@redhat.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> Thanks James.  I am just trying to determine if we may have any issues/bugs/
> undesired behavior based on this today.  Consider the cases mentioned above:
> mbind - I do not think this would cause any user visible issues.  mbind is
>         only dealing with newly allocated pages.  We do not unshare as the
>         result of a mbind call today.
> madvise(MADV_DONTDUMP) - It looks like this results in a flag (VM_DONTDUMP)
>         being set on the vma.  So, I do not believe sharing page tables
>         would cause any user visible issue.
>
> One somewhat strange things about two vmas after split sharing a PMD is
> that operations on one VMA can impact the other.  For example, suppose
> A VMA split via mbind happens.  Then later, mprotect is done on one of
> the VMAs in the range that is shared.  That would result in the area being
> unshared in both VMAs.  So, the 'other' vma could see minor faults after
> the mprotect.
>
> Just curious if you (or anyone) knows of a user visible issue caused by this
> today.  Trying to determine if we need a Fixes: tag.

I think I've come up with one... :) It only took many many hours of
staring at code to come up with:

1. Fault in PUD_SIZE-aligned hugetlb mapping
2. fork() (to actually share the PMDs)
3. Split VMA with MADV_DONTDUMP
4. Register the lower piece of the newly split VMA with
UFFDIO_REGISTER_MODE_WRITEPROTECT (this will call
hugetlb_unshare_all_pmds, but it will not attempt to unshare in the
unaligned bits now)
5. Now calling UFFDIO_WRITEPROTECT will drop into
hugetlb_change_protection and succeed in unsharing. That will hit the
WARN_ON_ONCE and *not write-protect anything*.

I'll see if I can confirm that this is indeed possible and send a
repro if it is.

60dfaad65a ("mm/hugetlb: allow uffd wr-protect none ptes") is the
commit that introduced the WARN_ON_ONCE; perhaps it's a good choice
for a Fixes: tag (if above is indeed true).

>
> Code changes look fine to me.

Thanks Mike!
- James