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:   Mon, 18 Jul 2022 14:34:01 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Liam Howlett <liam.howlett@oracle.com>
cc:     Hugh Dickins <hughd@google.com>,
        David Hildenbrand <david@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Yu Zhao <yuzhao@google.com>,
        Matthew Wilcox <willy@infradead.org>,
        "maple-tree@lists.infradead.org" <maple-tree@lists.infradead.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] maple_tree: Fix sparse reported issues
In-Reply-To: <20220718174733.dya2xjigqeud6clx@revolver>
Message-ID: <a6736ccf-fb45-5777-ca28-575297f1879f@google.com>
References: <20220713132926.3sl7gs67dyjj7kit@revolver> <44a478e8-2ccc-e82-bd5a-172778c01529@google.com> <20220713175013.aoemaelds45aavc4@revolver> <20220715195301.r7ozt6ph2scti7vz@revolver> <fc9c2193-f5d7-d494-8e4e-c9f340ae8625@google.com>
 <20220718022718.wtlw7grwp6dv5fcp@revolver> <1098dd4d-8f12-4493-5aff-1ee489d8e7@google.com> <20220718125649.cpatlh7ublgf7bvg@revolver> <20220718134541.ucbpuqdfcnfxravx@revolver> <7db5a8c5-9084-a7fe-6e83-713e52ed8539@google.com>
 <20220718174733.dya2xjigqeud6clx@revolver>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Mon, 18 Jul 2022, Liam Howlett wrote:
> > 
> > I said before that I expected the test run to hit the swapops.h
> > migration entry !PageLocked BUG, but it did not.  It ran for
> > nearly 7 hours, and then one of its builds terminated with
> > 
> > {standard input}: Assembler messages:
> > {standard input}: Error: open CFI at the end of file;
> >  missing .cfi_endproc directive
> > gcc: fatal error: Killed signal terminated program cc1
> > compilation terminated.
> > 
> > which I've never seen before.  Usually I'd put something like that down
> > to a error in swap, or a TLB flushing error (but I include Nadav's fix
> > in my kernel, and wouldn't get very far without it): neither related to
> > the maple tree patchset.
> > 
> > But on this occasion, my guess is that it's actually an example of what
> > the swapops.h migration entry !PageLocked BUG is trying to alert us to.
> > 
> > Imagine when such a "stale" migration entry is found, but the page it
> > points to (now reused for something else) just happens to be PageLocked
> > at that instant.  Then the BUG won't fire, and we proceed to use the
> > page as if it's ours, but it's not.  I think that's what happened.
> > 
> > I must get on with the day: more testing, and thinking.
> 
> 
> I think this is the same issue seen here:
> https://lore.kernel.org/linux-mm/YsQt3IHbJnAhsSWl@casper.infradead.org/

Yes, that's a swapops.h migration entry !PageLocked BUG on brk.

> 
> Note that on 20220616, the maple tree was in the next.
> 
> I suspect I am doing something wrong in do_brk_munmap().  I am using a
> false VMA to munmap a partial vma by setting it up like the part of the
> VMA that would have been split, inserted into the tree, then removed and
> freed.  I must be missing something necessary for this to function
> correctly.

Thanks for pointing to that, yes, the vma_init(&unmap, mm) etc in
do_brk_munmap(): I hadn't noticed struct vma_area_struct unmap before.

And almost coincident with your mail, my next test run crashed on
kernel BUG at mm/huge_memory.c:2013, VM_BUG_ON_VMA(vma->vm_start > haddr),
in __split_huge_pmd_locked(), while servicing do_brk_munmap():
no doubt for a (different but) related reason.

Presumably you noticed an opportunity to optimize out some maple tree
operations by giving do_brk_munmap() its own processing.  But I think
you're going to have to undo all that, and make do_brk_munmap() do
things in the standard, less direct, munmap() way - using
do_mas_align_munmap() I presume.

(Oh dear, I see that doing mas_preallocate() at the beginning,
but then __split_vma()s inside, and __split_vma() will do a
vma_adjust(), which will do a mas_preallocate().  Ah, but they
are on distinct "mas"es at different levels, so it's probably okay.)

If rmap is to be sure to find migration entries that it inserted
earlier, you must hold anon_vma_lock_write() (in the brk case) across
the tricky vma manipulations.  No doubt you could write an optimized
do_brk_munmap() which does everything under anon_vma_lock_write(), but
you'd then be duplicating far too much of the care in __vma_adjust()
for my taste (I'm already not so happy to find it duplicated in
vma_expand()).

I'll continue with some testing, but expect it to keep hitting these
issues until do_brk_munmap() is rewritten - perhaps it reduces to
little more than a wrapper like do_munmap() or do_mas_munmap().

Hugh