Re: A better dump_page()

[Matthew Wilcox]

On Thu, Jan 05, 2023 at 04:19:06PM +0100, Vlastimil Babka wrote:
> On 1/4/23 00:29, Matthew Wilcox wrote:
> > On Tue, Jan 03, 2023 at 03:07:12PM -0800, David Rientjes wrote:
> >> On Tue, 3 Jan 2023, Matthew Wilcox wrote:
> >> 
> >> > On Tue, Jan 03, 2023 at 11:42:11AM +0100, Vlastimil Babka wrote:
> >> > > Separately we should also make the __dump_page() more resilient.
> >> > 
> >> > Right.  It's not ideal when one of our best debugging tools obfuscates
> >> > the problem we're trying to debug.  I've seen probems like this before,
> >> > and the problem is that somebody calls dump_page() on a page that they
> >> > don't own a refcount on.  That lets the page mutate under us in some
> >> > fairly awkward ways (as you've seen here, it seems to be part of several
> >> > different compound allocations at various points during the dump
> >> > process).
> >> > 
> >> > One possibility I thought about was taking our own refcount on the
> >> > page at the start of dump_page().  That would kill off the possibility
> >> > of ever passing in a const struct page, and it would confuse people.
> >> > Also, what if somebody passes in a pointer to something that's not a
> >> > struct page?  Then we've (tried to) modify memory that's not a refcount.
> >> > 
> >> > I think the best we can do is to snapshot the struct page and the folio
> >> > it appears to belong to at the start of dump_page().  It'll take a
> >> > little care (for example, folio_pfn() must be passed the original
> >> > folio, and not the snapshot), but I think it's doable.
> >> > 
> >> 
> >> By snapshot do you mean memcpy() of the metadata to the stack?  I assume 
> >> this still leaves the opportunity for the underlying mutation of the page 
> >> but makes the window more narrow.
> > 
> > Right.  We'd need to memcpy() both the page and the folio, so around 192
> > bytes.  It doesn't make it consistent since it could be mutated during
> > the memcpy(), but it will be consistent throughout the execution of the
> > function, so we won't get calls like folio_entire_mapcount() aborting
> > due to the folio having become a tail page halfway through.
> 
> I'm afraid this problem can still happen if the snapshot is inconsistent in
> the first place and you e.g. snapshot the tail page as tail page, and the
> supposed folio head page as not head page.

Oh, there would have to be an explicit check that the head page pointed
to is actually a head page, and further that it really is the head page
of this page.  But the advantage is that we can check that we have a
consistent snapshot _once_ rather than in every helper that we use in
this function.

The question is what to do if we have an inconsistent snapshot.
The answer we usually use (eg in filemap, gup, etc) is to try again until
we get a consistent answer.  But I think that's the wrong answer here; if
we get an inconsistent snapshot, we should give up.  Whatever information
might have been gleaned from the head page is now gone, because we don't
have the original head page any more.  So we should just dump the page
as a non-compound page.

> The local copying still makes a lot of sense though, as anything checked on
> the copy that determines its further evaluation can be trusted to remain
> consistent without complicated piecemeal READ_ONCE()'s etc. And as you
> mentioned it will allow us to proceed with constification, where the
> possibility of dump_page() through VM_BUG_ON_PAGE() is IIRC a big blocker
> for constifying the various flags checking helpers etc.

Yes, that's a nice side-benefit ;-)