Re: [PATCH v2 6/9] mm/memory: convert print_bad_pte() to print_bad_page_map()

[David Hildenbrand <david@xxxxxxxxxx>]

> Yeah sorry I was in 'what locks do we need' mode and hadn't shifted back here,
> but I guess the intent is that the caller _must_ hold this lock.
>
> I know it's nitty and annoying (sorry!) but as asserting seems to not be a
> possibility here, could we spell these out as a series of points like:
>
> /*
>   * The caller MUST hold the following locks:
>   *
>   * - Leaf page table lock
>   * - Appropriate VMA lock to keep VMA stable
>   */
>
> I don't _actually_ think you need the rmap lock then, as none of the page tables
> you access would be impacted by any rmap action afaict, with these locks held.
I don't enjoy wrong comments ;)

This can be called from rmap code when doing a vm_normal_page() while 
holding the PTL.
Really, I think we are over-thinking a helper that is triggered in 
specific context when the world is about to collide.
This is not your general-purpose API.

Maybe I should have never added a comment. Maybe I should just not have 
done this patch, because I really don't want to do more than the bare 
minimum to print_bad_page_map().
Because I deeply detest it, and no comments we will add will change that.

[...]

> > > But can you truly be sure of these existing? And we should then assert them
> > > here no? For rmap though we'd need the folio/vma.
> > I hope you realize that this nastiness of a code is called in case our
> > system is already running into something extremely unexpected and will
> > probably be dead soon.
> >
> > So I am not to interested in adding anything more here. If you run into this
> > code you're in big trouble already.
> Yes am aware :) my concern is NULL ptr deref or UAF, but with the locks
> held as stated those won't occur.
>
> But f it's not sensible to do it then we don't have to :) I am a reasonable
> man, or like to think I am ;)
>
> But I think we need clarity as per the above.
>
> > > > +       pgdp = pgd_offset(mm, addr);
> > > > +       pgdv = pgd_val(*pgdp);
> > > Before I went and looked again at the commit msg I said:
> > >
> > >         "Shoudln't we strictly speaking use pgdp_get()? I see you use this
> > >          helper for other levels."
> > >
> > > But obviously yeah. You explained the insane reason why not.
> > Had to find out the hard way ... :)
> Pain.
>
> > [...]
> >
> > > > +/*
> > > > + * This function is called to print an error when a bad page table entry (e.g.,
> > > > + * corrupted page table entry) is found. For example, we might have a
> > > > + * PFN-mapped pte in a region that doesn't allow it.
> > > > + *
> > > > + * The calling function must still handle the error.
> > > > + */
> > > We have extremely strict locking conditions for the page table traversal... but
> > > no mention of them here?
> > Yeah, I can add that.
> Thanks!
>
> > > > +static void print_bad_page_map(struct vm_area_struct *vma,
> > > > +               unsigned long addr, unsigned long long entry, struct page *page)
> > > > +{
> > > > +       struct address_space *mapping;
> > > > +       pgoff_t index;
> > > > +
> > > > +       if (is_bad_page_map_ratelimited())
> > > > +               return;
> > > >
> > > >         mapping = vma->vm_file ? vma->vm_file->f_mapping : NULL;
> > > >         index = linear_page_index(vma, addr);
> > > >
> > > > -       pr_alert("BUG: Bad page map in process %s  pte:%08llx pmd:%08llx\n",
> > > > -                current->comm,
> > > > -                (long long)pte_val(pte), (long long)pmd_val(*pmd));
> > > > +       pr_alert("BUG: Bad page map in process %s  entry:%08llx", 
> > > > current->comm, entry);
> > > Sort of wonder if this is even useful if you don't know what the 'entry'
> > > is? But I guess the dump below will tell you.
> > You probably missed in the patch description:
> >
> > "Whether it is a PTE or something else will usually become obvious from the
> > page table dump or from the dumped stack. If ever required in the future, we
> > could pass the entry level type similar to "enum rmap_level". For now, let's
> > keep it simple."
> Yeah sorry I glossed over the commit msg, and now I pay for it ;) OK this
> is fine then.
Let me play with indicating the page table level, but it's the kind of 
stuff I wouldn't want to do in this series here.
> > > Then we have VM_IO, which strictly must not have an associated page right?
> > VM_IO just means read/write side-effects, I think you could have ones with
> > an memmap easily ... e.g., memory section (128MiB) spanning both memory and
> > MMIO regions.
> Hmm, but why not have two separate VMAs? I guess I need to look into more
> what this flag actually effects.
Oh, I meant, that we might have a "struct page" for MMIO memory 
(pfn_valid() == true).
In a MIXEDMAP that will get refcounted. Not sure if there are users that 
use VM_IO in a MIXEDMAP, I would assume so but didn't check.
So VM_IO doesn't really interact with vm_normal_page(), really. It's all 
about PFNMAP and MIXEDMAP.
--
Cheers,

David / dhildenb