Re: A better dump_page()

[Vlastimil Babka]

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. I'm afraid the only way to
prevent this is to make sure any helpers __dump_page() uses simply must not
contain any VM_BUG/WARN_ON sanity checks, and anything treated as a pointer
must be checked before dereferencing.

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.