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;
Date:   Thu, 3 Mar 2022 09:25:47 -0800 (PST)
From:   Hugh Dickins <hughd@google.com>
To:     Matthew Wilcox <willy@infradead.org>
cc:     Hugh Dickins <hughd@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Vlastimil Babka <vbabka@suse.cz>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Subject: Re: [PATCH mmotm] mm/munlock: mlock_vma_folio() check against
 VM_SPECIAL
In-Reply-To: <YiDPeJ+7DVWpsuRv@casper.infradead.org>
Message-ID: <39f9e61b-c66c-a741-5118-997a48dd440@google.com>
References: <9b95d366-1719-f8e2-a5a3-429f9e808288@google.com> <YiDPeJ+7DVWpsuRv@casper.infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Thu, 3 Mar 2022, Matthew Wilcox wrote:
> On Wed, Mar 02, 2022 at 05:35:30PM -0800, Hugh Dickins wrote:
> > Although mmap_region() and mlock_fixup() take care that VM_LOCKED
> > is never left set on a VM_SPECIAL vma, there is an interval while
> > file->f_op->mmap() is using vm_insert_page(s), when VM_LOCKED may
> > still be set while VM_SPECIAL bits are added: so mlock_vma_folio()
> > should ignore VM_LOCKED while any VM_SPECIAL bits are set.
> > 
> > This showed up as a "Bad page" still mlocked, when vfree()ing pages
> > which had been vm_inserted by remap_vmalloc_range_partial(): while
> > release_pages() and __page_cache_release(), and so put_page(), catch
> > pages still mlocked when freeing (and clear_page_mlock() caught them
> > when unmapping), the vfree() path is unprepared for them: fix it?
> > but these pages should not have been mlocked in the first place.
> > 
> > I assume that an mlockall(MCL_FUTURE) had been done in the past; or
> > maybe the user got to specify MAP_LOCKED on a vmalloc'ing driver mmap.
> > 
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> > ---
> > Diffed against top of next-20220301 or mmotm 2022-02-28-14-45.
> > This patch really belongs as a fix to the mm/munlock series in
> > Matthew's tree, so he might like to take it in there (but the patch
> > here is the foliated version, so easiest to place it after foliation).
> 
> It looks like it fixes "mm/munlock: mlock_pte_range() when mlocking or
> munlocking", so I'll fold it into that patch?

No and yes.

That's great if you're prepared to move it back before the foliation.
I think that just involves editing every "folio" to "page", including
in the title - I very nearly sent it out with mlock_vma_page() in title.

But I would prefer it to remain as a separate fix at the end of the
mm/munlock series: this case is too unusual, and only a "Bad page",
to mess with bisection prospects; and it's addressing an entirely
different issue from what the "mlock_pte_range()..." is dealing with.
Each of them needs its own explanation.

So I would prefer it as a separate fix about "page"s, on top of the
mm/munlock series, and you then adjust your foliation commit accordingly.

Thank you: this is what I really wanted, but was afraid to ask of you
(and of course, other fixes may turn out to be required, too late to
adjust across the page<->folio barrier in this way: so it's nice to
be able to do it this way, but rather beyond the call of duty).

Hugh