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

David Hildenbrand
2025-07-18T11:04:36Z

>
> 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