[PATCH v2] mm/page_vma_mapped.c: Detect mismatched pfn of hugetlbfs page in pfn_in_hpage()

[PATCH v2] mm/page_vma_mapped.c: Detect mismatched pfn of hugetlbfs page in pfn_in_hpage()

[Li Xinhai]

check_pte is called for hugetlbfs page and comparing pfn in pfn_in_page,
where pfn is compared in range [hpage_pfn, hpage_pfn+HPAGE_PMD_NR). For
hugetlbfs page, pfn must equal to hpage_pfn, so catch it as bug if they
are not equal.

Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 mm/page_vma_mapped.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
index eff4b45..713aaed 100644
--- a/mm/page_vma_mapped.c
+++ b/mm/page_vma_mapped.c
@@ -55,6 +55,7 @@ static bool map_pte(struct page_vma_mapped_walk *pvmw)
 static inline bool pfn_in_hpage(struct page *hpage, unsigned long pfn)
 {
 	unsigned long hpage_pfn = page_to_pfn(hpage);
+	VM_BUG_ON_PAGE(PageHuge(hpage) && pfn != hpage_pfn, hpage);
 
 	/* THP can be referenced by any subpage */
 	return pfn >= hpage_pfn && pfn - hpage_pfn < hpage_nr_pages(hpage);
-- 
1.8.3.1

[PATCH] mm/page_vma_mapped.c: Exactly compare hugetlbfs page's pfn in pfn_in_hpage()

[Li Xinhai]

check_pte is called for hugetlbfs page and comparing pfn in pfn_in_page,
where pfn is compared in range [hpage_pfn, hpage_pfn+HPAGE_PMD_NR). Now
change it to match exactly for hugetlbfs page to avoid hiding any
potential problems.

Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 mm/page_vma_mapped.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
index eff4b45..434978b 100644
--- a/mm/page_vma_mapped.c
+++ b/mm/page_vma_mapped.c
@@ -55,6 +55,8 @@ static bool map_pte(struct page_vma_mapped_walk *pvmw)
 static inline bool pfn_in_hpage(struct page *hpage, unsigned long pfn)
 {
 	unsigned long hpage_pfn = page_to_pfn(hpage);
+	if (unlikely(PageHuge(hpage)))
+		return pfn == hpage_pfn;
 
 	/* THP can be referenced by any subpage */
 	return pfn >= hpage_pfn && pfn - hpage_pfn < hpage_nr_pages(hpage);
-- 
1.8.3.1

Re: [PATCH] mm/page_vma_mapped.c: Exactly compare hugetlbfs page's pfn in pfn_in_hpage()

[Li Xinhai]

On 2020-01-09 at 22:26 Li Xinhai wrote:
>check_pte is called for hugetlbfs page and comparing pfn in pfn_in_page,
>where pfn is compared in range [hpage_pfn, hpage_pfn+HPAGE_PMD_NR). Now
>change it to match exactly for hugetlbfs page to avoid hiding any
>potential problems.
>
>Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
>Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>---
> mm/page_vma_mapped.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
>index eff4b45..434978b 100644
>--- a/mm/page_vma_mapped.c
>+++ b/mm/page_vma_mapped.c
>@@ -55,6 +55,8 @@ static bool map_pte(struct page_vma_mapped_walk *pvmw)
> static inline bool pfn_in_hpage(struct page *hpage, unsigned long pfn)
> {
> unsigned long hpage_pfn = page_to_pfn(hpage);
>+	if (unlikely(PageHuge(hpage)))
>+	return pfn == hpage_pfn;
>
> /* THP can be referenced by any subpage */
> return pfn >= hpage_pfn && pfn - hpage_pfn < hpage_nr_pages(hpage);
>--
>1.8.3.1
> 

sorry this been sent twice, please ignore this mail.

Re: [PATCH v2] mm/page_vma_mapped.c: Detect mismatched pfn of hugetlbfs page in pfn_in_hpage()

[Michal Hocko]

[Cc Mike who is hugetlb maintainer]

On Thu 09-01-20 14:26:02, Li Xinhai wrote:
> check_pte is called for hugetlbfs page and comparing pfn in pfn_in_page,
> where pfn is compared in range [hpage_pfn, hpage_pfn+HPAGE_PMD_NR). For
> hugetlbfs page, pfn must equal to hpage_pfn, so catch it as bug if they
> are not equal.

Why do we need this? Would that help to debug any past bug?

> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  mm/page_vma_mapped.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
> index eff4b45..713aaed 100644
> --- a/mm/page_vma_mapped.c
> +++ b/mm/page_vma_mapped.c
> @@ -55,6 +55,7 @@ static bool map_pte(struct page_vma_mapped_walk *pvmw)
>  static inline bool pfn_in_hpage(struct page *hpage, unsigned long pfn)
>  {
>  	unsigned long hpage_pfn = page_to_pfn(hpage);
> +	VM_BUG_ON_PAGE(PageHuge(hpage) && pfn != hpage_pfn, hpage);
>  
>  	/* THP can be referenced by any subpage */
>  	return pfn >= hpage_pfn && pfn - hpage_pfn < hpage_nr_pages(hpage);
> -- 
> 1.8.3.1
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] mm/page_vma_mapped.c: Detect mismatched pfn of hugetlbfs page in pfn_in_hpage()

[Mike Kravetz]

On 1/9/20 7:00 AM, Michal Hocko wrote:
> [Cc Mike who is hugetlb maintainer]
> 
> On Thu 09-01-20 14:26:02, Li Xinhai wrote:
>> check_pte is called for hugetlbfs page and comparing pfn in pfn_in_page,
>> where pfn is compared in range [hpage_pfn, hpage_pfn+HPAGE_PMD_NR). For
>> hugetlbfs page, pfn must equal to hpage_pfn, so catch it as bug if they
>> are not equal.
> 
> Why do we need this? Would that help to debug any past bug?

I have the same question as Michal.  Did you see an issue where such a check
could help?

There is nothing wrong with the proposed code.  However, it would be good to
know the reason for adding it.  I started looking at all possible code paths
where this could apply.  However, it would be better if you pointed out the
concern that caused you to create the patch.
-- 
Mike Kravetz

> 
>> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> ---
>>  mm/page_vma_mapped.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
>> index eff4b45..713aaed 100644
>> --- a/mm/page_vma_mapped.c
>> +++ b/mm/page_vma_mapped.c
>> @@ -55,6 +55,7 @@ static bool map_pte(struct page_vma_mapped_walk *pvmw)
>>  static inline bool pfn_in_hpage(struct page *hpage, unsigned long pfn)
>>  {
>>  	unsigned long hpage_pfn = page_to_pfn(hpage);
>> +	VM_BUG_ON_PAGE(PageHuge(hpage) && pfn != hpage_pfn, hpage);
>>  
>>  	/* THP can be referenced by any subpage */
>>  	return pfn >= hpage_pfn && pfn - hpage_pfn < hpage_nr_pages(hpage);
>> -- 
>> 1.8.3.1

Re: [PATCH v2] mm/page_vma_mapped.c: Detect mismatched pfn of hugetlbfs page in pfn_in_hpage()

[Li Xinhai]

On 2020-01-10 at 01:09 Mike Kravetz wrote:
>On 1/9/20 7:00 AM, Michal Hocko wrote:
>> [Cc Mike who is hugetlb maintainer]
>>
>> On Thu 09-01-20 14:26:02, Li Xinhai wrote:
>>> check_pte is called for hugetlbfs page and comparing pfn in pfn_in_page,
>>> where pfn is compared in range [hpage_pfn, hpage_pfn+HPAGE_PMD_NR). For
>>> hugetlbfs page, pfn must equal to hpage_pfn, so catch it as bug if they
>>> are not equal.
>>
>> Why do we need this? Would that help to debug any past bug?
>
>I have the same question as Michal.  Did you see an issue where such a check
>could help?
>
>There is nothing wrong with the proposed code.  However, it would be good to
>know the reason for adding it.  I started looking at all possible code paths
>where this could apply.  However, it would be better if you pointed out the
>concern that caused you to create the patch.
>--
>Mike Kravetz
> 
oops, I didn't write the code correctly. I should wrote it as 

	if (pfn >= hpage_pfn && pfn - hpage_pfn < hpage_nr_pages(hpage)) {
		VM_BUG_ON_PAGE(PageHuge(hpage) && pfn != hpage_pfn, hpage);
		return true;
	}

	return false;

hpage_nr_pages(hpage) give us HPAGE_PMD_NR for THP and hugetlbfs page,
but remapping PTE to a differrnt hugetlbfs page still allowed, so put the BUG code
into this condition is necessary. By this way, if it was not a exact match for PageHuge,
then it is a bug.

>>
>>> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
>>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>> ---
>>>  mm/page_vma_mapped.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
>>> index eff4b45..713aaed 100644
>>> --- a/mm/page_vma_mapped.c
>>> +++ b/mm/page_vma_mapped.c
>>> @@ -55,6 +55,7 @@ static bool map_pte(struct page_vma_mapped_walk *pvmw)
>>>  static inline bool pfn_in_hpage(struct page *hpage, unsigned long pfn)
>>>  {
>>>  unsigned long hpage_pfn = page_to_pfn(hpage);
>>> +	VM_BUG_ON_PAGE(PageHuge(hpage) && pfn != hpage_pfn, hpage);
>>> 
>>>  /* THP can be referenced by any subpage */
>>>  return pfn >= hpage_pfn && pfn - hpage_pfn < hpage_nr_pages(hpage);
>>> --
>>> 1.8.3.1

Re: [PATCH v2] mm/page_vma_mapped.c: Detect mismatched pfn of hugetlbfs page in pfn_in_hpage()

[Mike Kravetz]

On 1/9/20 2:48 PM, Li Xinhai wrote:
> oops, I didn't write the code correctly. I should wrote it as 
> 
> 	if (pfn >= hpage_pfn && pfn - hpage_pfn < hpage_nr_pages(hpage)) {
> 		VM_BUG_ON_PAGE(PageHuge(hpage) && pfn != hpage_pfn, hpage);
> 		return true;
> 	}
> 
> 	return false;
> 
> hpage_nr_pages(hpage) give us HPAGE_PMD_NR for THP and hugetlbfs page,
> but remapping PTE to a differrnt hugetlbfs page still allowed, so put the BUG code
> into this condition is necessary. By this way, if it was not a exact match for PageHuge,
> then it is a bug.

Thank you.  I think we all agree on what the proposed code is doing.
However, we would like to know why you believe this code should be added.
For example,
- Did you actually encounter this situation (PageHuge(hpage) && pfn !=
  hpage_pfn)?
- Did you discover some code path where we are likely to encounter this
  situation?
- Some other reason?
-- 
Mike Kravetz

Re: [PATCH v2] mm/page_vma_mapped.c: Detect mismatched pfn of hugetlbfs page in pfn_in_hpage()

[Li Xinhai]

On 2020-01-10 at 07:00 Mike Kravetz wrote:
>On 1/9/20 2:48 PM, Li Xinhai wrote:
>> oops, I didn't write the code correctly. I should wrote it as
>>
>> if (pfn >= hpage_pfn && pfn - hpage_pfn < hpage_nr_pages(hpage)) {
>> VM_BUG_ON_PAGE(PageHuge(hpage) && pfn != hpage_pfn, hpage);
>> return true;
>> }
>>
>> return false;
>>
>> hpage_nr_pages(hpage) give us HPAGE_PMD_NR for THP and hugetlbfs page,
>> but remapping PTE to a differrnt hugetlbfs page still allowed, so put the BUG code
>> into this condition is necessary. By this way, if it was not a exact match for PageHuge,
>> then it is a bug.
>
>Thank you.  I think we all agree on what the proposed code is doing.
>However, we would like to know why you believe this code should be added.
>For example,
>- Did you actually encounter this situation (PageHuge(hpage) && pfn !=
>  hpage_pfn)?
>- Did you discover some code path where we are likely to encounter this
>  situation?
>- Some other reason? 

I didn't actually encounter this condition.

There are two ways for faulty code,
1. one is from the 'hpage', it could be head or tail page of hugetlbfs (I see that
current code make sure always call with head page as you mentioned). Luckly,
we catch the tail page case as BUG at begining of this mapped_walk(the
page_hstate(page) return NULL for tail page).
2. The other is from the content stored in the PTE, wihch we used as 'pfn' and
compare with 'hpage'.

Current code matches 'pfn' and 'hpage' like below:
- normal 4k page: hpage_pfn <= pfn < hpage_pfn + 1
- THP, hugetlbfs page:  hpage_pfn <= pfn < hpage_pfn + HPAGE_PMD_NR
we need do exact match for normal 4K page and hugetlbfs page, and range
match for THP.

>--
>Mike Kravetz

Re: [PATCH v2] mm/page_vma_mapped.c: Detect mismatched pfn of hugetlbfs page in pfn_in_hpage()

[Michal Hocko]

On Fri 10-01-20 10:52:56, Li Xinhai wrote:
> On 2020-01-10 at 07:00 Mike Kravetz wrote:
> >On 1/9/20 2:48 PM, Li Xinhai wrote:
> >> oops, I didn't write the code correctly. I should wrote it as
> >>
> >> if (pfn >= hpage_pfn && pfn - hpage_pfn < hpage_nr_pages(hpage)) {
> >> VM_BUG_ON_PAGE(PageHuge(hpage) && pfn != hpage_pfn, hpage);
> >> return true;
> >> }
> >>
> >> return false;
> >>
> >> hpage_nr_pages(hpage) give us HPAGE_PMD_NR for THP and hugetlbfs page,
> >> but remapping PTE to a differrnt hugetlbfs page still allowed, so put the BUG code
> >> into this condition is necessary. By this way, if it was not a exact match for PageHuge,
> >> then it is a bug.
> >
> >Thank you.  I think we all agree on what the proposed code is doing.
> >However, we would like to know why you believe this code should be added.
> >For example,
> >- Did you actually encounter this situation (PageHuge(hpage) && pfn !=
> >  hpage_pfn)?
> >- Did you discover some code path where we are likely to encounter this
> >  situation?
> >- Some other reason? 
> 
> I didn't actually encounter this condition.
> 
> There are two ways for faulty code,
> 1. one is from the 'hpage', it could be head or tail page of hugetlbfs (I see that
> current code make sure always call with head page as you mentioned). Luckly,
> we catch the tail page case as BUG at begining of this mapped_walk(the
> page_hstate(page) return NULL for tail page).
> 2. The other is from the content stored in the PTE, wihch we used as 'pfn' and
> compare with 'hpage'.
> 
> Current code matches 'pfn' and 'hpage' like below:
> - normal 4k page: hpage_pfn <= pfn < hpage_pfn + 1
> - THP, hugetlbfs page:  hpage_pfn <= pfn < hpage_pfn + HPAGE_PMD_NR
> we need do exact match for normal 4K page and hugetlbfs page, and range
> match for THP.

This still doesn't really explain why to add the BUG_ON, I am afraid.
pfn_in_hpage is called from the vma walk. check_pte is reponsible to
check the page table mapping so the input to pfn_in_hpage should be
already sanitized. If it is not then that should be fixed and {VM_}BUG_ON
is not the best way to do such a sanitization IMHO. First of all this is
all under locks so crashing would likely mean a follow up problems.
On the other hand a failure can be handled gracefully AFAICS.

That being said I still do not see how this is going to help with
anything. Please note that adding {VM}_BUG_ON as general asserts is
strongly discouraged. Those should be added only when there is a
data corruption detected (and then it should likely be BUG_ON rather
than VM_BUG_ON) that cannot be handled gracefully or when it
considerably improves debugability of very subtle problems.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] mm/page_vma_mapped.c: Detect mismatched pfn of hugetlbfs page in pfn_in_hpage()

[Li Xinhai]

On 2020-01-10 at 14:22 Michal Hocko wrote:
>On Fri 10-01-20 10:52:56, Li Xinhai wrote:
>> On 2020-01-10 at 07:00 Mike Kravetz wrote:
>> >On 1/9/20 2:48 PM, Li Xinhai wrote:
>> >> oops, I didn't write the code correctly. I should wrote it as
>> >>
>> >> if (pfn >= hpage_pfn && pfn - hpage_pfn < hpage_nr_pages(hpage)) {
>> >> VM_BUG_ON_PAGE(PageHuge(hpage) && pfn != hpage_pfn, hpage);
>> >> return true;
>> >> }
>> >>
>> >> return false;
>> >>
>> >> hpage_nr_pages(hpage) give us HPAGE_PMD_NR for THP and hugetlbfs page,
>> >> but remapping PTE to a differrnt hugetlbfs page still allowed, so put the BUG code
>> >> into this condition is necessary. By this way, if it was not a exact match for PageHuge,
>> >> then it is a bug.
>> >
>> >Thank you.  I think we all agree on what the proposed code is doing.
>> >However, we would like to know why you believe this code should be added.
>> >For example,
>> >- Did you actually encounter this situation (PageHuge(hpage) && pfn !=
>> >  hpage_pfn)?
>> >- Did you discover some code path where we are likely to encounter this
>> >  situation?
>> >- Some other reason?
>>
>> I didn't actually encounter this condition.
>>
>> There are two ways for faulty code,
>> 1. one is from the 'hpage', it could be head or tail page of hugetlbfs (I see that
>> current code make sure always call with head page as you mentioned). Luckly,
>> we catch the tail page case as BUG at begining of this mapped_walk(the
>> page_hstate(page) return NULL for tail page).
>> 2. The other is from the content stored in the PTE, wihch we used as 'pfn' and
>> compare with 'hpage'.
>>
>> Current code matches 'pfn' and 'hpage' like below:
>> - normal 4k page: hpage_pfn <= pfn < hpage_pfn + 1
>> - THP, hugetlbfs page:  hpage_pfn <= pfn < hpage_pfn + HPAGE_PMD_NR
>> we need do exact match for normal 4K page and hugetlbfs page, and range
>> match for THP.
>
>This still doesn't really explain why to add the BUG_ON, I am afraid.
>pfn_in_hpage is called from the vma walk. check_pte is reponsible to
>check the page table mapping so the input to pfn_in_hpage should be
>already sanitized. If it is not then that should be fixed and {VM_}BUG_ON
>is not the best way to do such a sanitization IMHO. First of all this is
>all under locks so crashing would likely mean a follow up problems.
>On the other hand a failure can be handled gracefully AFAICS.
>
>That being said I still do not see how this is going to help with
>anything. Please note that adding {VM}_BUG_ON as general asserts is
>strongly discouraged. Those should be added only when there is a
>data corruption detected (and then it should likely be BUG_ON rather
>than VM_BUG_ON) that cannot be handled gracefully or when it
>considerably improves debugability of very subtle problems.
> 

pfn_in_hpage() has purpose to check all those three types of page, not
just THP as implied in its name. Change name as below would be clear

static inline bool pfn_is_matched(struct page *page, unsigned long pfn)
{
	unsigned long page_pfn = page_to_pfn(page);

	/* normal page and hugetlbfs page */
	if (!PageCompound(page) || PageHuge(page))
		return page_pfn == pfn;

	/* THP can be referenced by any subpage */
	return pfn >= page_pfn && pfn - page_pfn < hpage_nr_pages(page);
}

check_pte is doing right thing and helps to collect 'pfn' from both normal PTE
and migration entry. No chnages to it.

>--
>Michal Hocko
>SUSE Labs